Re: [PATCH] usb-core: Add MS_INTR_BINTERVAL USB quirk

[Alan Stern]

On Sun, 12 Mar 2017, Samuel Thibault wrote:

> Some USB 2.0 devices erroneously report millisecond values in
> bInterval. The generic config code manages to catch most of them,
> but in some cases it's not completely enough.
> 
> The case at stake here is a USB 2.0 braille device, which wants to
> announce 10ms and thus sets bInterval to 10, but with the USB 2.0
> computation that yields to 64ms.  It happens that one can type fast
> enough to reach this interval and get the device buffers overflown,
> leading to problematic latencies.  The generic config code does not
> catch this case because the 64ms is considered a sane enough value.

Interesting.  This is a high-speed device that mistakenly uses the 
low/full-speed encoding for an interrupt bInterval value?

That's pretty unusual.  Most HID devices (which includes the Braille
devices I have heard of) run at low speed, and a few of them run at 
full speed.  I can't remember any running at high speed.

> This change thus adds a USB_QUIRK_MS_INTR_BINTERVAL quirk to mark
> devices which actually report milliseconds in bInterval, and marks
> Vario Ultra devices as needing it.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>
> 
> --- a/include/linux/usb/quirks.h
> +++ b/include/linux/usb/quirks.h
> @@ -50,4 +50,10 @@
>  /* device can't handle Link Power Management */
>  #define USB_QUIRK_NO_LPM			BIT(10)
>  
> +/*
> + * Device reports its bInterval as milliseconds instead of the

This should be described as "linear frames", not "milliseconds", and 
its name should be USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL.

> + * USB 2.0 calculation.
> + */
> +#define USB_QUIRK_MS_INTR_BINTERVAL		BIT(11)
> +
>  #endif /* __LINUX_USB_QUIRKS_H */
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -280,6 +280,14 @@ static int usb_parse_endpoint(struct dev
>  
>  			/*
>  			 * Adjust bInterval for quirked devices.
> +			 */
> +			/*
> +			 * This quirk fixes bIntervals reported in ms.
> +			 */
> +			if (to_usb_device(ddev)->quirks &
> +				USB_QUIRK_MS_INTR_BINTERVAL)
> +				i = j = n;

You want to use the bInterval value the device intended to provide, not
a default value.  This should be like the microframe case, with the
value increased by 3:

				n = clamp(fls(d->bInterval) + 3, i, j);
				i = j = n;

> +			/*
>  			 * This quirk fixes bIntervals reported in
>  			 * linear microframes.
>  			 */
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -218,6 +218,14 @@ static const struct usb_device_id usb_qu
>  	/* INTEL VALUE SSD */
>  	{ USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
>  
> +	/* Baum Vario Ultra */
> +	{ USB_DEVICE(0x0904, 0x6101), .driver_info =
> +			USB_QUIRK_MS_INTR_BINTERVAL },
> +	{ USB_DEVICE(0x0904, 0x6102), .driver_info =
> +			USB_QUIRK_MS_INTR_BINTERVAL },
> +	{ USB_DEVICE(0x0904, 0x6103), .driver_info =
> +			USB_QUIRK_MS_INTR_BINTERVAL },

You didn't read the comment at the start of the file.  :-)  This list
is supposed to remain sorted by vendor and product ID.

Alan Stern

> +
>  	{ }  /* terminating entry must be last */
>  };
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>