usb: usb_new_device return codes consistency

PATCH New Paul Kocialkowski 3 participants 7 messages Apr 4, 2015 – Apr 8, 2015

lore

Share & tools

lore

Paul Kocialkowski

author

2015-04-04 13:12 UTC #0 lore raw

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 <[email protected]>

---
 common/usb.c | 58 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

common/usb.c +29 ~~-29~~

index 32e15cd..1288ca0 100644
@@ -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,

Signature · 1 line

1.9.1

Paul Kocialkowski

author

2015-04-04 13:12 UTC #1 lore raw

PATCH v3 2/3 usb: Check usb_new_device for failure

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 <[email protected]>

---
 common/usb.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

common/usb.c +10 -4

index 1288ca0..6ed3124 100644
@@ -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;
 	}

Signature · 1 line

1.9.1

Paul Kocialkowski

author

2015-04-04 13:12 UTC #2 lore raw

PATCH v3 3/3 usb: Early failure when the first descriptor read fails or is invalid

In reply to Paul Kocialkowski

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 <[email protected]>

---
 common/usb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

common/usb.c +4 -1

index 6ed3124..f648847 100644
@@ -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;

Signature · 1 line

1.9.1

Stephen Warren

2015-04-08 05:07 UTC #3 lore raw

PATCH v3 3/3 usb: Early failure when the first descriptor read fails or is invalid

Quoted 3 lines · Paul Kocialkowski

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.

common/usb.c int usb_new_device(struct usb_device *dev) · line 962

Quoted 11 lines

common/usb.c +1 -1

@@ -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.

Stephen Warren

2015-04-08 05:41 UTC #4 lore raw

PATCH v3 3/3 usb: Early failure when the first descriptor read fails or is invalid

Quoted 28 lines · Stephen Warren

Quoted 3 lines · Paul Kocialkowski
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.
common/usb.c int usb_new_device(struct usb_device *dev) · line 962
Quoted 11 lines
common/usb.c +1 -1
@@ -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) {

Quoted 7 lines

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:

Quoted 3 lines

int chunk_msg(struct usb_device *dev, unsigned long pipe, int *pid, int in,
void *buffer, int len, bool ignore_ack)
{

...

Quoted 1 line

	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:

Quoted 4 lines

/* 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. */

Paul Kocialkowski

author

2015-04-08 20:59 UTC #5 lore raw

PATCH v3 3/3 usb: Early failure when the first descriptor read fails or is invalid

Le mardi 07 avril 2015 ? 23:41 -0600, Stephen Warren a ?crit :

Quoted 8 lines · Stephen Warren

Quoted 7 lines · Paul Kocialkowski
Quoted 3 lines
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!

Quoted 14 lines

Quoted 14 lines
common/usb.c int usb_new_device(struct usb_device *dev) · line 962
Quoted 11 lines
common/usb.c +1 -1
@@ -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.

Quoted 21 lines

Quoted 5 lines
Given the description of what this patch is attempting to achieve, shouldn't the replacement check be:
	if (err <= 0) {
Quoted 7 lines
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:
Quoted 3 lines
int chunk_msg(struct usb_device *dev, unsigned long pipe, int *pid, int in,
void *buffer, int len, bool ignore_ack)
{
...
Quoted 1 line
	int max = usb_maxpacket(dev, pipe);

Quoted 8 lines

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.

Quoted 6 lines

... 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.

Quoted 13 lines

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:
Quoted 4 lines
/* 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. */

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150408/b0770c86/attachment.sig>

Marek Vasut

2015-04-06 15:05 UTC #6 lore raw

PATCH v3 1/3 usb: usb_new_device return codes consistency

In reply to Paul Kocialkowski

Quoted 4 lines · Paul Kocialkowski

This makes use of errno return codes for representing error codes in a unified way.
Signed-off-by Paul Kocialkowski <[email protected]>

Applied all three to u-boot-usb/next . Thanks !

Best regards,
Marek Vasut