[U-Boot] [PATCH v3 3/3] usb: Early failure when the first descriptor read fails or is invalid

[Stephen Warren <swarren@wwwdotorg.org>]

On 04/07/2015 11:07 PM, Stephen Warren wrote:
> On 04/04/2015 07:12 AM, Paul Kocialkowski wrote:
>> This may happen when using an USB1 device on a controller that only supports
>> USB2 (e.g. EHCI). Reading the first descriptor will fail (read 0 byte), so we
>> can abort the process at this point instead of failing later and wasting time.
> 
> FYI, this patch breaks USB keyboard (or perhaps any USB 1.x device)
> support on the RPi.
> 
>> diff --git a/common/usb.c b/common/usb.c
> 
>> @@ -956,7 +956,7 @@ int usb_new_device(struct usb_device *dev)
>>  	 */
>>  #ifndef CONFIG_USB_XHCI
>>  	err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
>> -	if (err < 0) {
>> +	if (err < sizeof(struct usb_device_descriptor)) {
>>  		debug("usb_new_device: usb_get_descriptor() failed\n");
>>  		return -EIO;
>>  	}
> 
> The value returned here is 8 not 18 (the sizeof the descriptor), so the
> code bails out with an error.
> 
> Given the description of what this patch is attempting to achieve,
> shouldn't the replacement check be:
> 
> 	if (err <= 0) {
> 
> My guess for why the value 8 is returned is because the device's
> maxpacket is 8, and the DWC2 driver only attempts to transfer 1 packet
> because the requested transfer size is 64, and the default packet size
> is 64, which means 1 packet. Note that DWC2 HW (perhaps unlike e.g.
> EHCI) requires the driver to specify the number of packets transferred,
> not a byte count. However, I haven't validated that yet. My device is a
> USB 1.x FS keyboard.

Yes, I can avoid the error by hacking dwc2.c's assignment of max:

> int chunk_msg(struct usb_device *dev, unsigned long pipe, int *pid, int in,
> 	      void *buffer, int len, bool ignore_ack)
> {
...
> 	int max = usb_maxpacket(dev, pipe);

and forcing that to 8 rather than 64 for the data transfer phase of the
first transaction to each USB device (my keyboard has a built-in hub, so
I need to override a few different transactions).

Thinking about the fix more, perhaps something like the following is
appropriate:

if (err < offsetof(struct usb_device_descriptor, idVendor))

... since idVendor is the first field that's not used by the code that
processes the results of the initial descriptor read. Hopefully there
are no devices with a control endpoint bMaxPacketSize0 less than 8
(which is what that offsetof evaluates to).
http://www.usbmadesimple.co.uk/ums_3.htm seems to imply that's the case,
saying for control transfers:

The max packet size for the data stage is 8 bytes at low speed, 8, 16,
32 or 64@full Speed and 64 for high speed.

(although one of my keyboards has a maxPacket of 4 for EP 2, but I
thinkt hat's something different)

Also note the comment a little before the code this patch affects in
U-Boot's common/usb.c:

> 	/* send 64-byte GET-DEVICE-DESCRIPTOR request.  Since the descriptor is
> 	 * only 18 bytes long, this will terminate with a short packet.  But if
> 	 * the maxpacket size is 8 or 16 the device may be waiting to transmit
> 	 * some more, or keeps on retransmitting the 8 byte header. */