[U-Boot] [PATCH] usb: Early failure when the first descriptor read fails, one way or another

[U-Boot] [PATCH] usb: Early failure when the first descriptor read fails, one way or another

[Paul Kocialkowski]

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 32e15cd..d1b3316 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -950,7 +950,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 1;
 	}
@@ -990,6 +990,9 @@ int usb_new_device(struct usb_device *dev)
 	case 64:
 		dev->maxpacketsize = PACKET_SIZE_64;
 		break;
+	default:
+		debug("usb_new_device: invalid max packet size\n");
+		return 1;
 	}
 	dev->devnum = addr;
 
-- 
1.9.1

[U-Boot] [PATCH] usb: Early failure when the first descriptor read fails, one way or another

[Marek Vasut]

On Saturday, March 28, 2015 at 06:23:58 PM, Paul Kocialkowski wrote:
> 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 32e15cd..d1b3316 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -950,7 +950,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 1;
>  	}
> @@ -990,6 +990,9 @@ int usb_new_device(struct usb_device *dev)
>  	case 64:
>  		dev->maxpacketsize = PACKET_SIZE_64;
>  		break;
> +	default:
> +		debug("usb_new_device: invalid max packet size\n");

Hi,

since this is an error, this should probably be a printf(). Also,
to make the error message useful, it should state the invalid value
due to which it failed.

Thanks!

> +		return 1;
>  	}
>  	dev->devnum = addr;

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb: Early failure when the first descriptor read fails, one way or another

[Paul Kocialkowski]

Le samedi 28 mars 2015 ? 18:26 +0100, Marek Vasut a ?crit :
> On Saturday, March 28, 2015 at 06:23:58 PM, Paul Kocialkowski wrote:
> > 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 32e15cd..d1b3316 100644
> > --- a/common/usb.c
> > +++ b/common/usb.c
> > @@ -950,7 +950,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 1;
> >  	}
> > @@ -990,6 +990,9 @@ int usb_new_device(struct usb_device *dev)
> >  	case 64:
> >  		dev->maxpacketsize = PACKET_SIZE_64;
> >  		break;
> > +	default:
> > +		debug("usb_new_device: invalid max packet size\n");
> 
> Hi,
> 
> since this is an error, this should probably be a printf(). Also,
> to make the error message useful, it should state the invalid value
> due to which it failed.

Well, it is not unexpected behaviour in my use case (but I reckon it may
be generally speaking). Plugging an USB1 device on a controller that
doesn't support USB1 should normally fail, this is not some kind of
run-time error.

When this happens in Linux, it just fails (silently) and tries ohci
instead. I'm afraid there is no such mechanism in U-Boot, so the best we
can do is to treat the device as unsupported.

If you're not convinced by this, I can still make a v2 with printf, I
just don't think it's a necessity.

> Thanks!
> 
> > +		return 1;
> >  	}
> >  	dev->devnum = addr;
> 
> Best regards,
> Marek Vasut

-------------- 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/20150328/761a9c72/attachment.sig>

[U-Boot] [PATCH] usb: Early failure when the first descriptor read fails, one way or another

[Paul Kocialkowski]

> > > @@ -990,6 +990,9 @@ int usb_new_device(struct usb_device *dev)
> > >  	case 64:
> > >  		dev->maxpacketsize = PACKET_SIZE_64;
> > >  		break;
> > > +	default:
> > > +		debug("usb_new_device: invalid max packet size\n");
> > 
> > Hi,
> > 
> > since this is an error, this should probably be a printf(). Also,
> > to make the error message useful, it should state the invalid value
> > due to which it failed.
> 
> Well, it is not unexpected behaviour in my use case (but I reckon it may
> be generally speaking). Plugging an USB1 device on a controller that
> doesn't support USB1 should normally fail, this is not some kind of
> run-time error.
> 
> When this happens in Linux, it just fails (silently) and tries ohci
> instead. I'm afraid there is no such mechanism in U-Boot, so the best we
> can do is to treat the device as unsupported.
> 
> If you're not convinced by this, I can still make a v2 with printf, I
> just don't think it's a necessity.

Nervermind this, I have sent out a v2 addressing you concern.

> > Thanks!
> > 
> > > +		return 1;
> > >  	}
> > >  	dev->devnum = addr;

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: http://www.replicant.us/
Blog: http://blog.replicant.us/
Wiki/tracker/forums: http://redmine.replicant.us/
-------------- 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/20150329/c5622822/attachment.sig>

[U-Boot] [PATCH] usb: Early failure when the first descriptor read fails, one way or another

[Marek Vasut]

On Saturday, March 28, 2015 at 06:31:42 PM, Paul Kocialkowski wrote:
> Le samedi 28 mars 2015 ? 18:26 +0100, Marek Vasut a ?crit :
> > On Saturday, March 28, 2015 at 06:23:58 PM, Paul Kocialkowski wrote:
> > > 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 32e15cd..d1b3316 100644
> > > --- a/common/usb.c
> > > +++ b/common/usb.c
> > > @@ -950,7 +950,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 1;
> > >  	
> > >  	}
> > > 
> > > @@ -990,6 +990,9 @@ int usb_new_device(struct usb_device *dev)
> > > 
> > >  	case 64:
> > >  		dev->maxpacketsize = PACKET_SIZE_64;
> > >  		break;
> > > 
> > > +	default:
> > > +		debug("usb_new_device: invalid max packet size\n");
> > 
> > Hi,
> > 
> > since this is an error, this should probably be a printf(). Also,
> > to make the error message useful, it should state the invalid value
> > due to which it failed.
> 
> Well, it is not unexpected behaviour in my use case (but I reckon it may
> be generally speaking).

Why is this not unexpected in your case please ?

> Plugging an USB1 device on a controller that
> doesn't support USB1 should normally fail, this is not some kind of
> run-time error.

Yes.

> When this happens in Linux, it just fails (silently) and tries ohci
> instead. I'm afraid there is no such mechanism in U-Boot, so the best we
> can do is to treat the device as unsupported.

There's no such mechanism indeed. It'd be nice if someone implemented
this handover though.

> If you're not convinced by this, I can still make a v2 with printf, I
> just don't think it's a necessity.

Let me just understand what you're seeing a bit better first please .

Best regards,

[U-Boot] [PATCH] usb: Early failure when the first descriptor read fails, one way or another

[Paul Kocialkowski]

Le lundi 30 mars 2015 ? 01:47 +0200, Marek Vasut a ?crit :
> On Saturday, March 28, 2015 at 06:31:42 PM, Paul Kocialkowski wrote:
> > Le samedi 28 mars 2015 ? 18:26 +0100, Marek Vasut a ?crit :
> > > On Saturday, March 28, 2015 at 06:23:58 PM, Paul Kocialkowski wrote:
> > > > 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 32e15cd..d1b3316 100644
> > > > --- a/common/usb.c
> > > > +++ b/common/usb.c
> > > > @@ -950,7 +950,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 1;
> > > >  	
> > > >  	}
> > > > 
> > > > @@ -990,6 +990,9 @@ int usb_new_device(struct usb_device *dev)
> > > > 
> > > >  	case 64:
> > > >  		dev->maxpacketsize = PACKET_SIZE_64;
> > > >  		break;
> > > > 
> > > > +	default:
> > > > +		debug("usb_new_device: invalid max packet size\n");
> > > 
> > > Hi,
> > > 
> > > since this is an error, this should probably be a printf(). Also,
> > > to make the error message useful, it should state the invalid value
> > > due to which it failed.
> > 
> > Well, it is not unexpected behaviour in my use case (but I reckon it may
> > be generally speaking).
> 
> Why is this not unexpected in your case please ?

Because using an USB low speed device with an EHCI controller should
normally fail (that is, the first descriptor read returns 0 bytes), or
at least it does on the sunxi platforms. I thought there was no need to
report that as an error, but I changed my mind.

Since the end result is that the USB device won't work, I think it makes
sense to print an error.

> > If you're not convinced by this, I can still make a v2 with printf, I
> > just don't think it's a necessity.
> 
> Let me just understand what you're seeing a bit better first please .

At this point, v2 of this patch was sent as: usb: Early failure when the
first descriptor read fails or is invalid

Let me know if you need some more context on this.

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: http://www.replicant.us/
Blog: http://blog.replicant.us/
Wiki/tracker/forums: http://redmine.replicant.us/

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

[U-Boot] [PATCH] usb: Early failure when the first descriptor read fails, one way or another

[Marek Vasut]

On Monday, March 30, 2015 at 03:35:03 PM, Paul Kocialkowski wrote:
> Le lundi 30 mars 2015 ? 01:47 +0200, Marek Vasut a ?crit :

[...]

> > > > Hi,
> > > > 
> > > > since this is an error, this should probably be a printf(). Also,
> > > > to make the error message useful, it should state the invalid value
> > > > due to which it failed.
> > > 
> > > Well, it is not unexpected behaviour in my use case (but I reckon it
> > > may be generally speaking).
> > 
> > Why is this not unexpected in your case please ?
> 
> Because using an USB low speed device with an EHCI controller should
> normally fail (that is, the first descriptor read returns 0 bytes), or
> at least it does on the sunxi platforms. I thought there was no need to
> report that as an error, but I changed my mind.

Indeed.

> Since the end result is that the USB device won't work, I think it makes
> sense to print an error.

Yes, I agree.

> > > If you're not convinced by this, I can still make a v2 with printf, I
> > > just don't think it's a necessity.
> > 
> > Let me just understand what you're seeing a bit better first please .
> 
> At this point, v2 of this patch was sent as: usb: Early failure when the
> first descriptor read fails or is invalid
> 
> Let me know if you need some more context on this.

This is OK, thank you for clarifying !

Best regards,
Marek Vasut