Re: [PATCH net v5 1/6] net: phy: split phy_probe() error paths

[Maxime Chevallier]

Hi,

On 8/23/26 05:55, 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.
> 
> After a successful probe and remove, phy_led_triggers_unregister() can
> leave phy_num_led_triggers non-zero after freeing the trigger array. If a
> subsequent probe fails before LED trigger registration, the common error
> path calls phy_led_triggers_unregister() with a NULL array and stale count,
> causing a NULL dereference.
> 
> Split the cleanup by initialization stage so each failure path unwinds only
> the resources that may have been initialized. Unregister LED triggers
> before releasing the SFP upstream and ports, because the LED triggers are
> initialized after those resources and must be unwound first.
> 
> Fixes: c8dbdc6e380e ("net: phy: register phy led_triggers during probe to avoid AB-BA deadlock")
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>

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

Maxime

> ---
>  drivers/net/phy/phy_device.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..2cf70471ae089 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3706,7 +3706,7 @@ static int phy_probe(struct device *dev)
>  	if (phydev->drv->probe) {
>  		err = phydev->drv->probe(phydev);
>  		if (err)
> -			goto out;
> +			goto out_reset;
>  	}
>  
>  	phy_disable_interrupts(phydev);
> @@ -3727,7 +3727,7 @@ static int phy_probe(struct device *dev)
>  		err = genphy_read_abilities(phydev);
>  
>  	if (err)
> -		goto out;
> +		goto out_reset;
>  
>  	if (!linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
>  			       phydev->supported))
> @@ -3744,7 +3744,7 @@ static int phy_probe(struct device *dev)
>  
>  	err = phy_setup_ports(phydev);
>  	if (err)
> -		goto out;
> +		goto out_sfp_release;
>  
>  	phy_advertise_supported(phydev);
>  
> @@ -3753,7 +3753,7 @@ static int phy_probe(struct device *dev)
>  	 */
>  	err = genphy_c45_read_eee_adv(phydev, phydev->advertising_eee);
>  	if (err)
> -		goto out;
> +		goto out_sfp_release;
>  
>  	/* Get the EEE modes we want to prohibit. */
>  	of_set_phy_eee_broken(phydev);
> @@ -3806,20 +3806,22 @@ static int phy_probe(struct device *dev)
>  	if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev)) {
>  		err = of_phy_leds(phydev);
>  		if (err)
> -			goto out;
> +			goto out_unreg_led_triggers;
>  	}
>  
>  	return 0;
>  
> -out:
> +out_unreg_led_triggers:
> +	if (!phydev->is_on_sfp_module)
> +		phy_led_triggers_unregister(phydev);
> +
> +out_sfp_release:
>  	sfp_bus_del_upstream(phydev->sfp_bus);
>  	phydev->sfp_bus = NULL;
>  
>  	phy_cleanup_ports(phydev);
>  
> -	if (!phydev->is_on_sfp_module)
> -		phy_led_triggers_unregister(phydev);
> -
> +out_reset:
>  	/* Re-assert the reset signal on error */
>  	phy_device_reset(phydev, 1);
>