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

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

[Łukasz Stelmach]

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(-)

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

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

[Russell King - ARM Linux admin]

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!

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

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 2974 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Russell King - ARM Linux admin]

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!

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

[Lukasz Stelmach]

[-- Attachment #1: Type: text/plain, Size: 4636 bytes --]

It was <2020-10-14 śro 23:04>, when Russell King - ARM Linux admin wrote:
> 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://protect2.fireeye.com/v1/url?k=d1d33944-8c07852c-d1d2b20b-0cc47a3356b2-cede44db8a43e7c3&q=1&e=d1dea895-d1df-46b9-8413-ac329b0bbfa7&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.9%2Fsource%2Fdrivers%2Fnet%2Fmii.c%23L174
>> [2]
>> https://protect2.fireeye.com/v1/url?k=c840e942-9594552a-c841620d-0cc47a3356b2-510c0749446ea7df&q=1&e=d1dea895-d1df-46b9-8413-ac329b0bbfa7&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.9%2Fsource%2Fdrivers%2Fnet%2Fmii.c%23L215
>
> 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.

Yes.

> 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.

Yes.

> 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.

Yes.

Thanks.

tl;dr: v2 is coming.

Let's take a look at what ethtool prints. With AN enabled

--8<---------------cut here---------------start------------->8---
Settings for enx00249b14f06e:
        Supported ports: [ TP MII ]
        Supported link modes:   10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Half 1000baseT/Full 
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Advertised pause frame use: Symmetric
        Advertised auto-negotiation: Yes

[...]

        Link detected: yes
--8<---------------cut here---------------end--------------->8---

with AN disabled

--8<---------------cut here---------------start------------->8---
Settings for enx00249b14f06e:
        Supported ports: [ TP MII ]
        Supported link modes:   10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Half 1000baseT/Full 
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  Not reported
        Advertised pause frame use: No
        Advertised auto-negotiation: No

[...]

        Link detected: yes
--8<---------------cut here---------------end--------------->8---

The driver is ax88179_178a which calls mii_ethtool_get_link_ksettings()

The first line that differs is "Advertised link modes". Apparently[1]
ethtool does not consider ADVERTISED_TP and ADVERTISED_MII as
interesting modes. Indeed I should add an else clause to clear
cmd->link_modes.advertising (and lp_advertising) and set only
ETHTOOL_LINK_MODE_TP_BIT and ETHTOOL_LINK_MODE_MII_BIT.


[1] https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/ethtool.c?h=v5.8#n656
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]