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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

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