[PATCH net v2 0/5] net: phy: fix cleanup after probe failure

[PATCH net v2 0/5] net: phy: fix cleanup after probe failure

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

phy_probe() initializes the PHY driver, ports, SFP upstream, and LEDs in
stages. Its error paths do not always release only the resources acquired
at each stage or restore the PHY device state.

Port setup also leaves SFP cleanup split between phy_sfp_probe(),
phy_setup_ports(), and phy_probe(), and default port setup ignores errors
from attaching the port to the PHY driver.

This series makes each initialization layer own its cleanup.

Patch 1 splits the phy_probe() cleanup by initialization stage.

Patch 2 makes SFP and port setup unwind their resources in the required
order.

Patch 3 restores the PHY device state after probe failure.

Patch 4 calls the PHY driver remove callback after later probe failures.

Patch 5 propagates errors from default port setup.

---
Changes:
v2:
  Patch 1:
  - Limit this patch to splitting phy_probe() error paths, moving the SFP
    teardown fixes to Patch 2.

  Patch 2 (new):
  - makes SFP and port setup unwind their resources in the required order.
  - Add phy_sfp_release() for complete SFP teardown instead of open-coding
    sfp_bus_del_upstream(). (Andrew Lunn, Maxime Chevallier.)

  Patch 3 (new):
  - Restore PHY_DOWN and clear phydev->drv after probe failure.

  Patch 4:
  - Move the former Patch 2 to Patch 4; no functional changes.

  Patch 5 (new):
  - Propagate errors from default port setup.

v1: https://lore.kernel.org/all/20260812125127.106255-1-xuanqiang.luo@linux.dev/

Xuanqiang Luo (5):
  net: phy: split phy_probe() error paths
  net: phy: unregister SFP upstream before port cleanup
  net: phy: restore device state after probe failure
  net: phy: call driver remove when core initialization fails
  net: phy: propagate errors from default port setup

 drivers/net/phy/phy_device.c | 77 +++++++++++++++++++++++++++---------
 1 file changed, 58 insertions(+), 19 deletions(-)


base-commit: f5bbbfec59b4e2fb7520a91de3df8a6174325d6a
-- 
2.43.0

[PATCH net v2 1/5] 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.

Split the cleanup by initialization stage 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 | 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 0615228459ef4..c9976ffa7128c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3681,7 +3681,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 +3702,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 +3719,7 @@ static int phy_probe(struct device *dev)
 
 	err = phy_setup_ports(phydev);
 	if (err)
-		goto out;
+		goto out_ports;
 
 	phy_advertise_supported(phydev);
 
@@ -3728,7 +3728,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 +3781,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

[PATCH net v2 2/5] net: phy: unregister SFP upstream before port cleanup

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

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")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 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 c9976ffa7128c..9c7ed9c61e6d5 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;
@@ -3522,13 +3551,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);
@@ -3555,7 +3584,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;
 }
@@ -3719,7 +3750,7 @@ static int phy_probe(struct device *dev)
 
 	err = phy_setup_ports(phydev);
 	if (err)
-		goto out_ports;
+		goto out_reset;
 
 	phy_advertise_supported(phydev);
 
@@ -3791,9 +3822,7 @@ static int phy_probe(struct device *dev)
 		phy_led_triggers_unregister(phydev);
 
 out_ports:
-	sfp_bus_del_upstream(phydev->sfp_bus);
-	phydev->sfp_bus = NULL;
-
+	phy_sfp_release(phydev);
 	phy_cleanup_ports(phydev);
 
 out_reset:
@@ -3817,9 +3846,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)
-- 
2.43.0

[PATCH net v2 3/5] net: phy: restore device state after probe failure

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

phy_probe() sets phydev->drv before calling the PHY driver probe
callback, but does not clear it if probing later fails. It also sets
PHY_READY before of_phy_leds(), leaving the state ready if LED setup
fails.

Clear phydev->drv on every error path and restore PHY_DOWN after LED
setup failure.

Fixes: 00db8189d984 ("This patch adds a PHY Abstraction Layer to the Linux Kernel, enabling ethernet drivers to remain as ignorant as is reasonable of the connected PHY's design and operation details.")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/phy_device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9c7ed9c61e6d5..c9e75bd3b81a2 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3821,6 +3821,8 @@ static int phy_probe(struct device *dev)
 	if (!phydev->is_on_sfp_module)
 		phy_led_triggers_unregister(phydev);
 
+	phydev->state = PHY_DOWN;
+
 out_ports:
 	phy_sfp_release(phydev);
 	phy_cleanup_ports(phydev);
@@ -3828,6 +3830,7 @@ static int phy_probe(struct device *dev)
 out_reset:
 	/* Re-assert the reset signal on error */
 	phy_device_reset(phydev, 1);
+	phydev->drv = NULL;
 
 	return err;
 }
-- 
2.43.0

[PATCH net v2 4/5] 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 c9e75bd3b81a2..e429e6d702933 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3733,7 +3733,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))
@@ -3750,7 +3750,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);
 
@@ -3827,6 +3827,10 @@ static int phy_probe(struct device *dev)
 	phy_sfp_release(phydev);
 	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

[PATCH net v2 5/5] net: phy: propagate errors from default port setup

[Xuanqiang Luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

phy_default_setup_single_port() ignores errors from phy_add_port() and
always reports success. If a PHY driver attach_mdi_port() callback fails,
the phy_port is leaked and PHY probing continues without the expected
default port.

Destroy the port and return the error.

Fixes: 589e934d2735 ("net: phy: Introduce PHY ports representation")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/phy_device.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e429e6d702933..b0060b4147348 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3458,6 +3458,7 @@ static int phy_default_setup_single_port(struct phy_device *phydev)
 {
 	struct phy_port *port = phy_port_alloc();
 	unsigned long mode;
+	int ret;
 
 	if (!port)
 		return -ENOMEM;
@@ -3484,9 +3485,11 @@ static int phy_default_setup_single_port(struct phy_device *phydev)
 		port->pairs = max_t(int, port->pairs,
 				    ethtool_linkmode_n_pairs(mode));
 
-	phy_add_port(phydev, port);
+	ret = phy_add_port(phydev, port);
+	if (ret)
+		phy_port_destroy(port);
 
-	return 0;
+	return ret;
 }
 
 static int of_phy_ports(struct phy_device *phydev)
-- 
2.43.0

Re: [PATCH net v2 3/5] net: phy: restore device state after probe failure

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: phy: restore device state after probe failure

phy_probe() sets phydev->drv up front and PHY_READY before calling
of_phy_leds(), but never undoes either if a later step fails. The patch
clears phydev->drv on the common error path and resets the state to
PHY_DOWN when LED setup fails.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 9c7ed9c61e6d5..c9e75bd3b81a2 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3821,6 +3821,8 @@ static int phy_probe(struct device *dev)
>  	if (!phydev->is_on_sfp_module)
>  		phy_led_triggers_unregister(phydev);
>  
> +	phydev->state = PHY_DOWN;
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but since the subject says the
device state is restored after probe failure, is the set of fields being
restored complete?

Earlier in phy_probe() a few fields that describe the hardware are
overwritten based on the matched driver:

	if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
		phydev->irq = PHY_POLL;

	if (phydrv->flags & PHY_IS_INTERNAL)
		phydev->is_internal = true;

and later:

	if (!linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
			       phydev->supported))
		phydev->autoneg = 0;

None of these are undone here. phydev->irq is only ever derived from
bus->irq[addr] in phy_device_create(), and phy_attach_direct() can only
force it to PHY_POLL again:

	if (!phy_drv_supports_irq(phydev->drv) && phy_interrupt_is_valid(phydev))
		phydev->irq = PHY_POLL;

So if a driver's probe fails after those assignments and the same phydev is
later bound again (genphy fallback in phy_attach_direct(), or another module
matching the same phy_id), does the PHY stay stuck in polling mode, keep the
is_internal flag from the failed driver, and keep autoneg disabled?

>  out_ports:
>  	phy_sfp_release(phydev);
>  	phy_cleanup_ports(phydev);
> @@ -3828,6 +3830,7 @@ static int phy_probe(struct device *dev)
>  out_reset:
>  	/* Re-assert the reset signal on error */
>  	phy_device_reset(phydev, 1);
> +	phydev->drv = NULL;

[Severity: Medium]
Can this store race with a concurrent phy_attach_direct() and produce a
NULL dereference?

At this point phydev->mdio.dev.driver is still set: really_probe() only
clears dev->driver later in device_unbind_cleanup(), after the
BUS_NOTIFY_DRIVER_NOT_BOUND notifiers, device_links_no_driver() and
devres_release_all(). Meanwhile phydev->drv is already NULL.

phy_attach_direct() decides on the genphy fallback using the other field:

	if (!d->driver) {
		...
		phydev->is_genphy_driven = 1;
	}

so in that window it skips the fallback probe (nothing repopulates
phydev->drv) and then does:

	if (!phy_drv_supports_irq(phydev->drv) && phy_interrupt_is_valid(phydev))
		phydev->irq = PHY_POLL;

with phy_drv_supports_irq() dereferencing unconditionally:

	static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
	{
		return phydrv->config_intr && phydrv->handle_interrupt;
	}

phy_attach_direct() holds only RTNL, not device_lock() and not phydev->lock,
and phy_probe() takes no phylib lock either. Before this patch the same race
read a stale but non-NULL pointer, so it did not oops.

Related: devres-registered interfaces created by the PHY driver's own probe
(hwmon, PTP) stay live until devres_release_all(), i.e. after this store,
and the paged register helpers dereference phydev->drv unconditionally:

	drivers/net/phy/phy-core.c:__phy_read_page() {
		if (WARN_ONCE(!phydev->drv->read_page,
			      "read_page callback not available, PHY driver not loaded?\n"))
			return -EOPNOTSUPP;

		return phydev->drv->read_page(phydev);
	}

That ordering already exists in phy_remove(), but does this patch newly
extend it to the probe-failure path?

[Severity: Medium]
This is a pre-existing issue and phy_attach_direct() is not touched by this
patch, but while looking at error-path symmetry in phy_probe(): is there any
unwind for a successful phy_probe() followed by a failed
device_bind_driver()?

	drivers/net/phy/phy_device.c:phy_attach_direct() {
		if (phydev->is_genphy_driven) {
			err = d->driver->probe(d);
			if (err >= 0)
				err = device_bind_driver(d);

			if (err)
				goto error_module_put;
		}
	...
	error_module_put:
		module_put(d->driver->owner);
		phydev->is_genphy_driven = 0;
		d->driver = NULL;
	error_put_device:
		put_device(d);
	}

Since the device was never bound, the driver core never calls phy_remove(),
and this path only does module_put()/put_device(). Does that leak the
kmalloc'ed phy->led_link_trigger and phy->phy_led_triggers[] objects that
phy_led_triggers_register() published into the global LED trigger list, plus
the phy_port objects from phy_setup_ports() and any SFP upstream
registration, while drv->remove() is never called?

phydev->drv and PHY_READY are also left set for an unbound device, and a
later attach re-runs phy_probe() which overwrites phy->phy_led_triggers and
phy->led_link_trigger. Would that orphan the earlier registrations in the
global trigger list?

>  
>  	return err;
>  }
-- 
pw-bot: cr

Re: [PATCH net v2 1/5] net: phy: split phy_probe() error paths

[Jakub Kicinski]

On Thu, 13 Aug 2026 21:29:41 +0800 Xuanqiang Luo wrote:
> +out_led_triggers:

the labels should include the verb

out_unreg_led_triggers:

> +	if (!phydev->is_on_sfp_module)
> +		phy_led_triggers_unregister(phydev);
> +
> +out_ports:

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:

this one is fine

>  	/* Re-assert the reset signal on error */
>  	phy_device_reset(phydev, 1);