Re: [PATCH net v1 1/2] net: phy: split phy_probe() error paths

[Andrew Lunn]

On Wed, Aug 12, 2026 at 08:51:26PM +0800, Xuanqiang Luo wrote:
> From: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>
> 
> phy_probe() uses one cleanup path for failures at every initialization
> stage. This runs cleanup for resources that have not been initialized
> and leaves phy_setup_ports() relying on its caller to remove an SFP
> upstream after a partial failure.
> 
> Make phy_setup_ports() unwind the SFP upstream before removing its ports.
> Then split the phy_probe() cleanup so each failure path unwinds only the
> resources that may have been initialized.
> 
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>
> ---
>  drivers/net/phy/phy_device.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 0615228459ef4..f8e434daab66e 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3556,6 +3556,9 @@ static int phy_setup_ports(struct phy_device *phydev)
>  	return 0;
>  
>  out:
> +	sfp_bus_del_upstream(phydev->sfp_bus);
> +	phydev->sfp_bus = NULL;
> +
>  	phy_cleanup_ports(phydev);
>  	return ret;

This does not look correct. phy_sfp_probe() may fail, and you then
call sfp_bus_del_upstream() on something which never happened.

Also, it is not obvious that sfp_bus_del_upstream() is the correct
thing to do. You are trying to undo phy_sfp_probe() so i would expect
you to call a function like phy_sfp_release().

It also looks like phy_sfp_probe() does not correctly clean up on
phy_setup_sfp_port() returning an error. But that is a different
issue.

    Andrew

---
pw-bot: cr