Re: Regression due to 59cf44575456 ("USB: core: Fix oversight in SuperSpeed initialization")

Takashi Iwai
2024-04-22T19:22:13Z

On Tue, 09 Apr 2024 16:56:53 +0200,
Alan Stern wrote:
>
> On Tue, Apr 09, 2024 at 03:49:01PM +0200, Oliver Neukum wrote:
> > Hi,
> >
> > with the following device:
> >
> > Bus 002 Device 002: ID fb5d:0001 BHYVE HID Tablet
> > Device Descriptor:
> > bLength 18
> > bDescriptorType 1
> > bcdUSB 3.00
> > bDeviceClass 0
> > bDeviceSubClass 0
> > bDeviceProtocol 0
> > bMaxPacketSize0 8
>
> A USB-3 device, running at SuperSpeed with its bMaxPacketSize0 set to 8
> instead of 9? Presumably this thing never received a USB certification.
> Does the packaging use the USB logo?

IIUC from the report, this is a virtualization environment, no real
device:
https://bhyve.npulse.net/

Takashi

> > idVendor 0xfb5d
> > idProduct 0x0001
> > bcdDevice 0.00
> > iManufacturer 1 BHYVE
> > iProduct 2 HID Tablet
> > iSerial 3 01
> > bNumConfigurations 1
>
> Why on earth would an HID tablet need to run at SuperSpeed?
>
> > Binary Object Store Descriptor:
> > bLength 5
> > bDescriptorType 15
> > wTotalLength 0x000f
> > bNumDeviceCaps 1
> > SuperSpeed USB Device Capability:
> > bLength 10
> > bDescriptorType 16
> > bDevCapabilityType 3
> > bmAttributes 0x00
> > wSpeedsSupported 0x0008
> > Device can operate at SuperSpeed (5Gbps)
> > bFunctionalitySupport 3
> > Lowest fully-functional device speed is SuperSpeed (5Gbps)
> > bU1DevExitLat 10 micro seconds
> > bU2DevExitLat 32 micro seconds
>
> A tablet not capable of running at any speed below 5 Gbps?
>
> > we are getting a regression on enumeration. It used to work with the
> > code prior to your patch. Takashi is proposing the attached fixed.
> > It looks like we are a bit too restrictive and should just try.
> >
> > Regards
> > Oliver
>
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] USB: hub: Workaround for buggy max packet size with super
> > speed
> > Patch-mainline: Not yet, testing
> > References: bsc#1220569
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >
> > ---
> > drivers/usb/core/hub.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index e38a4124f610..64193effc456 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -4830,7 +4830,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
> > const char *driver_name;
> > bool do_new_scheme;
> > const bool initial = !dev_descr;
> > - int maxp0;
> > + int maxp0, ep0_maxp;
> > struct usb_device_descriptor *buf, *descr;
> >
> > buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO);
> > @@ -5070,7 +5070,8 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
> > else
> > i = 0; /* Invalid */
> > }
> > - if (usb_endpoint_maxp(&udev->ep0.desc) == i) {
> > + ep0_maxp = usb_endpoint_maxp(&udev->ep0.desc);
> > + if (ep0_maxp == i) {
>
> This variable looks like it was left over from earlier testing. It's
> not really needed.
>
> > ; /* Initial ep0 maxpacket guess is right */
> > } else if ((udev->speed == USB_SPEED_FULL ||
> > udev->speed == USB_SPEED_HIGH) &&
> > @@ -5082,9 +5083,15 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
> > dev_warn(&udev->dev, "Using ep0 maxpacket: %d\n", i);
> > udev->ep0.desc.wMaxPacketSize = cpu_to_le16(i);
> > usb_ep0_reinit(udev);
> > + } else if (udev->speed >= USB_SPEED_SUPER && initial) {
> > + /* FIXME: should be more restrictive? */
> > + /* Initial guess is wrong; use the descriptor's value */
> > + dev_warn(&udev->dev, "Using ep0 maxpacket: %d\n", i);
> > + udev->ep0.desc.wMaxPacketSize = cpu_to_le16(i);
> > + usb_ep0_reinit(udev);
>
> This could be merged with the previous case fairly easily.
>
> > } else {
> > /* Initial guess is wrong and descriptor's value is invalid */
> > - dev_err(&udev->dev, "Invalid ep0 maxpacket: %d\n", maxp0);
> > + dev_err(&udev->dev, "Invalid ep0 maxpacket: %d, expected %d\n", maxp0, ep0_maxp);
>
> This also looks like a remnant from earlier testing.
>
> Alan Stern
>
> > retval = -EMSGSIZE;
> > goto fail;
> > }
> > --
> > 2.35.3
> >
>