[PATCH net-next v5 0/4] net: phy: make PHY driver unbind safe against attach and use

[PATCH net-next v5 0/4] net: phy: make PHY driver unbind safe against attach and use

[Aleksei Sviridkin]

Unbinding a PHY driver through sysfs while its MAC brings the port up
can oops. On a Keenetic KN-1012 (MT7981, mtk_eth_soc), an unbind of the
wan PHY driver racing "ip link set wan up" faulted on the first attempt,
with no delay added anywhere. The PHY driver's config_init read
phydev->drv after phy_remove() had cleared it. Patch 3 has the trace.

Serialising attach against unbind is not enough on its own. The unbind
that loses the race then removes the driver from a PHY that is now
attached, and the consumer faults one step later. A DSA port gets there
without any race, because DSA keeps its PHYs attached from switch setup
to teardown. Patch 4 has both cases.

1: refuse a second attach of an attached PHY up front.
2: put the module reference phy_attach_direct() took, not whatever
   driver is bound at detach time.
3: a per-PHY mutex and a "bound" flag. An attach either is done with the
   driver before phy_remove() starts tearing it down, or is refused with
   -EAGAIN.
4: phy_remove() waits for phy_detach() when the PHY is attached, except
   when the PHY device itself is being deleted.

1 to 3 do not need 4, but without it the winner of the race still
ends up with a PHY whose driver is gone.

On the lock inversion (Paolo): phy_probe() and phy_remove() take rtnl
under the device lock (SFP bus, netdev LED trigger), and attach may run
under rtnl, so attach cannot take the device lock. Nothing under the
new mutex takes rtnl.

Vladimir, patch 4 falls short of what you asked for in [1]: that
unbinding a PHY driver should not "explode ... even in uncontrolled
situations where the netdev isn't carefully disconnected from the PHY
first". It makes phy_remove() wait for the detach, so the unbind blocks
until the consumer lets go. For DSA that is switch teardown. Other
options, and why not:
- stop at patch 3: the oops moves one frame up;
- the caller holds the lock across bringup: widens into phylink,
  leaves every later use;
- a killable wait: ->remove cannot fail, so a kill ends in the oops;
- suppress_bind_attrs: removes the unbind your use case relies on;
- a managed device link MAC -> PHY: takes the MAC or switch down too.

The wait has costs. The hung-task detector reports the blocked unbind.
Suspend waits behind it and reboot hangs, because both take the PHY's
device lock. Patch 4 names a device-link case where the wait never ends.

Attach still binds the generic driver without the device lock, so a
real driver binding at the same moment can collide with it. Two
consumers attaching and detaching one PHY at once are unsupported: the
end of a detach can still reset the other's PHY, and with patch 4 its
generic driver release waits for it (a deadlock if both hold rtnl).

Tested on a KN-1012 (OpenWrt 6.18 backport, PROVE_LOCKING, hung-task
panic) with lan4 holding its EN8811H. The image carried 03101a02b2fa,
which differs from this posting only in the patch 4 message, one
comment and the WRITE_ONCE() on removing.
- second attach of lan4's PHY, with or without a netdev: -EBUSY
- air_en8811h unbind under lan4: waits, returns on switch unbind
- free PHY: attach 0, again -EBUSY, refcount 0/1/1/0; on genphy too
- irq back to the bus number after each generic-driver detach
- two attaches racing without rtnl, 200 rounds: one winner each time
- unbind of an attached PHY waits for detach, with both drivers
- phy_device_remove() releases a waiting unbind
Lockdep stayed quiet. The one splat was a known kernfs warning from the
DSA teardown order.

[1] https://lore.kernel.org/netdev/20260311203410.rio7m6nuf72hs5p6@skbuf/

Changes in v5 (since v4):
https://lore.kernel.org/netdev/20261001130120.104628-1-f@lex.la/
- Rebased on the applied phylib interrupt series. Patch 4 moves its
  detach-side restore ahead of the point where a new attach can start.
- Patches 1 and 3: the "already attached" test runs under bind_lock.
- Patches 3 and 4: detach clears phy_link_change, the recorded module
  and the attached flag in one bind_lock section.
- Patch 2: the driver module is read once.
- Patch 4: READ_ONCE()/WRITE_ONCE() on attached, WRITE_ONCE() on
  removing.
- Messages: patch 1 on the generic driver, patch 3 on its exception
  and -EAGAIN at probe time, patch 4 on the reboot hang and the
  device-link case.
- The bind_lock kernel-doc names phy_probe() and phy_remove().

Changes in v4 (since v3):
https://lore.kernel.org/netdev/20260924215951.2127682-1-f@lex.la/
- Retargeted to net-next, as asked.
- Patch 1: the test also catches a PHY attached without a netdev.
- Patch 3: a Return: section for __phy_probe(). The forward declaration
  stays, see the v4 notes of patch 3.
- Patch 4: a line over 80 columns wrapped.

Changes in v3 (since v2):
https://lore.kernel.org/netdev/20260919015340.499675-1-f@lex.la/
- Retargeted to net. The race is closed with locking instead of a NULL
  test, as asked in review.
- Patch 2 split out. New patch 1 (no detach of the first consumer on a
  second attach) and patch 4 (the unbind waits).

Aleksei Sviridkin (4):
  net: phy: refuse a second attach before touching the PHY
  net: phy: put the driver module the attach took
  net: phy: serialise attach and detach with PHY driver bind and unbind
  net: phy: make an unbind wait for the attached consumer to detach

 drivers/net/phy/phy_device.c | 123 +++++++++++++++++++++++++++++------
 include/linux/phy.h          |  12 ++++
 2 files changed, 116 insertions(+), 19 deletions(-)

-- 
2.53.0

[PATCH net-next v5 1/4] net: phy: refuse a second attach before touching the PHY

[Aleksei Sviridkin]

phy_attach_direct() tests phydev->attached_dev only after it has taken
the driver module and, for a PHY with no driver, bound the generic one,
and then leaves through the error path that calls
phy_detach_internal(). That call works on the PHY's current attachment,
so a second attach of a PHY in use tears down the first consumer: it
clears that netdev's phydev pointer and the PHY's attached_dev, removes
the sysfs links and suspends the PHY, while the first consumer goes on
using it. A PHY on the generic driver never reaches the test: the
attach runs its probe again and, when the bind fails, clears d->driver
under the first consumer.

Found in review of a change that makes phy_detach() put only the
module the attach recorded: with it, this path leaves the first
consumer's reference pinned. On a Keenetic KN-1012, a test module
attaching a second netdev to the EN8811H that the lan4 DSA port holds
got -EBUSY and left lan4 with no PHY; with this change lan4 kept it.

Test whether the PHY is attached before the driver module is taken or
the generic driver bound, and refuse from there. A PHY attached without
a netdev, as DSA does for its CPU and link ports, has no attached_dev,
so test phy_link_change, which every attach sets and every detach
clears. A second attach of such a PHY used to succeed and share it;
with that change it would also leak a module reference.

Fixes: a7dac9f9c169 ("phy: fix error case of phy_led_triggers_(un)register")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
Changes in v5:
- The test moved below the device and bus references; patch 3 puts it
  under bind_lock.
- The message says what a second attach did to a PHY on the generic
  driver.

 drivers/net/phy/phy_device.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index bfce8b893644..6bc9cdb7fa69 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1811,6 +1811,13 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	get_device(d);
 
+	/* Set by every attach, with or without a netdev */
+	if (phydev->phy_link_change) {
+		phydev_err(phydev, "PHY already attached\n");
+		err = -EBUSY;
+		goto error_put_device;
+	}
+
 	/* Assume that if there is no driver, that it doesn't
 	 * exist, and we should use the genphy driver.
 	 */
@@ -1838,12 +1845,6 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 			goto error_module_put;
 	}
 
-	if (phydev->attached_dev) {
-		dev_err(&dev->dev, "PHY already attached\n");
-		err = -EBUSY;
-		goto error;
-	}
-
 	phydev->phy_link_change = phy_link_change;
 	if (dev) {
 		phydev->attached_dev = dev;
-- 
2.53.0

[PATCH net-next v5 2/4] net: phy: put the driver module the attach took

[Aleksei Sviridkin]

phy_attach_direct() pins the PHY driver module through d->driver, and
phy_detach() releases it by reading d->driver again. Unbinding the PHY
driver while the PHY is attached clears that pointer, so a detach that
runs while the driver is still unbound skips the put and the module can
no longer be unloaded; if a different driver binds in between, the put
lands on a module that was never pinned. The NULL test added by
commit c2b727df7caa ("net: phy: Avoid NPD upon phy_detach() when driver
is unbound") avoids the oops but skips the put.

Found by reading phy_detach() while chasing a PHY driver unbind race on
a Keenetic KN-1012 (MT7981, air_en8811h built as a module). The leak
itself was not observed there: unbinding air_en8811h under the attached
lan4 DSA port and then unbinding the switch faults earlier on that
board, in phy_free_interrupt() or under phy_stop(), before
phy_detach() gets to the put.

Remember which module was pinned and release that one.

Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY driver")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
Changes in v5:
- The driver module is read once, so the module pinned and the module
  recorded cannot differ.
- Rebased on net-next: the failed generic bind unwind keeps the
  interrupt restore it got there.
- On its own, this patch can still leak a reference when one consumer
  attaches while another detaches the same PHY. Patch 3 closes that.

 drivers/net/phy/phy_device.c | 12 ++++++++----
 include/linux/phy.h          |  2 ++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6bc9cdb7fa69..68691c487eee 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1728,8 +1728,8 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
 	phydev->phy_link_change = NULL;
 	phydev->phylink = NULL;
 
-	if (phydev->mdio.dev.driver)
-		module_put(phydev->mdio.dev.driver->owner);
+	module_put(phydev->drv_owner);
+	phydev->drv_owner = NULL;
 
 	/* If the device had no specific driver before (i.e. - it
 	 * was using the generic driver), we unbind the device
@@ -1794,6 +1794,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	struct mii_bus *bus = phydev->mdio.bus;
 	struct device *d = &phydev->mdio.dev;
 	struct module *ndev_owner = NULL;
+	struct module *drv_owner;
 	int irq = phydev->irq;
 	int err;
 
@@ -1830,11 +1831,13 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		phydev->is_genphy_driven = 1;
 	}
 
-	if (!try_module_get(d->driver->owner)) {
+	drv_owner = d->driver->owner;
+	if (!try_module_get(drv_owner)) {
 		phydev_err(phydev, "failed to get the device driver module\n");
 		err = -EIO;
 		goto error_put_device;
 	}
+	phydev->drv_owner = drv_owner;
 
 	if (phydev->is_genphy_driven) {
 		err = d->driver->probe(d);
@@ -1942,7 +1945,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	return err;
 
 error_module_put:
-	module_put(d->driver->owner);
+	module_put(phydev->drv_owner);
+	phydev->drv_owner = NULL;
 	phydev->irq = irq;
 	phydev->is_genphy_driven = 0;
 	d->driver = NULL;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7c5098a0dd6c..a5a419bc400e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -578,6 +578,7 @@ struct phy_oatc14_sqi_capability {
  *
  * @mdio: MDIO bus this PHY is on
  * @drv: Pointer to the driver for this PHY instance
+ * @drv_owner: Driver module phy_attach_direct() took a reference on
  * @devlink: Create a link between phy dev and mac dev, if the external phy
  *           used by current mac interface is managed by another mac interface.
  * @phyindex: Unique id across the phy's parent tree of phys to address the PHY
@@ -689,6 +690,7 @@ struct phy_device {
 	/* Information about the PHY type */
 	/* And management functions */
 	const struct phy_driver *drv;
+	struct module *drv_owner;
 
 	struct device_link *devlink;
 
-- 
2.53.0

[PATCH net-next v5 3/4] net: phy: serialise attach and detach with PHY driver bind and unbind

[Aleksei Sviridkin]

phy_attach_direct() reads phydev->drv and d->driver with nothing held
against the driver core, then calls into the driver through
phy_init_hw() and phy_resume(). An unbind that runs during an attach
can remove the driver under it, and an attach that runs while a driver
is still probing can call config_init before the driver's probe has
finished.

Found on a Keenetic KN-1012 (MT7981) while testing how a port copes
with its PHY driver coming and going, on an OpenWrt 6.18 kernel.
Unbinding the wan PHY driver through sysfs while "ip link set wan up"
ran oopsed on the first attempt, with nothing added to widen the
window, inside the driver's own calibration code:

  Unable to handle kernel access to user memory outside uaccess
  routines at virtual address 0000000000000098
  Comm: ip
  Call trace:
   tx_amp_fill_result.isra.0+0x90/0x3a0 (P)
   mt798x_phy_calibration+0x164/0x520
   mt798x_phy_config_init+0x470/0x6f8
   phy_init_hw+0x64/0xa0
   phy_attach_direct+0x184/0x380
   phylink_fwnode_phy_connect+0x198/0x27c
   phylink_of_phy_connect+0x18/0x20
   mtk_open+0x38/0xb70
   __dev_open+0xf8/0x1e0

phy_remove() had cleared phydev->drv while config_init was running,
and the driver read phydev->drv->phy_id. No NULL test in phylib reaches
that dereference.

The device lock cannot be taken here: attach may run under rtnl, while
phy_probe() and phy_remove() take rtnl under the device lock, through
the SFP bus of a PHY with a cage and through the netdev LED trigger.
Add a per-PHY mutex and a flag saying that a driver has finished
probing and is not being removed. The driver-core callbacks take the
mutex only to flip the flag; attach and detach hold it while they call
into the driver. An attach that finds a driver bound but the flag clear
refuses with -EAGAIN; a MAC or switch that attaches from its own probe
then fails that probe rather than call into a half-probed driver. The
"already attached" test moves under the mutex too, so two attaches of
one PHY cannot both pass it. Detach clears phy_link_change and takes
the recorded module in one section under it, so an attach that passes
the test right after cannot lose its module reference to that detach.

phy_remove() clears the flag before any teardown, so the rest of it can
run without the mutex: an attach that loses the race is refused, and
one that wins holds the mutex until it is done with the driver. The
unbind then goes on to remove the driver from the PHY that attach just
attached; what the consumer does with it after that is not changed
here. The generic driver fallback is not covered either: attach binds
it without the device lock, so a real driver binding through the
driver core at the same moment can still collide with it.

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.")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
Changes in v5:
- The "already attached" test from patch 1 runs under bind_lock and
  leaves through error_unlock.
- The message names the generic-driver exception and the -EAGAIN a
  consumer probing at that moment gets.
- The bind_lock kernel-doc names phy_probe() and phy_remove().
- Detach clears phy_link_change and takes the recorded module in one
  section under bind_lock, and puts the module it took there.

 drivers/net/phy/phy_device.c | 72 +++++++++++++++++++++++++++++++-----
 include/linux/phy.h          |  6 +++
 2 files changed, 68 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 68691c487eee..0ffaa456a308 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -711,6 +711,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	dev->max_n_ports = 1;
 
 	mutex_init(&dev->lock);
+	mutex_init(&dev->bind_lock);
 	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
 
 	/* Request the appropriate module unconditionally; don't
@@ -1668,6 +1669,8 @@ static void phy_sfp_release(struct phy_device *phydev)
 	}
 }
 
+static int __phy_probe(struct device *dev);
+
 static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
 {
 	return phydrv->config_intr && phydrv->handle_interrupt;
@@ -1685,6 +1688,7 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
 {
 	struct net_device *dev = phydev->attached_dev;
 	struct module *ndev_owner = NULL;
+	struct module *drv_owner;
 	struct mii_bus *bus;
 
 	if (phydev->devlink) {
@@ -1702,7 +1706,10 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
 		sysfs_remove_file(&phydev->mdio.dev.kobj,
 				  &dev_attr_phy_standalone.attr);
 
-	phy_suspend(phydev);
+	mutex_lock(&phydev->bind_lock);
+	if (phydev->bound)
+		phy_suspend(phydev);
+	mutex_unlock(&phydev->bind_lock);
 
 	if (notify_bus && phydev->mdio.bus->notify_phy_detach)
 		phydev->mdio.bus->notify_phy_detach(phydev);
@@ -1725,11 +1732,18 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
 		phy_link_topo_del_phy(dev, phydev);
 	}
 
-	phydev->phy_link_change = NULL;
 	phydev->phylink = NULL;
 
-	module_put(phydev->drv_owner);
+	/* A new attach may pass its "already attached" test as soon as
+	 * phy_link_change is clear, so take the owner in the same section.
+	 */
+	mutex_lock(&phydev->bind_lock);
+	phydev->phy_link_change = NULL;
+	drv_owner = phydev->drv_owner;
 	phydev->drv_owner = NULL;
+	mutex_unlock(&phydev->bind_lock);
+
+	module_put(drv_owner);
 
 	/* If the device had no specific driver before (i.e. - it
 	 * was using the generic driver), we unbind the device
@@ -1782,11 +1796,15 @@ EXPORT_SYMBOL(phy_detach);
  *
  * Description: Called by drivers to attach to a particular PHY
  *     device. The phy_device is found, and properly hooked up
- *     to the phy_driver.  If no driver is attached, then a
+ *     to the phy_driver.  If no driver is bound, then a
  *     generic driver is used.  The phy_device is given a ptr to
  *     the attaching device, and given a callback for link status
  *     change.  The phy_device is returned to the attaching driver.
  *     This function takes a reference on the phy device.
+ *
+ * Return: 0 on success, -EAGAIN if a driver is being bound to or
+ * unbound from the PHY, -EBUSY if the PHY is already attached, or
+ * another negative error code.
  */
 int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		      u32 flags, phy_interface_t interface)
@@ -1812,11 +1830,13 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	get_device(d);
 
+	mutex_lock(&phydev->bind_lock);
+
 	/* Set by every attach, with or without a netdev */
 	if (phydev->phy_link_change) {
 		phydev_err(phydev, "PHY already attached\n");
 		err = -EBUSY;
-		goto error_put_device;
+		goto error_unlock;
 	}
 
 	/* Assume that if there is no driver, that it doesn't
@@ -1829,23 +1849,29 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 			d->driver = &genphy_driver.mdiodrv.driver;
 
 		phydev->is_genphy_driven = 1;
+	} else if (!phydev->bound) {
+		phydev_err(phydev, "driver is binding or unbinding\n");
+		err = -EAGAIN;
+		goto error_unlock;
 	}
 
 	drv_owner = d->driver->owner;
 	if (!try_module_get(drv_owner)) {
 		phydev_err(phydev, "failed to get the device driver module\n");
 		err = -EIO;
-		goto error_put_device;
+		goto error_unlock;
 	}
 	phydev->drv_owner = drv_owner;
 
 	if (phydev->is_genphy_driven) {
-		err = d->driver->probe(d);
+		err = __phy_probe(d);
 		if (err >= 0)
 			err = device_bind_driver(d);
 
 		if (err)
 			goto error_module_put;
+
+		phydev->bound = true;
 	}
 
 	phydev->phy_link_change = phy_link_change;
@@ -1928,6 +1954,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	phy_resume(phydev);
 
+	mutex_unlock(&phydev->bind_lock);
+
 	/**
 	 * If the external phy used by current mac interface is managed by
 	 * another mac interface, so we should create a device link between
@@ -1940,6 +1968,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	return err;
 
 error:
+	mutex_unlock(&phydev->bind_lock);
 	/* phy_detach_internal() does all of the cleanup below */
 	phy_detach_internal(phydev, false);
 	return err;
@@ -1950,7 +1979,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	phydev->irq = irq;
 	phydev->is_genphy_driven = 0;
 	d->driver = NULL;
-error_put_device:
+error_unlock:
+	mutex_unlock(&phydev->bind_lock);
 	put_device(d);
 	if (ndev_owner != bus->owner)
 		module_put(bus->owner);
@@ -3654,12 +3684,14 @@ struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode)
 EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
 
 /**
- * phy_probe - probe and init a PHY device
+ * __phy_probe - probe and init a PHY device
  * @dev: device to probe and init
  *
  * Take care of setting up the phy_device structure, set the state to READY.
+ *
+ * Return: 0 on success or a negative error code.
  */
-static int phy_probe(struct device *dev)
+static int __phy_probe(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
 	struct device_driver *drv = phydev->mdio.dev.driver;
@@ -3807,10 +3839,30 @@ static int phy_probe(struct device *dev)
 	return err;
 }
 
+static int phy_probe(struct device *dev)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	int err;
+
+	err = __phy_probe(dev);
+	if (err)
+		return err;
+
+	mutex_lock(&phydev->bind_lock);
+	phydev->bound = true;
+	mutex_unlock(&phydev->bind_lock);
+
+	return 0;
+}
+
 static int phy_remove(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
 
+	mutex_lock(&phydev->bind_lock);
+	phydev->bound = false;
+	mutex_unlock(&phydev->bind_lock);
+
 	cancel_delayed_work_sync(&phydev->state_queue);
 
 	if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev))
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a5a419bc400e..db7c8696743c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -671,6 +671,8 @@ struct phy_oatc14_sqi_capability {
  * @n_ports: Number of ports currently attached to the PHY
  * @max_n_ports: Max number of ports this PHY can expose
  * @lock:  Mutex for serialization access to PHY
+ * @bind_lock: Serialises attach and detach with phy_probe() and phy_remove()
+ * @bound: A driver has finished probing and is not being removed
  * @state_queue: Work queue for state machine
  * @link_down_events: Number of times link was lost
  * @shared: Pointer to private data shared by phys in one package
@@ -802,6 +804,10 @@ struct phy_device {
 
 	struct mutex lock;
 
+	/* Protects bound */
+	struct mutex bind_lock;
+	bool bound;
+
 	/* This may be modified under the rtnl lock */
 	bool sfp_bus_attached;
 	struct sfp_bus *sfp_bus;
-- 
2.53.0

[PATCH net-next v5 4/4] net: phy: make an unbind wait for the attached consumer to detach

[Aleksei Sviridkin]

With attach serialised against unbind, the unbind that loses the race
waits for the attach and then removes the driver from a PHY that is now
attached. phylink uses phydev->drv right after the attach returns, and
phylib uses it again in later phy_start(), phy_stop() and state machine
runs.

Found on the same KN-1012 by repeating the wan race on a kernel with
the previous patch. The attach completed, the unbind then removed the
driver, and phylink faulted one frame up:

  Unable to handle kernel access to user memory outside uaccess
  routines at virtual address 00000000000000a0
  Comm: ip
  Call trace:
   phylink_bringup_phy+0x680/0x784 (P)
   phylink_fwnode_phy_connect+0x1b8/0x27c
   phylink_of_phy_connect+0x18/0x20
   mtk_open+0x38/0xb70

A DSA port gets there without any race, and did on the same board
before this series: unbinding the lan4 PHY driver returns at once, and
with lan4 up, tearing the switch down later faults in
_phy_state_machine(), called by phy_stop() from dsa_user_close().

The driver core gives a driver no way to refuse an unbind, so make
phy_remove() wait until phy_detach() has run. An unbind of a PHY in use
now blocks until the consumer lets go: ifdown for a MAC that connects
in ndo_open, the switch teardown for DSA, which connects at probe.
A reboot behind such an unbind hangs: device_shutdown() takes the
device lock the waiting unbind holds.
Detach clears the attached flag in the section where it clears
phy_link_change, so it cannot overwrite the flag of an attach that
comes right after. For the same reason, the restore of phydev->irq
for a PHY on the generic driver moves ahead of that section.

Deleting the PHY device through phy_device_remove(), as
mdiobus_unregister() does, keeps today's behaviour and does not wait,
and it releases an unbind that is already waiting. Some MAC drivers
unregister their MDIO bus with the PHY still attached and never detach
it (greth), so waiting there would hang their removal for good.

One case is known to wait for good and is untested: when the PHY and
its MAC or switch are managed device-link consumers of one supplier,
unbinding the supplier can unbind the PHY first, and phy_remove() then
waits for a detach that only the same thread would run later.

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.")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
Changes in v5:
- READ_ONCE()/WRITE_ONCE() on attached. Detach clears it in the
  bind_lock section that clears phy_link_change, then wakes the waiter.
- Rebased on net-next: the interrupt restore in phy_detach_internal()
  now comes before the bind_lock section that lets a new attach in, so
  it cannot land on that attach.
- The message names the device-link case where the wait never ends.
- The comment on bind_lock names every flag it protects.
- The message says that a reboot behind a waiting unbind hangs.
- WRITE_ONCE() on removing as well.

 drivers/net/phy/phy_device.c | 34 +++++++++++++++++++++++++++++++---
 include/linux/phy.h          |  6 +++++-
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0ffaa456a308..a271fe76c3f8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1065,6 +1065,13 @@ void phy_device_remove(struct phy_device *phydev)
 	unregister_mii_timestamper(phydev->mii_ts);
 	pse_control_put(phydev->psec);
 
+	mutex_lock(&phydev->bind_lock);
+	WRITE_ONCE(phydev->removing, true);
+	mutex_unlock(&phydev->bind_lock);
+	/* Order the store before waking an unbind waiting in phy_remove() */
+	smp_mb();
+	wake_up_var(&phydev->attached);
+
 	device_del(&phydev->mdio.dev);
 
 	/* Assert the reset signal */
@@ -1734,25 +1741,33 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
 
 	phydev->phylink = NULL;
 
+	/* Before phy_link_change is cleared, so it cannot land on a new attach */
+	if (phydev->is_genphy_driven)
+		phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
+
 	/* A new attach may pass its "already attached" test as soon as
-	 * phy_link_change is clear, so take the owner in the same section.
+	 * phy_link_change is clear, so take the owner and clear attached in
+	 * the same section.
 	 */
 	mutex_lock(&phydev->bind_lock);
 	phydev->phy_link_change = NULL;
 	drv_owner = phydev->drv_owner;
 	phydev->drv_owner = NULL;
+	WRITE_ONCE(phydev->attached, false);
 	mutex_unlock(&phydev->bind_lock);
 
 	module_put(drv_owner);
 
+	/* Order the store before waking an unbind waiting in phy_remove() */
+	smp_mb();
+	wake_up_var(&phydev->attached);
+
 	/* If the device had no specific driver before (i.e. - it
 	 * was using the generic driver), we unbind the device
 	 * from the generic driver so that there's a chance a
 	 * real driver could be loaded
 	 */
 	if (phydev->is_genphy_driven) {
-		/* The release below lets phy_probe() write this field. */
-		phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
 		device_release_driver(&phydev->mdio.dev);
 		phydev->is_genphy_driven = 0;
 	}
@@ -1954,6 +1969,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	phy_resume(phydev);
 
+	WRITE_ONCE(phydev->attached, true);
 	mutex_unlock(&phydev->bind_lock);
 
 	/**
@@ -3858,11 +3874,23 @@ static int phy_probe(struct device *dev)
 static int phy_remove(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
+	bool attached;
 
 	mutex_lock(&phydev->bind_lock);
 	phydev->bound = false;
+	attached = READ_ONCE(phydev->attached) && !phydev->removing;
 	mutex_unlock(&phydev->bind_lock);
 
+	/* The driver core cannot refuse an unbind, and the consumer keeps
+	 * using phydev->drv until it detaches.
+	 */
+	if (attached) {
+		phydev_warn(phydev, "unbind waits for the PHY to be detached\n");
+		wait_var_event(&phydev->attached,
+			       !READ_ONCE(phydev->attached) ||
+			       READ_ONCE(phydev->removing));
+	}
+
 	cancel_delayed_work_sync(&phydev->state_queue);
 
 	if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev))
diff --git a/include/linux/phy.h b/include/linux/phy.h
index db7c8696743c..8fa3685b24e6 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -673,6 +673,8 @@ struct phy_oatc14_sqi_capability {
  * @lock:  Mutex for serialization access to PHY
  * @bind_lock: Serialises attach and detach with phy_probe() and phy_remove()
  * @bound: A driver has finished probing and is not being removed
+ * @attached: phy_attach_direct() succeeded and phy_detach() has not run
+ * @removing: phy_device_remove() is deleting the device
  * @state_queue: Work queue for state machine
  * @link_down_events: Number of times link was lost
  * @shared: Pointer to private data shared by phys in one package
@@ -804,9 +806,11 @@ struct phy_device {
 
 	struct mutex lock;
 
-	/* Protects bound */
+	/* Protects bound, attached and removing */
 	struct mutex bind_lock;
 	bool bound;
+	bool attached;
+	bool removing;
 
 	/* This may be modified under the rtnl lock */
 	bool sfp_bus_attached;
-- 
2.53.0

Re: [PATCH net-next v5 2/4] net: phy: put the driver module the attach took

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] At this commit, `phydev->drv_owner` is read and written with
  no lock.
- [Low] The commit body does not say what this patch depends on.

Pre-existing issues:
- [Medium] In `phy_attach_direct()` (drivers/net/phy/phy_device.c),
  `d->driver` is tested at line 1825 (`if (!d->driver)`) and…

--

net: phy: put the driver module the attach took

This commit stores the PHY driver module pinned by phy_attach_direct() in
a new field, phydev->drv_owner. phy_detach_internal() and the attach error
path now release that stored module instead of reading d->driver again.
The goal is to stop skipping the put, or putting the wrong module, when
the PHY driver is unbound while the PHY is attached.

> Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY driver")

[Severity: Low]
Should the commit message say which other patches this one depends on?
Because of the Fixes: tag, stable tooling could pick up this patch by
itself. Its correctness seems to depend on two other patches in the
series:

  "net: phy: refuse a second attach before touching the PHY", which needs
  to be applied first.

  "net: phy: serialise attach and detach with PHY driver bind and
  unbind", which closes a leak this patch leaves open (see below).

The only place this is mentioned is the v5 changelog below the --- line:

  "On its own, this patch can still leak a reference when one consumer
  attaches while another detaches the same PHY. Patch 3 closes that."

That text is dropped when the patch is applied.

Without the first patch, the baseline phy_attach_direct() still reaches
the drv_owner store on a PHY that is already attached. Its
if (phydev->attached_dev) test only runs after try_module_get():

phy_attach_direct()      /* second attach, same PHY */
    try_module_get(A);
    phydev->drv_owner = A;
    if (phydev->attached_dev) goto error;
    phy_detach_internal()
        module_put(phydev->drv_owner);    /* puts A */
        phydev->drv_owner = NULL;

When the first consumer later calls phy_detach(), it does
module_put(NULL), so A is left with an extra reference that is never
dropped. The baseline also allows a second attach without a netdev
(DSA-style), and that overwrites the slot in the same way.

This patch's hunks don't overlap the first patch's hunks, so it applies
cleanly without it. Could the body name the companion commits, or could
the series be reordered or squashed?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 6bc9cdb7fa699..68691c487eee5 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1728,8 +1728,8 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
>  	phydev->phy_link_change = NULL;
>  	phydev->phylink = NULL;
>  
> -	if (phydev->mdio.dev.driver)
> -		module_put(phydev->mdio.dev.driver->owner);
> +	module_put(phydev->drv_owner);
> +	phydev->drv_owner = NULL;

[Severity: Medium]
Can a concurrent attach lose its module reference here, or have it put on
the wrong module? At this commit, phydev->drv_owner is read and written
without any lock.

Setting phydev->phy_link_change = NULL opens the unlocked "already
attached" test in phy_attach_direct() before this module_put() runs:

phy_attach_direct() {
    ...
	if (phydev->phy_link_change) {
		phydev_err(phydev, "PHY already attached\n");
    ...
	phydev->drv_owner = drv_owner;
    ...
}

So a second consumer that attaches in that window can store its owner in
the slot this detach is about to read and clear:

CPU1 (detach #1)                    CPU2 (attach #2)
phy_detach_internal()
  phydev->phy_link_change = NULL;
                                    phy_attach_direct()
                                      phy_link_change test passes
                                      try_module_get(X);
                                      phydev->drv_owner = X;
  module_put(phydev->drv_owner);
  phydev->drv_owner = NULL;

If X is the same driver A, detach #1 puts A and then its NULL store wipes
out attach #2's record. Attach #2's later detach puts NULL, so A leaks a
reference.

Now suppose A was unbound and driver B bound while the PHY was attached.
Detach #1 then puts B, which it never pinned, and A leaks. Attach #2 is
using B but holds no reference on it. Could B be unloaded while attach #2
is still using it?

If attach #2 then fails through goto error, phy_detach_internal() may
read drv_owner == B again and put B a second time. The error_module_put
path also reads phydev->drv_owner instead of the local drv_owner.

This looks like the "put lands on a module that was never pinned" case
the commit message describes, and the v5 changelog mentions the gap.

Later in the series, "net: phy: serialise attach and detach with PHY
driver bind and unbind" appears to close it:

  phy_attach_direct() holds bind_lock from before the phy_link_change
  test until after the drv_owner store.

  phy_detach_internal() clears phy_link_change, saves and clears
  drv_owner, and clears attached in a single bind_lock section, then
  calls module_put() on the saved value.

[ ... ]

> @@ -1830,11 +1831,13 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  		phydev->is_genphy_driven = 1;
>  	}
>  
> -	if (!try_module_get(d->driver->owner)) {
> +	drv_owner = d->driver->owner;

[Severity: Medium]
This isn't a bug introduced by this patch, but can d->driver change
between the earlier if (!d->driver) test in phy_attach_direct() and this
dereference?

phy_attach_direct() doesn't hold device_lock(d), and at this commit
nothing else blocks a driver core unbind. A sysfs unbind, or
driver_unregister() at module exit, can run at the same time:

__device_release_driver()
    phy_remove()
    dev->driver = NULL;

Could that cause a NULL dereference of ->owner here? Could it instead
read ->owner from the driver struct of a module that is being freed?

The old try_module_get(d->driver->owner) had the same check-then-
dereference pattern, so the race predates this patch.

"net: phy: serialise attach and detach with PHY driver bind and unbind",
later in the series, appears to close it. It holds phydev->bind_lock from
before the d->driver test through try_module_get(), and returns -EAGAIN
when d->driver is set but phydev->bound is not.

> +	if (!try_module_get(drv_owner)) {
>  		phydev_err(phydev, "failed to get the device driver module\n");
>  		err = -EIO;
>  		goto error_put_device;
>  	}
> +	phydev->drv_owner = drv_owner;
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009180551.1234107-1-f%40lex.la

Re: [PATCH net-next v5 3/4] net: phy: serialise attach and detach with PHY driver bind and unbind

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 1 (1) · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] phy_detach_internal() clears phy_link_change, and swaps out
  drv_owner, inside one bind_lock section (phy_device.c:1740-1744).
- [Medium] phy_attach_direct() now returns -EAGAIN when d->driver is set
  but phydev->bound is false (drivers/net/phy/phy_device.c:1852-1855).
- [Low] The struct phy_device comment for the new mutex says `/* Protects
  bound */` (include/linux/phy.h).
- [Low] The struct mii_bus kernel-doc for notify_phy_attach lists its
  calling context: process context, may sleep, may hold RTNL, must not…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but bind_lock does
  not cover the driver core's own bookkeeping around phy_remove().
- [High] This problem wasn't introduced by this patch, but the genphy
  fallback in phy_attach_direct() still writes d->driver = genphy…
- [Medium] (Baseline: discarded, as a pre-existing medium issue.) In the
  genphy path of phy_attach_direct(), if __phy_probe(d) succeeds and…

--

net: phy: serialise attach and detach with PHY driver bind and unbind

This commit adds a per-PHY bind_lock mutex and a bound flag. Together they
serialise phy_attach_direct() and phy_detach() against phy_probe() and
phy_remove(). An attach that finds a driver bound but not done probing
returns -EAGAIN. The "already attached" test and the drv_owner hand-off
in detach now run under the new mutex.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 68691c487eee5..0ffaa456a308b 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1725,11 +1732,18 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
>  		phy_link_topo_del_phy(dev, phydev);
>  	}
>  
> -	phydev->phy_link_change = NULL;
>  	phydev->phylink = NULL;
>  
> -	module_put(phydev->drv_owner);
> +	/* A new attach may pass its "already attached" test as soon as
> +	 * phy_link_change is clear, so take the owner in the same section.
> +	 */
> +	mutex_lock(&phydev->bind_lock);
> +	phydev->phy_link_change = NULL;
> +	drv_owner = phydev->drv_owner;
>  	phydev->drv_owner = NULL;
> +	mutex_unlock(&phydev->bind_lock);
> +
> +	module_put(drv_owner);
>  
>  	/* If the device had no specific driver before (i.e. - it
>  	 * was using the generic driver), we unbind the device

[Severity: High]
After this section drops bind_lock, a second consumer's phy_attach_direct()
can take it and pass the "already attached" test. The rest of the detach
then runs with no lock held:

phy_detach_internal() {
    ...
	if (phydev->is_genphy_driven) {
		/* The release below lets phy_probe() write this field. */
		phydev->irq = phydev->mdio.bus->irq[phydev->mdio.addr];
		device_release_driver(&phydev->mdio.dev);
		phydev->is_genphy_driven = 0;
	}

	/* Assert the reset signal */
	phy_device_reset(phydev, 1);
    ...
}

Take a genphy-driven PHY. The new attach would find d->driver still set
to genphy, bound still true and is_genphy_driven still 1. Would it then
run __phy_probe(d) and device_bind_driver(d) again on a device that is
already probed?

A few outcomes look possible from there.

phy_led_triggers_register() fails with -EEXIST on the duplicate trigger
name and sets phy_num_led_triggers to 0. phy_led_triggers still points at
the registered array. A later phy_led_triggers_unregister() loops zero
times and kfree()s that array, which leaves freed entries on the global
trigger list. phy_setup_ports() also adds duplicate ports.

If device_bind_driver() fails with -EEXIST, error_module_put sets
d->driver = NULL while the driver core still has the device bound. The
old detach's device_release_driver() then does nothing, and later
attaches keep failing with -EEXIST.

If the bind succeeds after the old detach's driver_sysfs_remove(), the
old detach's phy_remove() runs against the new attachment. With the
later patch in this series that makes an unbind wait for the attached
consumer, phy_remove() would wait for the second consumer to detach. The
first detach thread meanwhile holds the device lock and usually rtnl.

For any PHY driver, not only genphy, can the old detach's
phy_device_reset(phydev, 1) run after the new attach's phy_init_hw() has
deasserted reset? That would leave the new consumer's PHY held in reset.

The last patch in the series, "net: phy: make an unbind wait for the
attached consumer to detach", moves only the irq restore ahead of this
locked section. device_release_driver(), the is_genphy_driven clear and
the reset assert still come after it.

[ ... ]

> @@ -1829,23 +1849,29 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  			d->driver = &genphy_driver.mdiodrv.driver;
>  
>  		phydev->is_genphy_driven = 1;
> +	} else if (!phydev->bound) {
> +		phydev_err(phydev, "driver is binding or unbinding\n");
> +		err = -EAGAIN;
> +		goto error_unlock;
>  	}

[Severity: Medium]
This state only lasts while the PHY driver is probing or being removed.
Callers that attach from their own probe pass the error straight up,
for example ethoc_mdio_probe() in drivers/net/ethernet/ethoc.c:

	return dev_err_probe(&dev->dev, err, "could not attach to PHY\n");

and hbg_phy_connect() in drivers/net/ethernet/hisilicon/hibmcge/hbg_mdio.c:

	return dev_err_probe(dev, ret, "failed to connect phy\n");

driver_probe_device() in drivers/base/dd.c calls driver_deferred_probe_add()
only for -EPROBE_DEFER. Any other error, -EAGAIN included, is logged as a
failed probe and is never retried.

Suppose a MAC probe overlaps a PHY driver probe, for example while a PHY
module loads or during async probing at boot. Would the MAC stay unbound
even though the PHY finishes probing moments later?

The commit message says such a MAC "then fails that probe". Would
returning -EPROBE_DEFER here let it recover automatically? The other
option is a kernel-doc note telling probe-time callers to convert
-EAGAIN themselves.

>  
>  	drv_owner = d->driver->owner;

[ ... ]

>  	if (phydev->is_genphy_driven) {
> -		err = d->driver->probe(d);
> +		err = __phy_probe(d);
>  		if (err >= 0)
>  			err = device_bind_driver(d);
>  
>  		if (err)
>  			goto error_module_put;

[Severity: Medium]
This isn't a bug introduced by this patch, since the old
d->driver->probe(d) call had the same gap. If __phy_probe(d) succeeds
and device_bind_driver(d) then fails, error_module_put restores only a
few fields:

error_module_put:
	module_put(phydev->drv_owner);
	phydev->drv_owner = NULL;
	phydev->irq = irq;
	phydev->is_genphy_driven = 0;
	d->driver = NULL;

Several things set up by __phy_probe() are left behind:

- the LED triggers stay registered
- the phy_port entries stay on phydev->ports
- phydev->drv still points at genphy

phy_remove() never runs, because the driver core never recorded the
binding.

On the next attach, phy_led_triggers_register() would fail with -EEXIST
and zero phy_num_led_triggers while the old array is still registered.
Can a later phy_led_triggers_unregister() then kfree() triggers that are
still on the trigger list?

> +
> +		phydev->bound = true;
>  	}

[Severity: High]
This is a pre-existing issue, and the commit message says the generic
driver fallback is not covered. Still, the fallback writes d->driver
without the device lock and runs __phy_probe(d) holding only bind_lock.

A real driver can bind through the driver core at the same time, via a
sysfs bind or __driver_attach() after a module load.
__driver_probe_device() checks for that under device_lock:

	if (dev->driver)
		return -EBUSY;

really_probe() then sets dev->driver, calls driver_sysfs_add() and calls
phy_probe(). phy_probe() runs __phy_probe() before it takes bind_lock.

If that check runs before attach writes d->driver, can two __phy_probe()
calls run on the same phydev at once? Both would write phydev->drv, add
phy_port entries to phydev->ports without a lock, and register duplicate
LED triggers and SFP upstreams.

Attach's device_bind_driver() would then usually fail with -EEXIST.
error_module_put then sets d->driver = NULL while really_probe() is
still running. Would driver_bound() or device_unbind_cleanup() then
dereference a NULL dev->driver?

[ ... ]

> @@ -1928,6 +1954,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  
>  	phy_resume(phydev);
>  
> +	mutex_unlock(&phydev->bind_lock);
> +

[Severity: Low]
bus->notify_phy_attach() is now called with phydev->bind_lock held:

	if (phydev->mdio.bus->notify_phy_attach) {
		err = phydev->mdio.bus->notify_phy_attach(phydev);
		if (err)
			goto error;
	}

The struct mii_bus kernel-doc for notify_phy_attach covers process
context, sleeping and rtnl, but does not mention bind_lock. A bus
implementation that attached, detached or rebound the PHY from this
callback would deadlock on bind_lock through phy_probe() or
phy_remove().

Should the kernel-doc say so? The only in-tree implementer,
otto_emdio_notify_phy_attach() in mdio-realtek-rtl9300.c, does not do
any of this.

A smaller point: the notify_phy_detach doc says it is called "right
after phy_suspend()". phy_detach_internal() now skips phy_suspend() when
bound is false.

>  	/**
>  	 * If the external phy used by current mac interface is managed by
>  	 * another mac interface, so we should create a device link between

[ ... ]

> @@ -3807,10 +3839,30 @@ static int phy_probe(struct device *dev)

[ ... ]

>  static int phy_remove(struct device *dev)
>  {
>  	struct phy_device *phydev = to_phy_device(dev);
>  
> +	mutex_lock(&phydev->bind_lock);
> +	phydev->bound = false;
> +	mutex_unlock(&phydev->bind_lock);
> +
>  	cancel_delayed_work_sync(&phydev->state_queue);

[Severity: High]
This isn't a bug introduced by this patch, but bind_lock does not cover
what the driver core does after phy_remove() returns. In
__device_release_driver(), device_unbind_cleanup() clears dev->driver.
Only after that do device_links_driver_cleanup() and
klist_remove(&dev->p->knode_driver) run.

An attach that takes bind_lock in that gap sees !d->driver and takes the
genphy fallback:

phy_attach_direct()
   d->driver = &genphy_driver.mdiodrv.driver;   (no device lock)
   __phy_probe(d)
   device_bind_driver(d)
      driver_sysfs_add()   succeeds, the old links are already gone
      driver_bound()       device_is_bound() still true, only warns

The unbind thread then runs klist_remove(). The device is left with
dev->driver = genphy and knode_driver on no list.

The next device_release_driver() can come from phy_detach() or from
device_del(). Can klist_remove()->klist_del()->klist_put() then
dereference a NULL knode_klist(n) in lib/klist.c?

[ ... ]

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index a5a419bc400ec..db7c8696743cf 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h

[ ... ]

> @@ -802,6 +804,10 @@ struct phy_device {
>  
>  	struct mutex lock;
>  
> +	/* Protects bound */
> +	struct mutex bind_lock;
> +	bool bound;
> +

[Severity: Low]
This isn't a bug, but the comment says bind_lock protects only bound.
The code relies on it for more than that:

- phy_attach_direct() reads phy_link_change under it as the "already
  attached" test
- phy_detach_internal() clears phy_link_change and swaps drv_owner
  under it

Meanwhile phylink_bringup_phy() in drivers/net/phy/phylink.c writes
phy_link_change without bind_lock:

	phy->phy_link_change = phylink_phy_change;

Today that write only replaces one non-NULL value with another, so the
-EBUSY test still works. It is still a plain write racing with locked
reads.

Should the comment list phy_link_change and drv_owner too? The last
patch in the series changes it to "Protects bound, attached and
removing", which still leaves them out.

>  	/* This may be modified under the rtnl lock */
>  	bool sfp_bus_attached;
>  	struct sfp_bus *sfp_bus;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009180551.1234107-1-f%40lex.la

Re: [PATCH net-next v5 4/4] net: phy: make an unbind wait for the attached consumer to detach

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 3 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] phy_remove() can wait forever on its own thread.
- [High] net: phy: driver state corruption due to attach/detach race
- [High] net: phy: deadlock in phy_detach_internal() waiting for unbind
- [Medium] The new wait_var_event() in phy_remove()
  (drivers/net/phy/phy_device.c, lines 3887-3892) has no upper bound and…

--

net: phy: make an unbind wait for the attached consumer to detach

With this patch, phy_remove() waits for phy_detach() to run before it
takes the driver away from a PHY that is still attached. Without the
wait, phylink and phylib keep using phydev->drv after the unbind. Removing
the device through phy_device_remove() sets a new removing flag, which
skips the wait and wakes any unbind that is already waiting.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 0ffaa456a308b..a271fe76c3f8e 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -3858,11 +3874,23 @@ static int phy_probe(struct device *dev)
>  static int phy_remove(struct device *dev)
>  {
>  	struct phy_device *phydev = to_phy_device(dev);
> +	bool attached;
>  
>  	mutex_lock(&phydev->bind_lock);
>  	phydev->bound = false;
> +	attached = READ_ONCE(phydev->attached) && !phydev->removing;
>  	mutex_unlock(&phydev->bind_lock);
>  
> +	/* The driver core cannot refuse an unbind, and the consumer keeps
> +	 * using phydev->drv until it detaches.
> +	 */
> +	if (attached) {
> +		phydev_warn(phydev, "unbind waits for the PHY to be detached\n");
> +		wait_var_event(&phydev->attached,
> +			       !READ_ONCE(phydev->attached) ||
> +			       READ_ONCE(phydev->removing));
> +	}

[Severity: High]
Can the detach of a genphy-driven PHY end up waiting for a different
consumer? The commit message expects an attach to come right after
this section. For a PHY on the generic driver, though, the detach is
not finished at that point.

attached is cleared and bind_lock is dropped before
device_release_driver() runs. phy_remove() only samples attached when
it takes bind_lock again from inside the driver core. As far as I can
tell, __device_release_driver() calls driver_sysfs_remove() before
device_remove(), and dev->driver is cleared only afterwards, in
device_unbind_cleanup().

A phy_attach_direct() that lands in that window sees:

- phy_link_change == NULL
- d->driver still set
- phydev->bound still true
- is_genphy_driven still 1

It runs __phy_probe() again. device_bind_driver() now succeeds because
the driver sysfs links are already gone; driver_bound() only warns that
the device is already bound. The attach then completes and sets
attached to true.

The detaching thread then gets to phy_remove() and sees attached set
and removing clear. It sleeps in wait_var_event() with the PHY's device
lock held until the new consumer detaches.

The old consumer usually gets here through phylink_disconnect_phy() or
phy_disconnect() from ndo_stop with RTNL held. The ifdown then hangs
in D state behind an unrelated consumer. Suppose that consumer attached
without RTNL, for example from a probe path, but needs RTNL for its
own detach. Then the wait can never end.

Even when the wait does end, the new attach has re-run __phy_probe()
and device_bind_driver() on a device that was halfway through its
unbind.

Could phy_link_change stay set until device_release_driver() has
returned, with only attached cleared before it? An attach in this
window would then be refused with -EBUSY instead.

[Severity: High]
Does the genphy release at the end of phy_detach_internal() still race
with a new attach? The irq restore now comes ahead of the bind_lock
section so that a new attach cannot see it. However, is_genphy_driven
and d->driver are still used after bind_lock has been dropped.

Once phy_link_change is NULL, a concurrent phy_attach_direct() passes
its "already attached" test. At that point d->driver still points at
the genphy driver, phydev->bound is still true and is_genphy_driven is
still 1. The attach therefore takes the genphy path again and calls
__phy_probe() and device_bind_driver() on a device that is still bound.

If the driver sysfs links have not been removed yet, driver_sysfs_add()
fails with -EEXIST. The attach then unwinds through error_module_put,
which clears is_genphy_driven and sets d->driver to NULL.

The detaching thread then does one of two things:

- It sees is_genphy_driven == 0 and skips device_release_driver().

- It reaches __device_release_driver() after dev->driver was cleared
  and does nothing.

Either way the device stays on the genphy driver's klist with its
driver sysfs links in place, but with a NULL driver pointer. Since
phy_remove() never runs, phydev->bound stays true.

>From then on, every genphy attach fails the same way in
driver_sysfs_add(). When the device is later deleted,
bus_remove_device() finds no driver to release. As far as I can tell,
the knode_driver of a freed device is then left on the genphy driver's
list.

Would it work to keep phy_link_change set until
device_release_driver() has returned? Only attached would be cleared,
and the waiter woken, before it. A new attach would then get -EBUSY
for the whole genphy release. Or is there another way you would
prefer to keep phy_attach_direct() off a device whose genphy driver is
still being released?

[Severity: Medium]
This wait has no timeout and cannot be interrupted. It also runs while
the driver core holds the PHY's device lock. __wait_var_event() is:

include/linux/wait_bit.h:
#define __wait_var_event(var, condition)
	___wait_var_event(var, condition, TASK_UNINTERRUPTIBLE, 0, 0,
			  schedule())

and phy_remove() is reached under the device lock (plus the parent lock,
when the bus needs it):

drivers/base/dd.c:device_release_driver_internal() {
    ...
	__device_driver_lock(dev, parent);

	if (!drv || drv == dev->driver)
		__device_release_driver(dev, parent);

	__device_driver_unlock(dev, parent);
    ...
}

Only phy_detach_internal() or phy_device_remove() can end the wait. For
a MAC that connects in ndo_open, that means an ifdown. For DSA, or a MAC
that connects at probe, it means tearing down the consumer driver. Until
then the unbinding task sits in D state and keeps holding dev->mutex.

The commit message covers the reboot hang in device_shutdown(). There
seem to be other effects as well:

- The freezer cannot freeze a task in TASK_UNINTERRUPTIBLE without
  TASK_FREEZABLE, so system suspend and hibernate would abort.

- The hung task detector would report the task every
  hung_task_timeout_secs, and with hung_task_panic=1 the machine would
  panic.

- Anything else that needs this device's lock would block too, such as
  a later bind, a driver_override write, or loading a driver that
  matches the device.

The dependency is a bool flag plus wait_var_event(), and dev->mutex uses
a lockdep novalidate class. As far as I can tell, lockdep cannot report
any of these hangs.

Was a different design considered? For example, a managed device link
from the consumer to the PHY would make the driver core unbind the
consumer before the PHY, so phy_remove() would not have to block under
the device lock.

[Severity: High]
Can this wait end up waiting on its own thread forever? The commit
message says:

  One case is known to wait for good and is untested: when the PHY and
  its MAC or switch are managed device-link consumers of one supplier,
  unbinding the supplier can unbind the PHY first, and phy_remove() then
  waits for a detach that only the same thread would run later.

When a supplier is unbound, device_links_unbind_consumers() unbinds each
active managed consumer synchronously, one at a time:

drivers/base/core.c:device_links_unbind_consumers() {
    ...
		if (status == DL_STATE_ACTIVE) {
			struct device *consumer = link->consumer;

			get_device(consumer);

			device_links_write_unlock();

			device_release_driver_internal(consumer, NULL,
						       consumer->parent);
			put_device(consumer);
			goto start;
		}
    ...
}

The PHY can be a consumer of a third-party supplier through fw_devlink,
for example via reset-gpios, interrupts, pinctrl or a clock. If its link
is processed before the link of the MAC or switch that holds it
attached, the call chain is:

device_release_driver_internal(supplier)
  __device_release_driver(supplier)
    device_links_unbind_consumers(supplier)
      device_release_driver_internal(phy)
        phy_remove()
          wait_var_event()

The phy_detach() that would wake the waiter runs only when the MAC or
switch consumer is unbound. That happens later in the same loop, on the
same thread, after phy_remove() returns. The thread then hangs for good
while holding both the supplier's and the PHY's device locks, and any
later reboot hangs in device_shutdown() as well.

The MAC-to-PHY link created in phy_attach_direct() does not help here:

	phydev->devlink = device_link_add(dev->dev.parent, &phydev->mdio.dev,
					  DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);

Because it is stateless, device_links_unbind_consumers() ignores it and
does not unbind the MAC before the PHY.

This patch knowingly ships a hang that cannot be recovered. Could it be
avoided before merging, for example with a timeout, a managed link, or
by skipping the wait when the unbind comes from a supplier?

>  	cancel_delayed_work_sync(&phydev->state_queue);
>  
>  	if (IS_ENABLED(CONFIG_PHYLIB_LEDS) && !phy_driver_is_genphy(phydev))

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009180551.1234107-1-f%40lex.la

Re: [PATCH net-next v5 4/4] net: phy: make an unbind wait for the attached consumer to detach

[Aleksei Sviridkin]

Russell,

Sashiko's review of this patch showed that waiting in phy_remove() for
the consumer to detach can hang: on the unbinding thread itself through
device links, and with the device lock held across reboot and suspend.

For v6 I would rather not wait. phy_remove() would call back into the
consumer under rtnl, and phylink would answer with
phylink_disconnect_phy(), the path it already takes when an SFP module
with a PHY goes away. Consumers without the callback keep today's
behaviour.

Is a callback from phylib into phylink like this acceptable to you, or
would you rather see it shaped differently?

Aleksei

pw-bot: cr