[v2] net: phy: Prevent reporting advertised modes when autoneg is off

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
Related show

Commit Message

Lukasz Stelmach Oct. 15, 2020, 8:44 a.m.
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(-)

Comments

Andrew Lunn Oct. 16, 2020, 6:09 p.m. | #1
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
Lukasz Stelmach Oct. 16, 2020, 7:37 p.m. | #2
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
Andrew Lunn Oct. 16, 2020, 9 p.m. | #3
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

Patch

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;