Re: [PATCH] net: usb: lan78xx: Enforce a minimum interrupt polling period

[Andrew Lunn <andrew@lunn.ch>]

> > Ah, O.K. This tells me the PHY is a lan88xx. And there is a workaround
> > involved for an issue in this PHY. Often PHYs are driven by polling
> > for status changes once per second. Not all PHYs/boards support
> > interrupts. It could be this workaround has only been tested with
> > polling, not interrupts, and so is broken when interrupts are used.
> > 
> > As a quick hack test, in lan78xx_phy_init()
> > 
> > 	/* if phyirq is not set, use polling mode in phylib */
> > 	if (dev->domain_data.phyirq > 0)
> > 		phydev->irq = dev->domain_data.phyirq;
> > 	else
> > 		phydev->irq = PHY_POLL;
> > 
> > Hard code phydev->irq to PHY_POLL, so interrupts are not used.
> > 
> > See if you can reproduce the issue when interrupts are not used.
> It took a while, but I'm fairly confident now that the workaround works,
> I've had over 1000 boots on the hardware in question and didn't see the
> bug. Someone going by upsampled reported the same in the issue on Github
> [1], and pointed out that people working with some Nvidia board and a
> LAN7800 USB device came to the same conclusion a while ago [2].
> 
> That leaves me with the question, what does that mean going forward?
> Would it make sense to add a quirk to unconditionally force polling on
> lan88xx, at least until/unless the interrupt handling can be fixed?

I don't think you need a quirk:

static struct phy_driver microchip_phy_driver[] = {
{
        .phy_id         = 0x0007c132,
        /* This mask (0xfffffff2) is to differentiate from
         * LAN8742 (phy_id 0x0007c130 and 0x0007c131)
         * and allows future phy_id revisions.
         */
        .phy_id_mask    = 0xfffffff2,
        .name           = "Microchip LAN88xx",

        /* PHY_GBIT_FEATURES */

        .probe          = lan88xx_probe,
        .remove         = lan88xx_remove,

        .config_init    = lan88xx_config_init,
        .config_aneg    = lan88xx_config_aneg,
        .link_change_notify = lan88xx_link_change_notify,

        .config_intr    = lan88xx_phy_config_intr,
        .handle_interrupt = lan88xx_handle_interrupt,

Just remove .config_intr and .handle_interrupt. If these are not
provided, phylib will poll, even if an interrupt number has been
passed. And since these functions are not shared with any other PHY,
you can remove them.

Please write a good commit message, we want it clear why they where
removed, to try to prevent somebody putting them back again.

And please aim this for net, not net-next:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

The change will then get back ported to stable kernels.

	Andrew