| Message ID | 559FF5A3.2030607@list.ru |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 10/07/15 09:41, Stas Sergeev wrote: > > Currently fixed_phy driver recognizes only the link-up state. > This simple patch adds an implementation of link-down state. > The actual change is 1-line, the rest is an indentation. It is not clear to me how this is useful, if you have a link_update callback manipulating the link state, the fixed PHY driver returns appropriate MII_BMSR values and always re-initializes everything. Is this meant to be some sort of optimization? If so, you could just avoid the re-intendation completely and do a goto instead? > > Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net> > > CC: Florian Fainelli <f.fainelli@gmail.com> > CC: netdev@vger.kernel.org > CC: linux-kernel@vger.kernel.org > --- > drivers/net/phy/fixed_phy.c | 99 +++++++++++++++++++++++---------------------- > 1 file changed, 50 insertions(+), 49 deletions(-) > > diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c > index 1960b46..a5d93cf 100644 > --- a/drivers/net/phy/fixed_phy.c > +++ b/drivers/net/phy/fixed_phy.c > @@ -52,58 +52,59 @@ static int fixed_phy_update_regs(struct fixed_phy *fp) > u16 lpagb = 0; > u16 lpa = 0; > > - if (fp->status.duplex) { > - bmcr |= BMCR_FULLDPLX; > - > - switch (fp->status.speed) { > - case 1000: > - bmsr |= BMSR_ESTATEN; > - bmcr |= BMCR_SPEED1000; > - lpagb |= LPA_1000FULL; > - break; > - case 100: > - bmsr |= BMSR_100FULL; > - bmcr |= BMCR_SPEED100; > - lpa |= LPA_100FULL; > - break; > - case 10: > - bmsr |= BMSR_10FULL; > - lpa |= LPA_10FULL; > - break; > - default: > - pr_warn("fixed phy: unknown speed\n"); > - return -EINVAL; > - } > - } else { > - switch (fp->status.speed) { > - case 1000: > - bmsr |= BMSR_ESTATEN; > - bmcr |= BMCR_SPEED1000; > - lpagb |= LPA_1000HALF; > - break; > - case 100: > - bmsr |= BMSR_100HALF; > - bmcr |= BMCR_SPEED100; > - lpa |= LPA_100HALF; > - break; > - case 10: > - bmsr |= BMSR_10HALF; > - lpa |= LPA_10HALF; > - break; > - default: > - pr_warn("fixed phy: unknown speed\n"); > - return -EINVAL; > - } > - } > - > - if (fp->status.link) > + if (fp->status.link) { > bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE; > > - if (fp->status.pause) > - lpa |= LPA_PAUSE_CAP; > + if (fp->status.duplex) { > + bmcr |= BMCR_FULLDPLX; > + > + switch (fp->status.speed) { > + case 1000: > + bmsr |= BMSR_ESTATEN; > + bmcr |= BMCR_SPEED1000; > + lpagb |= LPA_1000FULL; > + break; > + case 100: > + bmsr |= BMSR_100FULL; > + bmcr |= BMCR_SPEED100; > + lpa |= LPA_100FULL; > + break; > + case 10: > + bmsr |= BMSR_10FULL; > + lpa |= LPA_10FULL; > + break; > + default: > + pr_warn("fixed phy: unknown speed\n"); > + return -EINVAL; > + } > + } else { > + switch (fp->status.speed) { > + case 1000: > + bmsr |= BMSR_ESTATEN; > + bmcr |= BMCR_SPEED1000; > + lpagb |= LPA_1000HALF; > + break; > + case 100: > + bmsr |= BMSR_100HALF; > + bmcr |= BMCR_SPEED100; > + lpa |= LPA_100HALF; > + break; > + case 10: > + bmsr |= BMSR_10HALF; > + lpa |= LPA_10HALF; > + break; > + default: > + pr_warn("fixed phy: unknown speed\n"); > + return -EINVAL; > + } > + } > > - if (fp->status.asym_pause) > - lpa |= LPA_PAUSE_ASYM; > + if (fp->status.pause) > + lpa |= LPA_PAUSE_CAP; > + > + if (fp->status.asym_pause) > + lpa |= LPA_PAUSE_ASYM; > + } > > fp->regs[MII_PHYSID1] = 0; > fp->regs[MII_PHYSID2] = 0; >
10.07.2015 23:44, Florian Fainelli пишет: > On 10/07/15 09:41, Stas Sergeev wrote: >> Currently fixed_phy driver recognizes only the link-up state. >> This simple patch adds an implementation of link-down state. >> The actual change is 1-line, the rest is an indentation. > It is not clear to me how this is useful, if you have a link_update > callback manipulating the link state, the fixed PHY driver returns > appropriate MII_BMSR values and always re-initializes everything. It returns the appropriate values only for link status (when its down), but it still returns speed, duplex etc as if the link is up. I had hard times finding the relevant specs, but from what I have googled, when link is down, the speed/duplex/etc status fields should _also_ be zero, which is what my patch does. What is more important is that fixed_phy_add() would return -EINVAL if you didn't specify speed while the link is down. This is an absolute must-fix, or I will have to add an arbitrary speed value again, on which you already objected. > Is this meant to be some sort of optimization? If so, you could just > avoid the re-intendation completely and do a goto instead? Oh, c'mon... Adding goto just to keep the _patch_ smaller? (not smaller code, just a smaller patch) Well, this is certainly something that can be done, feel free to request that explicitly and I'll release v3 next week. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/07/15 14:14, Stas Sergeev wrote: > 10.07.2015 23:44, Florian Fainelli пишет: >> On 10/07/15 09:41, Stas Sergeev wrote: >>> Currently fixed_phy driver recognizes only the link-up state. >>> This simple patch adds an implementation of link-down state. >>> The actual change is 1-line, the rest is an indentation. >> It is not clear to me how this is useful, if you have a link_update >> callback manipulating the link state, the fixed PHY driver returns >> appropriate MII_BMSR values and always re-initializes everything. > It returns the appropriate values only for link status (when its down), > but it still returns speed, duplex etc as if the link is up. I had hard > times finding the relevant specs, but from what I have googled, > when link is down, the speed/duplex/etc status fields should _also_ > be zero, which is what my patch does. > What is more important is that fixed_phy_add() would return > -EINVAL if you didn't specify speed while the link is down. > This is an absolute must-fix, or I will have to add an arbitrary > speed value again, on which you already objected. Ok, but that does not seems to be a code path that you can hit, unless you are already modifying drivers/of/of_mdio.c::of_fixed_phy_register_link() and overriding how status.link is defined, am I missing something? > >> Is this meant to be some sort of optimization? If so, you could just >> avoid the re-intendation completely and do a goto instead? > Oh, c'mon... Adding goto just to keep the _patch_ smaller? Well, yes, so it's easy to audit the changes? > (not smaller code, just a smaller patch) > Well, this is certainly something that can be done, feel free > to request that explicitly and I'll release v3 next week. I hereby explicitly request that you make this a new iteration using a goto. Thank you.
11.07.2015 03:15, Florian Fainelli пишет: > On 10/07/15 14:14, Stas Sergeev wrote: >> 10.07.2015 23:44, Florian Fainelli пишет: >>> On 10/07/15 09:41, Stas Sergeev wrote: >>>> Currently fixed_phy driver recognizes only the link-up state. >>>> This simple patch adds an implementation of link-down state. >>>> The actual change is 1-line, the rest is an indentation. >>> It is not clear to me how this is useful, if you have a link_update >>> callback manipulating the link state, the fixed PHY driver returns >>> appropriate MII_BMSR values and always re-initializes everything. >> It returns the appropriate values only for link status (when its down), >> but it still returns speed, duplex etc as if the link is up. I had hard >> times finding the relevant specs, but from what I have googled, >> when link is down, the speed/duplex/etc status fields should _also_ >> be zero, which is what my patch does. >> What is more important is that fixed_phy_add() would return >> -EINVAL if you didn't specify speed while the link is down. >> This is an absolute must-fix, or I will have to add an arbitrary >> speed value again, on which you already objected. > Ok, but that does not seems to be a code path that you can hit, unless > you are already modifying > drivers/of/of_mdio.c::of_fixed_phy_register_link() and overriding how > status.link is defined, am I missing something? I think you can. The drivers that do autonegotiation (eg mvneta) should take a special care to not reset speed when link is down. Or to nor read the speed when link is down (this is discouraged anyway of course, but better to follow the real MDIO hw in that). So while the work-arounds are simple, you can nevertheless hit the bug if you try to. >>> Is this meant to be some sort of optimization? If so, you could just >>> avoid the re-intendation completely and do a goto instead? >> Oh, c'mon... Adding goto just to keep the _patch_ smaller? > Well, yes, so it's easy to audit the changes? So you don't trust me that I only indented the code? OKey. :) >> (not smaller code, just a smaller patch) >> Well, this is certainly something that can be done, feel free >> to request that explicitly and I'll release v3 next week. > I hereby explicitly request that you make this a new iteration using a goto. OKey, will do in v3. Of course if you point me to the coding guidelines that explain this part, I'll be more comfortable. But this is purely optional, I simply don't like to add gotos where unneeded, but its not a big deal at all. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c index 1960b46..a5d93cf 100644 --- a/drivers/net/phy/fixed_phy.c +++ b/drivers/net/phy/fixed_phy.c @@ -52,58 +52,59 @@ static int fixed_phy_update_regs(struct fixed_phy *fp) u16 lpagb = 0; u16 lpa = 0; - if (fp->status.duplex) { - bmcr |= BMCR_FULLDPLX; - - switch (fp->status.speed) { - case 1000: - bmsr |= BMSR_ESTATEN; - bmcr |= BMCR_SPEED1000; - lpagb |= LPA_1000FULL; - break; - case 100: - bmsr |= BMSR_100FULL; - bmcr |= BMCR_SPEED100; - lpa |= LPA_100FULL; - break; - case 10: - bmsr |= BMSR_10FULL; - lpa |= LPA_10FULL; - break; - default: - pr_warn("fixed phy: unknown speed\n"); - return -EINVAL; - } - } else { - switch (fp->status.speed) { - case 1000: - bmsr |= BMSR_ESTATEN; - bmcr |= BMCR_SPEED1000; - lpagb |= LPA_1000HALF; - break; - case 100: - bmsr |= BMSR_100HALF; - bmcr |= BMCR_SPEED100; - lpa |= LPA_100HALF; - break; - case 10: - bmsr |= BMSR_10HALF; - lpa |= LPA_10HALF; - break; - default: - pr_warn("fixed phy: unknown speed\n"); - return -EINVAL; - } - } - - if (fp->status.link) + if (fp->status.link) { bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE; - if (fp->status.pause) - lpa |= LPA_PAUSE_CAP; + if (fp->status.duplex) { + bmcr |= BMCR_FULLDPLX; + + switch (fp->status.speed) { + case 1000: + bmsr |= BMSR_ESTATEN; + bmcr |= BMCR_SPEED1000; + lpagb |= LPA_1000FULL; + break; + case 100: + bmsr |= BMSR_100FULL; + bmcr |= BMCR_SPEED100; + lpa |= LPA_100FULL; + break; + case 10: + bmsr |= BMSR_10FULL; + lpa |= LPA_10FULL; + break; + default: + pr_warn("fixed phy: unknown speed\n"); + return -EINVAL; + } + } else { + switch (fp->status.speed) { + case 1000: + bmsr |= BMSR_ESTATEN; + bmcr |= BMCR_SPEED1000; + lpagb |= LPA_1000HALF; + break; + case 100: + bmsr |= BMSR_100HALF; + bmcr |= BMCR_SPEED100; + lpa |= LPA_100HALF; + break; + case 10: + bmsr |= BMSR_10HALF; + lpa |= LPA_10HALF; + break; + default: + pr_warn("fixed phy: unknown speed\n"); + return -EINVAL; + } + } - if (fp->status.asym_pause) - lpa |= LPA_PAUSE_ASYM; + if (fp->status.pause) + lpa |= LPA_PAUSE_CAP; + + if (fp->status.asym_pause) + lpa |= LPA_PAUSE_ASYM; + } fp->regs[MII_PHYSID1] = 0; fp->regs[MII_PHYSID2] = 0;
Currently fixed_phy driver recognizes only the link-up state. This simple patch adds an implementation of link-down state. The actual change is 1-line, the rest is an indentation. Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net> CC: Florian Fainelli <f.fainelli@gmail.com> CC: netdev@vger.kernel.org CC: linux-kernel@vger.kernel.org --- drivers/net/phy/fixed_phy.c | 99 +++++++++++++++++++++++---------------------- 1 file changed, 50 insertions(+), 49 deletions(-)