[U-Boot] [PATCH] EHCI: zero out QH transfer overlay in ehci_submit_async()

[U-Boot] [PATCH] EHCI: zero out QH transfer overlay in ehci_submit_async()

[Sergei Shtylyov]

ehci_submit_async() doesn't really zero out the QH transfer overlay (as the EHCI
specification suggests) which leads to the controller seeing the 'token' field
as the previous call has left it, i.e.:
- if a timeout occured on the previous call (Active bit left as 1), controller
  incorrectly tries to complete a previous transaction on a newly programmed
  endpoint;
- if a halt occured on the previous call (Halted bit set to 1), controller just
  ignores the newly programmed TD(s) and the function then keeps returning error
  ad infinitum.

This turned out to be caused by the wrong orger of the arguments to the memset()
call in ehci_alloc(), so the allocated TDs weren't cleared either.

While at it, stop needlessly initializing the "next" pointers in the QH transfer
overlay...

Signed-off-by: Sergei Shtylyov <sshtylyov@mvista.com>

---
This is quite serious error, so would be good to have the patch in v2010.06...

 drivers/usb/host/ehci-hcd.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Index: u-boot/drivers/usb/host/ehci-hcd.c
===================================================================
--- u-boot.orig/drivers/usb/host/ehci-hcd.c
+++ u-boot/drivers/usb/host/ehci-hcd.c
@@ -275,7 +275,7 @@ static void *ehci_alloc(size_t sz, size_
 		return NULL;
 	}
 
-	memset(p, sz, 0);
+	memset(p, 0, sz);
 	return p;
 }
 
@@ -349,8 +349,6 @@ ehci_submit_async(struct usb_device *dev
 	    (dev->portnr << 23) |
 	    (dev->parent->devnum << 16) | (0 << 8) | (0 << 0);
 	qh->qh_endpt2 = cpu_to_hc32(endpt);
-	qh->qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
-	qh->qh_overlay.qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
 
 	td = NULL;
 	tdp = &qh->qh_overlay.qt_next;

[U-Boot] [PATCH] EHCI: zero out QH transfer overlay in ehci_submit_async()

[Remy Bohmer]

Hi Wolfgang/Sergei,

2010/6/25 Sergei Shtylyov <sshtylyov@ru.mvista.com>:
> ehci_submit_async() doesn't really zero out the QH transfer overlay (as the EHCI
> specification suggests) which leads to the controller seeing the 'token' field
> as the previous call has left it, i.e.:
> - if a timeout occured on the previous call (Active bit left as 1), controller
> ?incorrectly tries to complete a previous transaction on a newly programmed
> ?endpoint;
> - if a halt occured on the previous call (Halted bit set to 1), controller just
> ?ignores the newly programmed TD(s) and the function then keeps returning error
> ?ad infinitum.
>
> This turned out to be caused by the wrong orger of the arguments to the memset()
> call in ehci_alloc(), so the allocated TDs weren't cleared either.
>
> While at it, stop needlessly initializing the "next" pointers in the QH transfer
> overlay...
>
> Signed-off-by: Sergei Shtylyov <sshtylyov@mvista.com>

Acked-by: Remy Bohmer <linux@bohmer.net>

Wolfgang, can you please pull this patch in the v2010.06 release?

Kind regards,

Remy

> ---
> This is quite serious error, so would be good to have the patch in v2010.06...
>
> ?drivers/usb/host/ehci-hcd.c | ? ?4 +---
> ?1 file changed, 1 insertion(+), 3 deletions(-)
>
> Index: u-boot/drivers/usb/host/ehci-hcd.c
> ===================================================================
> --- u-boot.orig/drivers/usb/host/ehci-hcd.c
> +++ u-boot/drivers/usb/host/ehci-hcd.c
> @@ -275,7 +275,7 @@ static void *ehci_alloc(size_t sz, size_
> ? ? ? ? ? ? ? ?return NULL;
> ? ? ? ?}
>
> - ? ? ? memset(p, sz, 0);
> + ? ? ? memset(p, 0, sz);
> ? ? ? ?return p;
> ?}
>
> @@ -349,8 +349,6 @@ ehci_submit_async(struct usb_device *dev
> ? ? ? ? ? ?(dev->portnr << 23) |
> ? ? ? ? ? ?(dev->parent->devnum << 16) | (0 << 8) | (0 << 0);
> ? ? ? ?qh->qh_endpt2 = cpu_to_hc32(endpt);
> - ? ? ? qh->qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
> - ? ? ? qh->qh_overlay.qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
>
> ? ? ? ?td = NULL;
> ? ? ? ?tdp = &qh->qh_overlay.qt_next;
>

[U-Boot] [PATCH] EHCI: zero out QH transfer overlay in ehci_submit_async()

[Sergei Shtylyov]

Hello.

Remy Bohmer wrote:

> 2010/6/25 Sergei Shtylyov <sshtylyov@ru.mvista.com>:
>> ehci_submit_async() doesn't really zero out the QH transfer overlay (as the EHCI
>> specification suggests) which leads to the controller seeing the 'token' field
>> as the previous call has left it, i.e.:
>> - if a timeout occured on the previous call (Active bit left as 1), controller
>>  incorrectly tries to complete a previous transaction on a newly programmed
>>  endpoint;
>> - if a halt occured on the previous call (Halted bit set to 1), controller just
>>  ignores the newly programmed TD(s) and the function then keeps returning error
>>  ad infinitum.

>> This turned out to be caused by the wrong orger of the arguments to the memset()
>> call in ehci_alloc(), so the allocated TDs weren't cleared either.

>> While at it, stop needlessly initializing the "next" pointers in the QH transfer
>> overlay...

>> Signed-off-by: Sergei Shtylyov <sshtylyov@mvista.com>

> Acked-by: Remy Bohmer <linux@bohmer.net>

> Wolfgang, can you please pull this patch in the v2010.06 release?

    Don't pull it yet, I have a fixed up version coming shortly.

> Kind regards,

> Remy

>> ---
>> This is quite serious error, so would be good to have the patch in v2010.06...

>>  drivers/usb/host/ehci-hcd.c |    4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)

>> Index: u-boot/drivers/usb/host/ehci-hcd.c
>> ===================================================================
>> --- u-boot.orig/drivers/usb/host/ehci-hcd.c
>> +++ u-boot/drivers/usb/host/ehci-hcd.c
>> @@ -275,7 +275,7 @@ static void *ehci_alloc(size_t sz, size_
>>                return NULL;
>>        }
>>
>> -       memset(p, sz, 0);
>> +       memset(p, 0, sz);
>>        return p;
>>  }
>>
>> @@ -349,8 +349,6 @@ ehci_submit_async(struct usb_device *dev
>>            (dev->portnr << 23) |
>>            (dev->parent->devnum << 16) | (0 << 8) | (0 << 0);
>>        qh->qh_endpt2 = cpu_to_hc32(endpt);
>> -       qh->qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);

    This line is still needed as it turned out.

WBR, Sergei