Skip to contentSkip to conversation

EHCI: zero out QH transfer overlay in ehci_submit_async()

PATCH Superseded Sergei Shtylyov 3 participants 3 messages Jun 25, 2010 – Jun 28, 2010

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 EHCI: zero out QH transfer overlay in ehci_submit_async()

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...

---
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(-)
drivers/usb/host/ehci-hcd.c +1 -3
Index: 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;
#1 lore raw

PATCH 1/1 EHCI: zero out QH transfer overlay in ehci_submit_async()

Hi Wolfgang/Sergei,

2010/6/25 Sergei Shtylyov <[email protected]>:

Quoted 10 lines

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.

Quoted 7 lines

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...

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

Kind regards,

Remy

Quoted 29 lines

---
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
===================================================================

drivers/usb/host/ehci-hcd.c +1 -1
@@ -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;

#2 lore raw

PATCH 1/1 EHCI: zero out QH transfer overlay in ehci_submit_async()

Hello.

Quoted 14 lines · Remy Bohmer

2010/6/25 Sergei Shtylyov <[email protected]>:

Quoted 12 lines

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.

Quoted 8 lines
Quoted 4 lines

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

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.

Quoted 23 lines

Kind regards,

Remy

Quoted 19 lines

---
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
===================================================================

drivers/usb/host/ehci-hcd.c +1 -1
@@ -275,7 +275,7 @@ static void *ehci_alloc(size_t sz, size_
               return NULL;
       }
 
-       memset(p, sz, 0);
+       memset(p, 0, sz);
       return p;
 }
 
Quoted 5 lines

drivers/usb/host/ehci-hcd.c ehci_submit_async(struct usb_device *dev · line 352

Quoted 5 lines
drivers/usb/host/ehci-hcd.c +0 -1
@@ -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