[PATCH 1/1] usb: request only 8 bytes for USB_SPEED_FULL bMaxPacketSize0 discovery

[PATCH 1/1] usb: request only 8 bytes for USB_SPEED_FULL bMaxPacketSize0 discovery

[Janne Grunau]

Fixes probing of various keyboards with DWC3 as integrated into Apple
silicon SoCs. The problem appears to be requesting more data than the
devices's bMaxPacketSize0 of 8. Older Logitech unifying receivers
(bcdDevice 12.03 or 12.10) are for eaxample affected.

Signed-off-by: Janne Grunau <j@jannau.net>
Tested-by: Thomas Glanzmann <thomas@glanzmann.de>
---
 common/usb.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index 6fcf1e8428e9..48a310e8599d 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -993,10 +993,12 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
 		 *
 		 * At least the DWC2 controller needs to be programmed with
 		 * the number of packets in addition to the number of bytes.
-		 * A request for 64 bytes of data with the maxpacket guessed
-		 * as 64 (above) yields a request for 1 packet.
+		 * Requesting more than 8 bytes causes probing errors on the
+		 * DWC3 controller integrated into Apple silicon SoCs with
+		 * devices with bMaxPacketSize0 of 8. So limit the read request
+		 * to the used size of 8 bytes.
 		 */
-		err = get_descriptor_len(dev, 64, 8);
+		err = get_descriptor_len(dev, 8, 8);
 		if (err)
 			return err;
 	}
-- 
2.35.1

Re: [PATCH 1/1] usb: request only 8 bytes for USB_SPEED_FULL bMaxPacketSize0 discovery

[Mark Kettenis]

> From: Janne Grunau <j@jannau.net>
> Date: Mon, 29 Aug 2022 08:31:27 +0200
> 
> Fixes probing of various keyboards with DWC3 as integrated into Apple
> silicon SoCs. The problem appears to be requesting more data than the
> devices's bMaxPacketSize0 of 8. Older Logitech unifying receivers
> (bcdDevice 12.03 or 12.10) are for eaxample affected.

I somehow suspect this is a more general issue that doesn't really
depend on the particular USB host controller that is in use, but
rather on the particular USB device.  Would be interesting if someone
could test this on other hardware.  Probably the easiest way to fix
this is to find a keyboard that currently doesn't work with u-boot and
see whether this fixes it when used on something like a Raspberry Pi 4
(which has a PCI XHCI controller).

Don't think this should hold up this fix though.

> Signed-off-by: Janne Grunau <j@jannau.net>
> Tested-by: Thomas Glanzmann <thomas@glanzmann.de>
> ---
>  common/usb.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/common/usb.c b/common/usb.c
> index 6fcf1e8428e9..48a310e8599d 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -993,10 +993,12 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
>  		 *
>  		 * At least the DWC2 controller needs to be programmed with
>  		 * the number of packets in addition to the number of bytes.
> -		 * A request for 64 bytes of data with the maxpacket guessed
> -		 * as 64 (above) yields a request for 1 packet.
> +		 * Requesting more than 8 bytes causes probing errors on the
> +		 * DWC3 controller integrated into Apple silicon SoCs with
> +		 * devices with bMaxPacketSize0 of 8. So limit the read request
> +		 * to the used size of 8 bytes.
>  		 */
> -		err = get_descriptor_len(dev, 64, 8);
> +		err = get_descriptor_len(dev, 8, 8);
>  		if (err)
>  			return err;
>  	}
> -- 
> 2.35.1
> 
> 

Re: [PATCH 1/1] usb: request only 8 bytes for USB_SPEED_FULL bMaxPacketSize0 discovery

[Marek Vasut]

On 8/29/22 08:31, Janne Grunau wrote:
> Fixes probing of various keyboards with DWC3 as integrated into Apple
> silicon SoCs. The problem appears to be requesting more data than the
> devices's bMaxPacketSize0 of 8. Older Logitech unifying receivers
> (bcdDevice 12.03 or 12.10) are for eaxample affected.
> 
> Signed-off-by: Janne Grunau <j@jannau.net>
> Tested-by: Thomas Glanzmann <thomas@glanzmann.de>
> ---
>   common/usb.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/common/usb.c b/common/usb.c
> index 6fcf1e8428e9..48a310e8599d 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -993,10 +993,12 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
>   		 *
>   		 * At least the DWC2 controller needs to be programmed with
>   		 * the number of packets in addition to the number of bytes.
> -		 * A request for 64 bytes of data with the maxpacket guessed
> -		 * as 64 (above) yields a request for 1 packet.
> +		 * Requesting more than 8 bytes causes probing errors on the
> +		 * DWC3 controller integrated into Apple silicon SoCs with
> +		 * devices with bMaxPacketSize0 of 8. So limit the read request
> +		 * to the used size of 8 bytes.
>   		 */
> -		err = get_descriptor_len(dev, 64, 8);
> +		err = get_descriptor_len(dev, 8, 8);
>   		if (err)
>   			return err;
>   	}

Sorry for the delay.

Can you be more specific about those logitech receivers ? I might have 
one of those devices, and I have DWC3 in i.MX8MP and i.MX8MQ, as well as 
ZynqMP, so I should be able to try and trigger the problem. Can you 
share the reproducer test case for this problem ?

If the problem is specific to the Apple instance of the controller, 
maybe we need some sort of quirk instead ?

Thanks

Re: [PATCH 1/1] usb: request only 8 bytes for USB_SPEED_FULL bMaxPacketSize0 discovery

[Janne Grunau]

On 2022-09-26 01:52:37 +0200, Marek Vasut wrote:
> On 8/29/22 08:31, Janne Grunau wrote:
> > Fixes probing of various keyboards with DWC3 as integrated into Apple
> > silicon SoCs. The problem appears to be requesting more data than the
> > devices's bMaxPacketSize0 of 8. Older Logitech unifying receivers
> > (bcdDevice 12.03 or 12.10) are for eaxample affected.
> > 
> > Signed-off-by: Janne Grunau <j@jannau.net>
> > Tested-by: Thomas Glanzmann <thomas@glanzmann.de>
> > ---
> >   common/usb.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/common/usb.c b/common/usb.c
> > index 6fcf1e8428e9..48a310e8599d 100644
> > --- a/common/usb.c
> > +++ b/common/usb.c
> > @@ -993,10 +993,12 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
> >   		 *
> >   		 * At least the DWC2 controller needs to be programmed with
> >   		 * the number of packets in addition to the number of bytes.
> > -		 * A request for 64 bytes of data with the maxpacket guessed
> > -		 * as 64 (above) yields a request for 1 packet.
> > +		 * Requesting more than 8 bytes causes probing errors on the
> > +		 * DWC3 controller integrated into Apple silicon SoCs with
> > +		 * devices with bMaxPacketSize0 of 8. So limit the read request
> > +		 * to the used size of 8 bytes.
> >   		 */
> > -		err = get_descriptor_len(dev, 64, 8);
> > +		err = get_descriptor_len(dev, 8, 8);
> >   		if (err)
> >   			return err;
> >   	}
> 
> Sorry for the delay.
> 
> Can you be more specific about those logitech receivers ?

In my case it's a device a little bit larger than a USB-A plug. ~7mm 
heigh black plastic case with "logitech" written on the end and and 
their unifying logo in orange on a side (idVendor: 0x046d, idProduct: 
0xc52b, bcdDevice: 12.03). Russell has one with bcdDevice: 12.10

The problem is not limited to the logitech receivers. It reproduces for 
other keyboards with bMaxPacketSize0 == 8 as well. I suspect it affects 
all devices with bMaxPacketSize0 == 8. Keyboards are simply the type of 
device those breakage is noticed inside u-boot / efi bootloaders on 
desktop class devices and don't transfer that much data so that 
bMaxPacketSize0 == 8 doesn't hurt.

> I might have one
> of those devices, and I have DWC3 in i.MX8MP and i.MX8MQ, as well as ZynqMP,
> so I should be able to try and trigger the problem. Can you share the
> reproducer test case for this problem ?

The device is not detected. It is not listed as detected usb device 
during u-boot's USB probing. If the device is a keyboard or you have a 
matching logitech wireless keyboard it will not work to interrupt the 
auto boot or on the u-boot prompt.

> If the problem is specific to the Apple instance of the controller, 
> maybe we need some sort of quirk instead ?

The code looks evidently broken to me. The comment says that we can only 
expect to receive a single packet. We request 64 bytes but devices might 
have a bMaxPacketSize0 of 8. Requesting more than 8 bytes looks also 
unnecessary as we are only interested in bMaxPacketSize0 which is within 
the first 8 bytes of the device descriptor.

Thanks

Janne

Re: [PATCH 1/1] usb: request only 8 bytes for USB_SPEED_FULL bMaxPacketSize0 discovery

[Thomas Glanzmann]

Hallo Marek,

> Can you be more specific about those logitech receivers ? I might have one
> of those devices, and I have DWC3 in i.MX8MP and i.MX8MQ, as well as ZynqMP,
> so I should be able to try and trigger the problem. Can you share the
> reproducer test case for this problem ?

I can reproduce the issue with Keychron K1 and Realforce keyboards on the
usb-c port of my mac mini 2020 model running u-boot. The two keyboards
don't work without Jannes patch. With Jannes patch, they work. Other
keyboards like my Thinkpad compact keyboard work with and without the
patch.

https://www.keychron.com/products/keychron-k1-wireless-mechanical-keyboard
https://www.realforcekeyboards.com
https://www.lenovo.com/de/de/accessories-and-monitors/keyboards-and-mice/keyboards/KEYBOARD-German/p/0B47202

Cheers,
        Thomas

Re: [PATCH 1/1] usb: request only 8 bytes for USB_SPEED_FULL bMaxPacketSize0 discovery

[Marek Vasut]

On 9/26/22 09:34, Thomas Glanzmann wrote:
> Hallo Marek,

Hi,

>> Can you be more specific about those logitech receivers ? I might have one
>> of those devices, and I have DWC3 in i.MX8MP and i.MX8MQ, as well as ZynqMP,
>> so I should be able to try and trigger the problem. Can you share the
>> reproducer test case for this problem ?
> 
> I can reproduce the issue with Keychron K1 and Realforce keyboards on the
> usb-c port of my mac mini 2020 model running u-boot. The two keyboards
> don't work without Jannes patch. With Jannes patch, they work. Other
> keyboards like my Thinkpad compact keyboard work with and without the
> patch.
> 
> https://www.keychron.com/products/keychron-k1-wireless-mechanical-keyboard
> https://www.realforcekeyboards.com
> https://www.lenovo.com/de/de/accessories-and-monitors/keyboards-and-mice/keyboards/KEYBOARD-German/p/0B47202
> 

I can also reproduce it with the logitech unifying receiver on iMX8MP 
DWC3 controller, likely close to what Russell has (046d:c52b Version 18.16).

The same device does work on iMX8MM ChipIdea USB 2.0 controller.

I wonder if this might be specific to the DWC3 controller (driver?) 
somehow ?