net: phy: Prevent reporting advertised modes when autoneg is off

Message ID 20201014125650.12137-1-l.stelmach@samsung.com
State New
Headers show
Series
  • net: phy: Prevent reporting advertised modes when autoneg is off
Related show

Commit Message

Lukasz Stelmach Oct. 14, 2020, 12:56 p.m.
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(-)

Comments

Russell King (Oracle) Oct. 14, 2020, 1:32 p.m. | #1
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

> 

> 


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Lukasz Stelmach Oct. 14, 2020, 2:39 p.m. | #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

>> 

>> 


-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics
Russell King (Oracle) Oct. 14, 2020, 10:04 p.m. | #3
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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Patch

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;