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