[PATCH net v1 0/2] net: phy: fix driver cleanup after probe failure

[PATCH net v1 0/2] net: phy: fix driver cleanup after probe failure

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

Patch 1 splits the phy_probe() error paths and makes phy_setup_ports()
unwind its own resources.

Patch 2 calls the PHY driver remove callback when phy_probe() fails after
the driver probe callback succeeds.

Xuanqiang Luo (2):
  net: phy: split phy_probe() error paths
  net: phy: call driver remove when core initialization fails

 drivers/net/phy/phy_device.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)


base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
-- 
2.43.0

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

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

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@kylinos.cn>
---
 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;
 }
@@ -3681,7 +3684,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);
@@ -3702,7 +3705,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))
@@ -3719,7 +3722,7 @@ static int phy_probe(struct device *dev)
 
 	err = phy_setup_ports(phydev);
 	if (err)
-		goto out;
+		goto out_reset;
 
 	phy_advertise_supported(phydev);
 
@@ -3728,7 +3731,7 @@ static int phy_probe(struct device *dev)
 	 */
 	err = genphy_c45_read_eee_adv(phydev, phydev->advertising_eee);
 	if (err)
-		goto out;
+		goto out_ports;
 
 	/* Get the EEE modes we want to prohibit. */
 	of_set_phy_eee_broken(phydev);
@@ -3781,20 +3784,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_led_triggers;
 	}
 
 	return 0;
 
-out:
+out_led_triggers:
+	if (!phydev->is_on_sfp_module)
+		phy_led_triggers_unregister(phydev);
+
+out_ports:
 	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);
 
-- 
2.43.0

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@kylinos.cn>
> 
> 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@kylinos.cn>
> ---
>  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

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

[Maxime Chevallier]

Hi,

On 8/12/26 15:41, Andrew Lunn wrote:
> On Wed, Aug 12, 2026 at 08:51:26PM +0800, Xuanqiang Luo wrote:
>> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>>
>> 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@kylinos.cn>
>> ---
>>  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().

I agree with having phy_sfp_release(), we have even more to do with phy SFP
in the future with phy_port, this is less error prone.

> 
> 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.

In practise, this is cleaned in the phy_probe's "out" failure label,
but indeed this is not pretty.

Maxime

[PATCH net v1 2/2] net: phy: call driver remove when core initialization fails

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

phy_probe() may fail while querying features or completing other core
initialization after the PHY driver probe callback has succeeded. The
driver core does not run the remove path after a probe error, so
resources that the PHY driver releases in its remove callback are
leaked.

Call the PHY driver remove callback on these failures.

Fixes: efbdfdc29bdd ("net: phy: Add support for asking the PHY its abilities")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/phy_device.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f8e434daab66e..be714758360b2 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3705,7 +3705,7 @@ static int phy_probe(struct device *dev)
 		err = genphy_read_abilities(phydev);
 
 	if (err)
-		goto out_reset;
+		goto out_remove;
 
 	if (!linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
 			       phydev->supported))
@@ -3722,7 +3722,7 @@ static int phy_probe(struct device *dev)
 
 	err = phy_setup_ports(phydev);
 	if (err)
-		goto out_reset;
+		goto out_remove;
 
 	phy_advertise_supported(phydev);
 
@@ -3799,6 +3799,10 @@ static int phy_probe(struct device *dev)
 
 	phy_cleanup_ports(phydev);
 
+out_remove:
+	if (phydev->drv->remove)
+		phydev->drv->remove(phydev);
+
 out_reset:
 	/* Re-assert the reset signal on error */
 	phy_device_reset(phydev, 1);
-- 
2.43.0