| Message ID | 8809e6c5-59d5-4d2d-ac8f-6d106658ad73@rowland.harvard.edu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 59cf445754566984fd55af19ba7146c76e6627bc |
| Headers | show |
| Series | USB: core: Fix oversight in SuperSpeed initialization | expand |
On Fri, Aug 11, 2023 at 01:38:46PM -0400, Alan Stern wrote: > Commit 85d07c556216 ("USB: core: Unite old scheme and new scheme > descriptor reads") altered the way USB devices are enumerated > following detection, and in the process it messed up the > initialization of SuperSpeed (or faster) devices: > > [ 31.650759] usb 2-1: new SuperSpeed Plus Gen 2x1 USB device number 2 using xhci_hcd > [ 31.663107] usb 2-1: device descriptor read/8, error -71 > [ 31.952697] usb 2-1: new SuperSpeed Plus Gen 2x1 USB device number 3 using xhci_hcd > [ 31.965122] usb 2-1: device descriptor read/8, error -71 > [ 32.080991] usb usb2-port1: attempt power cycle > ... > > The problem was caused by the commit forgetting that in SuperSpeed or > faster devices, the device descriptor uses a logarithmic encoding of > the bMaxPacketSize0 value. (For some reason I thought the 255 case in > the switch statement was meant for these devices, but it isn't -- it > was meant for Wireless USB and is no longer needed.) > > We can fix the oversight by testing for buf->bMaxPacketSize0 = 9 > (meaning 512, the actual maxpacket size for ep0 on all SuperSpeed > devices) and straightening out the logic that checks and adjusts our > initial guesses of the maxpacket value. > > Reported-and-tested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Closes: https://lore.kernel.org/linux-usb/20230810002257.nadxmfmrobkaxgnz@synopsys.com/ > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > Fixes: 85d07c556216 ("USB: core: Unite old scheme and new scheme descriptor reads") > > --- > > drivers/usb/core/hub.c | 36 ++++++++++++++++++++++++------------ > 1 file changed, 24 insertions(+), 12 deletions(-) Process nit, when you add a patch to an existing patch series like this, when I run `b4` to download and apply the patch, it sucks in the original patch series, not this add-on patch (which then of course fails as the original patch series was already applied.) So, if it's easy enough, can you just send add-on patches for stuff that has been applied, as a new thread? That way the tools handle it automatically and I don't have to hand edit/apply the patch? thanks, greg k-h
On Sat, Aug 12, 2023 at 10:05:27AM +0200, Greg KH wrote: > Process nit, when you add a patch to an existing patch series like this, > when I run `b4` to download and apply the patch, it sucks in the > original patch series, not this add-on patch (which then of course fails > as the original patch series was already applied.) > > So, if it's easy enough, can you just send add-on patches for stuff that > has been applied, as a new thread? That way the tools handle it > automatically and I don't have to hand edit/apply the patch? Sure. I don't use b4, so I wasn't aware of this behavior. If I ever forget in the future, feel free to yell at me. :-) Alan Stern
Index: usb-devel/drivers/usb/core/hub.c =================================================================== --- usb-devel.orig/drivers/usb/core/hub.c +++ usb-devel/drivers/usb/core/hub.c @@ -4705,7 +4705,7 @@ static int get_bMaxPacketSize0(struct us buf, size, initial_descriptor_timeout); switch (buf->bMaxPacketSize0) { - case 8: case 16: case 32: case 64: case 255: + case 8: case 16: case 32: case 64: case 9: if (buf->bDescriptorType == USB_DT_DEVICE) { rc = buf->bMaxPacketSize0; break; @@ -4992,23 +4992,35 @@ hub_port_init(struct usb_hub *hub, struc if (retval) goto fail; - if (maxp0 == 0xff || udev->speed >= USB_SPEED_SUPER) - i = 512; - else - i = maxp0; - if (usb_endpoint_maxp(&udev->ep0.desc) != i) { - if (udev->speed == USB_SPEED_LOW || - !(i == 8 || i == 16 || i == 32 || i == 64)) { - dev_err(&udev->dev, "Invalid ep0 maxpacket: %d\n", i); - retval = -EMSGSIZE; - goto fail; - } + /* + * Check the ep0 maxpacket guess and correct it if necessary. + * maxp0 is the value stored in the device descriptor; + * i is the value it encodes (logarithmic for SuperSpeed or greater). + */ + i = maxp0; + if (udev->speed >= USB_SPEED_SUPER) { + if (maxp0 <= 16) + i = 1 << maxp0; + else + i = 0; /* Invalid */ + } + if (usb_endpoint_maxp(&udev->ep0.desc) == i) { + ; /* Initial ep0 maxpacket guess is right */ + } else if ((udev->speed == USB_SPEED_FULL || + udev->speed == USB_SPEED_HIGH) && + (i == 8 || i == 16 || i == 32 || i == 64)) { + /* Initial guess is wrong; use the descriptor's value */ if (udev->speed == USB_SPEED_FULL) dev_dbg(&udev->dev, "ep0 maxpacket = %d\n", i); else dev_warn(&udev->dev, "Using ep0 maxpacket: %d\n", i); udev->ep0.desc.wMaxPacketSize = cpu_to_le16(i); usb_ep0_reinit(udev); + } else { + /* Initial guess is wrong and descriptor's value is invalid */ + dev_err(&udev->dev, "Invalid ep0 maxpacket: %d\n", maxp0); + retval = -EMSGSIZE; + goto fail; } descr = usb_get_device_descriptor(udev);
Commit 85d07c556216 ("USB: core: Unite old scheme and new scheme descriptor reads") altered the way USB devices are enumerated following detection, and in the process it messed up the initialization of SuperSpeed (or faster) devices: [ 31.650759] usb 2-1: new SuperSpeed Plus Gen 2x1 USB device number 2 using xhci_hcd [ 31.663107] usb 2-1: device descriptor read/8, error -71 [ 31.952697] usb 2-1: new SuperSpeed Plus Gen 2x1 USB device number 3 using xhci_hcd [ 31.965122] usb 2-1: device descriptor read/8, error -71 [ 32.080991] usb usb2-port1: attempt power cycle ... The problem was caused by the commit forgetting that in SuperSpeed or faster devices, the device descriptor uses a logarithmic encoding of the bMaxPacketSize0 value. (For some reason I thought the 255 case in the switch statement was meant for these devices, but it isn't -- it was meant for Wireless USB and is no longer needed.) We can fix the oversight by testing for buf->bMaxPacketSize0 = 9 (meaning 512, the actual maxpacket size for ep0 on all SuperSpeed devices) and straightening out the logic that checks and adjusts our initial guesses of the maxpacket value. Reported-and-tested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Closes: https://lore.kernel.org/linux-usb/20230810002257.nadxmfmrobkaxgnz@synopsys.com/ Signed-off-by: Alan Stern <stern@rowland.harvard.edu> Fixes: 85d07c556216 ("USB: core: Unite old scheme and new scheme descriptor reads") --- drivers/usb/core/hub.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-)