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

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

Am 10.03.25 um 19:06 schrieb Alan Stern:
> On Mon, Mar 10, 2025 at 05:59:31PM +0100, Fiona Klute wrote:
>> If a new reset event appears before the previous one has been
>> processed, the device can get stuck into a reset loop. This happens
>> rarely, but blocks the device when it does, and floods the log with
>> messages like the following:
>>
>>    lan78xx 2-3:1.0 enp1s0u3: kevent 4 may have been dropped
>>
>> The only bit that the driver pays attention to in the interrupt data
>> is "link was reset". If there's a flapping status bit in that endpoint
>> data (such as if PHY negotiation needs a few tries to get a stable
>> link), polling at a slower rate allows the state to settle.
>>
>> This is a simplified version of a patch that's been in the Raspberry
>> Pi downstream kernel since their 4.14 branch, see also:
>> https://github.com/raspberrypi/linux/issues/2447
>>
>> Signed-off-by: Fiona Klute <fiona.klute@gmx.de>
>> Cc: kernel-list@raspberrypi.com
>> Cc: stable@vger.kernel.org
>> ---
>> For the stable crew: I've *tested* the patch with 6.12.7 and 6.13.5 on
>> a Revolution Pi Connect 4 (Raspberry Pi CM4 based device with built-in
>> LAN7800 as second ethernet port), according to the linked issue for
>> the RPi downstream kernel the problem should be present in all
>> maintained longterm kernel versions, too (based on how long they've
>> carried a patch).
>>
>>   drivers/net/usb/lan78xx.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
>> index a91bf9c7e31d..7bf01a31a932 100644
>> --- a/drivers/net/usb/lan78xx.c
>> +++ b/drivers/net/usb/lan78xx.c
>> @@ -173,6 +173,12 @@
>>   #define INT_EP_GPIO_1			(1)
>>   #define INT_EP_GPIO_0			(0)
>>
>> +/* highspeed device, so polling interval is in microframes (eight per
>> + * millisecond)
>> + */
>> +#define INT_URB_MICROFRAMES_PER_MS	8
>> +#define MIN_INT_URB_INTERVAL_MS		8
>> +
>>   static const char lan78xx_gstrings[][ETH_GSTRING_LEN] = {
>>   	"RX FCS Errors",
>>   	"RX Alignment Errors",
>> @@ -4527,7 +4533,11 @@ static int lan78xx_probe(struct usb_interface *intf,
>>   	if (ret < 0)
>>   		goto out4;
>>
>> -	period = ep_intr->desc.bInterval;
>> +	period = max(ep_intr->desc.bInterval,
>> +		     MIN_INT_URB_INTERVAL_MS * INT_URB_MICROFRAMES_PER_MS);
>
> This calculation is completely wrong.  For high-speed interrupt
> endpoints, the bInterval value is encoded using a logarithmic scheme.
> The actual interval in microframes is given by 2^(bInterval - 1) (see
> Table 9-13 in the USB 2.0 spec).  Furthermore, when the value is passed
> to usb_fill_int_urb(), the interval argument must be encoded in the same
> way (see the kerneldoc for usb_fill_int_urb() in include/linux/usb.h).
>
> The encoded value corresponding to 8 ms is 7, not 64, since 8 ms = 64
> uframes and 64 = 2^(7-1).

Thanks, I'll have to fix that if I send a revision. Though after Andrew
Lunn's response and seeing the bug again today despite the patch after a
few hundred good boots I suspect a different fix is needed.

>> +	dev_info(&intf->dev,
>> +		 "interrupt urb period set to %d, bInterval is %d\n",
>> +		 period, ep_intr->desc.bInterval);
>
> I doubt that this dev_info() will be very helpful to anyone (in addition
> to being wrong since the value in "period" is not the actual period).
I figured it'd be useful to note that the interval has been modified,
especially for possible debugging.

Best regards,
Fiona