Re: [PATCH net] net: usb: lan78xx: only stop phylink if the interface is running

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The fix is incomplete.
- [Low] The commit message says 'phylink_start() is only called from
  lan78xx_open()'.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the link does
  not come back after a normal resume.

--

net: usb: lan78xx: only stop phylink if the interface is running

This patch changes lan78xx_disconnect() so it calls phylink_stop() only
when the net device is running. The goal is to stop the phy_stop()
warning that fires when the PHY is in PHY_READY or PHY_HALTED at unplug
time.

> lan78xx_disconnect() unconditionally calls phylink_stop(), but
> phylink_start() is only called from lan78xx_open(). When the device is

[Severity: Low]

Is this statement accurate? In drivers/net/usb/lan78xx.c,
lan78xx_reset_resume() calls phylink_resume() without checking
EVENT_DEV_OPEN or netif_running():

lan78xx_reset_resume() {
    ...
	rtnl_lock();
	phylink_resume(dev->phylink);
	rtnl_unlock();
    ...
}

If the interface was down at suspend time, lan78xx_suspend() never calls
phylink_suspend(). PHYLINK_DISABLE_MAC_WOL is therefore clear, and
phylink_resume() calls phylink_start(). That runs phy_start()
(PHY_READY -> PHY_UP) while netif_running() is false.

Suppose the adapter is then unplugged or unbound. The new check in
lan78xx_disconnect() skips the phylink_stop() that used to balance this
start. phylink_disconnect_phy() and phylink_destroy() then run with
phylink still started.

This probably doesn't crash. phy_disconnect() still stops the PHY because
phy_is_started() is true, and phylink_destroy() cancels the resolve work.
It does leave phylink's start/stop unbalanced at teardown, since
mac_link_down() is not run.

Could the commit message be corrected?

> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 181e8c087655c..6dc622a6ae783 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -4594,7 +4594,8 @@ static void lan78xx_disconnect(struct usb_interface *intf)
>  	net = dev->net;
>  
>  	rtnl_lock();
> -	phylink_stop(dev->phylink);
> +	if (netif_running(net))
> +		phylink_stop(dev->phylink);

[Severity: High]

Can netif_running() be used as a reliable sign that phylink has been
started here?

If the interface is up during system sleep, lan78xx_suspend() stops
phylink but does not clear __LINK_STATE_START:

lan78xx_suspend() {
    ...
		rtnl_lock();
		phylink_suspend(dev->phylink, false);
		rtnl_unlock();
    ...
}

lan78xx_phylink_mac_ops has no .mac_wol_set, so phylink_suspend() always
takes the phylink_stop() branch, and phy_stop() leaves the PHY in
PHY_HALTED. netif_device_detach() and netif_device_attach() only toggle
__LINK_STATE_PRESENT.

The only phylink_resume() call is in lan78xx_reset_resume(), and it runs
only after both lan78xx_reset() and lan78xx_resume() succeed. The plain
lan78xx_resume() never restarts phylink.

So lan78xx_disconnect() can still run with netif_running() true and the
PHY halted, for example:

  - the adapter is unplugged while the system sleeps. usb_resume_interface()
    skips the driver for NOTATTACHED or UNBINDING interfaces.
  - a normal (non-reset) lan78xx_resume(), then unplug or unbind.
  - lan78xx_reset() or lan78xx_resume() fails inside
    lan78xx_reset_resume(), then unplug or unbind.

pl->phydev is still attached at that point because
phylink_disconnect_phy() runs afterwards, so this happens:

lan78xx_disconnect()
  phylink_stop()
    phy_stop()
      WARN(1, "called from state %s\n", ...)    /* HALTED */

This is the same HALTED warning that the commit message says is fixed.

Would it be simpler to remove the phylink_stop() call from
lan78xx_disconnect()? phy_disconnect() already checks phy_is_started().
For a running interface, unregister_netdev()->lan78xx_stop() also calls
phylink_stop().

The other option is to track the real phylink started state and fix the
resume path.

>  	phylink_disconnect_phy(dev->phylink);
>  	rtnl_unlock();
>  

[Severity: High]

This problem was not introduced by this patch, but does the link ever
come back after a normal (non-reset) resume?

When the interface is open, lan78xx_suspend() calls
phylink_suspend(dev->phylink, false). For this driver that always means
phylink_stop(): PHYLINK_DISABLE_STOPPED is set and the PHY moves to
PHY_HALTED.

The only matching phylink_resume() is in lan78xx_reset_resume().
lan78xx_resume() resubmits the interrupt URB and the deferred TX URBs.
It restarts the TX path, NAPI and the stat timer, and rewrites the wake
registers. It never calls phylink_resume() or phylink_start().

A plain USB resume happens when the device keeps power, for example with
s2idle, or with remote wakeup enabled (lan78xx_probe() enables it via
device_set_wakeup_enable()). In that case the PHY stays halted and the
phylink resolver stays disabled.

Carrier doesn't return until the interface is cycled. The ifdown in that
cycle hits the same phy_stop() "called from state HALTED" warning through
lan78xx_stop()->phylink_stop().

This looks like it dates back to e110bc825897 ("net: usb: lan78xx:
Convert to PHYLINK for improved PHY and MAC management"). That commit
notes that PM runtime testing was limited. It is also why the
netif_running() check above is not enough by itself.

Should lan78xx_resume() pair the phylink_suspend() with a
phylink_resume()?

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