On Tuesday, May 31, 2011 10:07:13 PM Matt Evans wrote: > Hi Dmitry, > > On 01/06/11 07:37, Dmitry Torokhov wrote: > > On Tue, May 31, 2011 at 02:00:46PM -0700, Sarah Sharp wrote: > >> On Tue, May 31, 2011 at 12:08:14PM -0700, Dmitry Torokhov wrote: > >>> On Tue, May 31, 2011 at 11:34:58AM -0700, Sarah Sharp wrote: > >>>> On Tue, May 31, 2011 at 09:49:41AM -0700, Dmitry Torokhov wrote: > >>>>> Hi Matt, > >>>>> > >>>>> On Tue, May 31, 2011 at 03:52:16PM +1000, Matt Evans wrote: > >>>>>> Commit dfa49c4ad120a784ef1ff0717168aa79f55a483a > >>>>>> > >>>>>> "USB: xhci - fix math in xhci_get_endpoint_interval()" > >>>>>> > >>>>>> expands the clamping of the interval value to 0-15, which is only > >>>>>> legal for HS/SS endpoints. For FS, it must be 3-11. When > >>>>>> testing with FS isochronous endpoints (an audio device), values > >>>>>> outside this range caused > >>>>>> > >>>>>> the following error 0x11 (again) on an NEC xHCI controller: > >>>>>> xhci_hcd 0000:01:00.0: ERROR: unexpected command completion > >>>>>> code 0x11. usb 1-1: Not enough bandwidth for altsetting 1 > >>>>> > >>>>> Hmm, however USB2.0 spec (to whcih high/full/low-speed devices > >>>>> should conform states: > >>>>> > >>>>> "For full-/high-speed isochronous endpoints, this value must be in > >>>>> the range from 1 to 16. The bInterval value bInterval-1 is used > >>>>> as the exponent for a 2 value; e.g., a 4-1 bInterval of 4 means a > >>>>> period of 8 (2 )." > >>>> > >>>> It doesn't really matter what the USB 2.0 specification says. If > >>>> the xHCI specification says that xHCI hosts can only handle FS > >>>> intervals of 3-11, and host controllers are going to reject > >>>> Configure Endpoint commands based on what the spec says, then the > >>>> xHCI driver needs to limit the interval in the same way. > >>>> > >>>> It could be that the xHCI spec architect chose to limit the FS > >>>> intervals for a reason, much the same way they chose to limit the > >>>> interval to powers of two. Or it could have just been a typo. > >>>> > >>>> If the xHCI spec is wrong, then you should email xhcispec@xxxxxxxxx > >>>> and try to get an errata put in for it. But even then, we won't > >>>> be able to tell which hosts implement the errata, so we'll > >>>> probably have to stick with the lower interval limit. > >>> > >>> xHCI spec seems to be confused indeed. Compare: > >>> > >>> Table 56: > >>> "The legal range of values is 3 to 10 for Full- or Low-speed > >>> Interrupt endpoints (1 to 256 ms.) and 0 to 15 for all other > >>> endpoint types (125 us to 4096 us)." > >>> > >>> This suggests that full- and low-speed interrupt endpoints specify > >>> interval in exponent-based form. > >>> > >>> However 6.2.3.6 states: > >>> > >>> For low-speed Interrupt and full-speed Interrupt and Isoch endpoints > >>> the bInterval field declared by a Full-or Low-speed device is > >>> computed as bInterval * 1ms., where bInterval = 1 to 255." > >> > >> My updated version of the 1.0 spec (with all the errata up to Jan. > >> 11, 2011) has a nice table in that section instead. I'm just going > >> to attach a screenshot of it. Basically, it says to clamp FS > >> devices at 10, with the exception of FS isochronous devices, which > >> can go up to 18. > >> > >> Of course, it's possible that the NEC host controller is doing some > >> internal checking of the interval, and won't accept FS isochronous > >> intervals greater than 10. I guess we would need to clamp to 18 only > >> for xHCI 1.0 hosts. > > > > No, what we were missingis that units of measurement for FS isoc > > endpoints is frames, not microframes. I think the following patch is > > what we need. > > Aha, great! I missed that unit-change subtlety in my patch. Though my > test machine's unavailable ATM, the code will give the right result (3) > for the bInterval (1) in my USB audio isoch endpoints. Happy for my > patch to be kicked down the well in favour of this. :) > > > Thanks, > > Dmitry > > > > > > Subject: USB: xhci - fix interval calculation for FS isoc endpoints > > > > Full-speed isoc endpoints specify interval in exponent based form > > in frames, not microframes, so we need to adjust accordingly. > > > > Reported-by: Matt Evans <matt@xxxxxxxxxx> > > Signed-off-by: Dmitry Torokhov <dtor@xxxxxxxxxx> > > Acked-by: Matt Evans <matt@xxxxxxxxxx> Thanks Matt. > > Should also CC stable to follow your original patch, no? > I'll defer to Sarah on that - if she's happy with this version of the patch then yes, I believe we should mark it for stable as well. Thanks. -- Dmitry -- 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
