| Message ID | 20201014125650.12137-1-l.stelmach@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: Prevent reporting advertised modes when autoneg is off | expand |
On Wed, Oct 14, 2020 at 02:56:50PM +0200, Łukasz Stelmach wrote: > Do not report advertised link modes when autonegotiation is turned > off. mii_ethtool_get_link_ksettings() exhibits the same behaviour. Please explain why this is a desirable change. Referring to some other piece of code isn't a particularly good reason especially when that piece of code is likely derived from fairly old code (presumably mii_ethtool_get_link_ksettings()'s behaviour is designed to be compatible with mii_ethtool_gset()). In any case, the mii.c code does fill in the advertising mask even when autoneg is disabled, because, rightly or wrongly, the advertising mask contains more than just the link modes, it includes the interface(s) as well. Your change means phylib no longer reports the interface modes which is at odds with the mii.c code. > Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com> > --- > drivers/net/phy/phy.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 35525a671400..3cadf224fdb2 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -315,7 +315,8 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev, > struct ethtool_link_ksettings *cmd) > { > linkmode_copy(cmd->link_modes.supported, phydev->supported); > - linkmode_copy(cmd->link_modes.advertising, phydev->advertising); > + if (phydev->autoneg) > + linkmode_copy(cmd->link_modes.advertising, phydev->advertising); > linkmode_copy(cmd->link_modes.lp_advertising, phydev->lp_advertising); > > cmd->base.speed = phydev->speed; > -- > 2.26.2 > >
It was <2020-10-14 śro 14:32>, when Russell King - ARM Linux admin wrote: > On Wed, Oct 14, 2020 at 02:56:50PM +0200, Łukasz Stelmach wrote: >> Do not report advertised link modes when autonegotiation is turned >> off. mii_ethtool_get_link_ksettings() exhibits the same behaviour. > > Please explain why this is a desirable change. > To make the behavior uniform accross different drivers. For example ethtool shows different reports on different hardware depending on whether the driver uses phylib or mii. I don't insist on accepting my patch. I merely propos it as a means of the unification. Maybe it is mii.c that should be changed. > Referring to some other piece of code isn't a particularly good reason > especially when that piece of code is likely derived from fairly old > code (presumably mii_ethtool_get_link_ksettings()'s behaviour is > designed to be compatible with mii_ethtool_gset()). Well according to git phy_ethtool_ksettings_get() was first (2011-04-15, phy_ethtool_get_link_ksettings() soon after) while mii_ethtool_get_link_ksettings() is half a year younger. Indeed, maybe I should patch mii_ethtool_get_link_ksettings() instead. > In any case, the mii.c code does fill in the advertising mask even > when autoneg is disabled, because, rightly or wrongly, the advertising > mask contains more than just the link modes, it includes the > interface(s) as well. Your change means phylib no longer reports the > interface modes which is at odds with the mii.c code. I am afraid you are wrong. There is a rather big if()[1], which depending on AN beeing enabled fills more or less information. Yes this if() looks like it has been yanked from mii_ethtool_gset(). When advertising is converted and copied to cmd->link_modes.advertising at the end of mii_ethtool_get_link_ksettings() it is 0[2] if autonegotiation is disabled. [1] https://elixir.bootlin.com/linux/v5.9/source/drivers/net/mii.c#L174 [2] https://elixir.bootlin.com/linux/v5.9/source/drivers/net/mii.c#L215 >> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com> >> --- >> drivers/net/phy/phy.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >> index 35525a671400..3cadf224fdb2 100644 >> --- a/drivers/net/phy/phy.c >> +++ b/drivers/net/phy/phy.c >> @@ -315,7 +315,8 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev, >> struct ethtool_link_ksettings *cmd) >> { >> linkmode_copy(cmd->link_modes.supported, phydev->supported); >> - linkmode_copy(cmd->link_modes.advertising, phydev->advertising); >> + if (phydev->autoneg) >> + linkmode_copy(cmd->link_modes.advertising, phydev->advertising); >> linkmode_copy(cmd->link_modes.lp_advertising, phydev->lp_advertising); >> >> cmd->base.speed = phydev->speed; >> -- >> 2.26.2 >> >>
On Wed, Oct 14, 2020 at 04:39:47PM +0200, Lukasz Stelmach wrote: > It was <2020-10-14 śro 14:32>, when Russell King - ARM Linux admin wrote: > > In any case, the mii.c code does fill in the advertising mask even > > when autoneg is disabled, because, rightly or wrongly, the advertising > > mask contains more than just the link modes, it includes the > > interface(s) as well. Your change means phylib no longer reports the > > interface modes which is at odds with the mii.c code. > > I am afraid you are wrong. There is a rather big if()[1], which > depending on AN beeing enabled fills more or less information. Yes this > if() looks like it has been yanked from mii_ethtool_gset(). When > advertising is converted and copied to cmd->link_modes.advertising at > the end of mii_ethtool_get_link_ksettings() it is 0[2] if autonegotiation > is disabled. > > [1] https://elixir.bootlin.com/linux/v5.9/source/drivers/net/mii.c#L174 > [2] https://elixir.bootlin.com/linux/v5.9/source/drivers/net/mii.c#L215 I'm very sorry, but I have to disagree. I'll quote the code here: advertising = ADVERTISED_TP | ADVERTISED_MII; // This is your big if() if (bmcr & BMCR_ANENABLE) { advertising |= ADVERTISED_Autoneg; ... } else { ... } ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.advertising, advertising); So, when AN is disabled, we still end up with TP and MII in the advertised link modes. I call TP and MII "interface modes" above to separate them from the "link modes" that describe the speed and duplex etc. Note that only lp_advertising is zeroed in the "else" clause of the above "if" statement - advertising remains as-is with TP and MII set. Your patch, on the other hand, merely avoids setting anything in cmd->link_modes.advertising, which means we do not end up with the "interface modes". I hope that this helps you see my point.
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 35525a671400..3cadf224fdb2 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -315,7 +315,8 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev, struct ethtool_link_ksettings *cmd) { linkmode_copy(cmd->link_modes.supported, phydev->supported); - linkmode_copy(cmd->link_modes.advertising, phydev->advertising); + if (phydev->autoneg) + linkmode_copy(cmd->link_modes.advertising, phydev->advertising); linkmode_copy(cmd->link_modes.lp_advertising, phydev->lp_advertising); cmd->base.speed = phydev->speed;
Do not report advertised link modes when autonegotiation is turned off. mii_ethtool_get_link_ksettings() exhibits the same behaviour. Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com> --- drivers/net/phy/phy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)