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

[Oleksij Rempel <o.rempel@pengutronix.de>]

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 |