[U-Boot] [PATCH v3 1/3] usb: usb_new_device return codes consistency

[U-Boot] [PATCH v3 1/3] usb: usb_new_device return codes consistency

[Paul Kocialkowski]

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,
-- 
1.9.1

[U-Boot] [PATCH v3 2/3] usb: Check usb_new_device for failure

[Paul Kocialkowski]

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;
 	}
-- 
1.9.1

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

[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 <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;
 
-- 
1.9.1

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

[Stephen Warren]

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.

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

[Stephen Warren]

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

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

[Paul Kocialkowski]

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

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

[U-Boot] [PATCH v3 1/3] usb: usb_new_device return codes consistency

[Marek Vasut]

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