On Friday, May 13, 2011 12:57:57 PM Greg KH wrote:
> On Fri, May 13, 2011 at 12:09:37PM -0700, Sarah Sharp wrote:
> > On Fri, May 13, 2011 at 09:35:11AM -0700, Sarah Sharp wrote:
> > > Hi Thomas,
> > >
> > > I'd like to figure out why your mouse doesn't work under the USB 3.0
> > > port.
> > >
> > > You said this commit caused your issue:
> > >
> > > commit 926008c9386dde09b015753b6681c502177baa30
> > > Author: Dmitry Torokhov <dtor@xxxxxxxxxx>
> > > Date: Wed Mar 23 20:47:05 2011 -0700
> > >
> > > USB: xhci: simplify logic of skipping missed isoc TDs
> >
> > Ok, I checked with some full speed devices I have, and I also see your
> > problem on my system with an NEC add-in card. This commit causes the
> > issue:
> >
> > commit dfa49c4ad120a784ef1ff0717168aa79f55a483a
> > Author: Dmitry Torokhov <dtor@xxxxxxxxxx>
> > Date: Wed Mar 23 22:41:23 2011 -0700
> >
> > USB: xhci - fix math in xhci_get_endpoint_interval()
> >
> > When parsing exponent-expressed intervals we subtract 1 from the
> > value and then expect it to match with original + 1, which is
> >
> > highly unlikely, and we end with frequent spew:
> > usb 3-4: ep 0x83 - rounding interval to 512 microframes
> >
> > Also, parsing interval for fullspeed isochronous endpoints was
> > incorrect - according to USB spec they use exponent-based
> > intervals (but xHCI spec claims frame-based intervals). I trust
> > USB spec more, especially since USB core agrees with it.
> >
> > This should be queued for stable kernels back to 2.6.31.
> >
> > Reviewed-by: Micah Elizabeth Scott <micah@xxxxxxxxxx>
> > Signed-off-by: Dmitry Torokhov <dtor@xxxxxxxxxx>
> > Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxx
> >
> > Greg, this was marked as being needed to be queued for stable. Do you
> > know if it has already made it into the stable trees?
>
> Yes it has, you should have received emails asking about it when it went
> into the review tree and when the -rc was released.
>
> It is in the 2.6.38.4, 2.6.32.39, and 2.6.33.12 release, and might also
> be in a .34-longterm and .35-longterm release as well, but I don't have
> those trees here to check.
>
> > We might need to revert it or make a bug fix patch on top of it.
>
> Thanks for letting us know.
>
> > On the current Linus tree, both a FS hub and a FS USB to ethernet
> > device do not work under an NEC 0.96 host controller. When I revert
> > the patch, they do work.
> >
> > Dmitry, I'm pretty sure your patch isn't correct, now that I look at
> > it more closely. The USB 2.0 spec says in table 9-13 about
> > bInterval:
> >
> > "Expressed in frames or microframes depending on the device operating
> > speed (i.e., either 1 millisecond or 125 Îs units)."
> >
> > I assume that means frames for LS/FS devices and microframes for HS
> > devices.
> >
> > "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 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."
> >
> > But your patch thinks full speed interrupt endpoints should encoded in
> >
> > exponent style:
> > case USB_SPEED_FULL:
> > + if (usb_endpoint_xfer_int(&ep->desc)) {
> > + interval = xhci_parse_exponent_interval(udev,
> > ep); + break;
> > + }
> > + /*
> > + * Fall through for isochronous endpoint interval
> > decoding + * since it uses the same rules as low speed
> > interrupt + * endpoints.
> > + */
> > +
> >
> > case USB_SPEED_LOW:
> > if (usb_endpoint_xfer_int(&ep->desc) ||
> >
> > - usb_endpoint_xfer_isoc(&ep->desc)) {
> > - interval = fls(8*ep->desc.bInterval) - 1;
> > - if (interval > 10)
> > - interval = 10;
> > - if (interval < 3)
> > - interval = 3;
> > - if ((1 << interval) != 8*ep->desc.bInterval)
> > - dev_warn(&udev->dev,
> > - "ep %#x - rounding
> > interval" - " to %d
> > microframes, " - "ep
> > desc says %d microframes\n", -
> > ep->desc.bEndpointAddress, -
> > 1 << interval, -
> > 8*ep->desc.bInterval); +
> > usb_endpoint_xfer_isoc(&ep->desc)) {
> > +
> > + interval = xhci_parse_frame_interval(udev,
> > ep);
> >
> > }
> >
> > Note xfer_int, not xfer_isoc in that first if statement. The comment
> > is also wrong, since full speed isochronous endpoints use the same
> > interval decoding as high speed isochronous endpoints. I think you
> > meant that full speed *interrupt* endpoints use the same rules as low
> > speed interrupt endpoints.
> >
> > Greg, what do you want to do in this case? Revert Dmitry's patch from
> > Linus' tree and have him try again? Make a bug fix patch on top of
> > Dmitry's patch? The worst that can happen if Dmitry's patch is
> > reverted is that there will be more messages in dmesg about interval
> > mismatch, and full speed isochronous endpoints will be polled less
> > frequently than they should be.
>
> It sounds like reverting it is the best decision for now, it's easiest
> and lets everyone work on getting it right the second time around.
>
> Dmitry, any objection to me reverting this, or do you see a simple fix
> for it?
No objections, unless Sarah's patch that she just posted fixes Thomas's
issue. Unfortunately I was not able to reproduce the breakage locally
as my mouse manages to work both with and without the offending patch.
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
