usb: usb_new_device return codes consistency

PATCH Superseded Paul Kocialkowski 3 participants 8 messages Mar 29, 2015 – Apr 3, 2015

lore

Share & tools

lore

Paul Kocialkowski

author

2015-03-29 10:28 UTC #0 lore raw

PATCH v2 1/3 usb: usb_new_device return codes consistency

Signed-off-by Paul Kocialkowski <[email protected]>

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

common/usb.c +4 -4

index 32e15cd..ea5b406 100644
@@ -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 -1;
 	}
 	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 -1;
 	}
 
 	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 -1;
 		}
 	} else {
 		usb_reset_root_port();

@@ -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 -1;
 	}
 	memcpy(&dev->descriptor, tmpbuf, sizeof(dev->descriptor));
 	/* correct le values */

Signature · 1 line

1.9.1

Paul Kocialkowski

author

2015-03-29 10:28 UTC #1 lore raw

PATCH v2 2/3 usb: Check usb_new_device for failure

Signed-off-by Paul Kocialkowski <[email protected]>

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

common/usb.c +10 -4

index ea5b406..67e2350 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

Marek Vasut

2015-04-03 02:03 UTC #2 lore raw

PATCH v2 2/3 usb: Check usb_new_device for failure

Quoted 1 line · Paul Kocialkowski

Signed-off-by Paul Kocialkowski <[email protected]>

I don't really want to spell it out, but I guess I have to, sorry ...

Commit message please.

Best regards,
Marek Vasut

Paul Kocialkowski

author

2015-03-29 10:28 UTC #3 lore raw

PATCH v2 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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

common/usb.c +5 -2

index 67e2350..fb00c95 100644
@@ -956,8 +956,8 @@ 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) {
-		debug("usb_new_device: usb_get_descriptor() failed\n");
+	if (err < sizeof(struct usb_device_descriptor)) {
+		printf("usb_new_device: usb_get_descriptor() failed\n");
 		return -1;
 	}

@@ -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 -1;
 	}
 	dev->devnum = addr;

Signature · 1 line

1.9.1

Marek Vasut

2015-04-03 02:03 UTC #4 lore raw

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

Quoted 7 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.
Signed-off-by Paul Kocialkowski <[email protected]>
---

Please just fix the return -1 (same thing Lukasz was complaining about in 1/3), I will pick it then.

Thanks!

Best regards,
Marek Vasut

Lukasz Majewski

2015-03-30 08:06 UTC #5 lore raw

PATCH v2 1/3 usb: usb_new_device return codes consistency

In reply to Paul Kocialkowski

Hi Paul,

Quoted 10 lines

Signed-off-by Paul Kocialkowski <[email protected]>
---
 common/usb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
common/usb.c +0 -0
index 32e15cd..ea5b406 100644

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

Quoted 5 lines

common/usb.c +1 -1

@@ -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 -1;

If you are going to provide consistency with error codes, then I think that it would be beneficial to return -Exxx codes (like -EINVAL, etc).

Quoted 32 lines

common/usb.c +1 -1

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

common/usb.c +1 -1

 	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 -1;
 		}
 	} else {

common/usb.c +1 -1

 		usb_reset_root_port();

@@ -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 -1;
 	}
 	memcpy(&dev->descriptor, tmpbuf, sizeof(dev->descriptor));
 	/* correct le values */

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Paul Kocialkowski

author

2015-03-30 13:36 UTC #6 lore raw

PATCH v2 1/3 usb: usb_new_device return codes consistency

Le lundi 30 mars 2015 ? 10:06 +0200, Lukasz Majewski a ?crit :

Quoted 7 lines

Hi Paul,
Quoted 5 lines
Signed-off-by Paul Kocialkowski <[email protected]>
---
 common/usb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Quoted 13 lines

common/usb.c int usb_new_device(struct usb_device *dev) · line 918
Quoted 10 lines
common/usb.c +1 -1
index 32e15cd..ea5b406 100644
@@ -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 -1;
If you are going to provide consistency with error codes, then I think that it would be beneficial to return -Exxx codes (like -EINVAL, etc).

That makes sense, I'll give it a try soon (I'm not sure I'll get all the appropriate error codes right at first try though).

Quoted 24 lines

common/usb.c +1 -1

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

common/usb.c +1 -1

 	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 -1;
 		}
 	} else {

common/usb.c +0 -0

 		usb_reset_root_port();

@@ -1014,7 +1014,7 @@ int usb_new_device(struct usb_device *dev)

Quoted 11 lines

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

Quoted 8 lines

common/usb.c +1 -1

 		else
 			printf("USB device descriptor short read " \
 				"(expected %i, got %i)\n", tmp, err);
-		return 1;
+		return -1;
 	}
 	memcpy(&dev->descriptor, tmpbuf, sizeof(dev->descriptor));
 	/* correct le values */

-------------- 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/20150330/c92ee3f7/attachment.sig>

Marek Vasut

2015-04-02 17:12 UTC #7 lore raw

PATCH v2 1/3 usb: usb_new_device return codes consistency

Quoted 21 lines · Paul Kocialkowski

Le lundi 30 mars 2015 ? 10:06 +0200, Lukasz Majewski a ?crit :
Quoted 20 lines
Hi Paul,
Quoted 18 lines
Signed-off-by Paul Kocialkowski <[email protected]>
---
 common/usb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
common/usb.c +0 -0
index 32e15cd..ea5b406 100644
@@ -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);
 

Quoted 8 lines

Quoted 5 lines
common/usb.c
Quoted 2 lines
common/usb.c +1 -1
-		return 1;
+		return -1;
If you are going to provide consistency with error codes, then I think that it would be beneficial to return -Exxx codes (like -EINVAL, etc).
That makes sense, I'll give it a try soon (I'm not sure I'll get all the appropriate error codes right at first try though).

I agree, using proper errno is a step in the right direction. Thanks!

Best regards,
Marek Vasut