Re: [PATCH net v5 2/6] net: phy: unregister SFP upstream before port cleanup

[Maxime Chevallier]

Hi,

On 8/23/26 05:55, Xuanqiang Luo wrote:
> From: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>
> 
> Commit 4497f5028675 ("net: phy: Clean the phy_ports after unregistering
> the downstream SFP bus") established that an SFP upstream must be
> unregistered before its phy_ports are destroyed because SFP callbacks
> may access these ports.
> 
> phy_setup_ports() does not follow this order when a later port setup
> step fails after phy_sfp_probe() succeeds. It destroys the SFP phy_port
> and leaves phy_probe() to unregister the upstream later, creating a race
> between port destruction and SFP upstream callbacks.
> 
> The error unwind is also split across three functions. If
> phy_setup_sfp_port() fails, phy_sfp_probe() leaves the upstream
> registered and relies on phy_probe() to remove it after
> phy_setup_ports() returns.
> 
> Make each layer unwind the resources it successfully set up. Unregister
> only the upstream in phy_sfp_probe() when SFP port setup fails, since
> the failed port has already been destroyed. Add phy_sfp_release() for a
> successful SFP probe, and make phy_setup_ports() use it before cleaning
> up the remaining ports. Once phy_setup_ports() has rolled back all port
> setup, make phy_probe() skip this cleanup.
> 
> Fixes: 589e934d2735 ("net: phy: Introduce PHY ports representation")
> Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>

Thanks for this cleanup,

Reviewed-by: Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx>

Maxime

> ---
>  drivers/net/phy/phy_device.c | 49 ++++++++++++++++++++++++++++--------
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 2cf70471ae089..4b9b2300422fb 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1723,12 +1723,41 @@ static int phy_sfp_probe(struct phy_device *phydev)
>  			phydev->sfp_bus = NULL;
>  	}
>  
> -	if (!ret && phydev->sfp_bus)
> +	if (!ret && phydev->sfp_bus) {
>  		ret = phy_setup_sfp_port(phydev);
> +		if (ret) {
> +			sfp_bus_del_upstream(phydev->sfp_bus);
> +			phydev->sfp_bus = NULL;
> +		}
> +	}
>  
>  	return ret;
>  }
>  
> +/**
> + * phy_sfp_release - release resources set up by phy_sfp_probe()
> + * @phydev: the PHY device
> + *
> + * Release the SFP resources set up by a successful phy_sfp_probe(). Unregister
> + * the upstream before destroying its phy_port, so SFP upstream callbacks cannot
> + * race with port destruction.
> + */
> +static void phy_sfp_release(struct phy_device *phydev)
> +{
> +	struct phy_port *port, *tmp;
> +
> +	sfp_bus_del_upstream(phydev->sfp_bus);
> +	phydev->sfp_bus = NULL;
> +
> +	list_for_each_entry_safe(port, tmp, &phydev->ports, head) {
> +		if (!port->is_sfp)
> +			continue;
> +
> +		phy_del_port(phydev, port);
> +		phy_port_destroy(port);
> +	}
> +}
> +
>  static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
>  {
>  	return phydrv->config_intr && phydrv->handle_interrupt;
> @@ -3547,13 +3576,13 @@ static int phy_setup_ports(struct phy_device *phydev)
>  	if (!phydev->is_genphy_driven) {
>  		ret = phy_sfp_probe(phydev);
>  		if (ret)
> -			goto out;
> +			goto err_ports;
>  	}
>  
>  	if (phydev->n_ports < phydev->max_n_ports) {
>  		ret = phy_default_setup_single_port(phydev);
>  		if (ret)
> -			goto out;
> +			goto err_sfp;
>  	}
>  
>  	linkmode_zero(ports_supported);
> @@ -3580,7 +3609,9 @@ static int phy_setup_ports(struct phy_device *phydev)
>  
>  	return 0;
>  
> -out:
> +err_sfp:
> +	phy_sfp_release(phydev);
> +err_ports:
>  	phy_cleanup_ports(phydev);
>  	return ret;
>  }
> @@ -3744,7 +3775,7 @@ static int phy_probe(struct device *dev)
>  
>  	err = phy_setup_ports(phydev);
>  	if (err)
> -		goto out_sfp_release;
> +		goto out_reset;
>  
>  	phy_advertise_supported(phydev);
>  
> @@ -3816,9 +3847,7 @@ static int phy_probe(struct device *dev)
>  		phy_led_triggers_unregister(phydev);
>  
>  out_sfp_release:
> -	sfp_bus_del_upstream(phydev->sfp_bus);
> -	phydev->sfp_bus = NULL;
> -
> +	phy_sfp_release(phydev);
>  	phy_cleanup_ports(phydev);
>  
>  out_reset:
> @@ -3842,9 +3871,7 @@ static int phy_remove(struct device *dev)
>  
>  	phydev->state = PHY_DOWN;
>  
> -	sfp_bus_del_upstream(phydev->sfp_bus);
> -	phydev->sfp_bus = NULL;
> -
> +	phy_sfp_release(phydev);
>  	phy_cleanup_ports(phydev);
>  
>  	if (phydev->drv && phydev->drv->remove)