linux-kernel - Re: [PATCH net-next v19 00/15] net: phy: Introduce PHY ports representation

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: <f753719e-2370-401d-a001-821bdd5ee838@bootlin.com>
Date: Thu, 27 Nov 2025 16:53:12 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Chris Mason <clm@...a.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 thomas.petazzoni@...tlin.com, Andrew Lunn <andrew@...n.ch>,
 Eric Dumazet <edumazet@...gle.com>, Russell King <linux@...linux.org.uk>,
 linux-arm-kernel@...ts.infradead.org,
 Christophe Leroy <christophe.leroy@...roup.eu>,
 Herve Codina <herve.codina@...tlin.com>,
 Florian Fainelli <f.fainelli@...il.com>,
 Heiner Kallweit <hkallweit1@...il.com>,
 Vladimir Oltean <vladimir.oltean@....com>,
 Köry Maincent <kory.maincent@...tlin.com>,
 Marek Behún <kabel@...nel.org>,
 Oleksij Rempel <o.rempel@...gutronix.de>,
 Nicolò Veronese <nicveronese@...il.com>,
 Simon Horman <horms@...nel.org>, mwojtas@...omium.org,
 Antoine Tenart <atenart@...nel.org>, devicetree@...r.kernel.org,
 Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Rob Herring <robh@...nel.org>,
 Romain Gantois <romain.gantois@...tlin.com>,
 Daniel Golle <daniel@...rotopia.org>,
 Dimitri Fedrau <dimitri.fedrau@...bherr.com>,
 Tariq Toukan <tariqt@...dia.com>
Subject: Re: [PATCH net-next v19 00/15] net: phy: Introduce PHY ports
 representation

Hi Chris,

On 27/11/2025 04:00, Jakub Kicinski wrote:
> On Sat, 22 Nov 2025 13:42:59 +0100 Maxime Chevallier wrote:
>> This is v19 of the phy_port work. Patches 2 and 3 lack PHY maintainers reviews.
>>
>> This v19 has no changes compared to v18, but patch 2 was rebased on top
>> of the recent 1.6T linkmodes.
>>
>> Thanks for everyone's patience and reviews on that work ! Now, the
>> usual blurb for the series description.
> 
> Hopefully we can still make v6.19, but we hooked up Claude Code review
> to patchwork this week, and it points out some legit issues here :(
> Some look transient but others are definitely legit, please look thru
> this:
> 
> https://netdev-ai.bots.linux.dev/ai-review.html?id=5388d317-98c9-458e-8655-d60f31112574

I was told by Paolo to reach out with any feedback on the LLM reviews :

First, I find the whole thing pretty accurate in the reviews, congrats.

I did find one review that's not totally accurate :

https://netdev-ai.bots.linux.dev/ai-review.html?id=5388d317-98c9-458e-8655-d60f31112574#patch-7

Extract from the comment :


--------x8----------------------------------------------------------------------

> @@ -473,89 +473,70 @@ static int mv2222_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>
> -static int mv2222_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
> +static int mv2222_configure_serdes(struct phy_port *port, bool enable,
> +				   phy_interface_t interface)
>  {
> -	struct phy_device *phydev = upstream;
> -	const struct sfp_module_caps *caps;
> -	phy_interface_t sfp_interface;
> +	struct phy_device *phydev = port_phydev(port);
>  	struct mv2222_data *priv;
> -	struct device *dev;
> -	int ret;
> +	int ret = 0;
>
>  	priv = phydev->priv;
> -	dev = &phydev->mdio.dev;
> -
> -	caps = sfp_get_module_caps(phydev->sfp_bus);
> -
> -	phydev->port = caps->port;
> -	sfp_interface = sfp_select_interface(phydev->sfp_bus, caps->link_modes);
> -
> -	dev_info(dev, "%s SFP module inserted\n", phy_modes(sfp_interface));
> +	priv->line_interface = interface;
>
> -	if (sfp_interface != PHY_INTERFACE_MODE_10GBASER &&
> -	    sfp_interface != PHY_INTERFACE_MODE_1000BASEX &&
> -	    sfp_interface != PHY_INTERFACE_MODE_SGMII) {
> -		dev_err(dev, "Incompatible SFP module inserted\n");
> +	if (enable) {
> +		linkmode_and(priv->supported, phydev->supported, port->supported);
>
> -		return -EINVAL;
> -	}
> -
> -	priv->line_interface = sfp_interface;
> -	linkmode_and(priv->supported, phydev->supported, caps->link_modes);
> +		ret = mv2222_config_line(phydev);
> +		if (ret < 0)
> +			return ret;
>
> -	ret = mv2222_config_line(phydev);
> -	if (ret < 0)
> -		return ret;
> +		if (mutex_trylock(&phydev->lock)) {
> +			ret = mv2222_config_aneg(phydev);
> +			mutex_unlock(&phydev->lock);
> +		}
>
> -	if (mutex_trylock(&phydev->lock)) {
> -		ret = mv2222_config_aneg(phydev);
> -		mutex_unlock(&phydev->lock);
> +	} else {
> +		linkmode_zero(priv->supported);
>  	}
>
>  	return ret;
>  }
>
> -static void mv2222_sfp_remove(void *upstream)
> -{
> -	struct phy_device *phydev = upstream;
> -	struct mv2222_data *priv;
> -
> -	priv = phydev->priv;
> -
> -	priv->line_interface = PHY_INTERFACE_MODE_NA;
> -	linkmode_zero(priv->supported);
> -	phydev->port = PORT_NONE;
> -}
             ^^^^

Does priv->line_interface need to be reset when enable is false? The
old mv2222_sfp_remove() explicitly set it to PHY_INTERFACE_MODE_NA,
but the new code leaves it at whatever value was set during the last
module insertion. Functions like mv2222_config_aneg() check whether
priv->line_interface equals PHY_INTERFACE_MODE_NA to determine if a
module is present.

--------x8----------------------------------------------------------------------


Looking at the call-sites, we can see that when the .configure_mii port ops is
called with enabled = false, the interface is always PHY_INTERFACE_MODE_NA.

Looks like the potential problem was identified correctly, but it failed to see
that this can't ever happen. It's a bit tricky I guess, as the call-site in question
is introduced by a previous patch in the same series though.

Maxime