[PATCH 0/3] Additional USB fixes

[Kevin O'Connor]

This series ports Paolo's uhci fixes to the ehci code.  Unfortunately,
I'm still seeing an occasional hang with my flash drives on my e350m1
(ehci controller), so there's still some other bug lurking in the ehci
code.

I also noticed an error in the usb-uhci (missing GET_FLATPTR) which is
also in this series.

Kevin O'Connor (3):
  usb-uhci: Be sure to wrap pipe->iobase in GET_FLATPTR().
  usb-ehci: Convert ehci_wait_qh to ehci_wait_pipe.
  usb-ehci: Fix race with host controller.

src/usb-ehci.c |   70 ++++++++++++++++++++++++-------------------------------
 src/usb-uhci.c |    7 +++--
 2 files changed, 35 insertions(+), 42 deletions(-)

[PATCH 1/3] usb-uhci: Be sure to wrap pipe->iobase in GET_FLATPTR().

[Kevin O'Connor]

Signed-off-by: Kevin O'Connor 
---
 src/usb-uhci.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/usb-uhci.c b/src/usb-uhci.c
index 242f3ba..a78dbca 100644
--- a/src/usb-uhci.c
+++ b/src/usb-uhci.c
@@ -248,13 +248,14 @@ wait_pipe(struct uhci_pipe *pipe, int timeout)
             return 0;
         if (check_tsc(end)) {
             warn_timeout();
+            u16 iobase = GET_FLATPTR(pipe->iobase);
             struct uhci_td *td = (void*)(el_link & ~UHCI_PTR_BITS);
             dprintf(1, "Timeout on wait_pipe %p (td=%p s=%x c=%x/%x)\n"
                     , pipe, (void*)el_link, GET_FLATPTR(td->status)
-                    , inw(pipe->iobase + USBCMD)
-                    , inw(pipe->iobase + USBSTS));
+                    , inw(iobase + USBCMD)
+                    , inw(iobase + USBSTS));
             SET_FLATPTR(pipe->qh.element, UHCI_PTR_TERM);
-            uhci_waittick(pipe->iobase);
+            uhci_waittick(iobase);
             return -1;
         }
         yield();

[PATCH 2/3] usb-ehci: Convert ehci_wait_qh to ehci_wait_pipe.

[Kevin O'Connor]

Convert ehci_wait_qh to ehci_wait_pipe to mirror the similar change
made to the uhci code.

Signed-off-by: Kevin O'Connor 
---
 src/usb-ehci.c |   61 +++++++++++++++++++++++++++----------------------------
 1 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/src/usb-ehci.c b/src/usb-ehci.c
index a60c607..8ce68ba 100644
--- a/src/usb-ehci.c
+++ b/src/usb-ehci.c
@@ -303,22 +303,12 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
  * End point communication
  ****************************************************************/

-static int
-ehci_wait_qh(struct usb_ehci_s *cntl, struct ehci_qh *qh)
-{
-    // XXX - 500ms just a guess
-    u64 end = calc_future_tsc(500);
-    for (;;) {
-        if (qh->qtd_next & EHCI_PTR_TERM)
-            // XXX - confirm
-            return 0;
-        if (check_tsc(end)) {
-            warn_timeout();
-            return -1;
-        }
-        yield();
-    }
-}
+struct ehci_pipe {
+    struct ehci_qh qh;
+    struct ehci_qtd *next_td, *tds;
+    void *data;
+    struct usb_pipe pipe;
+};

// Wait for next USB async frame to start - for ensuring safe memory release.
 static void
@@ -362,12 +352,29 @@ ehci_waittick(struct usb_ehci_s *cntl)
     writel(&cntl->regs->usbsts, STS_IAA);
 }

-struct ehci_pipe {
-    struct ehci_qh qh;
-    struct ehci_qtd *next_td, *tds;
-    void *data;
-    struct usb_pipe pipe;
-};
+static int
+ehci_wait_pipe(struct ehci_pipe *pipe, int timeout)
+{
+    u64 end = calc_future_tsc(timeout);
+    for (;;) {
+        u32 el_link = GET_FLATPTR(pipe->qh.qtd_next);
+        if (el_link & EHCI_PTR_TERM)
+            // XXX - confirm
+            return 0;
+        if (check_tsc(end)) {
+            warn_timeout();
+            SET_FLATPTR(pipe->qh.token, QTD_STS_HALT);
+            SET_FLATPTR(pipe->qh.qtd_next, EHCI_PTR_TERM);
+            SET_FLATPTR(pipe->qh.alt_next, EHCI_PTR_TERM);
+            // XXX - halt qh?
+            struct usb_ehci_s *cntl = container_of(
+                GET_FLATPTR(pipe->pipe.cntl), struct usb_ehci_s, usb);
+            ehci_waittick(cntl);
+            return -1;
+        }
+        yield();
+    }
+}

void
 ehci_free_pipe(struct usb_pipe *p)
@@ -462,8 +469,6 @@ ehci_control(struct usb_pipe *p, int dir, const void *cmd, int cmdsize
         return -1;
     }
     struct ehci_pipe *pipe = container_of(p, struct ehci_pipe, pipe);
-    struct usb_ehci_s *cntl = container_of(
-        pipe->pipe.cntl, struct usb_ehci_s, usb);

u16 maxpacket = pipe->pipe.maxpacket;
     int speed = pipe->pipe.speed;
@@ -515,13 +520,7 @@ ehci_control(struct usb_pipe *p, int dir, const void *cmd, int cmdsize
     pipe->qh.qtd_next = (u32)tds;
     barrier();
     pipe->qh.token = 0;
-    int ret = ehci_wait_qh(cntl, &pipe->qh);
-    pipe->qh.token = QTD_STS_HALT;
-    if (ret) {
-        pipe->qh.qtd_next = pipe->qh.alt_next = EHCI_PTR_TERM;
-        // XXX - halt qh?
-        ehci_waittick(cntl);
-    }
+    int ret = ehci_wait_pipe(pipe, 500);
     free(tds);
     return ret;
 }

[PATCH 3/3] usb-ehci: Fix race with host controller.

[Kevin O'Connor]

Port UHCI fix to uhci_send_bulk to EHCI code.

Signed-off-by: Kevin O'Connor 
---
 src/usb-ehci.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/src/usb-ehci.c b/src/usb-ehci.c
index 8ce68ba..8d5ebdf 100644
--- a/src/usb-ehci.c
+++ b/src/usb-ehci.c
@@ -629,15 +629,8 @@ ehci_send_bulk(struct usb_pipe *p, int dir, void *data, int datasize)
         data += transfer;
         datasize -= transfer;
     }
-    int i;
-    for (i=0; i<STACKQTDS; i++) {
-        struct ehci_qtd *td = &tds[tdpos++ % STACKQTDS];
-        int ret = ehci_wait_td(td);
-        if (ret)
-            goto fail;
-    }

-    return 0;
+    return ehci_wait_pipe(pipe, 5000);
 fail:
     dprintf(1, "ehci_send_bulk failed\n");
     SET_FLATPTR(pipe->qh.qtd_next, EHCI_PTR_TERM);

[Paolo Bonzini]

On 11/18/2011 04:22 AM, Kevin O'Connor wrote:
> This series ports Paolo's uhci fixes to the ehci code.  Unfortunately,
> I'm still seeing an occasional hang with my flash drives on my e350m1
> (ehci controller), so there's still some other bug lurking in the ehci
> code.
>
> I also noticed an error in the usb-uhci (missing GET_FLATPTR) which is
> also in this series.
>
> Kevin O'Connor (3):
>    usb-uhci: Be sure to wrap pipe->iobase in GET_FLATPTR().
>    usb-ehci: Convert ehci_wait_qh to ehci_wait_pipe.
>    usb-ehci: Fix race with host controller.
>
> src/usb-ehci.c |   70 ++++++++++++++++++++++++-------------------------------
>   src/usb-uhci.c |    7 +++--
>   2 files changed, 35 insertions(+), 42 deletions(-)

I tested this with QEMU and it seems to work there.

Reviewed-by: Paolo Bonzini

Paolo

[Kevin O'Connor]

On Thu, Nov 17, 2011 at 10:22:58PM -0500, Kevin O'Connor wrote:
> This series ports Paolo's uhci fixes to the ehci code.  Unfortunately,
> I'm still seeing an occasional hang with my flash drives on my e350m1
> (ehci controller), so there's still some other bug lurking in the ehci
> code.

I think I found the issue on EHCI.  The EHCI controller actually has
the opposite requirement from UHCI - the TD is updated before the QH.
So, I've created a new patch which always checks the TDs to verify
transfer completion.  (Control transfers used to just check the QH,
and it actually only checked that the last TD was in progress.)

This has been very stable on my e350m1 with several different USB
drives.  I have seen a few timeouts, but they're so sporadic now that
I haven't been able to track them down.  (The e350m1 also has a known
usb init bug which could be causing the remaining failures.)

So, please ignore the previous patch 2/3 and use the patch below.
(I've committed the unrelated patch 1 of the previous series.)

-Kevin