[U-Boot] [PATCH v3 1/3] usb: usb_new_device return codes consistency
This makes use of errno return codes for representing error codes in a unified way.
Signed-off-by: Paul Kocialkowski contact@paulk.fr --- common/usb.c | 58 +++++++++++++++++++++++++++++----------------------------- 1 file changed, 29 insertions(+), 29 deletions(-)
diff --git a/common/usb.c b/common/usb.c index 32e15cd..1288ca0 100644 --- a/common/usb.c +++ b/common/usb.c @@ -116,7 +116,7 @@ int usb_init(void) if (controllers_initialized == 0) puts("USB error: all controllers failed lowlevel init\n");
- return usb_started ? 0 : -1; + return usb_started ? 0 : -ENODEV; }
/****************************************************************************** @@ -186,7 +186,7 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe,
if ((timeout == 0) && (!asynch_allowed)) { /* request for a asynch control pipe is not allowed */ - return -1; + return -EINVAL; }
/* set setup command */ @@ -201,7 +201,7 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe, dev->status = USB_ST_NOT_PROC; /*not yet processed */
if (submit_control_msg(dev, pipe, data, size, setup_packet) < 0) - return -1; + return -EIO; if (timeout == 0) return (int)size;
@@ -224,17 +224,17 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe,
/*------------------------------------------------------------------- * submits bulk message, and waits for completion. returns 0 if Ok or - * -1 if Error. + * negative if Error. * synchronous behavior */ int usb_bulk_msg(struct usb_device *dev, unsigned int pipe, void *data, int len, int *actual_length, int timeout) { if (len < 0) - return -1; + return -EINVAL; dev->status = USB_ST_NOT_PROC; /*not yet processed */ if (submit_bulk_msg(dev, pipe, data, len) < 0) - return -1; + return -EIO; while (timeout--) { if (!((volatile unsigned long)dev->status & USB_ST_NOT_PROC)) break; @@ -244,7 +244,7 @@ int usb_bulk_msg(struct usb_device *dev, unsigned int pipe, if (dev->status == 0) return 0; else - return -1; + return -EIO; }
@@ -350,11 +350,11 @@ static int usb_parse_config(struct usb_device *dev, if (head->bDescriptorType != USB_DT_CONFIG) { printf(" ERROR: NOT USB_CONFIG_DESC %x\n", head->bDescriptorType); - return -1; + return -EINVAL; } if (head->bLength != USB_DT_CONFIG_SIZE) { printf("ERROR: Invalid USB CFG length (%d)\n", head->bLength); - return -1; + return -EINVAL; } memcpy(&dev->config, head, USB_DT_CONFIG_SIZE); dev->config.no_of_if = 0; @@ -383,7 +383,7 @@ static int usb_parse_config(struct usb_device *dev, if (ifno >= USB_MAXINTERFACES) { puts("Too many USB interfaces!\n"); /* try to go on with what we have */ - return 1; + return -EINVAL; } if_desc = &dev->config.if_desc[ifno]; dev->config.no_of_if++; @@ -421,7 +421,7 @@ static int usb_parse_config(struct usb_device *dev, if (epno > USB_MAXENDPOINTS) { printf("Interface %d has too many endpoints!\n", if_desc->desc.bInterfaceNumber); - return 1; + return -EINVAL; } /* found an endpoint */ if_desc->no_of_ep++; @@ -459,7 +459,7 @@ static int usb_parse_config(struct usb_device *dev, break; default: if (head->bLength == 0) - return 1; + return -EINVAL;
debug("unknown Description Type : %x\n", head->bDescriptorType); @@ -479,7 +479,7 @@ static int usb_parse_config(struct usb_device *dev, index += head->bLength; head = (struct usb_descriptor_header *)&buffer[index]; } - return 1; + return 0; }
/*********************************************************************** @@ -546,14 +546,14 @@ int usb_get_configuration_no(struct usb_device *dev, else printf("config descriptor too short " \ "(expected %i, got %i)\n", 9, result); - return -1; + return -EIO; } length = le16_to_cpu(config->wTotalLength);
if (length > USB_BUFSIZ) { printf("%s: failed to get descriptor - too long: %d\n", __func__, length); - return -1; + return -EIO; }
result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno, buffer, length); @@ -595,7 +595,7 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) } if (!if_face) { printf("selecting invalid interface %d", interface); - return -1; + return -EINVAL; } /* * We should return now for devices with only one alternate setting. @@ -634,7 +634,7 @@ static int usb_set_configuration(struct usb_device *dev, int configuration) dev->toggle[1] = 0; return 0; } else - return -1; + return -EIO; }
/******************************************************************** @@ -748,7 +748,7 @@ static int usb_string_sub(struct usb_device *dev, unsigned int langid, }
if (rc < 2) - rc = -1; + rc = -EINVAL;
return rc; } @@ -767,7 +767,7 @@ int usb_string(struct usb_device *dev, int index, char *buf, size_t size) unsigned int u, idx;
if (size <= 0 || !buf || !index) - return -1; + return -EINVAL; buf[0] = 0; tbuf = &mybuf[0];
@@ -777,10 +777,10 @@ int usb_string(struct usb_device *dev, int index, char *buf, size_t size) if (err < 0) { debug("error getting string descriptor 0 " \ "(error=%lx)\n", dev->status); - return -1; + return -EIO; } else if (tbuf[0] < 4) { debug("string descriptor 0 too short\n"); - return -1; + return -EIO; } else { dev->have_langid = -1; dev->string_langid = tbuf[2] | (tbuf[3] << 8); @@ -893,7 +893,7 @@ int usb_new_device(struct usb_device *dev) */ if (usb_alloc_device(dev)) { printf("Cannot allocate device context to get SLOT_ID\n"); - return -1; + return -EINVAL; }
/* We still haven't set the Address yet */ @@ -915,7 +915,7 @@ int usb_new_device(struct usb_device *dev) if (err < 8) { printf("\n USB device not responding, " \ "giving up (status=%lX)\n", dev->status); - return 1; + return -EIO; } memcpy(&dev->descriptor, tmpbuf, 8); #else @@ -952,7 +952,7 @@ int usb_new_device(struct usb_device *dev) err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64); if (err < 0) { debug("usb_new_device: usb_get_descriptor() failed\n"); - return 1; + return -EIO; }
dev->descriptor.bMaxPacketSize0 = desc->bMaxPacketSize0; @@ -968,7 +968,7 @@ int usb_new_device(struct usb_device *dev) err = hub_port_reset(dev->parent, dev->portnr - 1, &portstatus); if (err < 0) { printf("\n Couldn't reset port %i\n", dev->portnr); - return 1; + return -EIO; } } else { usb_reset_root_port(); @@ -998,7 +998,7 @@ int usb_new_device(struct usb_device *dev) if (err < 0) { printf("\n USB device not accepting new address " \ "(error=%lX)\n", dev->status); - return 1; + return -EIO; }
mdelay(10); /* Let the SET_ADDRESS settle */ @@ -1014,7 +1014,7 @@ int usb_new_device(struct usb_device *dev) else printf("USB device descriptor short read " \ "(expected %i, got %i)\n", tmp, err); - return 1; + return -EIO; } memcpy(&dev->descriptor, tmpbuf, sizeof(dev->descriptor)); /* correct le values */ @@ -1028,7 +1028,7 @@ int usb_new_device(struct usb_device *dev) printf("usb_new_device: Cannot read configuration, " \ "skipping device %04x:%04x\n", dev->descriptor.idVendor, dev->descriptor.idProduct); - return -1; + return -EIO; } usb_parse_config(dev, tmpbuf, 0); usb_set_maxpacket(dev); @@ -1036,7 +1036,7 @@ int usb_new_device(struct usb_device *dev) if (usb_set_configuration(dev, dev->config.desc.bConfigurationValue)) { printf("failed to set default configuration " \ "len %d, status %lX\n", dev->act_len, dev->status); - return -1; + return -EIO; } debug("new device strings: Mfr=%d, Product=%d, SerialNumber=%d\n", dev->descriptor.iManufacturer, dev->descriptor.iProduct,
This checks that a new USB device is correctly initialized and frees it if not. In addition, this doesn't report that USB was started when no device was found.
Signed-off-by: Paul Kocialkowski contact@paulk.fr --- common/usb.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/common/usb.c b/common/usb.c index 1288ca0..6ed3124 100644 --- a/common/usb.c +++ b/common/usb.c @@ -95,18 +95,24 @@ int usb_init(void) start_index = dev_index; printf("scanning bus %d for devices... ", i); dev = usb_alloc_new_device(ctrl); + if (!dev) + break; + /* * device 0 is always present * (root hub, so let it analyze) */ - if (dev) - usb_new_device(dev); + ret = usb_new_device(dev); + if (ret) + usb_free_device();
- if (start_index == dev_index) + if (start_index == dev_index) { puts("No USB Device found\n"); - else + continue; + } else { printf("%d USB Device(s) found\n", dev_index - start_index); + }
usb_started = 1; }
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.
Signed-off-by: Paul Kocialkowski contact@paulk.fr --- common/usb.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/common/usb.c b/common/usb.c index 6ed3124..f648847 100644 --- 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; } @@ -996,6 +996,9 @@ int usb_new_device(struct usb_device *dev) case 64: dev->maxpacketsize = PACKET_SIZE_64; break; + default: + printf("usb_new_device: invalid max packet size\n"); + return -EIO; } dev->devnum = addr;
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.
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 at 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. */
Le mardi 07 avril 2015 à 23:41 -0600, Stephen Warren a écrit :
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.
Damn, I'm really sorry about that, I wish I had had more hardware available to test it thoroughly and that I had thought twice about it and foresaw that issue. My sincerest apologies for that mistake and of course, thanks for noticing and picking it up!
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.
That makes sense on USB low speed devices.
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))
I would be happy with using 8 directly since that's AFAIK the smallest packet size that should be used on USB. Using offsetof, while not requiring any hardcoded value, seems a bit unclean to me as it abstracts that fact.
... 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:
That makes sense, too.
The max packet size for the data stage is 8 bytes at low speed, 8, 16, 32 or 64 at 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. */
On Saturday, April 04, 2015 at 03:12:27 PM, Paul Kocialkowski wrote:
This makes use of errno return codes for representing error codes in a unified way.
Signed-off-by: Paul Kocialkowski contact@paulk.fr
Applied all three to u-boot-usb/next . Thanks !
Best regards, Marek Vasut
participants (3)
-
Marek Vasut -
Paul Kocialkowski -
Stephen Warren