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

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

[YuanZhe Liu]

lan78xx_disconnect() unconditionally calls phylink_stop(), but
phylink_start() is only called from lan78xx_open(). 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. Call phylink_stop() only when the network device is
running, which is when phylink_start() has been called.

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>
---
 drivers/net/usb/lan78xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 5655941f1478..68e61004b3d7 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -4581,7 +4581,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);
 	phylink_disconnect_phy(dev->phylink);
 	rtnl_unlock();
 
-- 
2.34.1

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

[YuanZhe Liu]

Hi Andrew,

Could you please review this patch?

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

It calls phylink_stop() during lan78xx disconnect only when the network
interface is running.

The patch is unchanged. I am copying this address for visibility.

Original submission:
https://lore.kernel.org/r/20261029160204.2567-1-25031212351@stu.xidian.edu.cn

Thanks,
YuanZhe

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

[Andrew Lunn]

On Sat, Oct 31, 2026 at 01:38:41AM +0800, YuanZhe Liu wrote:
> Hi Andrew,
> 
> Could you please review this patch?

You should respond to what the sashiko-bot@kernel.org has said.

    Andrew

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

[Andrew Lunn]

On Fri, Oct 30, 2026 at 12:02:04AM +0800, YuanZhe Liu wrote:
> lan78xx_disconnect() unconditionally calls phylink_stop(), but
> phylink_start() is only called from lan78xx_open(). 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. Call phylink_stop() only when the network device is
> running, which is when phylink_start() has been called.
> 
> Fixes: e110bc825897 ("net: usb: lan78xx: Convert to PHYLINK for improved PHY and MAC management")

Why did you pick this commit?

    Andrew

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

[YuanZhe Liu]

On Fri, Oct 30, 2026 at 04:02:36PM +0000, sashiko-bot@kernel.org wrote:
> Does calling phylink_stop() and phylink_disconnect_phy() before
> unregister_netdev(net) in drivers/net/usb/lan78xx.c:lan78xx_disconnect()
> cause out-of-order teardown, a duplicate phylink_stop() call, and a race
> window when rtnl_lock() is dropped?

Yes, the ordering is wrong. v2 drops the explicit phylink_stop() from the
disconnect path and unregisters the net device first:

	unregister_netdev(net);

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

unregister_netdev() closes a running device, so phylink_stop() is done by
lan78xx_stop() with dev->dev_mutex held and after the TX/RX URBs have been
terminated, which is the same sequence as a normal ifdown. When the
interface was never started no phylink_stop() happens at all, and that is
exactly the case phy_stop() complains about with "called from state READY".

This also removes the second phylink_stop() invocation and the window in
which the net device was still registered with no PHY attached.

On the remaining points:

- The duplicate phylink_stop() was harmless in practice, because after
  phylink_disconnect_phy() pl->phydev is NULL and PHYLINK_DISABLE_STOPPED
  is already set. It is gone now anyway.

- The PHY interrupt is not released by phylink_disconnect_phy(). The virq
  created by irq_create_mapping() is disposed in lan78xx_remove_irq_domain(),
  called from lan78xx_unbind(), which also clears dev->domain_data.phyirq;
  lan78xx_status() tests that field before generic_handle_irq_safe(). The
  intr URB is killed later in lan78xx_disconnect(), and that ordering is
  unchanged by this patch.

- Andrew also asked why the Fixes tag points at e110bc825897: that commit
  added both the phylink instance and the unconditional phylink_stop() call
  in lan78xx_disconnect(), and

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

  shows that the line has not been touched since.

v2 is sent as a separate thread.

Thanks,
YuanZhe

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

[Oleksij Rempel]

Hi,

Thank you for your patch, i'm testing it right now on my HW.

On Sun, Nov 01, 2026 at 12:52:34AM +0800, YuanZhe Liu wrote:
> On Fri, Oct 30, 2026 at 04:02:36PM +0000, sashiko-bot@kernel.org wrote:
> > Does calling phylink_stop() and phylink_disconnect_phy() before
> > unregister_netdev(net) in drivers/net/usb/lan78xx.c:lan78xx_disconnect()
> > cause out-of-order teardown, a duplicate phylink_stop() call, and a race
> > window when rtnl_lock() is dropped?
> 
> Yes, the ordering is wrong. v2 drops the explicit phylink_stop() from the
> disconnect path and unregisters the net device first:
> 
> 	unregister_netdev(net);
> 
> 	rtnl_lock();
> 	phylink_disconnect_phy(dev->phylink);
> 	rtnl_unlock();
> 
> unregister_netdev() closes a running device, so phylink_stop() is done by
> lan78xx_stop() with dev->dev_mutex held and after the TX/RX URBs have been
> terminated, which is the same sequence as a normal ifdown. When the
> interface was never started no phylink_stop() happens at all, and that is
> exactly the case phy_stop() complains about with "called from state READY".
> 
> This also removes the second phylink_stop() invocation and the window in
> which the net device was still registered with no PHY attached.
> 
> On the remaining points:
> 
> - The duplicate phylink_stop() was harmless in practice, because after
>   phylink_disconnect_phy() pl->phydev is NULL and PHYLINK_DISABLE_STOPPED
>   is already set. It is gone now anyway.
> 
> - The PHY interrupt is not released by phylink_disconnect_phy(). The virq
>   created by irq_create_mapping() is disposed in lan78xx_remove_irq_domain(),
>   called from lan78xx_unbind(), which also clears dev->domain_data.phyirq;
>   lan78xx_status() tests that field before generic_handle_irq_safe(). The
>   intr URB is killed later in lan78xx_disconnect(), and that ordering is
>   unchanged by this patch.

If I see it correctly, in the practices it is a dead code, please see:
30a41ed32d30 ("net: phy: microchip: force IRQ polling mode for lan88xx")

> - Andrew also asked why the Fixes tag points at e110bc825897: that commit
>   added both the phylink instance and the unconditional phylink_stop() call
>   in lan78xx_disconnect(), and
> 
>     git log -S'phylink_stop(dev->phylink)' -- drivers/net/usb/lan78xx.c
> 
>   shows that the line has not been touched since.

Re-testing it right now.

Do you have access to a real hw or you testing it on QEMU?

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[YuanZhe Liu]

Hi Oleksij,

Thanks for testing this on real hardware - that is worth much more than
what I can do here.

To answer your question first: I have no LAN7800/LAN7850/LAN7801 board.
I test on a virtual device that closely matches this driver, generated by
an automated virtual-device generation tool we are developing for this
purpose; our plan is to run targeted tests against all USB drivers in the
Linux kernel with it. The WARN reproduces reliably there, but I cannot
tell you how the patch behaves with a real PHY.

You are right about the interrupt path, and it is a better explanation
than the one I gave. 30a41ed32d30 removed config_intr() and
handle_interrupt() from the internal LAN88xx driver, so
phy_drv_supports_irq() is false and phy_attach_direct() overwrites the
irq that lan78xx_phy_init() has just set:

  drivers/net/usb/lan78xx.c:2896	phydev->irq = dev->domain_data.phyirq;
  drivers/net/phy/phy_device.c:1854	if (!phy_drv_supports_irq(phydev->drv) &&
  drivers/net/phy/phy_device.c:1855	    phy_interrupt_is_valid(phydev))
  drivers/net/phy/phy_device.c:1855		phydev->irq = PHY_POLL;

phylink_bringup_phy() then skips phy_request_interrupt() because
phy_interrupt_is_valid() is false (phylink.c:2213), so nobody calls
request_threaded_irq() for the mapping, lan78xx_irq_unmask() never runs
for it and the PHY bit is never programmed into INT_EP_CTL through the
irqchip. So for the internal PHY the PHY interrupt path is dead, and my
remark about lan78xx_remove_irq_domain() was describing a path that does
not execute in this configuration. Thank you for pointing it out.

That does not affect this patch. The warning comes from phy_stop()
reaching a PHY that was never started, which has nothing to do with how
the PHY interrupt is delivered, and the patch touches no interrupt
handling at all. It only moves unregister_netdev() ahead of the phylink
teardown, so that the interface is stopped by lan78xx_stop() when it was
running and not stopped when it was not. The dead interrupt path behaves
exactly as before with the patch applied.

It is also only dead for the internal PHY. For LAN7801 with an external
RGMII PHY (lan78xx_get_phy(), is_internal = false) the PHY driver can
provide config_intr and handle_interrupt, and then phyirq is really
requested.

I kept the change small on purpose so it can go to stable. I would
rather remove the dead PHY interrupt plumbing for the internal PHY in a
separate net-next patch than mix it into this fix.

If your test still produces the warning with the patch applied, could you
send me the trace? Then I can see which path it takes on your board.

Thanks,
YuanZhe

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

[Oleksij Rempel]

On Fri, Oct 30, 2026 at 12:02:04AM +0800, YuanZhe Liu wrote:
> lan78xx_disconnect() unconditionally calls phylink_stop(), but
> phylink_start() is only called from lan78xx_open(). 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. Call phylink_stop() only when the network device is
> running, which is when phylink_start() has been called.
> 
> 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>

Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>

I reproduced the phy_stop() warning on a LAN7850 behind an i.MX8MP, by
unbinding the driver or by writing 0 to the USB device's "authorized"
attribute:

- interface never opened: "called from state READY"
- after ifup and ifdown: "called from state HALTED"
- ifup/ifdown loop during the removal: "called from state HALTED"

On kernels before the phylink conversion, none of these three cases hit
the phy_stop() warning.

With this patch, none of them warns, on net-next and on ba28ef3ccfa9
("Merge branch 'convert-lan78xx-driver-to-the-phylink'") with
312c816c6bc3 ("net: usb: lan78xx: fix WARN in __netif_napi_del_locked on
disconnect") applied. Removals with the interface open, sending, with
link up, link down or link flapping did not warn either. I ran each
case twice per patched kernel, with lockdep, KASAN and kmemleak enabled.
After re-adding the device the interface came up again.

The suspend/resume problems in the Sashiko review are not caused or
fixed by this patch.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[netdev-bot+sashiko]

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

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

[Yuanzhe Liu]

Oleksij,

Thanks for the testing, and for both tags.

One thing to sort out first: they arrived on the v1 posting, and the patch
changed shape in v2, which I posted separately on 2026-10-31:

  https://lore.kernel.org/netdev/20261031165213.2738-1-25031212351@stu.xidian.edu.cn/

v2 drops the explicit phylink_stop() and moves unregister_netdev() ahead of
the phylink teardown instead, so that closing the netdev goes through the
normal lan78xx_stop() path. That removes the netif_running() test; and the
three cases you reproduced -- never opened, after ifdown, and ifup/ifdown
during removal -- are all cases where the interface is not running at that
point, so v2 does not call phylink_stop() there either, same as v1.

The path that does differ is a removal with the interface up: v1 stopped
phylink directly from lan78xx_disconnect(), v2 gets it from lan78xx_stop()
via unregister_netdev(). Your "interface open, sending, link up, link down,
link flapping" runs cover that path in its v1 form only. If you want to
re-run them against v2 before the tags stand on it, that would be welcome;
otherwise I will take your Tested-by and Reviewed-by as applying to v2 as
posted -- tell me which you prefer.

It is also the approach 312c816c6bc3 took for the NAPI teardown, so the
shape is not new to this driver.

Thanks for checking the pre-phylink kernels too -- that is the direct
confirmation of the Fixes: tag Andrew asked about, so I think that question
is settled.

Thanks,
YuanZhe