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

[Marek Vasut <marex@denx.de>]

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,