usb: Early failure when the first descriptor read fails, one way or another
PATCH 1/1 usb: Early failure when the first descriptor read fails, one way or another
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.
--- common/usb.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
common/usb.c +4 -1
@@ -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;
Signature · 1 line
1.9.1
PATCH 1/1 usb: Early failure when the first descriptor read fails, one way or another
Quoted 23 lines · 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 <[email protected]>--- common/usb.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)common/usb.c +1
-1@@ -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 ; }
common/usb.c int usb_new_device(struct usb_device *dev) · line 994
Quoted 5 lines
common/usb.c +2
-0@@ -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!
common/usb.c int usb_new_device(struct usb_device *dev) · line 997
Quoted 3 lines
common/usb.c +1
-0+return 1 ; } dev->devnum = addr;
Best regards,
Marek Vasut
PATCH 1/1 usb: Early failure when the first descriptor read fails, one way or another
Le samedi 28 mars 2015 ? 18:26 +0100, Marek Vasut a ?crit :
Quoted 32 lines · Paul Kocialkowski
Quoted 23 lines
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 | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)common/usb.c +1
-1@@ -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 ; }common/usb.c int usb_new_device(struct usb_device *dev) · line 994
Quoted 5 lines
common/usb.c +2
-0@@ -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,
Quoted 3 lines
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.
Quoted 8 lines
Thanks!
common/usb.c int usb_new_device(struct usb_device *dev) · line 997
Quoted 3 lines
common/usb.c +1
-0+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>
PATCH 1/1 usb: Early failure when the first descriptor read fails, one way or another
Quoted 18 lines
Quoted 9 lines
common/usb.c int usb_new_device(struct usb_device *dev) · line 994
Quoted 6 lines
common/usb.c +2
-0@@ -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,
Quoted 3 lines
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.
Quoted 6 lines
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.
Quoted 5 lines
Quoted 5 lines
Thanks!
Quoted 3 lines
common/usb.c +1
-0+return 1 ; } dev->devnum = addr;
Signature · 16 lines
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>
PATCH 1/1 usb: Early failure when the first descriptor read fails, one way or another
In reply to Paul KocialkowskiQuoted 44 lines · Paul Kocialkowski
Le samedi 28 mars 2015 ? 18:26 +0100, Marek Vasut a ?crit :
Quoted 43 lines · Paul Kocialkowski
Quoted 32 lines
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 | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)common/usb.c +1
-1@@ -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 ; }common/usb.c int usb_new_device(struct usb_device *dev) · line 996
Quoted 7 lines
common/usb.c +2
-0@@ -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,
Quoted 6 lines
Quoted 3 lines
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 ?
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 2 lines
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,
PATCH 1/1 usb: Early failure when the first descriptor read fails, one way or another
Le lundi 30 mars 2015 ? 01:47 +0200, Marek Vasut a ?crit :
Quoted 45 lines · Paul Kocialkowski
Quoted 44 lines
Le samedi 28 mars 2015 ? 18:26 +0100, Marek Vasut a ?crit :
Quoted 43 lines · Paul Kocialkowski
Quoted 32 lines
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 | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)common/usb.c +1
-1@@ -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 ; }common/usb.c int usb_new_device(struct usb_device *dev) · line 996
Quoted 7 lines
common/usb.c +2
-0@@ -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,
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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.
Quoted 4 lines
Quoted 2 lines
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.
Signature · 17 lines
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>
PATCH 1/1 usb: Early failure when the first descriptor read fails, one way or another
Quoted 1 line · Paul Kocialkowski
Le lundi 30 mars 2015 ? 01:47 +0200, Marek Vasut a ?crit :
[...]
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
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).
Quoted 6 lines
Quoted 1 line
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.
Quoted 2 lines
Since the end result is that the USB device won't work, I think it makes sense to print an error.
Yes, I agree.
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
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