On Thu, May 09, 2013 at 01:46:51PM -0700, Sarah Sharp wrote:
> Thanks Alan, I will add this to my for-usb-linus queue.
One note though: I've been bitten by this code before, by someone who
refactored code and fixed a bug at the same time. The math was wrong,
and I ended up breaking full-speed devices on all the stable kernels.
You are of course, a methodical coder, but I would like to be cautious
with this code change.
I'm going to strip the stable Cc from the patch, and send this off to
stable after it's had a couple weeks to be tested in the 3.10-rc kernel,
if that's all right with you.
Sarah Sharp
> On Wed, May 08, 2013 at 11:18:05AM -0400, Alan Stern wrote:
> > This patch shortens the logic in xhci_endpoint_init() by moving common
> > calculations involving max_packet and max_burst outside the switch
> > statement, rather than repeating the same code in multiple
> > case-specific statements. It also replaces two usages of max_packet
> > which were clearly intended to be max_burst all along.
> >
> > More importantly, it compensates for a common bug in high-speed bulk
> > endpoint descriptors. In many devices there is a bulk endpoint having
> > a wMaxPacketSize value smaller than 512, which is forbidden by the USB
> > spec. Some xHCI controllers can't handle this and refuse to accept
> > the endpoint. This patch changes the max_packet value to 512, which
> > allows the controller to use the endpoint properly.
> >
> > In practice the bogus maxpacket size doesn't matter, because none of
> > the transfers sent via these endpoints are longer than the maxpacket
> > value anyway.
> >
> > Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > Reported-and-tested-by: "Aurélien Leblond" <blablack@xxxxxxxxx>
> > CC: <stable@xxxxxxxxxxxxxxx>
> >
> > ---
> >
> > Question: Should this be handled in usbcore instead? The code that
> > parses endpoint descriptors already warns about high-speed bulk
> > endpoints with maxpacket different from 512. It could easily override
> > the bogus values.
> >
> > Also: This probably won't work in cases where the bogus maxpacket
> > value is _larger_ than 512. I vaguely recall seeing a device with a
> > bulk maxpacket size of 1024. But such an endpoint is unlikely to work
> > under an xHCI controller in any case, although it might work with
> > EHCI.
> >
> >
> > [as1678]
> >
> > drivers/usb/host/xhci-mem.c | 17 +++++++++--------
> > 1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > Index: usb-3.9/drivers/usb/host/xhci-mem.c
> > ===================================================================
> > --- usb-3.9.orig/drivers/usb/host/xhci-mem.c
> > +++ usb-3.9/drivers/usb/host/xhci-mem.c
> > @@ -1423,15 +1423,17 @@ int xhci_endpoint_init(struct xhci_hcd *
> > ep_ctx->ep_info2 |= cpu_to_le32(xhci_get_endpoint_type(udev, ep));
> >
> > /* Set the max packet size and max burst */
> > + max_packet = GET_MAX_PACKET(usb_endpoint_maxp(&ep->desc));
> > + max_burst = 0;
> > switch (udev->speed) {
> > case USB_SPEED_SUPER:
> > - max_packet = usb_endpoint_maxp(&ep->desc);
> > - ep_ctx->ep_info2 |= cpu_to_le32(MAX_PACKET(max_packet));
> > /* dig out max burst from ep companion desc */
> > - max_packet = ep->ss_ep_comp.bMaxBurst;
> > - ep_ctx->ep_info2 |= cpu_to_le32(MAX_BURST(max_packet));
> > + max_burst = ep->ss_ep_comp.bMaxBurst;
> > break;
> > case USB_SPEED_HIGH:
> > + /* Some devices get this wrong */
> > + if (usb_endpoint_xfer_bulk(&ep->desc))
> > + max_packet = 512;
> > /* bits 11:12 specify the number of additional transaction
> > * opportunities per microframe (USB 2.0, section 9.6.6)
> > */
> > @@ -1439,17 +1441,16 @@ int xhci_endpoint_init(struct xhci_hcd *
> > usb_endpoint_xfer_int(&ep->desc)) {
> > max_burst = (usb_endpoint_maxp(&ep->desc)
> > & 0x1800) >> 11;
> > - ep_ctx->ep_info2 |= cpu_to_le32(MAX_BURST(max_burst));
> > }
> > - /* Fall through */
> > + break;
> > case USB_SPEED_FULL:
> > case USB_SPEED_LOW:
> > - max_packet = GET_MAX_PACKET(usb_endpoint_maxp(&ep->desc));
> > - ep_ctx->ep_info2 |= cpu_to_le32(MAX_PACKET(max_packet));
> > break;
> > default:
> > BUG();
> > }
> > + ep_ctx->ep_info2 |= cpu_to_le32(MAX_PACKET(max_packet) |
> > + MAX_BURST(max_burst));
> > max_esit_payload = xhci_get_max_esit_payload(xhci, udev, ep);
> > ep_ctx->tx_info = cpu_to_le32(MAX_ESIT_PAYLOAD_FOR_EP(max_esit_payload));
> >
> >
> > --
> > 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
> --
> 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
--
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
