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

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

[Paul Kocialkowski]

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 common/usb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index 32e15cd..ea5b406 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -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 */
-- 
1.9.1

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

[Paul Kocialkowski]

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 ea5b406..67e2350 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 v2 2/3] usb: Check usb_new_device for failure

[Marek Vasut]

On Sunday, March 29, 2015 at 12:28:18 PM, Paul Kocialkowski wrote:
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>

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

Commit message please.

Best regards,
Marek Vasut

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

diff --git a/common/usb.c b/common/usb.c
index 67e2350..fb00c95 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -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;
 
-- 
1.9.1

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

[Marek Vasut]

On Sunday, March 29, 2015 at 12:28:19 PM, 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.
> 
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---

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

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

[Lukasz Majewski]

Hi Paul,

> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---
>  common/usb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/common/usb.c b/common/usb.c
> index 32e15cd..ea5b406 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -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).

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



-- 
Best regards,

Lukasz Majewski

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

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

[Paul Kocialkowski]

Le lundi 30 mars 2015 ? 10:06 +0200, Lukasz Majewski a ?crit :
> Hi Paul,
> 
> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > ---
> >  common/usb.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/usb.c b/common/usb.c
> > index 32e15cd..ea5b406 100644
> > --- a/common/usb.c
> > +++ b/common/usb.c
> > @@ -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).

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

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

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

[Marek Vasut]

On Monday, March 30, 2015 at 03:36:29 PM, Paul Kocialkowski wrote:
> Le lundi 30 mars 2015 ? 10:06 +0200, Lukasz Majewski a ?crit :
> > Hi Paul,
> > 
> > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > > ---
> > > 
> > >  common/usb.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/common/usb.c b/common/usb.c
> > > index 32e15cd..ea5b406 100644
> > > --- a/common/usb.c
> > > +++ b/common/usb.c
> > > @@ -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).

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

Best regards,
Marek Vasut