Re: [U-Boot] [PATCH v3 3/3] usb: Early failure when the first descriptor read fails or is invalid
Le mardi 07 avril 2015 à 23:41 -0600, Stephen Warren a écrit :
> On 04/07/2015 11:07 PM, Stephen Warren wrote:
> > On 04/04/2015 07:12 AM, 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.
> >
> > FYI, this patch breaks USB keyboard (or perhaps any USB 1.x device)
> > support on the RPi.
Damn, I'm really sorry about that, I wish I had had more hardware
available to test it thoroughly and that I had thought twice about it
and foresaw that issue. My sincerest apologies for that mistake and of
course, thanks for noticing and picking it up!
> >> diff --git a/common/usb.c b/common/usb.c
> >
> >> @@ -956,7 +956,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 -EIO;
> >>}
> >
> > The value returned here is 8 not 18 (the sizeof the descriptor), so the
> > code bails out with an error.
That makes sense on USB low speed devices.
> > Given the description of what this patch is attempting to achieve,
> > shouldn't the replacement check be:
> >
> > if (err <= 0) {
> >
> > My guess for why the value 8 is returned is because the device's
> > maxpacket is 8, and the DWC2 driver only attempts to transfer 1 packet
> > because the requested transfer size is 64, and the default packet size
> > is 64, which means 1 packet. Note that DWC2 HW (perhaps unlike e.g.
> > EHCI) requires the driver to specify the number of packets transferred,
> > not a byte count. However, I haven't validated that yet. My device is a
> > USB 1.x FS keyboard.
>
> Yes, I can avoid the error by hacking dwc2.c's assignment of max:
>
> > int chunk_msg(struct usb_device *dev, unsigned long pipe, int *pid, int in,
> > void *buffer, int len, bool ignore_ack)
> > {
> ...
> > int max = usb_maxpacket(dev, pipe);
>
> and forcing that to 8 rather than 64 for the data transfer phase of the
> first transaction to each USB device (my keyboard has a built-in hub, so
> I need to override a few different transactions).
>
> Thinking about the fix more, perhaps something like the following is
> appropriate:
>
> if (err < offsetof(struct usb_device_descriptor, idVendor))
I would be happy with using 8 directly since that's AFAIK the smallest
packet size that should be used on USB. Using offsetof, while not
requiring any hardcoded value, seems a bit unclean to me as it abstracts
that fact.
> ... since idVendor is the first field that's not used by the code that
> processes the results of the initial descriptor read. Hopefully there
> are no devices with a control endpoint bMaxPacketSize0 less than 8
> (which is what that offsetof evaluates to).
> http://www.usbmadesimple.co.uk/ums_3.htm seems to imply that's the case,
> saying for control transfers:
That makes sense, too.
> The max packet size for the data stage is 8 bytes at low speed, 8, 16,
> 32 or 64 at full Speed and 64 for high speed.
>
> (although one of my keyboards has a maxPacket of 4 for EP 2, but I
> thinkt hat's something different)
>
> Also note the comment a little before the code this patch affects in
> U-Boot's common/usb.c:
>
> > /* send 64-byte GET-DEVICE-DESCRIPTOR request. Since the descriptor is
> > * only 18 bytes long, this will terminate with a short packet. But if
> > * the maxpacket size is 8 or 16 the device may be waiting to transmit
> > * some more, or keeps on retransmitting the 8 byte header. */
signature.asc
Description: This is a digitally signed message part
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 3/3] usb: Early failure when the first descriptor read fails or is invalid
On 04/07/2015 11:07 PM, Stephen Warren wrote:
> On 04/04/2015 07:12 AM, 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.
>
> FYI, this patch breaks USB keyboard (or perhaps any USB 1.x device)
> support on the RPi.
>
>> diff --git a/common/usb.c b/common/usb.c
>
>> @@ -956,7 +956,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 -EIO;
>> }
>
> The value returned here is 8 not 18 (the sizeof the descriptor), so the
> code bails out with an error.
>
> Given the description of what this patch is attempting to achieve,
> shouldn't the replacement check be:
>
> if (err <= 0) {
>
> My guess for why the value 8 is returned is because the device's
> maxpacket is 8, and the DWC2 driver only attempts to transfer 1 packet
> because the requested transfer size is 64, and the default packet size
> is 64, which means 1 packet. Note that DWC2 HW (perhaps unlike e.g.
> EHCI) requires the driver to specify the number of packets transferred,
> not a byte count. However, I haven't validated that yet. My device is a
> USB 1.x FS keyboard.
Yes, I can avoid the error by hacking dwc2.c's assignment of max:
> int chunk_msg(struct usb_device *dev, unsigned long pipe, int *pid, int in,
> void *buffer, int len, bool ignore_ack)
> {
...
> int max = usb_maxpacket(dev, pipe);
and forcing that to 8 rather than 64 for the data transfer phase of the
first transaction to each USB device (my keyboard has a built-in hub, so
I need to override a few different transactions).
Thinking about the fix more, perhaps something like the following is
appropriate:
if (err < offsetof(struct usb_device_descriptor, idVendor))
... since idVendor is the first field that's not used by the code that
processes the results of the initial descriptor read. Hopefully there
are no devices with a control endpoint bMaxPacketSize0 less than 8
(which is what that offsetof evaluates to).
http://www.usbmadesimple.co.uk/ums_3.htm seems to imply that's the case,
saying for control transfers:
The max packet size for the data stage is 8 bytes at low speed, 8, 16,
32 or 64 at full Speed and 64 for high speed.
(although one of my keyboards has a maxPacket of 4 for EP 2, but I
thinkt hat's something different)
Also note the comment a little before the code this patch affects in
U-Boot's common/usb.c:
> /* send 64-byte GET-DEVICE-DESCRIPTOR request. Since the descriptor is
>* only 18 bytes long, this will terminate with a short packet. But if
>* the maxpacket size is 8 or 16 the device may be waiting to transmit
>* some more, or keeps on retransmitting the 8 byte header. */
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 3/3] usb: Early failure when the first descriptor read fails or is invalid
On 04/04/2015 07:12 AM, 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.
FYI, this patch breaks USB keyboard (or perhaps any USB 1.x device)
support on the RPi.
> diff --git a/common/usb.c b/common/usb.c
> @@ -956,7 +956,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 -EIO;
> }
The value returned here is 8 not 18 (the sizeof the descriptor), so the
code bails out with an error.
Given the description of what this patch is attempting to achieve,
shouldn't the replacement check be:
if (err <= 0) {
My guess for why the value 8 is returned is because the device's
maxpacket is 8, and the DWC2 driver only attempts to transfer 1 packet
because the requested transfer size is 64, and the default packet size
is 64, which means 1 packet. Note that DWC2 HW (perhaps unlike e.g.
EHCI) requires the driver to specify the number of packets transferred,
not a byte count. However, I haven't validated that yet. My device is a
USB 1.x FS keyboard.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v3 3/3] usb: Early failure when the first descriptor read fails or is invalid
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
---
common/usb.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/common/usb.c b/common/usb.c
index 6ed3124..f648847 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -956,7 +956,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 -EIO;
}
@@ -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 -EIO;
}
dev->devnum = addr;
--
1.9.1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
