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

[Lukasz Stelmach]

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@xxxxxxxxxxx>
>> ---
>>  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: signature.asc
Description: PGP signature