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