From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Tue, 07 Apr 2015 23:41:02 -0600 Subject: [U-Boot] [PATCH v3 3/3] usb: Early failure when the first descriptor read fails or is invalid In-Reply-To: <5524B779.5090308@wwwdotorg.org> References: <1428153149-3738-1-git-send-email-contact@paulk.fr> <1428153149-3738-3-git-send-email-contact@paulk.fr> <5524B779.5090308@wwwdotorg.org> Message-ID: <5524BF6E.5090308@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. */