linux-kernel - Re: [PATCH net-next v4 09/13] net: phylink: Use phy_caps

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: <20250306111220.28798e6b@fedora.home>
Date: Thu, 6 Mar 2025 11:12:20 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: davem@...emloft.net, Andrew Lunn <andrew@...n.ch>, Jakub Kicinski
 <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>, Russell King
 <linux@...linux.org.uk>, Heiner Kallweit <hkallweit1@...il.com>,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 thomas.petazzoni@...tlin.com, linux-arm-kernel@...ts.infradead.org,
 Christophe Leroy <christophe.leroy@...roup.eu>, Herve Codina
 <herve.codina@...tlin.com>, Florian Fainelli <f.fainelli@...il.com>,
 Vladimir Oltean <vladimir.oltean@....com>, Köry Maincent
 <kory.maincent@...tlin.com>, Oleksij Rempel <o.rempel@...gutronix.de>,
 Simon Horman <horms@...nel.org>, Romain Gantois
 <romain.gantois@...tlin.com>
Subject: Re: [PATCH net-next v4 09/13] net: phylink: Use phy_caps_lookup for
 fixed-link configuration

On Thu, 6 Mar 2025 09:56:32 +0100
Paolo Abeni <pabeni@...hat.com> wrote:

> On 3/3/25 10:03 AM, Maxime Chevallier wrote:
> > @@ -879,8 +880,10 @@ static int phylink_parse_fixedlink(struct phylink *pl,
> >  	linkmode_copy(pl->link_config.advertising, pl->supported);
> >  	phylink_validate(pl, pl->supported, &pl->link_config);
> >  
> > -	s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
> > -			       pl->supported, true);
> > +	c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
> > +			    pl->supported, true);
> > +	if (c)
> > +		linkmode_and(match, pl->supported, c->linkmodes);  
> 
> How about using only the first bit from `c->linkmodes`, to avoid
> behavior changes?

If what we want is to keep the exact same behaviour, then we need to
go one step further and make sure we keep the same one as before, and
it's not guaranteed that the first bit in c->linkmodes is this one.

We could however have a default supported mask for fixed-link in phylink
that contains all the linkmodes we allow for fixed links, then filter
with the lookup, something like :


-       linkmode_fill(pl->supported);
+       /* (in a dedicated helper) Linkmodes reported for fixed links below
+        * 10G */
+       linkmode_zero(pl->supported);
+
+       linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, pl->supported);
+       linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, pl->supported);
+       linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, pl->supported);
+       linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, pl->supported);
+       linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, pl->supported);
+       linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, pl->supported);
+       linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, pl->supported);
+       linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, pl->supported);
+       linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, pl->supported);
+
        linkmode_copy(pl->link_config.advertising, pl->supported);
        phylink_validate(pl, pl->supported, &pl->link_config);
 
-       s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
-                              pl->supported, true);
+       c = phy_caps_lookup(pl->link_config.speed, pl->link_config.duplex,
+                           pl->supported, true);
+       if (c)
+               linkmode_and(match, pl->supported, c->linkmodes);

That way we should have a consistent behaviour with what we currently
have, and to me it's more explicit what will the  fixed-link linkmodes
be.

I'd like to make sure Russell is OK with that though :)

Thanks you for the review Paolo !

Maxime