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

[Fiona Klute <fiona.klute@gmx.de>]

Am 10.04.25 um 16:43 schrieb Andrew Lunn:
>>> 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.

I just sent the new patch, I hope I got that right, thank you! Test got 
to about 650 successful boots by now. :-)

Best regards,
Fiona