2017-03-12 20:10:54

by Samuel Thibault

[permalink] [raw]
Subject: [PATCH] usb-core: Add MS_INTR_BINTERVAL USB quirk

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.

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 <[email protected]>

--- 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
+ * 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;
+ /*
* 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 },
+
{ } /* terminating entry must be last */
};



2017-03-12 21:19:08

by Alan Stern

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

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 <[email protected]>
>
> --- 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2017-03-12 21:47:25

by Samuel Thibault

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

Alan Stern, on dim. 12 mars 2017 17:18:51 -0400, wrote:
> Interesting. This is a high-speed device that mistakenly uses the
> low/full-speed encoding for an interrupt bInterval value?

Yes...

> 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.

That's the first device for which we have such issue. We'll try to
check whether some other devices need the same quirk.

> > + */
> > + /*
> > + * 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.

n is alreay computed as such above, but OK :) (and better clamp anyway)

> > + /* 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.

D'oh, sorry :)

Samuel

2017-03-12 21:52:36

by Dave Mielke

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

[quoted lines by Alan Stern on 2017/03/12 at 17:18 -0400]

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

Yes.

>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.

According to my collection of data, 5 say 1.00, 15 say 1.1, and 21 say 2.0.

--
Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God.
Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/
EMail: [email protected] | Canada K2A 1H7 | http://FamilyRadio.org/

2017-03-13 01:31:09

by Alan Stern

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

On Sun, 12 Mar 2017, Dave Mielke wrote:

> [quoted lines by Alan Stern on 2017/03/12 at 17:18 -0400]
>
> >Interesting. This is a high-speed device that mistakenly uses the
> >low/full-speed encoding for an interrupt bInterval value?
>
> Yes.
>
> >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.
>
> According to my collection of data, 5 say 1.00, 15 say 1.1, and 21 say 2.0.

A device's speed is only partially related to its USB version. A
USB-1.1 device can run at low speed or full speed. A USB-2 device can
run at low, full, or high speed. And a USB-3 device can run at low,
full, high, or Super speed.

Alan Stern

2017-03-13 01:40:42

by Alan Stern

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

On Sun, 12 Mar 2017, Dave Mielke wrote:

> [quoted lines by Alan Stern on 2017/03/12 at 21:31 -0400]
>
> >A device's speed is only partially related to its USB version. A
> >USB-1.1 device can run at low speed or full speed. A USB-2 device can
> >run at low, full, or high speed. And a USB-3 device can run at low,
> >full, high, or Super speed.
>
> Yes, I did know this, so maybe I misunderstood what you were wondering about.
> Were you wondering why 64ms was too long?

No, I was wondering why an HID device would run at high speed. Both
you and Samuel implied that this was because it was a USB-2 device.
But that is not an adequate answer, because it is perfectly valid for a
USB-2 device to run at full speed.

Alan Stern

2017-03-13 01:46:58

by Dave Mielke

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

[quoted lines by Alan Stern on 2017/03/12 at 21:40 -0400]

>No, I was wondering why an HID device would run at high speed. Both
>you and Samuel implied that this was because it was a USB-2 device.
>But that is not an adequate answer, because it is perfectly valid for a
>USB-2 device to run at full speed.

What should we look at to find out what speed it wants to operate at? We didn't
look into it becuaase the real problem, from our perspective, was that the 64ms
interval was being honoured by the host when it should be the 10ms as is
literally in the endpoint descriptor. Our assumption was that since it says
it's USB 2.0 then bInterval must be intterpreted in light of that regardless of
the actual speed used for communication.

--
Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God.
Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/
EMail: [email protected] | Canada K2A 1H7 | http://FamilyRadio.org/

2017-03-13 01:47:36

by Dave Mielke

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

[quoted lines by Alan Stern on 2017/03/12 at 21:31 -0400]

>A device's speed is only partially related to its USB version. A
>USB-1.1 device can run at low speed or full speed. A USB-2 device can
>run at low, full, or high speed. And a USB-3 device can run at low,
>full, high, or Super speed.

Yes, I did know this, so maybe I misunderstood what you were wondering about.
Were you wondering why 64ms was too long?

--
Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God.
Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/
EMail: [email protected] | Canada K2A 1H7 | http://FamilyRadio.org/

2017-03-13 07:07:06

by Samuel Thibault

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

Alan Stern, on dim. 12 mars 2017 21:40:33 -0400, wrote:
> On Sun, 12 Mar 2017, Dave Mielke wrote:
> > [quoted lines by Alan Stern on 2017/03/12 at 21:31 -0400]
> >
> > >A device's speed is only partially related to its USB version. A
> > >USB-1.1 device can run at low speed or full speed. A USB-2 device can
> > >run at low, full, or high speed. And a USB-3 device can run at low,
> > >full, high, or Super speed.
> >
> > Yes, I did know this, so maybe I misunderstood what you were wondering about.
> > Were you wondering why 64ms was too long?
>
> No, I was wondering why an HID device would run at high speed. Both
> you and Samuel implied that this was because it was a USB-2 device.
> But that is not an adequate answer, because it is perfectly valid for a
> USB-2 device to run at full speed.

to_usb_device(ddev)->speed really is USB_SPEED_HIGH, otherwise the quirk
wouldn't work :)

Samuel

2017-03-13 10:38:34

by Dave Mielke

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

[quoted lines by Alan Stern on 2017/03/12 at 21:40 -0400]

>No, I was wondering why an HID device would run at high speed. Both
>you and Samuel implied that this was because it was a USB-2 device.
>But that is not an adequate answer, because it is perfectly valid for a
>USB-2 device to run at full speed.

I think I've misunderstood something about how to interpret bInterval. I'm now
suspecting that bInterval must be interpreted the new (mocro frame) way only if
the device is operating at least at high speed, and that, even if the device
advertizes itself as USB 2.0, bInterval must still be interpreted the old way
if the device is operating at full or low speed. Is that correct?

If the above is correct, how can we tell from usbfs which way to interpret
bInterval?

--
Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God.
Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/
EMail: [email protected] | Canada K2A 1H7 | http://FamilyRadio.org/

2017-03-14 01:43:48

by Alan Stern

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

On Sun, 12 Mar 2017, Dave Mielke wrote:

> [quoted lines by Alan Stern on 2017/03/12 at 21:40 -0400]
>
> >No, I was wondering why an HID device would run at high speed. Both
> >you and Samuel implied that this was because it was a USB-2 device.
> >But that is not an adequate answer, because it is perfectly valid for a
> >USB-2 device to run at full speed.
>
> What should we look at to find out what speed it wants to operate at? We didn't

The speed is reported in the kernel log and in
/sys/kernel/debug/usb/devices.

> look into it becuaase the real problem, from our perspective, was that the 64ms
> interval was being honoured by the host when it should be the 10ms as is
> literally in the endpoint descriptor. Our assumption was that since it says
> it's USB 2.0 then bInterval must be intterpreted in light of that regardless of
> the actual speed used for communication.

You should read the USB specification. Specifically, table 9-13 in
section 9.6.6 describes bInterval like this (in part):

Interval for polling endpoint for data transfers.
Expressed in frames or microframes depending on the
device operating speed (i.e., either 1 millisecond or
125 μs units).

For full-/high-speed isochronous endpoints, this value
must be in the range from 1 to 16. The bInterval value
is used as the exponent for a 2^(bInterval-1) value; e.g., a
bInterval of 4 means a period of 8 (2^(4-1)).

For full-/low-speed interrupt endpoints, the value of
this field may be from 1 to 255.

For high-speed interrupt endpoints, the bInterval value
is used as the exponent for a 2^(bInterval-1) value; e.g., a
bInterval of 4 means a period of 8 (2^(4-1)). This value
must be from 1 to 16.

> Yes, I did know this, so maybe I misunderstood what you were wondering about.
> Were you wondering why 64ms was too long?

No. I was wondering why an HID device would run at high speed.

> I think I've misunderstood something about how to interpret bInterval. I'm now
> suspecting that bInterval must be interpreted the new (mocro frame) way only if
> the device is operating at least at high speed, and that, even if the device
> advertizes itself as USB 2.0, bInterval must still be interpreted the old way
> if the device is operating at full or low speed. Is that correct?

Like I said, read the spec.

> If the above is correct, how can we tell from usbfs which way to
> interpret bInterval?

There is an ioctl you can use to find the device speed:
USBDEVFS_CONNECTINFO. Unfortunately, this ioctl is badly out of date
and only distinguishes between low speed and non-low speed. A patch to
remedy this situation would be welcome.

However, in general, users of usbfs don't need to know the device
speed. In particular, you don't need to interpret bInterval. Just
submit URBs quickly enough that the pipeline never empties out.

Alan Stern

2017-03-14 01:44:48

by Alan Stern

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

On Mon, 13 Mar 2017, Samuel Thibault wrote:

> Alan Stern, on dim. 12 mars 2017 21:40:33 -0400, wrote:
> > On Sun, 12 Mar 2017, Dave Mielke wrote:
> > > [quoted lines by Alan Stern on 2017/03/12 at 21:31 -0400]
> > >
> > > >A device's speed is only partially related to its USB version. A
> > > >USB-1.1 device can run at low speed or full speed. A USB-2 device can
> > > >run at low, full, or high speed. And a USB-3 device can run at low,
> > > >full, high, or Super speed.
> > >
> > > Yes, I did know this, so maybe I misunderstood what you were wondering about.
> > > Were you wondering why 64ms was too long?
> >
> > No, I was wondering why an HID device would run at high speed. Both
> > you and Samuel implied that this was because it was a USB-2 device.
> > But that is not an adequate answer, because it is perfectly valid for a
> > USB-2 device to run at full speed.
>
> to_usb_device(ddev)->speed really is USB_SPEED_HIGH, otherwise the quirk
> wouldn't work :)

Indeed; I believe you. It's just odd and a little noteworthy, that's
all.

Alan Stern

2017-03-14 02:20:08

by Dave Mielke

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

Thank you for your very helpful answer. I really do appreciate it.

It's possible that this device is using high speed because it offers a feature
to transfer its internal clipboard to the host, and it allows that clipboard to
contain lots of data. Interestingly, though, hidden within a usage note, they
do observe that, so far, they've been unable to achieve a transfer speed faster
than 5,000 characters in seven minutes. It looks to me like this is exactly due
to their incorrect setting of bInterval. :-) I've sent them a note about it.

--
Dave Mielke | 2213 Fox Crescent | The Bible is the very Word of God.
Phone: 1-613-726-0014 | Ottawa, Ontario | http://Mielke.cc/bible/
EMail: [email protected] | Canada K2A 1H7 | http://FamilyRadio.org/

2017-03-14 08:45:40

by Samuel Thibault

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

Dave Mielke, on lun. 13 mars 2017 22:20:01 -0400, wrote:
> It's possible that this device is using high speed because it offers a feature
> to transfer its internal clipboard to the host, and it allows that clipboard to
> contain lots of data. Interestingly, though, hidden within a usage note, they
> do observe that, so far, they've been unable to achieve a transfer speed faster
> than 5,000 characters in seven minutes. It looks to me like this is exactly due
> to their incorrect setting of bInterval. :-)

Well, still, with a 10ms interval, one can achieve at best 100 key
press events per second, thus 50 characters per second, i.e. not even
1200bps...

Samuel