[PATCH net v2] net: usb: lan78xx: let unregister_netdev() stop phylink

[PATCH net v2] net: usb: lan78xx: let unregister_netdev() stop phylink

[YuanZhe Liu]

lan78xx_disconnect() stops phylink before unregistering the net device.
phylink_start() is only called from lan78xx_open(), so when the device is
unplugged before the interface has been brought up, the attached PHY is
still in PHY_READY and phy_stop() reports:

  called from state READY
  WARNING: drivers/net/phy/phy.c:1670 at phy_stop+0xad/0x380

The same warning is emitted after an ifup/ifdown cycle, where the PHY is
already in PHY_HALTED.

unregister_netdev() already closes a running device, which calls
lan78xx_stop() and phylink_stop() with dev->dev_mutex held and after the
URBs have been terminated. Unregister first and disconnect the PHY
afterwards, so the interface is stopped exactly once and only while it is
running, and the net device can no longer be opened while its phylink
instance has no PHY attached.

Fixes: e110bc825897 ("net: usb: lan78xx: Convert to PHYLINK for improved PHY and MAC management")
Cc: stable@vger.kernel.org
Co-developed-by: Bin Yu <byu@xidian.edu.cn>
Signed-off-by: Bin Yu <byu@xidian.edu.cn>
Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
---
v2:
 - Drop the explicit phylink_stop() and call unregister_netdev() first, as
   pointed out in review. A running interface is stopped by lan78xx_stop()
   with dev->dev_mutex held and after the URBs have been terminated; an
   interface that was never started is not stopped at all.
 - The PHY interrupt mapping is released by lan78xx_remove_irq_domain() in
   lan78xx_unbind(), not by phylink_disconnect_phy(); that ordering is
   unchanged by this patch.
 - Link: https://lore.kernel.org/r/20261029160204.2567-1-25031212351@stu.xidian.edu.cn

 drivers/net/usb/lan78xx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 5655941f1478..c1f3141326ac 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -4580,13 +4580,12 @@ static void lan78xx_disconnect(struct usb_interface *intf)
 
 	net = dev->net;
 
+	unregister_netdev(net);
+
 	rtnl_lock();
-	phylink_stop(dev->phylink);
 	phylink_disconnect_phy(dev->phylink);
 	rtnl_unlock();
 
-	unregister_netdev(net);
-
 	timer_shutdown_sync(&dev->stat_monitor);
 	set_bit(EVENT_DEV_DISCONNECT, &dev->flags);
 	cancel_delayed_work_sync(&dev->wq);
-- 
2.34.1

Re: [PATCH net v2] net: usb: lan78xx: let unregister_netdev() stop phylink

[Andrew Lunn]

> Fixes: e110bc825897 ("net: usb: lan78xx: Convert to PHYLINK for improved PHY and MAC management")

I asked why you picked this Fixes: tag. Did you reply?

  Andrew

Re: [PATCH net v2] net: usb: lan78xx: let unregister_netdev() stop phylink

[YuanZhe Liu]

Hi Andrew,

Yes, I did, but it ended up at the bottom of my reply to the sashiko-bot
review, which was not the right place for it. Here it is directly.

e110bc825897 is the commit that introduced the call which warns. Before
it, lan78xx_disconnect() did:

	unregister_netdev(net);
	...
	phydev = net->phydev;
	phy_disconnect(net->phydev);

and phy_stop() only existed in lan78xx_stop(), guarded by
"if (net->phydev)". So disconnect never called phy_stop() on a PHY that
had not been started, and the warning was not reachable from this path.

e110bc825897 converted the driver to phylink: it moved
unregister_netdev() after the phylink teardown and added an
unconditional phylink_stop() before phylink_disconnect_phy(). Since
phylink_stop() reaches phy_stop(), which warns unless the PHY is at
least PHY_UP, unplugging before ifup started producing "called from
state READY".

	git log -S'phylink_stop(dev->phylink)' -- drivers/net/usb/lan78xx.c

returns e110bc825897 and nothing else apart from my own change, so that
line has not been touched since it was added.

This is also why v2 puts unregister_netdev() back in front: it restores
the pre-conversion ordering, so an interface that was never started is
not stopped at all, and a running one is stopped by lan78xx_stop().

Thanks,
YuanZhe

Re: [PATCH net v2] net: usb: lan78xx: let unregister_netdev() stop phylink

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0 (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 reordered disconnect still calls phy_stop() on a halted
  PHY after a normal system suspend/resume of an open interface.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but lan78xx has an
  AB-BA lock inversion between dev->dev_mutex and rtnl_mutex.
- [Low] This problem wasn't introduced by this patch, but lan78xx_stop()
  uses timer_pending() to decide whether to synchronize with the…

--

net: usb: lan78xx: let unregister_netdev() stop phylink

This patch reorders lan78xx_disconnect() so that unregister_netdev() runs
first and phylink_disconnect_phy() runs after it. The explicit
phylink_stop() call is dropped, so that phylink is only stopped through
lan78xx_stop() when the interface is running. The aim is to avoid the
phy_stop() "called from state READY/HALTED" warning on unplug.

> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 181e8c087655c..4780aef87736e 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -4593,13 +4593,12 @@ static void lan78xx_disconnect(struct usb_interface *intf)
>
>  	net = dev->net;
>
> +	unregister_netdev(net);
> +

[Severity: Medium]
The commit message says the interface is "stopped exactly once and only
while it is running". Does that still hold after a normal system suspend
and resume of an open interface?

Consider this sequence:

ifup
  lan78xx_open()
    phylink_start()

system suspend
  lan78xx_suspend()
    phylink_suspend(dev->phylink, false)
      phylink_stop()
        phy_stop()      /* PHY now PHY_HALTED, IFF_UP still set */

lan78xx_phylink_mac_ops has no .mac_set_wol, so mac_wol is false and
phylink_suspend() goes through phylink_stop().

normal (non-reset) resume
  lan78xx_resume()      /* no phylink_resume() or phylink_start() */

phylink_resume() is only called from lan78xx_reset_resume(). Because
pc->mac_managed_pm = true, phylib does not restart the PHY either.

unplug
  lan78xx_disconnect()
    unregister_netdev()
      dev_close()
        lan78xx_stop()
          phylink_stop()
            phy_stop()

That reaches this check in phy_stop() with the state HALTED:

	if (!phy_is_started(phydev) && phydev->state != PHY_DOWN &&
	    phydev->state != PHY_ERROR) {
		WARN(1, "called from state %s\n",

Before this patch the warning fired twice in this sequence. Now it fires
once.

The same gap also leaves the link down after such a resume, and a plain
ifdown afterwards hits the same warning.

Would it make sense to also restart phylink on the normal resume path in
lan78xx_resume(), in addition to this reordering?

[Severity: High]
This isn't a bug introduced by this patch, but the commit message relies on
lan78xx_stop() taking dev->dev_mutex under RTNL via unregister_netdev().
Is there a lock order inversion between dev->dev_mutex and rtnl_mutex?

lan78xx_open() and lan78xx_stop() run as ndo_open/ndo_stop with RTNL
already held by __dev_open()/__dev_close_many(), and then take dev_mutex:

  rtnl_mutex -> dev->dev_mutex

lan78xx_suspend() does the reverse for an open interface:

	mutex_lock(&dev->dev_mutex);
	...
		rtnl_lock();
		phylink_suspend(dev->phylink, false);
		rtnl_unlock();

  dev->dev_mutex -> rtnl_mutex

The rtnl_lock() block in lan78xx_suspend() came in with e110bc825897, the
commit named in the Fixes tag.

With CONFIG_PROVE_LOCKING, ifup followed by a system suspend with the
interface open looks like it would produce a circular locking dependency
report.

A real deadlock would need a non-frozen context holding RTNL to call
ndo_open/ndo_stop on this device while lan78xx_suspend() holds dev_mutex.
One example is netns teardown in cleanup_net() moving the device back to
init_net, which goes through dev_close() and then lan78xx_stop().

[Severity: Low]
This is a pre-existing issue, but lan78xx_stop() is now the only place
that stops a running interface on disconnect. lan78xx_stop() uses
timer_pending() to decide whether to synchronize with the stat_monitor
timer:

	if (timer_pending(&dev->stat_monitor))
		timer_delete_sync(&dev->stat_monitor);
	...
	clear_bit(EVENT_STAT_UPDATE, &dev->flags);

	cancel_delayed_work_sync(&dev->wq);

Can the stats timer be re-armed after ifdown?

If lan78xx_delayedwork() has already passed test_bit(EVENT_STAT_UPDATE)
and is inside lan78xx_update_stats(), the timer is not pending, so the
sync delete is skipped. cancel_delayed_work_sync() then waits for the
work, and the work finishes with:

		mod_timer(&dev->stat_monitor,
			  jiffies + (STAT_UPDATE_TIMER * dev->delta));

lan78xx_delayedwork() has no EVENT_DEV_OPEN check. So the timer and work
keep cycling while the interface is down, autoresuming the device and
issuing USB stats reads.

The same thing can happen when lan78xx_stat_monitor() is already running
on another CPU. Its lan78xx_defer_kevent(dev, EVENT_STAT_UPDATE) can land
after the clear_bit() or after cancel_delayed_work_sync().

An unconditional timer_delete_sync() would not cover the first case on
its own. The work would also need to be cancelled before the timer is
shut down.

On disconnect, timer_shutdown_sync(), EVENT_DEV_DISCONNECT and the
cancel_delayed_work_sync() in lan78xx_disconnect() still clean this up.

>  	rtnl_lock();
> -	phylink_stop(dev->phylink);
>  	phylink_disconnect_phy(dev->phylink);
>  	rtnl_unlock();
>
> -	unregister_netdev(net);
> -
>  	timer_shutdown_sync(&dev->stat_monitor);
>  	set_bit(EVENT_DEV_DISCONNECT, &dev->flags);
>  	cancel_delayed_work_sync(&dev->wq);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261031165213.2738-1-25031212351%40stu.xidian.edu.cn