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

[netdev-bot+sashiko@kernel.org]

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