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

[PATCH net v3 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 fault was inside the PHY driver's
config_init: the driver read phydev->drv after phy_remove() had cleared
it. Patch 3 has the trace.

Serialising attach against unbind is not enough on its own. With only
that, an unbind that loses the race waits for the attach and then
removes the driver from a PHY that is now attached. On the board the
consumer then faulted one step later: in phylink_bringup_phy() right
after the attach returned, or in _phy_state_machine() at the next
ifdown. A DSA port gets there without any race, because DSA keeps its
PHYs attached from switch setup to teardown. Patch 4 makes the unbind
wait until the consumer has detached.

1: refuse a second attach of a PHY that is already attached, before
   taking anything. That -EBUSY path used to run phy_detach() on the
   first consumer's attachment. Found in review of patch 2, whose
   module accounting it unbalanced.
2: put the module reference phy_attach_direct() took, not whatever
   driver is bound at detach time. Found by reading. On the KN-1012 the
   sequence that would leak faults earlier, before the detach reaches
   the put.
3: a per-PHY mutex and a "bound" flag. An attach is either done with
   the driver before phy_remove() starts tearing it down, or is refused
   with -EAGAIN, or, after the unbind has finished, gets the generic
   driver as today.
4: phy_remove() waits for phy_detach() when the PHY is attached,
   except when the PHY device itself is being deleted.

1, 2 and 3 stand on their own. Without 4, an unbind that loses the race
to an attach still leaves that consumer with a PHY whose driver is
gone. 3 only guarantees the attach itself does not run into a driver
being removed.

On the lock inversion (Paolo): the PHY's device lock is the obvious lock
for this, but attach may run under rtnl, and rtnl is taken under that
device lock on bind and unbind in at least two places. For a PHY with an
SFP cage, phy_probe() and phy_remove() go through sfp_bus_add_upstream()
and sfp_bus_del_upstream(), which take rtnl. For a PHY LED on the netdev
trigger, led_classdev_register() and led_classdev_unregister() register
and unregister a netdevice notifier, which takes rtnl.

Removing the inversion would at least mean moving the SFP registration
out of probe and remove, and that is not net material. The SFP
upstream ops write netdev state that rtnl protects. Attach does not
always run under rtnl (DSA connects its ports before taking it), so
the registration cannot follow the attach either. Nothing under the
new mutex takes rtnl. On the generic-driver path, device_bind_driver()
can take a supplier's device lock for sync_state, as it already does
today.

Vladimir, this is where 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". What I looked at, and what each costs:

- A, phy_remove() waits for the detach (patch 4, kept): the unbind
  blocks, uninterruptibly, until the consumer detaches. For DSA that
  means until the switch is torn down, even with the port down, since
  DSA stays connected.
- B, stop at patch 3: the oops moves one frame up, into
  phylink_bringup_phy() or _phy_state_machine(). Patch 4 has the
  phylink trace.
- C, let the caller hold the lock across attach and bringup: widens
  the series to phylink and still leaves every use after bringup.
- A with a killable wait: ->remove cannot fail, so after a kill it
  could only go on removing the driver, back into the oops.
- suppress_bind_attrs on PHY drivers: removes the sysfs unbind your
  use case relies on.
- A managed device link MAC -> PHY: the unbind would take the whole
  MAC or switch down with it.

A is a compromise. What it does not achieve: an unbind of a PHY in use
hangs instead of failing, and on DSA it hangs until switch teardown,
even with the port down.

On the KN-1012, lan4 is a DSA port on an EN8811H. Without the series,
with lan4 down, unbinding air_en8811h returned at once. Bringing lan4
up after that gave a WARN in phy_start() and no oops within 10 s.
Unbinding the switch instead faulted in phy_free_interrupt() from
phy_disconnect(). With the series (an earlier revision with the same
attach and wait code, before the device-deletion change), the same
unbind blocks with lan4 up or down, and returns when the switch is
unbound.

Vladimir, should patch 4 go separately, to net-next or as an RFC, with
1 to 3 going to net now?

Patch 4 has other costs too. The hung-task detector, when enabled,
reports the blocked unbind after its timeout. System suspend and
reboot wait on that PHY's device lock meanwhile. device_shutdown()
takes it and does not detach PHYs, so a reboot behind a blocked unbind
hangs. sysfs shows the driver link gone while ->remove is still
waiting, because the driver core removes it first.

Deleting the PHY device does not wait and keeps today's behaviour, for
example mdiobus_unregister() from a MAC driver's remove. Some MAC
drivers never detach their PHY and would otherwise hang in their own
removal. Andrew asked in 2020 whether an unbind could be blocked while
the interface is up [2]. Florian Fainelli answered that nothing bad
happens, which the traces here no longer bear out.

This series does not close one more window. phy_attach_direct() still
stores the generic driver in d->driver and calls device_bind_driver()
without the device lock the driver core expects. A real driver binding
through the driver core at the same moment can still collide with it.

I tested on the KN-1012 with OpenWrt's 6.18 kernel and a backport of
the series, on top of the board's pending phylib/phylink patches for
its late PHY and test-only mt7530 fixes. Patches 2 to 4 ran as an
earlier three-patch revision without patch 1 and otherwise identical
in code, with PROVE_LOCKING:
- the wan race, 450 iterations: no oops. In 447 the unbind was still
  pending after the attach and returned at ifdown.
- no lock-order report across the unbind/bind cycles. The controller
  unbind runs also show warnings from mtk_remove() stopping and
  disconnecting its netdevs without rtnl (the refcount underflow in
  mtk_stop() comes from stopping netdevs that were already down), and
  DSA teardown a kernfs WARN. Both predate this series.
- the device-deletion branch of patch 4, through a test module calling
  phy_device_remove() on the attached EN8811H, since unbinding the
  switch or the Ethernet controller here detaches the PHY first. The
  call returned, and it released an unbind already waiting.
- the -EAGAIN refusal of patch 3 was hit once, on an earlier revision
  with the same attach code: mtk_open() got -11 and "ip link set wan
  up" failed with "Resource temporarily unavailable".

Patch 1 ran on this revision, without PROVE_LOCKING. A test module
attaching lan1 to the EN8811H that lan4 holds got -EBUSY. lan4 kept
its PHY, and the air_en8811h refcount was unchanged. Without the
series the same call left lan4 with no PHY. The same wan race without
the series faulted on the first try.

[1] https://lore.kernel.org/netdev/20260311203410.rio7m6nuf72hs5p6@skbuf/
[2] https://lore.kernel.org/netdev/20200917131545.GL3526428@lunn.ch/

v2: https://lore.kernel.org/netdev/20260919015340.499675-1-f@lex.la/
- Retargeted to net, and the race closed with locking rather than a
  NULL test at attach entry, as asked in review.
- The module reference fix is its own patch.
- New patch 1: a second attach no longer detaches the first consumer.
- An unbind of an attached PHY now waits for the detach.

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 | 95 ++++++++++++++++++++++++++++++------
 include/linux/phy.h          | 12 +++++
 2 files changed, 92 insertions(+), 15 deletions(-)

-- 
2.53.0

[PATCH net v3 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
its references and, for a PHY with no driver, bound the generic one, and
then leaves through the error path that calls phy_detach(). That
phy_detach() 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,
suspends the PHY and, for the generic driver, unbinds it, while the
first consumer goes on using the PHY.

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 attached_dev before anything is taken and return from there.

Fixes: a7dac9f9c169 ("phy: fix error case of phy_led_triggers_(un)register")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
 drivers/net/phy/phy_device.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94b2e85e00a3..7046976c5b6a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1757,6 +1757,11 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	struct module *ndev_owner = NULL;
 	int err;
 
+	if (phydev->attached_dev) {
+		phydev_err(phydev, "PHY already attached\n");
+		return -EBUSY;
+	}
+
 	/* For Ethernet device drivers that register their own MDIO bus, we
 	 * will have bus->owner match ndev_mod, so we do not want to increment
 	 * our own module->refcnt here, otherwise we would not be able to
@@ -1798,12 +1803,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 v3 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>
---
 drivers/net/phy/phy_device.c | 8 +++++---
 include/linux/phy.h          | 2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7046976c5b6a..29d65f6cfd59 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1793,6 +1793,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		err = -EIO;
 		goto error_put_device;
 	}
+	phydev->drv_owner = d->driver->owner;
 
 	if (phydev->is_genphy_driven) {
 		err = d->driver->probe(d);
@@ -1894,7 +1895,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->is_genphy_driven = 0;
 	d->driver = NULL;
 error_put_device:
@@ -1955,8 +1957,8 @@ void phy_detach(struct phy_device *phydev)
 	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
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5f8d65868e0f..33a207ac5c30 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -560,6 +560,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
@@ -671,6 +672,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 v3 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.

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.

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>
---
 drivers/net/phy/phy_device.c | 57 +++++++++++++++++++++++++++++++-----
 include/linux/phy.h          |  6 ++++
 2 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 29d65f6cfd59..1355ea78c86f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -801,6 +801,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
@@ -1729,6 +1730,8 @@ static int phy_sfp_probe(struct phy_device *phydev)
 	return ret;
 }
 
+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;
@@ -1743,11 +1746,15 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
  *
  * 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)
@@ -1776,6 +1783,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	get_device(d);
 
+	mutex_lock(&phydev->bind_lock);
+
 	/* Assume that if there is no driver, that it doesn't
 	 * exist, and we should use the genphy driver.
 	 */
@@ -1786,22 +1795,28 @@ 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;
 	}
 
 	if (!try_module_get(d->driver->owner)) {
 		phydev_err(phydev, "failed to get the device driver module\n");
 		err = -EIO;
-		goto error_put_device;
+		goto error_unlock;
 	}
 	phydev->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;
+
+		phydev->bound = true;
 	}
 
 	phydev->phy_link_change = phy_link_change;
@@ -1878,6 +1893,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
@@ -1890,6 +1907,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	return err;
 
 error:
+	mutex_unlock(&phydev->bind_lock);
 	/* phy_detach() does all of the cleanup below */
 	phy_detach(phydev);
 	return err;
@@ -1899,7 +1917,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	phydev->drv_owner = NULL;
 	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);
@@ -1935,7 +1954,11 @@ void phy_detach(struct phy_device *phydev)
 		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 (dev) {
 		struct hwtstamp_provider *hwprov;
 
@@ -3678,12 +3701,12 @@ 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.
  */
-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;
@@ -3827,10 +3850,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 33a207ac5c30..8903123d25c8 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -653,6 +653,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 driver bind and unbind
+ * @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
@@ -784,6 +786,10 @@ struct phy_device {
 
 	struct mutex lock;
 
+	/* Serialises attach and detach with bind and unbind */
+	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 v3 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.

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.

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>
---
 drivers/net/phy/phy_device.c | 21 +++++++++++++++++++++
 include/linux/phy.h          |  4 ++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1355ea78c86f..5608e65f5a92 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1155,6 +1155,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);
+	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 */
@@ -1893,6 +1900,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	phy_resume(phydev);
 
+	phydev->attached = true;
 	mutex_unlock(&phydev->bind_lock);
 
 	/**
@@ -1983,6 +1991,8 @@ void phy_detach(struct phy_device *phydev)
 	module_put(phydev->drv_owner);
 	phydev->drv_owner = NULL;
 
+	store_release_wake_up(&phydev->attached, false);
+
 	/* 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
@@ -3869,11 +3879,22 @@ 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 = 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 8903123d25c8..3e005e6a3b65 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -655,6 +655,8 @@ struct phy_oatc14_sqi_capability {
  * @lock:  Mutex for serialization access to PHY
  * @bind_lock: Serialises attach and detach with driver bind and unbind
  * @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
@@ -789,6 +791,8 @@ struct phy_device {
 	/* Serialises attach and detach with bind and unbind */
 	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 v3 0/4] net: phy: make PHY driver unbind safe against attach and use

[Jakub Kicinski]

On Fri, 25 Sep 2026 00:59:46 +0300 Aleksei Sviridkin wrote:
> v2: https://lore.kernel.org/netdev/20260919015340.499675-1-f@lex.la/
> - Retargeted to net, and the race closed with locking rather than a
>   NULL test at attach entry, as asked in review.

AFAIU this is a long standing problem with PHYs, admin-only inflicted
self-harm. Let's switch to net-next until the merge window.

> - The module reference fix is its own patch.
> - New patch 1: a second attach no longer detaches the first consumer.
> - An unbind of an attached PHY now waits for the detach.

BTW what happened in v3? I only see a changelog for v2

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

[Jakub Kicinski]

On Fri, 25 Sep 2026 00:59:49 +0300 Aleksei Sviridkin wrote:
>  /**
> - * 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.
>   */
> -static int phy_probe(struct device *dev)
> +static int __phy_probe(struct device *dev)

You're touching it, you need to make the kdoc fully valid:

Warning: drivers/net/phy/phy_device.c:3709 No description found for return value of '__phy_probe'

> +static int __phy_probe(struct device *dev);

the forward declaration is unavoidable? we can't reorder?
-- 
pw-bot: cr