| Message ID | 9efbd569-7059-4575-983f-0ea30df41871@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Regression due to 59cf44575456 ("USB: core: Fix oversight in SuperSpeed initialization") | expand |
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@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 >
Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting for once, to make this easily accessible to everyone. Is anyone still working on fixing below regression? From here it looks stalled, but I might have missed something. Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page. #regzbot poke On 09.04.24 16:56, 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? > >> 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 >> >
On Mon, Apr 22, 2024 at 07:33:21PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote: > Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting > for once, to make this easily accessible to everyone. > > Is anyone still working on fixing below regression? From here it looks > stalled, but I might have missed something. I've been waiting to hear back from Oliver or Takashi. A revised patch taking my comments into account would be welcome; it should be a very small change (just one or two lines of code). Alan Stern > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > -- > Everything you wanna know about Linux kernel regression tracking: > https://linux-regtracking.leemhuis.info/about/#tldr > If I did something stupid, please tell me, as explained on that page. > > #regzbot poke > > On 09.04.24 16:56, 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? > > > >> 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 > >> > > >
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 > > >
On Mon, 22 Apr 2024 20:03:46 +0200, Alan Stern wrote: > > On Mon, Apr 22, 2024 at 07:33:21PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote: > > Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting > > for once, to make this easily accessible to everyone. > > > > Is anyone still working on fixing below regression? From here it looks > > stalled, but I might have missed something. > > I've been waiting to hear back from Oliver or Takashi. A revised patch > taking my comments into account would be welcome; it should be a very > small change (just one or two lines of code). As posted in another mail, it's a virtualized environment. Details are found in the original bug report https://bugzilla.suse.com/show_bug.cgi?id=1220569 About the patch change: I appreciate if you cook it rather by yourself since I'm not 100% sure what you suggested. I can provide the reporter a test kernel with the patch for confirmation, of course. thanks, Takashi > > Alan Stern > > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > > -- > > Everything you wanna know about Linux kernel regression tracking: > > https://linux-regtracking.leemhuis.info/about/#tldr > > If I did something stupid, please tell me, as explained on that page. > > > > #regzbot poke > > > > On 09.04.24 16:56, 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? > > > > > >> 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 > > >> > > > > >
On Mon, Apr 22, 2024 at 09:24:24PM +0200, Takashi Iwai wrote: > On Mon, 22 Apr 2024 20:03:46 +0200, > Alan Stern wrote: > > > > On Mon, Apr 22, 2024 at 07:33:21PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote: > > > Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting > > > for once, to make this easily accessible to everyone. > > > > > > Is anyone still working on fixing below regression? From here it looks > > > stalled, but I might have missed something. > > > > I've been waiting to hear back from Oliver or Takashi. A revised patch > > taking my comments into account would be welcome; it should be a very > > small change (just one or two lines of code). > > As posted in another mail, it's a virtualized environment. > Details are found in the original bug report > https://bugzilla.suse.com/show_bug.cgi?id=1220569 Hmmm. If this is a virtualized device, isn't the best solution to fix the emulation code for the device so that it presents a valid descriptor? > About the patch change: I appreciate if you cook it rather by > yourself since I'm not 100% sure what you suggested. I can > provide the reporter a test kernel with the patch for confirmation, of > course. Here's a condensed version of the patch you wrote. But I would prefer not to add this to the kernel if the problem can be fixed somewhere else. 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 @@ -5110,9 +5110,10 @@ hub_port_init(struct usb_hub *hub, struc } if (usb_endpoint_maxp(&udev->ep0.desc) == i) { ; /* Initial ep0 maxpacket guess is right */ - } else if ((udev->speed == USB_SPEED_FULL || + } else if (((udev->speed == USB_SPEED_FULL || udev->speed == USB_SPEED_HIGH) && - (i == 8 || i == 16 || i == 32 || i == 64)) { + (i == 8 || i == 16 || i == 32 || i == 64)) || + (udev->speed >= USB_SPEED_SUPER && i > 0)) { /* Initial guess is wrong; use the descriptor's value */ if (udev->speed == USB_SPEED_FULL) dev_dbg(&udev->dev, "ep0 maxpacket = %d\n", i);
On Tue, 23 Apr 2024 21:29:27 +0200, Alan Stern wrote: > > On Mon, Apr 22, 2024 at 09:24:24PM +0200, Takashi Iwai wrote: > > On Mon, 22 Apr 2024 20:03:46 +0200, > > Alan Stern wrote: > > > > > > On Mon, Apr 22, 2024 at 07:33:21PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote: > > > > Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting > > > > for once, to make this easily accessible to everyone. > > > > > > > > Is anyone still working on fixing below regression? From here it looks > > > > stalled, but I might have missed something. > > > > > > I've been waiting to hear back from Oliver or Takashi. A revised patch > > > taking my comments into account would be welcome; it should be a very > > > small change (just one or two lines of code). > > > > As posted in another mail, it's a virtualized environment. > > Details are found in the original bug report > > https://bugzilla.suse.com/show_bug.cgi?id=1220569 > > Hmmm. If this is a virtualized device, isn't the best solution to fix the > emulation code for the device so that it presents a valid descriptor? Honestly speaking, I don't know, but it smells like a hard way. After all, we brought some regression of the previously (even casually / unintended) working "device"... > > About the patch change: I appreciate if you cook it rather by > > yourself since I'm not 100% sure what you suggested. I can > > provide the reporter a test kernel with the patch for confirmation, of > > course. > > Here's a condensed version of the patch you wrote. But I would prefer not > to add this to the kernel if the problem can be fixed somewhere else. Thanks, I asked the report for testing the patch now. thanks, Takashi > > 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 > @@ -5110,9 +5110,10 @@ hub_port_init(struct usb_hub *hub, struc > } > if (usb_endpoint_maxp(&udev->ep0.desc) == i) { > ; /* Initial ep0 maxpacket guess is right */ > - } else if ((udev->speed == USB_SPEED_FULL || > + } else if (((udev->speed == USB_SPEED_FULL || > udev->speed == USB_SPEED_HIGH) && > - (i == 8 || i == 16 || i == 32 || i == 64)) { > + (i == 8 || i == 16 || i == 32 || i == 64)) || > + (udev->speed >= USB_SPEED_SUPER && i > 0)) { > /* Initial guess is wrong; use the descriptor's value */ > if (udev->speed == USB_SPEED_FULL) > dev_dbg(&udev->dev, "ep0 maxpacket = %d\n", i); >
On Wed, Apr 24, 2024 at 10:56:11AM +0200, Takashi Iwai wrote: > On Tue, 23 Apr 2024 21:29:27 +0200, > Alan Stern wrote: > > > > On Mon, Apr 22, 2024 at 09:24:24PM +0200, Takashi Iwai wrote: > > > As posted in another mail, it's a virtualized environment. > > > Details are found in the original bug report > > > https://bugzilla.suse.com/show_bug.cgi?id=1220569 > > > > Hmmm. If this is a virtualized device, isn't the best solution to fix the > > emulation code for the device so that it presents a valid descriptor? > > Honestly speaking, I don't know, but it smells like a hard way. After > all, we brought some regression of the previously (even casually / > unintended) working "device"... Still, it would be good to report the incorrect descriptor to the package's maintainer. Alan Stern
On Wed, 24 Apr 2024 16:14:23 +0200, Alan Stern wrote: > > On Wed, Apr 24, 2024 at 10:56:11AM +0200, Takashi Iwai wrote: > > On Tue, 23 Apr 2024 21:29:27 +0200, > > Alan Stern wrote: > > > > > > On Mon, Apr 22, 2024 at 09:24:24PM +0200, Takashi Iwai wrote: > > > > As posted in another mail, it's a virtualized environment. > > > > Details are found in the original bug report > > > > https://bugzilla.suse.com/show_bug.cgi?id=1220569 > > > > > > Hmmm. If this is a virtualized device, isn't the best solution to fix the > > > emulation code for the device so that it presents a valid descriptor? > > > > Honestly speaking, I don't know, but it smells like a hard way. After > > all, we brought some regression of the previously (even casually / > > unintended) working "device"... > > Still, it would be good to report the incorrect descriptor to the package's > maintainer. Well, it's no Linux package. Takashi
On Wed, Apr 24, 2024 at 04:22:44PM +0200, Takashi Iwai wrote: > On Wed, 24 Apr 2024 16:14:23 +0200, > Alan Stern wrote: > > > > On Wed, Apr 24, 2024 at 10:56:11AM +0200, Takashi Iwai wrote: > > > On Tue, 23 Apr 2024 21:29:27 +0200, > > > Alan Stern wrote: > > > > > > > > On Mon, Apr 22, 2024 at 09:24:24PM +0200, Takashi Iwai wrote: > > > > > As posted in another mail, it's a virtualized environment. > > > > > Details are found in the original bug report > > > > > https://bugzilla.suse.com/show_bug.cgi?id=1220569 > > > > > > > > Hmmm. If this is a virtualized device, isn't the best solution to fix the > > > > emulation code for the device so that it presents a valid descriptor? > > > > > > Honestly speaking, I don't know, but it smells like a hard way. After > > > all, we brought some regression of the previously (even casually / > > > unintended) working "device"... > > > > Still, it would be good to report the incorrect descriptor to the package's > > maintainer. > > Well, it's no Linux package. Doesn't matter. Bugs should always be reported when possible, for any kind of package. Especially after you spent so much time and effort to track this one down. The fact that xz isn't specifically a Windows package didn't prevent Andres Freund from reporting the recent backdoor problem. Alan Stern
On Wed, 24 Apr 2024 16:56:53 +0200, Alan Stern wrote: > > On Wed, Apr 24, 2024 at 04:22:44PM +0200, Takashi Iwai wrote: > > On Wed, 24 Apr 2024 16:14:23 +0200, > > Alan Stern wrote: > > > > > > On Wed, Apr 24, 2024 at 10:56:11AM +0200, Takashi Iwai wrote: > > > > On Tue, 23 Apr 2024 21:29:27 +0200, > > > > Alan Stern wrote: > > > > > > > > > > On Mon, Apr 22, 2024 at 09:24:24PM +0200, Takashi Iwai wrote: > > > > > > As posted in another mail, it's a virtualized environment. > > > > > > Details are found in the original bug report > > > > > > https://bugzilla.suse.com/show_bug.cgi?id=1220569 > > > > > > > > > > Hmmm. If this is a virtualized device, isn't the best solution to fix the > > > > > emulation code for the device so that it presents a valid descriptor? > > > > > > > > Honestly speaking, I don't know, but it smells like a hard way. After > > > > all, we brought some regression of the previously (even casually / > > > > unintended) working "device"... > > > > > > Still, it would be good to report the incorrect descriptor to the package's > > > maintainer. > > > > Well, it's no Linux package. > > Doesn't matter. Bugs should always be reported when possible, for any kind > of package. Especially after you spent so much time and effort to track > this one down. > > The fact that xz isn't specifically a Windows package didn't prevent Andres > Freund from reporting the recent backdoor problem. Yeah, sure. Any people can report to https://bhyve.npulse.net/ Takashi
On Wed, 24 Apr 2024 10:56:11 +0200, Takashi Iwai wrote: > > On Tue, 23 Apr 2024 21:29:27 +0200, > Alan Stern wrote: > > > > On Mon, Apr 22, 2024 at 09:24:24PM +0200, Takashi Iwai wrote: > > > On Mon, 22 Apr 2024 20:03:46 +0200, > > > Alan Stern wrote: > > > About the patch change: I appreciate if you cook it rather by > > > yourself since I'm not 100% sure what you suggested. I can > > > provide the reporter a test kernel with the patch for confirmation, of > > > course. > > > > Here's a condensed version of the patch you wrote. But I would prefer not > > to add this to the kernel if the problem can be fixed somewhere else. > > Thanks, I asked the report for testing the patch now. The reporter confirmed that it's still working. thanks, Takashi
On Sun, Apr 28, 2024 at 09:57:42AM +0200, Takashi Iwai wrote: > On Wed, 24 Apr 2024 10:56:11 +0200, > Takashi Iwai wrote: > > > > On Tue, 23 Apr 2024 21:29:27 +0200, > > Alan Stern wrote: > > > > > > On Mon, Apr 22, 2024 at 09:24:24PM +0200, Takashi Iwai wrote: > > > > On Mon, 22 Apr 2024 20:03:46 +0200, > > > > Alan Stern wrote: > > > > About the patch change: I appreciate if you cook it rather by > > > > yourself since I'm not 100% sure what you suggested. I can > > > > provide the reporter a test kernel with the patch for confirmation, of > > > > course. > > > > > > Here's a condensed version of the patch you wrote. But I would prefer not > > > to add this to the kernel if the problem can be fixed somewhere else. > > > > Thanks, I asked the report for testing the patch now. > > The reporter confirmed that it's still working. Can you get a Reported-and-Tested-by: from the reporter? Alan Stern
On Mon, 29 Apr 2024 15:28:59 +0200, Alan Stern wrote: > > On Sun, Apr 28, 2024 at 09:57:42AM +0200, Takashi Iwai wrote: > > On Wed, 24 Apr 2024 10:56:11 +0200, > > Takashi Iwai wrote: > > > > > > On Tue, 23 Apr 2024 21:29:27 +0200, > > > Alan Stern wrote: > > > > > > > > On Mon, Apr 22, 2024 at 09:24:24PM +0200, Takashi Iwai wrote: > > > > > On Mon, 22 Apr 2024 20:03:46 +0200, > > > > > Alan Stern wrote: > > > > > About the patch change: I appreciate if you cook it rather by > > > > > yourself since I'm not 100% sure what you suggested. I can > > > > > provide the reporter a test kernel with the patch for confirmation, of > > > > > course. > > > > > > > > Here's a condensed version of the patch you wrote. But I would prefer not > > > > to add this to the kernel if the problem can be fixed somewhere else. > > > > > > Thanks, I asked the report for testing the patch now. > > > > The reporter confirmed that it's still working. > > Can you get a Reported-and-Tested-by: from the reporter? Yes, I got it from bugzilla entry, please put Reported-and-tested-by: Roger Whittaker <roger.whittaker@suse.com> Link: https://bugzilla.suse.com/show_bug.cgi?id=1220569 Thanks! Takashi
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) { ; /* 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); } 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); retval = -EMSGSIZE; goto fail; } -- 2.35.3
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 idVendor 0xfb5d idProduct 0x0001 bcdDevice 0.00 iManufacturer 1 BHYVE iProduct 2 HID Tablet iSerial 3 01 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 0x0028 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 4 HID Tablet Device bmAttributes 0xa0 (Bus Powered) Remote Wakeup MaxPower 0mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 0 bNumEndpoints 1 bInterfaceClass 3 Human Interface Device bInterfaceSubClass 1 Boot Interface Subclass bInterfaceProtocol 2 Mouse iInterface 0 HID Device Descriptor: bLength 9 bDescriptorType 33 bcdHID 10.01 bCountryCode 0 Not supported bNumDescriptors 1 bDescriptorType 34 Report wDescriptorLength 74 Report Descriptors: ** UNAVAILABLE ** Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0008 1x 8 bytes bInterval 10 bMaxBurst 0 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 Device Status: 0x0000 (Bus Powered) 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