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?
thanks,
greg k-h
--
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
