Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935493AbdCLVTI (ORCPT ); Sun, 12 Mar 2017 17:19:08 -0400 Received: from netrider.rowland.org ([192.131.102.5]:36493 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S934692AbdCLVS7 (ORCPT ); Sun, 12 Mar 2017 17:18:59 -0400 Date: Sun, 12 Mar 2017 17:18:51 -0400 (EDT) From: Alan Stern X-X-Sender: stern@netrider.rowland.org To: Samuel Thibault cc: gregkh@linuxfoundation.org, , , , , , Subject: Re: [PATCH] usb-core: Add MS_INTR_BINTERVAL USB quirk In-Reply-To: <20170312201046.4jax6q5quugvpquh@var.youpi.perso.aquilenet.fr> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3336 Lines: 96 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 > > --- 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >