Re: [net-next,v6,07/10] net: phy: phy_port: Store information about a MII port's vacant state

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

Hi

On 06/03/2026 15:03, Simon Horman 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: phy_port: Store information about a MII port's vacant state
> 
> This patch adds a 'vacant' flag to phy_port to track whether an MII port
> currently has an SFP module installed. The flag is set to true when the
> port is created or when a module is removed, and set to false when a
> module is successfully inserted.
> 
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 8bff64040540..5d26d75b5f6b 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1550,6 +1550,7 @@ static int phy_sfp_module_insert(void *upstream, const struct sfp_eeprom_id *id)
>>  	struct phy_device *phydev = upstream;
>>  	const struct sfp_module_caps *caps;
>>  	struct phy_port *port;
>> +	int ret = 0;
>>
>>  	phy_interface_t iface;
>>
>> @@ -1578,9 +1579,12 @@ static int phy_sfp_module_insert(void *upstream, const struct sfp_eeprom_id *id)
>>  		phydev->port = caps->port;
>>
>>  	if (port->ops && port->ops->configure_mii)
>> -		return port->ops->configure_mii(port, true, iface);
>> +		ret = port->ops->configure_mii(port, true, iface);
>>
>> -	return 0;
>> +	if (!ret)
>> +		port->vacant = false;
>> +
>> +	return ret;
>>  }
> 
> [ ... ]
> 
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index 79f7811dec22..b575f5db9ed6 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
> 
> [ ... ]
> 
>> @@ -3917,6 +3918,9 @@ static int phylink_sfp_module_insert(void *upstream,
>>  	pl->sfp_may_have_phy = caps->may_have_phy;
>>  	pl->sfp_port = caps->port;
>>
>> +	if (pl->sfp_bus_port)
>> +		pl->sfp_bus_port->vacant = false;
>> +
>>  	/* If this module may have a PHY connecting later, defer until later */
>>  	if (pl->sfp_may_have_phy)
>>  		return 0;
> 
> Can the 'vacant' flag remain incorrectly set to false when the module is
> removed?
> 
> In phylink_sfp_module_insert(), the vacant flag is set to false before
> phylink_sfp_config_optical() is called. If phylink_sfp_config_optical()
> returns an error (for example, no common interface between MAC and SFP
> module), the error propagates back to sfp_sm_module(), which transitions
> the state machine to SFP_MOD_ERROR (enum value 1).
> 
> When the module is later physically removed, sfp_sm_mod_remove() checks
> 'sfp->sm_mod_state > SFP_MOD_WAITDEV' (enum value 3). Since SFP_MOD_ERROR
> is 1, which is less than 3, sfp_module_remove() is NOT called. This means
> phylink_sfp_module_remove() never executes and 'vacant' remains false even
> though the module was physically removed.
> 
> The PHY-side function phy_sfp_module_insert() in phy_device.c avoids this
> by gating the assignment:
> 
>     if (!ret)
>         port->vacant = false;
> 
> Should phylink_sfp_module_insert() use the same pattern, setting vacant to
> false only after phylink_sfp_config_optical() succeeds?

I'll rather reset-it upon failure, but that's correct and I tested this
error case to be indeed showing-up.

I'll address for the next revision

Maxime