| Message ID | 20201015084435.24368-1-l.stelmach@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] net: phy: Prevent reporting advertised modes when autoneg is off | expand |
On Thu, Oct 15, 2020 at 10:44:35AM +0200, Łukasz Stelmach wrote: > Do not report advertised link modes (local and remote) when > autonegotiation is turned off. mii_ethtool_get_link_ksettings() exhibits > the same behaviour and this patch aims at unifying the behavior of both > functions. Does ethtool allow you to configure advertised modes with autoneg off? If it can, it would be useful to see what is being configured, before it is actually turned on. ethtool -s eth42 autoneg off advertise 0xf does not give an error on an interface i have. Andrew
It was <2020-10-16 pią 20:09>, when Andrew Lunn wrote: > On Thu, Oct 15, 2020 at 10:44:35AM +0200, Łukasz Stelmach wrote: >> Do not report advertised link modes (local and remote) when >> autonegotiation is turned off. mii_ethtool_get_link_ksettings() exhibits >> the same behaviour and this patch aims at unifying the behavior of both >> functions. > > Does ethtool allow you to configure advertised modes with autoneg off? > If it can, it would be useful to see what is being configured, before > it is actually turned on. > > ethtool -s eth42 autoneg off advertise 0xf > > does not give an error on an interface i have. Yes, this is a good point. Do you think I should change the if()[1] in mii_ethtool_get_link_ksettings() instead? I really think these two function should report the same. [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#L145 Kind regards, -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics
On Fri, Oct 16, 2020 at 09:37:22PM +0200, Lukasz Stelmach wrote: > It was <2020-10-16 pią 20:09>, when Andrew Lunn wrote: > > On Thu, Oct 15, 2020 at 10:44:35AM +0200, Łukasz Stelmach wrote: > >> Do not report advertised link modes (local and remote) when > >> autonegotiation is turned off. mii_ethtool_get_link_ksettings() exhibits > >> the same behaviour and this patch aims at unifying the behavior of both > >> functions. > > > > Does ethtool allow you to configure advertised modes with autoneg off? > > If it can, it would be useful to see what is being configured, before > > it is actually turned on. > > > > ethtool -s eth42 autoneg off advertise 0xf > > > > does not give an error on an interface i have. > > Yes, this is a good point. Do you think I should change the if()[1] in > mii_ethtool_get_link_ksettings() instead? I really think these two > function should report the same. Yes, i would change mii. Ideally we want all drivers to use phylib/phylink, not mii. So i would modify mii to match phylib/phylink, not the other way around. And then there will be drivers which do their own PHY handling, hidden away in firmware, and not using either of mii or phylib/phylink. You can expect them to be inconsistent. Andrew
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 35525a671400..6ede9c1c138c 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -315,8 +315,17 @@ 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); - linkmode_copy(cmd->link_modes.lp_advertising, phydev->lp_advertising); + if (phydev->autoneg) { + linkmode_copy(cmd->link_modes.advertising, phydev->advertising); + linkmode_copy(cmd->link_modes.lp_advertising, phydev->lp_advertising); + } else { + linkmode_zero(cmd->link_modes.lp_advertising); + linkmode_zero(cmd->link_modes.advertising); + linkmode_set_bit(ETHTOOL_LINK_MODE_TP_BIT, + cmd->link_modes.advertising); + linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, + cmd->link_modes.advertising); + } cmd->base.speed = phydev->speed; cmd->base.duplex = phydev->duplex;
Do not report advertised link modes (local and remote) when autonegotiation is turned off. mii_ethtool_get_link_ksettings() exhibits the same behaviour and this patch aims at unifying the behavior of both functions. Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com> --- Changes in v2: - clear lp_advertising - set ETHTOOL_LINK_MODE_TP_BIT and ETHTOOL_LINK_MODE_MII_BIT in advertising drivers/net/phy/phy.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)