netdev - Re: [PATCH] net: dsa: fixed-link interface is reporting SPEED

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3661ec3f-1a13-26d8-f7dc-7a73ac210f08@gmail.com>
Date:   Fri, 12 Apr 2019 19:57:38 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>, f.fainelli@...il.com,
        andrew@...n.ch, davem@...emloft.net
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH] net: dsa: fixed-link interface is reporting SPEED_UNKNOWN

On 12.04.2019 01:01, Vladimir Oltean wrote:
> With Heiner's recent patch "b6163f194c69 net: phy: improve
> genphy_read_status", the phydev->speed is now initialized by default to
> SPEED_UNKNOWN even for fixed PHYs. This is not necessarily bad, since it
> is not correct to call genphy_config_init() and genphy_read_status() for
> a fixed PHY.
> 
What do you mean with "it is not correct"? Whether the calls are always
needed may be a valid question, but it's not forbidden to use these calls
with a fixed PHY. Actually in phylib polling mode genphy_read_status is
called every second also for a fixed PHY. swphy emulates all relevant
PHY registers.

> This dates back all the way to "39b0c705195e net: dsa: Allow
> configuration of CPU & DSA port speeds/duplex" (discussion thread:
> https://www.spinics.net/lists/netdev/msg340862.html).
> 
> I don't seem to understand why these calls were necessary back then, but
> removing these calls seemingly has no impact now apart from preventing
> the phydev->speed that was set in of_phy_register_fixed_link() from
> getting overwritten.
> 
I tend to agree with the change itself, but not with the justification.
For genphy_config_init I'd rather say:
genphy_config_init removes invalid modes based on the abilities read
from the chip. But as we emulate all registers anyway, this doesn't
provide any benefit for a swphy.

> Signed-off-by: Vladimir Oltean <olteanv@...il.com>
> ---
>  net/dsa/port.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 87769cb38c31..481412c892a7 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -485,9 +485,6 @@ static int dsa_port_fixed_link_register_of(struct dsa_port *dp)
>  		mode = PHY_INTERFACE_MODE_NA;
>  	phydev->interface = mode;
>  
> -	genphy_config_init(phydev);
> -	genphy_read_status(phydev);
> -
>  	if (ds->ops->adjust_link)
>  		ds->ops->adjust_link(ds, port, phydev);
>  
>