linux-kernel - Re: [PATCH net-next v4 02/13] net: phy: Use an internal, searchable storage for the linkmodes

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: <20250306095726.04125e5f@fedora.home>
Date: Thu, 6 Mar 2025 09:57:26 +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 02/13] net: phy: Use an internal, searchable
 storage for the linkmodes

Hello Paolo,

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

> On 3/3/25 10:03 AM, Maxime Chevallier wrote:
> [...]
> > +static int speed_duplex_to_capa(int speed, unsigned int duplex)
> > +{
> > +	if (duplex == DUPLEX_UNKNOWN ||
> > +	    (speed > SPEED_1000 && duplex != DUPLEX_FULL))
> > +		return -EINVAL;
> > +
> > +	switch (speed) {
> > +	case SPEED_10: return duplex == DUPLEX_FULL ?
> > +			      LINK_CAPA_10FD : LINK_CAPA_10HD;
> > +	case SPEED_100: return duplex == DUPLEX_FULL ?
> > +			       LINK_CAPA_100FD : LINK_CAPA_100HD;
> > +	case SPEED_1000: return duplex == DUPLEX_FULL ?
> > +				LINK_CAPA_1000FD : LINK_CAPA_1000HD;
> > +	case SPEED_2500: return LINK_CAPA_2500FD;
> > +	case SPEED_5000: return LINK_CAPA_5000FD;
> > +	case SPEED_10000: return LINK_CAPA_10000FD;
> > +	case SPEED_20000: return LINK_CAPA_20000FD;
> > +	case SPEED_25000: return LINK_CAPA_25000FD;
> > +	case SPEED_40000: return LINK_CAPA_40000FD;
> > +	case SPEED_50000: return LINK_CAPA_50000FD;
> > +	case SPEED_56000: return LINK_CAPA_56000FD;
> > +	case SPEED_100000: return LINK_CAPA_100000FD;
> > +	case SPEED_200000: return LINK_CAPA_200000FD;
> > +	case SPEED_400000: return LINK_CAPA_400000FD;
> > +	case SPEED_800000: return LINK_CAPA_800000FD;
> > +	}
> > +  
> 
> What about adding some code here to help future patch updating LINK_CAPA
> definition as needed?
> 
> Something alike:
> 
> 	pr_err_once("Unknown speed %d, please update LINK_CAPS\n", speed);
> 
> 
> > +	return -EINVAL;
> > +}
> > +
> > +/**
> > + * phy_caps_init() - Initializes the link_caps array from the link_mode_params.
> > + */
> > +void phy_caps_init(void)
> > +{
> > +	const struct link_mode_info *linkmode;
> > +	int i, capa;
> > +
> > +	/* Fill the caps array from net/ethtool/common.c */
> > +	for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; i++) {
> > +		linkmode = &link_mode_params[i];
> > +		capa = speed_duplex_to_capa(linkmode->speed, linkmode->duplex);
> > +
> > +		if (capa < 0)
> > +			continue;  
> 
> Or even error-out here.

Good point yes indeed. Russell raised the point for the need of keeping
this in sync with new SPEED_XXX definitions, I'll add a check that
errors out.

I hope that's OK though, as higher speeds are introduced and used by
NICs that usually don't use phylib at all, so there's a good chance
that the developper introducing the new speed won't have CONFIG_PHYLIB
enabled.

Is that still good ?

Maxime