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?
> 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@xxxxxxx>
> 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@xxxxxxx>
>
> ---
> 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
>
