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

[PATCH net-next v4 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 before taking anything.
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. Without 4, an unbind that loses the race to an
attach still leaves that consumer with a PHY whose driver is gone.

On the lock inversion (Paolo): patch 3 says why the PHY's device lock
cannot be used here. Removing the inversion would at least mean moving
the SFP registration out of probe and remove. 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, 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:
- A, phy_remove() waits for the detach (patch 4, kept): the unbind
  blocks, uninterruptibly, until the consumer detaches. For DSA that is
  switch teardown, even with the port down.
- B, stop at patch 3: the oops moves one frame up. 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: an unbind of a PHY in use hangs instead of failing.
It 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.

This series does not close one more window. phy_attach_direct() still
binds the generic 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.

On v4 (OpenWrt 6.18 backport, PROVE_LOCKING), a test module attached the
free EN8811H of a KN-1012 twice without a netdev. The second attach got
-EBUSY, the air_en8811h refcount went 0, 1, 1 and back to 0 after one
detach, and lockdep stayed quiet. Without the series the second attach
succeeded with a duplicate phy_standalone sysfs warning, and one
reference was left after the detach.

On the KN-1012, lan4 is a DSA port on an EN8811H. Without the series,
unbinding air_en8811h returns at once, and the switch teardown faults
later. 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.

Patches 2 to 4 changed in v4 only by the rebase and the fixes listed
below, so their v3 results on the KN-1012 (OpenWrt 6.18 backport,
PROVE_LOCKING) still apply. They are in the v3 cover letter:
https://lore.kernel.org/netdev/20260924215951.2127682-1-f@lex.la/

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

Changes in v4 (since v3):
https://lore.kernel.org/netdev/20260924215951.2127682-1-f@lex.la/
- Retargeted to net-next, as asked. There the detach code lives in
  phy_detach_internal(), and bind_lock also covers the new
  notify_phy_attach() bus hook.
- Patch 1: the test also catches a PHY attached without a netdev, as DSA
  does for its CPU and link ports.
- Patch 3: a Return: section for __phy_probe(). The forward declaration
  stays: dropping it means moving about 200 or over 500 lines, details
  in 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, 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 | 98 ++++++++++++++++++++++++++++++------
 include/linux/phy.h          | 12 +++++
 2 files changed, 95 insertions(+), 15 deletions(-)

-- 
2.53.0

[PATCH net-next v4 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_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,
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 whether the PHY is attached before anything is taken and return
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 v4:
- Rebased on net-next. The message names phy_detach_internal(), which
  the error path calls there.
- The test uses phy_link_change, so it also catches a PHY attached
  without a netdev.

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

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5b13a74e2fa9..0bdd2dc84d81 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1794,6 +1794,12 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	struct module *ndev_owner = NULL;
 	int err;
 
+	/* Set by every attach, with or without a netdev */
+	if (phydev->phy_link_change) {
+		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
@@ -1835,12 +1841,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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 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] The new 'already attached' guard in phy_attach_direct() is a
  lockless check-then-act.
- [Low] The commit message says the old error path ran
  phy_detach_internal() and, for the generic driver, unbound genphy.

--

net: phy: refuse a second attach before touching the PHY

This patch moves the "PHY already attached" test in phy_attach_direct()
to the start of the function, before any module or device references are
taken and before the generic driver is bound. It also tests
phydev->phy_link_change instead of phydev->attached_dev, so that a PHY
attached without a netdev is refused too.

> 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_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,
> suspends the PHY and, for the generic driver, unbinds it, while the
> first consumer goes on using the PHY.

[Severity: Low]
Is this description accurate for a PHY that the generic driver drives?

Since commit 2796ff1e3dca ("net: phy: add flag is_genphy_driven to struct
phy_device"), is_genphy_driven is a field of struct phy_device and no
longer a local variable. On a second attach of a genphy-driven PHY,
d->driver is already set and is_genphy_driven is still 1, so the old code
runs:

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

That runs phy_probe() again on a PHY that is in use. It registers the LED
triggers a second time, resets the link modes and sets the state to
PHY_READY.

device_bind_driver() then fails in driver_sysfs_add() with -EEXIST, since
the links from the first bind are still there. The code then reaches:

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

At that point the first consumer still has the PHY bound and attached.

On this path, the old attached_dev test and phy_detach_internal() are
never reached. Could the message describe this failure mode instead? Does
the Fixes: tag cover how the problem appears on current kernels?

The removed dev_err(&dev->dev, ...) also dereferenced a pointer built from
a NULL dev. That happens when a PHY already attached to a netdev is
attached again without one. Switching to phydev_err() fixes that as well.
Is it worth mentioning in the message?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 5b13a74e2fa92..0bdd2dc84d81a 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1794,6 +1794,12 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	struct module *ndev_owner = NULL;
>  	int err;
>  
> +	/* Set by every attach, with or without a netdev */
> +	if (phydev->phy_link_change) {
> +		phydev_err(phydev, "PHY already attached\n");
> +		return -EBUSY;
> +	}
> +

[Severity: Medium]
Can two concurrent attaches of the same phy_device both get past this
check?

phy_link_change is read here with no lock held. The claim is written much
later in phy_attach_direct():

	phydev->phy_link_change = phy_link_change;
	if (dev) {
		phydev->attached_dev = dev;
		dev->phydev = phydev;

Between the two are try_module_get(), get_device() and, for genphy,
d->driver->probe() and device_bind_driver(), which can sleep.

Not every caller holds RTNL. DSA attaches at probe time through
dsa_user_create()->dsa_user_phy_setup() before rtnl_lock(). CPU and DSA
link ports attach through
dsa_shared_port_phylink_register()->phylink_of_phy_connect(), also without
RTNL. MAC drivers attach from ndo_open with RTNL held.

So the following sequence looks possible:

CPU0                                 CPU1
phy_attach_direct()
  phy_link_change == NULL
                                     phy_attach_direct()
                                       phy_link_change == NULL
  phydev->phy_link_change = ...
  phydev->attached_dev = dev0
  dev0->phydev = phydev
                                       phydev->phy_link_change = ...
                                       phydev->attached_dev = dev1
                                       dev1->phydev = phydev
                                       phy_init_hw(phydev)

CPU1 overwrites attached_dev and phy_link_change and re-runs
phy_init_hw() on the PHY that dev0 is using, while dev0 keeps its phydev
pointer. For genphy, CPU1 re-runs the probe, device_bind_driver() fails,
and error_module_put clears d->driver and is_genphy_driven under the
first consumer.

The later patch in this series, "net: phy: serialise attach and detach
with PHY driver bind and unbind", adds phydev->bind_lock, but this check
stays outside it. An attacher that has passed the check blocks on
bind_lock while the first attach completes. It then takes the lock and
attaches again without checking a second time.

Would it make sense to do this test under bind_lock, or to claim the PHY
atomically, so the check and the assignment cannot be separated?

The reads and writes of phy_link_change here and in phy_detach_internal()
are also plain accesses without READ_ONCE()/WRITE_ONCE(). KCSAN would
probably report them as a data race.

[ ... ]

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

[PATCH net-next v4 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 v4:
- Rebased on net-next. The put is now in phy_detach_internal().

 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 0bdd2dc84d81..b5074599c988 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
@@ -1831,6 +1831,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);
@@ -1938,7 +1939,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:
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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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).

- [Medium] In phy_attach_direct() (drivers/net/phy/phy_device.c), this
  patch adds `phydev->drv_owner = d->driver->owner;` at line 1834, right…

--

net: phy: put the driver module the attach took

This commit adds a drv_owner field to struct phy_device. The field
records which PHY driver module phy_attach_direct() pinned. On detach,
phy_detach_internal() and the attach error path now release that
recorded module instead of reading phydev->mdio.dev.driver again.

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

[ ... ]

> @@ -1831,6 +1831,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;

[Severity: Medium]
Can this record a different module than the one just pinned? The
reference is taken from one load of d->driver, and the owner is saved
from a second, separate load:

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

At this commit, nothing in phy_attach_direct() blocks a driver core
unbind. A sysfs unbind or a module removal runs phy_remove() and then
device_unbind_cleanup(), which calls device_set_driver(dev, NULL).

Suppose that happens between the two loads:

CPU0 (attach, under RTNL)          CPU1 (unbind_store())
try_module_get(d->driver->owner)
                                   phy_remove()
                                   device_unbind_cleanup()
                                     device_set_driver(dev, NULL)
phydev->drv_owner = d->driver->owner

Would the second load then dereference a NULL d->driver? Before this
patch, the non-genphy path did not read d->driver a second time here,
and the read on the detach side checked for NULL.

Now suppose a different driver binds in that window instead. Then
drv_owner names a module that was never pinned. phy_detach_internal()
would call module_put() on that module, and the reference on the
original module would leak. That is the same mismatch this patch is
meant to remove.

The later patch in the series, "net: phy: serialise attach and detach
with PHY driver bind and unbind", closes the unbind window. It holds
phydev->bind_lock across both loads.

The genphy path still has a gap. When d->driver started out NULL, a
concurrent really_probe() does not take bind_lock, so it could still
change d->driver between the two loads.

Would it be simpler to load the owner once into a local variable? Then
the recorded module would always be the one that was pinned:

	struct module *owner = d->driver->owner;

	if (!try_module_get(owner)) {
		...
	}
	phydev->drv_owner = owner;

>  
>  	if (phydev->is_genphy_driven) {
>  		err = d->driver->probe(d);

[ ... ]

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

[PATCH net-next v4 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>
---
Changes in v4:
- A Return: section for __phy_probe().
- Rebased on net-next. The detach side of the lock is now in
  phy_detach_internal(), and the lock also covers the new
  notify_phy_attach() bus hook.
- The forward declaration of __phy_probe() stays. Dropping it means
  moving phy_attach_direct() (about 200 lines) below phy_probe(), or
  phy_probe() and the port and LED helpers it calls (over 500 lines)
  above it. The file already forward-declares genphy_driver for the
  same use in phy_attach_direct().
- The inline comment on bind_lock says what it protects instead of
  repeating its kernel-doc.

 drivers/net/phy/phy_device.c | 58 +++++++++++++++++++++++++++++++-----
 include/linux/phy.h          |  6 ++++
 2 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b5074599c988..544b2da6a1d9 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;
@@ -1702,7 +1705,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);
@@ -1780,11 +1786,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)
@@ -1814,6 +1824,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.
 	 */
@@ -1824,22 +1836,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;
@@ -1922,6 +1940,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
@@ -1934,6 +1954,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;
@@ -1943,7 +1964,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);
@@ -3647,12 +3669,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;
@@ -3800,10 +3824,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..3881a4651da0 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 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
@@ -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

Re: [PATCH net-next v4 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 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 (1) · Low: 0 (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] The genphy fallback in phy_attach_direct() is not serialised
  with driver-core probing, so the race this commit says it closes ('an…
- [Medium] phy_attach_direct() now returns -EAGAIN when d->driver is set
  but phydev->bound is false, meaning a PHY driver is in the middle of…

Pre-existing issues:
- [Medium] In the genphy path of phy_attach_direct(), if __phy_probe(d)
  succeeds and device_bind_driver(d) then fails, error_module_put only…
- [Low] The new kernel-doc for phy_attach_direct() promises '-EBUSY if
  the PHY is already attached'.
- [Low] phy_detach_internal() skips phy_suspend() when `bound` is false.

--

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

This commit adds a bind_lock mutex and a bound flag to each PHY in struct
phy_device. phy_probe() sets bound once the driver's probe has finished,
and phy_remove() clears it before teardown. phy_attach_direct() and
phy_detach_internal() hold bind_lock while they call into the driver, and
an attach that finds a driver set but bound clear fails with -EAGAIN.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index b5074599c988f..544b2da6a1d94 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1702,7 +1705,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);

[Severity: Low]
This regression was not introduced by this patch, but the detach path
still leaves the PHY powered after an unbind.

phy_remove() clears bound. With the later patch in the series that makes
phy_remove() wait for the consumer to detach, the consumer's detach skips
phy_suspend() here, even though phydev->drv is still valid at that point.

phy_remove() never suspends or powers down the PHY. It only asserts the
optional reset line. On boards without a reset GPIO or reset controller,
does the PHY stay powered with its link up after the MAC has detached?

The baseline ends up in the same state. An unbind while attached ran
phy_remove() first and cleared phydev->drv. The later phy_suspend() then
returned early here:

	if (phydev->suspended || !phydrv)
		return 0;

> @@ -1780,11 +1786,15 @@ EXPORT_SYMBOL(phy_detach);

[ ... ]

> + * 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.
>   */

[Severity: Low]
This is a pre-existing issue, but the -EBUSY documented here comes from a
check at the top of phy_attach_direct() that runs before bind_lock is
taken:

	if (phydev->phy_link_change) {
		phydev_err(phydev, "PHY already attached\n");
		return -EBUSY;
	}

The check is not repeated once bind_lock is held. Two attaches of the same
PHY that rtnl does not serialise could both pass it and then run one after
the other under bind_lock. Would the second one then overwrite
attached_dev, dev->phydev and drv_owner?

This needs two consumers attaching the same PHY without rtnl, which is a
misconfiguration. The baseline check was also unlocked.

> @@ -1814,6 +1824,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.
>  	 */
> @@ -1824,22 +1836,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;
>  	}

[Severity: Medium]
Many callers of phy_attach_direct() run from a MAC or DSA .probe(),
through phy_connect(), of_phy_connect(), phylink_of_phy_connect() or DSA
user port setup. They pass the error straight back. The driver core only
retries a probe that returns -EPROBE_DEFER.

The PHY driver's really_probe() may be somewhere between device_set_driver()
and phy_probe() setting bound when the attach happens. This can happen
during:

  - a deferred-probe retry of a PHY waiting on reset GPIOs, clocks or nvmem
  - a module load
  - a sysfs bind

Does the MAC or DSA probe then fail for good with -EAGAIN? The same applies
to the -ENODEV that of_phy_connect() callers usually return for a NULL
result.

Nothing in phy_attach_direct() or its wrappers maps this to -EPROBE_DEFER
or waits for bound to become true.

>  
>  	if (!try_module_get(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;
>  	}

[Severity: High]
The new bound check only applies when d->driver is already set. When
d->driver is NULL, the genphy branch checks and sets it while holding only
bind_lock:

	if (!d->driver) {
		if (phydev->is_c45)
			d->driver = &genphy_c45_driver.mdiodrv.driver;
		else
			d->driver = &genphy_driver.mdiodrv.driver;

It then calls __phy_probe(d) and device_bind_driver(d) without the device
lock.

The driver core never takes bind_lock. In drivers/base/dd.c,
__driver_probe_device() checks dev->driver under device_lock():

	if (dev->driver)
		return -EBUSY;

It then calls pm_runtime_get_suppliers(), pm_runtime_get_sync() on the
parent and device_links_check_suppliers(), all of which can sleep. Only
after that does really_probe() set the driver, without any check:

	device_set_driver(dev, drv);

On the PHY side, the new phy_probe() calls __phy_probe() before it takes
bind_lock:

	err = __phy_probe(dev);
	if (err)
		return err;

	mutex_lock(&phydev->bind_lock);

A driver-core probe of the PHY may overlap with phy_attach_direct() from
ndo_open or from a MAC probe. Examples are a PHY driver module loaded after
the PHY was registered, a deferred-probe retry, or a sysfs bind. Can both
sides then see dev->driver as NULL?

These outcomes look possible:

  - In attach, __phy_probe(d) reads phydev->mdio.dev.driver after
    really_probe() has installed the real driver. The real ->probe() then
    runs twice at the same time, and priv, the LED triggers, the ports and
    the SFP upstream are set up twice.

  - phydev->drv becomes the real driver while attach is in the genphy path.
    phy_init_hw() then calls the real config_init before that driver's
    probe has finished. That is the case the commit message says this patch
    closes.

  - The two driver_sysfs_add() calls collide on the driver link with
    -EEXIST. If attach loses, error_module_put sets d->driver = NULL while
    really_probe() is still running. If really_probe() loses, its
    device_unbind_cleanup() clears dev->driver after attach's
    driver_bound() has already put the device on the driver's klist.

The kernel-doc of device_bind_driver() says "This function must be called
with the device lock held." The new @bind_lock kernel-doc in
include/linux/phy.h says "Serialises attach and detach with driver bind and
unbind". Does either hold for the genphy fallback path?

The later patch in the series, "net: phy: make an unbind wait for the
attached consumer to detach", does not touch this path.

The device lock can't be taken here, so would this need another mechanism?
One option is a check that the driver core honours before
device_set_driver(). Another is to do the genphy bind through the driver
core.

[ ... ]

> @@ -1943,7 +1964,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);

[Severity: Medium]
This isn't a bug introduced by this patch, but the genphy error path does
not undo __phy_probe(). If __phy_probe(d) succeeds and device_bind_driver(d)
then fails, the error_module_put unwind only drops the module reference
and clears drv_owner, is_genphy_driven and d->driver.

Does this leak the LED triggers registered by phy_led_triggers_register()?
The phy_port entries and any SFP upstream registration made by
__phy_probe() also stay in place. phydev->drv still points at genphy, and
the reset line stays deasserted.

The baseline called d->driver->probe(d), which was phy_probe(), with the
same cleanup. The trigger is a driver_sysfs_add() failure such as -ENOMEM,
or a bus notifier refusing the bind.

[ ... ]

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

[PATCH net-next v4 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>
---
Changes in v4:
- Rebased on net-next. A line over 80 columns wrapped.

 drivers/net/phy/phy_device.c | 22 ++++++++++++++++++++++
 include/linux/phy.h          |  4 ++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 544b2da6a1d9..8a7cd53f9a14 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);
+	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 */
@@ -1737,6 +1744,8 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
 	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
@@ -1940,6 +1949,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	phy_resume(phydev);
 
+	phydev->attached = true;
 	mutex_unlock(&phydev->bind_lock);
 
 	/**
@@ -3843,11 +3853,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 = 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 3881a4651da0..1b7e7bf124cc 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 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
@@ -807,6 +809,8 @@ struct phy_device {
 	/* Protects bound */
 	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 v4 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 3 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 1 (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).

- [High] phy_remove() in drivers/net/phy/phy_device.c (lines 3866-3871)
  now calls wait_var_event(&phydev->attached,…
- [High] net: phy: ABBA deadlock between rtnl_lock and device_lock
- [Low] phy_remove() reads phydev->attached with a plain load under
  bind_lock (line 3860: 'attached = phydev->attached &&…

Pre-existing issues:
- [Low] When an unbind is waiting in phy_remove(), it has already set
  phydev->bound = false (line 3859).

--

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

This commit makes phy_remove() wait in wait_var_event() until
phy_detach() has run whenever the PHY is attached. The goal is to stop
the driver being removed from a PHY that a consumer is still using.
phy_device_remove() sets a new removing flag and wakes any waiter, so
mdiobus_unregister() does not hang.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 544b2da6a1d94..8a7cd53f9a144 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -3843,11 +3853,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;

[Severity: Low]
This isn't a bug introduced by this patch, but with the wait added
below, bound is cleared before the consumer detaches. As a result, the
detach that ends the wait skips the power-down in phy_detach_internal():

	mutex_lock(&phydev->bind_lock);
	if (phydev->bound)
		phy_suspend(phydev);
	mutex_unlock(&phydev->bind_lock);

At that point the driver is still present.

After the wait, phy_remove() only calls drv->remove() and
phy_device_reset(phydev, 1). mdio_device_reset() returns early when
there is no reset_gpio or reset_ctrl. A PHY that was attached but never
started, such as a DSA port that was never brought up, would keep the
power-up from phy_resume() in phy_attach_direct().

Before this series the unbind cleared phydev->drv first, and
phy_suspend() returned early on the NULL driver. So the PHY was not
powered down then either.

Could the detach still suspend the PHY in this state, since the driver
is now valid until the detach finishes?

> +	attached = phydev->attached && !phydev->removing;

[Severity: Low]
Can this plain read of phydev->attached race with the store in
phy_detach_internal()?

	store_release_wake_up(&phydev->attached, false);

That store runs after phy_detach_internal() has dropped bind_lock.
Holding bind_lock here does not serialise against it, so KCSAN would
probably report a plain read racing with a marked write.

phy_attach_direct() also writes attached = true with a plain store,
while the wait condition reads it with READ_ONCE().

A stale value does no harm at runtime, because wait_var_event()
re-checks the condition. Should the snapshot use READ_ONCE(), or should
the clear happen under bind_lock, so that the annotations match?

>  	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 this wake cause an ABBA deadlock between rtnl_lock() and the
PHY's device lock when the PHY is driven by genphy and has an SFP bus?

The unbinding task sleeps in phy_remove() with device_lock() of the
PHY held. The consumer usually detaches under rtnl_lock(), for example
ifdown through phy_disconnect(). This store wakes the unbinder, but
phy_detach_internal() then carries on to the genphy branch just below
and takes the same device lock:

CPU0 (sysfs unbind)              CPU1 (ifdown, rtnl_lock held)
device_lock(phydev)
phy_remove()
  wait_var_event()
                                 phy_detach_internal()
                                   store_release_wake_up()
  (woken)                          device_release_driver()
                                     device_lock(phydev)
  phy_sfp_release()
    sfp_bus_del_upstream()
      rtnl_lock()

CPU1 holds rtnl_lock() and waits for the device lock. CPU0 holds the
device lock and waits for rtnl_lock(). After that every rtnl user on
the system hangs as well. Unbinding needs root, but the result is a
wedged network stack rather than just a stuck sysfs write.

Before this patch phy_remove() never slept. The
device_release_driver() here only had to wait for a short remove, so
no rtnl dependency could form behind it.

Moving the store after device_release_driver() would not help either.
The detach would then block on a device lock held by a task that is
waiting for that very store. How should the genphy-driven case be
handled so that the device lock and rtnl_lock() cannot nest in
opposite orders here?

[Severity: High]
Could this leave the unbinding task in D state for an unbounded time,
with device_lock(phydev) held the whole time?

wait_var_event() sleeps in TASK_UNINTERRUPTIBLE and has no timeout.
phy_remove() is called with the device lock held:

unbind_store()
  device_driver_detach()
    device_release_driver_internal()    <- device_lock(phydev)
      __device_release_driver()
        device_remove()
          phy_remove()
            wait_var_event()

Only phy_detach_internal() and phy_device_remove() can end the wait,
and the unbinding task can trigger neither.

Some consumers connect at probe, such as DSA user ports or MACs that
call phy_connect() in probe. They detach only when their own driver is
torn down. dsa_switch_shutdown() does not disconnect the user port
PHYs.

While the wait lasts, device_shutdown() in drivers/base/core.c still
takes the lock of every device:

	if (parent)
		device_lock(parent);
	device_lock(dev);

The PHY is registered after its MAC or switch, so the backwards walk of
devices_kset reaches the PHY first. Would reboot, poweroff and kexec
then block forever on the PHY's device lock?

Suspend looks affected as well. The freezer cannot freeze the D-state
task, and device_prepare(), device_suspend() and device_resume() in
drivers/base/power/main.c also take device_lock(dev).

There may also be a deadlock through device links, although this case
is more speculative. device_links_unbind_consumers() unbinds consumers
one at a time:

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

Suppose the PHY and its MAC or switch are both managed consumers of the
same supplier, such as a GPIO or IRQ provider, and the loop reaches the
PHY first. phy_remove() would then wait for a detach that only happens
when the same loop later unbinds the MAC or switch.

__mdiobus_register() sets FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD on
bus->parent. That only suppresses links to bus->parent, not links to
other suppliers.

Before this patch the same unbind returned at once. The commit message
says:

    An unbind of a PHY in use
    now blocks until the consumer lets go

It does not say that the wait is uninterruptible, that device_lock
stays held during it, or that reboot and suspend are affected. Is this
the intended behaviour for these paths?

> +	}
> +
>  	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/20261001130120.104628-1-f%40lex.la