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.stelm...@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 >> >> -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics
signature.asc
Description: PGP signature
