Re: [net-next,v7,05/10] net: phy: Represent PHY-less SFP modules with phy_port

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: phy: Represent PHY-less SFP modules with phy_port

Now that the SFP bus infrastructure notifies when PHY-less modules are
connected, we can create a phy_port to represent it. Instead of letting
the SFP subsystem handle that, the Bus' upstream is in charge of
maintaining that phy_port and register it to the topology, as the
upstream (in this case a phy device) is directly interacting with the
underlying net_device.

Add a phy_caps helper to get the achievable modes on this module based
on what the phy_port representing the bus supports.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 4b06644866b2..faa58a780477 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1483,6 +1483,8 @@ static int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
>  	struct phy_device *phydev = upstream;
>  	struct net_device *dev = phydev->attached_dev;
>
> +	phydev->has_sfp_mod_phy = true;
> +
>  	if (dev)
>  		return phy_link_topo_add_phy(dev, phy, PHY_UPSTREAM_PHY, phydev);

If phy_link_topo_add_phy() fails, does has_sfp_mod_phy get cleaned up
correctly?

Looking at the SFP framework, when phy_link_topo_add_phy() fails here,
sfp_sm_probe_phy() in sfp.c will free the PHY but won't call
disconnect_phy (because sfp->mod_phy was not set). This leaves
has_sfp_mod_phy stuck as true.

On a subsequent SFP module insertion, phy_sfp_module_start() checks
!phydev->has_sfp_mod_phy:

drivers/net/phy/phy_device.c:phy_sfp_module_start() {
	if (!phydev->has_sfp_mod_phy)
		return phy_add_sfp_mod_port(phydev);
}

Seeing has_sfp_mod_phy is true (stale), it skips creating the mod_port
for a PHY-less module. The module then won't be represented in the link
topology.

Would it be better to either set has_sfp_mod_phy after
phy_link_topo_add_phy() succeeds, or clear it in an error path?