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

[Paolo Abeni <pabeni@redhat.com>]

On 3/12/26 11:47 AM, Paolo Abeni wrote:
> 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?

Out of sheer ignorance IDK if phy_sfp_module_start() could really happen
in practice after a phy_sfp_connect_phy() failure, but the inconsistency
is likely worthy a cleanup?

Leaving the series alive in PW, in case the phy crew disagree or a
follow-up is preferred to a repost.

/P