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

[Simon Horman]

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?