[Qemu-devel] [PATCH 00/10] ehci: various fixes

[Qemu-devel] [PATCH 00/10] ehci: various fixes

[Hans de Goede]

While working on moving usb-redir and usb-hid over to using async packet
handling for their interrupt input endpoints. I've found and fixed quite
a few ehci bugs.

Unfortunately the moving to async for interrupt endpoints turns out to be a
bad idea, as it causes issues for migration, an async completed packet will
not getting written back to guest memory until the next poll time, and if a
migration happens in between it will get lost!

So now I'm working on making all qemu usb-devices call wakeup when they
have interrupt data ready (something which is needed for xhci anyways),
and then we can still slowdown the frame timer when there are no isoc packets
in the periodic schedule, using the wakeup as a notifier to run the frame-timer
earlier.

While I'm working on this, the ehci fixes can already go upstream, as they
are ready and have been extensively tested.

Regards,

Hans

[Qemu-devel] [PATCH 01/10] ehci: Don't access packet after freeing it

[Hans de Goede]

ehci_state_writeback() will free the packet, so we should not access
the packet after calling ehci_state_writeback().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index ee6c9ae..a8b1a40 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -453,12 +453,13 @@ static EHCIPacket *ehci_alloc_packet(EHCIQueue *q)
 static void ehci_free_packet(EHCIPacket *p)
 {
     if (p->async == EHCI_ASYNC_FINISHED) {
-        int state = ehci_get_state(p->queue->ehci, p->queue->async);
+        EHCIQueue *q = p->queue;
+        int state = ehci_get_state(q->ehci, q->async);
         /* This is a normal, but rare condition (cancel racing completion) */
         fprintf(stderr, "EHCI: Warning packet completed but not processed\n");
-        ehci_state_executing(p->queue);
-        ehci_state_writeback(p->queue);
-        ehci_set_state(p->queue->ehci, p->queue->async, state);
+        ehci_state_executing(q);
+        ehci_state_writeback(q);
+        ehci_set_state(q->ehci, q->async, state);
         /* state_writeback recurses into us with async == EHCI_ASYNC_NONE!! */
         return;
     }
-- 
1.7.12.1

[Qemu-devel] [PATCH 02/10] ehci: Fixup q->qtdaddr after cancelling an already completed packet

[Hans de Goede]

This avoids the q->qtdaddr == p->qtdaddr asserts we have triggering, when
a queue contains multiple completed packages when we cancel the queue.

I triggered this with windows7 + async interrupt endpoint handling (*)
+ not detecting circles in ehci_fill_queue() properly, which makes the qtd
validation in ehci_fill_queue fail, causing cancellation of the queue on every
mouse event ...

*) Which is not going upstream as it will cause loss of interrupt events on
migration.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index a8b1a40..5e3b4a8 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -189,6 +189,7 @@ static const char *ehci_mmio_names[] = {
 
 static int ehci_state_executing(EHCIQueue *q);
 static int ehci_state_writeback(EHCIQueue *q);
+static int ehci_state_advqueue(EHCIQueue *q);
 static int ehci_fill_queue(EHCIPacket *p);
 
 static const char *nr2str(const char **n, size_t len, uint32_t nr)
@@ -459,6 +460,9 @@ static void ehci_free_packet(EHCIPacket *p)
         fprintf(stderr, "EHCI: Warning packet completed but not processed\n");
         ehci_state_executing(q);
         ehci_state_writeback(q);
+        if (!(q->qh.token & QTD_TOKEN_HALT)) {
+            ehci_state_advqueue(q);
+        }
         ehci_set_state(q->ehci, q->async, state);
         /* state_writeback recurses into us with async == EHCI_ASYNC_NONE!! */
         return;
-- 
1.7.12.1

[Qemu-devel] [PATCH 03/10] ehci: Better detection for qtd-s linked in circles

[Hans de Goede]

Windows links interrupt qtd-s in circles, which means that when interrupt
endpoints return USB_RET_ASYNC, combined with the recent
"ehci: Retry to fill the queue while waiting for td completion" patch,
we keep adding the tds to the queue over and over again, as we detect the
circle from fill_queue, but we call it over and over again ...

This patch fixes this by changing the circle detection to also detect
circling into tds already queued up previously.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 5e3b4a8..89b7520 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1790,7 +1790,7 @@ static int ehci_fill_queue(EHCIPacket *p)
     USBEndpoint *ep = p->packet.ep;
     EHCIQueue *q = p->queue;
     EHCIqtd qtd = p->qtd;
-    uint32_t qtdaddr, start_addr = p->qtdaddr;
+    uint32_t qtdaddr;
 
     for (;;) {
         if (NLPTR_TBIT(qtd.next) != 0) {
@@ -1801,8 +1801,10 @@ static int ehci_fill_queue(EHCIPacket *p)
          * Detect circular td lists, Windows creates these, counting on the
          * active bit going low after execution to make the queue stop.
          */
-        if (qtdaddr == start_addr) {
-            break;
+        QTAILQ_FOREACH(p, &q->packets, next) {
+            if (p->qtdaddr == qtdaddr) {
+                goto leave;
+            }
         }
         get_dwords(q->ehci, NLPTR_GET(qtdaddr),
                    (uint32_t *) &qtd, sizeof(EHCIqtd) >> 2);
@@ -1819,6 +1821,7 @@ static int ehci_fill_queue(EHCIPacket *p)
         assert(p->packet.status == USB_RET_ASYNC);
         p->async = EHCI_ASYNC_INFLIGHT;
     }
+leave:
     usb_device_flush_ep_queue(ep->dev, ep);
     return 1;
 }
-- 
1.7.12.1

[Qemu-devel] [PATCH 04/10] ehci: Add a ehci_writeback_async_complete_packet helper function

[Hans de Goede]

Also drop the warning printf, which was there mainly because this was an
untested code path (as the previous bug fixes to it show), but that no
longer is the case now :)

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 89b7520..7772c33 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -437,6 +437,22 @@ static inline bool ehci_periodic_enabled(EHCIState *s)
     return ehci_enabled(s) && (s->usbcmd & USBCMD_PSE);
 }
 
+/* Finish executing and writeback a packet outside of the regular
+   fetchqh -> fetchqtd -> execute -> writeback cycle */
+static void ehci_writeback_async_complete_packet(EHCIPacket *p)
+{
+    EHCIQueue *q = p->queue;
+    int state;
+
+    state = ehci_get_state(q->ehci, q->async);
+    ehci_state_executing(q);
+    ehci_state_writeback(q); /* Frees the packet! */
+    if (!(q->qh.token & QTD_TOKEN_HALT)) {
+        ehci_state_advqueue(q);
+    }
+    ehci_set_state(q->ehci, q->async, state);
+}
+
 /* packet management */
 
 static EHCIPacket *ehci_alloc_packet(EHCIQueue *q)
@@ -454,17 +470,7 @@ static EHCIPacket *ehci_alloc_packet(EHCIQueue *q)
 static void ehci_free_packet(EHCIPacket *p)
 {
     if (p->async == EHCI_ASYNC_FINISHED) {
-        EHCIQueue *q = p->queue;
-        int state = ehci_get_state(q->ehci, q->async);
-        /* This is a normal, but rare condition (cancel racing completion) */
-        fprintf(stderr, "EHCI: Warning packet completed but not processed\n");
-        ehci_state_executing(q);
-        ehci_state_writeback(q);
-        if (!(q->qh.token & QTD_TOKEN_HALT)) {
-            ehci_state_advqueue(q);
-        }
-        ehci_set_state(q->ehci, q->async, state);
-        /* state_writeback recurses into us with async == EHCI_ASYNC_NONE!! */
+        ehci_writeback_async_complete_packet(p);
         return;
     }
     trace_usb_ehci_packet_action(p->queue, p, "free");
-- 
1.7.12.1

[Qemu-devel] [PATCH 05/10] ehci: Add ehci_verify_qh and ehci_verify_qtd helper functions

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 7772c33..a694346 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -437,6 +437,33 @@ static inline bool ehci_periodic_enabled(EHCIState *s)
     return ehci_enabled(s) && (s->usbcmd & USBCMD_PSE);
 }
 
+static bool ehci_verify_qh(EHCIQueue *q, EHCIqh *qh)
+{
+    uint32_t devaddr = get_field(qh->epchar, QH_EPCHAR_DEVADDR);
+    uint32_t endp    = get_field(qh->epchar, QH_EPCHAR_EP);
+    if ((devaddr != get_field(q->qh.epchar, QH_EPCHAR_DEVADDR)) ||
+        (endp    != get_field(q->qh.epchar, QH_EPCHAR_EP)) ||
+        (memcmp(&qh->current_qtd, &q->qh.current_qtd,
+                                 9 * sizeof(uint32_t)) != 0) ||
+        (q->dev != NULL && q->dev->addr != devaddr)) {
+        return false;
+    } else {
+        return true;
+    }
+}
+
+static bool ehci_verify_qtd(EHCIPacket *p, EHCIqtd *qtd)
+{
+    if (p->qtdaddr != p->queue->qtdaddr ||
+        (!NLPTR_TBIT(p->qtd.next) && (p->qtd.next != qtd->next)) ||
+        (!NLPTR_TBIT(p->qtd.altnext) && (p->qtd.altnext != qtd->altnext)) ||
+        p->qtd.bufptr[0] != qtd->bufptr[0]) {
+        return false;
+    } else {
+        return true;
+    }
+}
+
 /* Finish executing and writeback a packet outside of the regular
    fetchqh -> fetchqtd -> execute -> writeback cycle */
 static void ehci_writeback_async_complete_packet(EHCIPacket *p)
@@ -1524,8 +1551,8 @@ out:
 
 static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
 {
+    uint32_t entry;
     EHCIPacket *p;
-    uint32_t entry, devaddr, endp;
     EHCIQueue *q;
     EHCIqh qh;
 
@@ -1552,13 +1579,7 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
      * The overlay area of the qh should never be changed by the guest,
      * except when idle, in which case the reset is a nop.
      */
-    devaddr = get_field(qh.epchar, QH_EPCHAR_DEVADDR);
-    endp    = get_field(qh.epchar, QH_EPCHAR_EP);
-    if ((devaddr != get_field(q->qh.epchar, QH_EPCHAR_DEVADDR)) ||
-        (endp    != get_field(q->qh.epchar, QH_EPCHAR_EP)) ||
-        (memcmp(&qh.current_qtd, &q->qh.current_qtd,
-                                 9 * sizeof(uint32_t)) != 0) ||
-        (q->dev != NULL && q->dev->addr != devaddr)) {
+    if (!ehci_verify_qh(q, &qh)) {
         if (ehci_reset_queue(q) > 0) {
             ehci_trace_guest_bug(ehci, "guest updated active QH");
         }
@@ -1572,7 +1593,8 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     }
 
     if (q->dev == NULL) {
-        q->dev = ehci_find_device(q->ehci, devaddr);
+        q->dev = ehci_find_device(q->ehci,
+                                  get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
     }
 
     if (p && p->async == EHCI_ASYNC_FINISHED) {
@@ -1724,10 +1746,7 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
 
     p = QTAILQ_FIRST(&q->packets);
     if (p != NULL) {
-        if (p->qtdaddr != q->qtdaddr ||
-            (!NLPTR_TBIT(p->qtd.next) && (p->qtd.next != qtd.next)) ||
-            (!NLPTR_TBIT(p->qtd.altnext) && (p->qtd.altnext != qtd.altnext)) ||
-            p->qtd.bufptr[0] != qtd.bufptr[0]) {
+        if (!ehci_verify_qtd(p, &qtd)) {
             ehci_cancel_queue(q);
             ehci_trace_guest_bug(q->ehci, "guest updated active QH or qTD");
             p = NULL;
-- 
1.7.12.1

[Qemu-devel] [PATCH 06/10] ehci: Verify guest does not change the token of inflight qtd-s

[Hans de Goede]

This is not allowed, except for clearing active on cancellation, so don't
warn when the new token does not have its active bit set.

This unifies the cancellation path for modified qtd-s, and prepares
ehci_verify_qtd to be used ad an extra check inside
ehci_writeback_async_complete_packet().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index a694346..e565d6a 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -457,6 +457,7 @@ static bool ehci_verify_qtd(EHCIPacket *p, EHCIqtd *qtd)
     if (p->qtdaddr != p->queue->qtdaddr ||
         (!NLPTR_TBIT(p->qtd.next) && (p->qtd.next != qtd->next)) ||
         (!NLPTR_TBIT(p->qtd.altnext) && (p->qtd.altnext != qtd->altnext)) ||
+        p->qtd.token != qtd->token ||
         p->qtd.bufptr[0] != qtd->bufptr[0]) {
         return false;
     } else {
@@ -1748,7 +1749,9 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
     if (p != NULL) {
         if (!ehci_verify_qtd(p, &qtd)) {
             ehci_cancel_queue(q);
-            ehci_trace_guest_bug(q->ehci, "guest updated active QH or qTD");
+            if (qtd.token & QTD_TOKEN_ACTIVE) {
+                ehci_trace_guest_bug(q->ehci, "guest updated active qTD");
+            }
             p = NULL;
         } else {
             p->qtd = qtd;
@@ -1757,11 +1760,6 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
     }
 
     if (!(qtd.token & QTD_TOKEN_ACTIVE)) {
-        if (p != NULL) {
-            /* transfer canceled by guest (clear active) */
-            ehci_cancel_queue(q);
-            p = NULL;
-        }
         ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
     } else if (p != NULL) {
         switch (p->async) {
-- 
1.7.12.1

[Qemu-devel] [PATCH 07/10] ehci: Don't verify the next pointer for periodic qh-s

[Hans de Goede]

Windows-XP likes to play tricks with the next pointer for periodic qh-s, so
far we were not hit by this as we never called fill_queue for periodic qh-s,
but with the move to async packet handling for interrupt endpoints this
becomes an issue.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index e565d6a..e83bdde 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -443,8 +443,10 @@ static bool ehci_verify_qh(EHCIQueue *q, EHCIqh *qh)
     uint32_t endp    = get_field(qh->epchar, QH_EPCHAR_EP);
     if ((devaddr != get_field(q->qh.epchar, QH_EPCHAR_DEVADDR)) ||
         (endp    != get_field(q->qh.epchar, QH_EPCHAR_EP)) ||
-        (memcmp(&qh->current_qtd, &q->qh.current_qtd,
-                                 9 * sizeof(uint32_t)) != 0) ||
+        (qh->current_qtd != q->qh.current_qtd) ||
+        (q->async && qh->next_qtd != q->qh.next_qtd) ||
+        (memcmp(&qh->altnext_qtd, &q->qh.altnext_qtd,
+                                 7 * sizeof(uint32_t)) != 0) ||
         (q->dev != NULL && q->dev->addr != devaddr)) {
         return false;
     } else {
@@ -455,7 +457,8 @@ static bool ehci_verify_qh(EHCIQueue *q, EHCIqh *qh)
 static bool ehci_verify_qtd(EHCIPacket *p, EHCIqtd *qtd)
 {
     if (p->qtdaddr != p->queue->qtdaddr ||
-        (!NLPTR_TBIT(p->qtd.next) && (p->qtd.next != qtd->next)) ||
+        (p->queue->async && !NLPTR_TBIT(p->qtd.next) &&
+            (p->qtd.next != qtd->next)) ||
         (!NLPTR_TBIT(p->qtd.altnext) && (p->qtd.altnext != qtd->altnext)) ||
         p->qtd.token != qtd->token ||
         p->qtd.bufptr[0] != qtd->bufptr[0]) {
-- 
1.7.12.1

[Qemu-devel] [PATCH 08/10] ehci: Move get / put_dwords upwards

[Hans de Goede]

No other changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 60 +++++++++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index e83bdde..13e9fd3 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -437,6 +437,36 @@ static inline bool ehci_periodic_enabled(EHCIState *s)
     return ehci_enabled(s) && (s->usbcmd & USBCMD_PSE);
 }
 
+/* TODO : Put in common header file, duplication from usb-ohci.c */
+
+/* Get an array of dwords from main memory */
+static inline int get_dwords(EHCIState *ehci, uint32_t addr,
+                             uint32_t *buf, int num)
+{
+    int i;
+
+    for(i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
+        dma_memory_read(ehci->dma, addr, buf, sizeof(*buf));
+        *buf = le32_to_cpu(*buf);
+    }
+
+    return 1;
+}
+
+/* Put an array of dwords in to main memory */
+static inline int put_dwords(EHCIState *ehci, uint32_t addr,
+                             uint32_t *buf, int num)
+{
+    int i;
+
+    for(i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
+        uint32_t tmp = cpu_to_le32(*buf);
+        dma_memory_write(ehci->dma, addr, &tmp, sizeof(tmp));
+    }
+
+    return 1;
+}
+
 static bool ehci_verify_qh(EHCIQueue *q, EHCIqh *qh)
 {
     uint32_t devaddr = get_field(qh->epchar, QH_EPCHAR_DEVADDR);
@@ -1038,36 +1068,6 @@ static void ehci_opreg_write(void *ptr, hwaddr addr,
 }
 
 
-// TODO : Put in common header file, duplication from usb-ohci.c
-
-/* Get an array of dwords from main memory */
-static inline int get_dwords(EHCIState *ehci, uint32_t addr,
-                             uint32_t *buf, int num)
-{
-    int i;
-
-    for(i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
-        dma_memory_read(ehci->dma, addr, buf, sizeof(*buf));
-        *buf = le32_to_cpu(*buf);
-    }
-
-    return 1;
-}
-
-/* Put an array of dwords in to main memory */
-static inline int put_dwords(EHCIState *ehci, uint32_t addr,
-                             uint32_t *buf, int num)
-{
-    int i;
-
-    for(i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
-        uint32_t tmp = cpu_to_le32(*buf);
-        dma_memory_write(ehci->dma, addr, &tmp, sizeof(tmp));
-    }
-
-    return 1;
-}
-
 /*
  *  Write the qh back to guest physical memory.  This step isn't
  *  in the EHCI spec but we need to do it since we don't share
-- 
1.7.12.1

[Qemu-devel] [PATCH 09/10] ehci: writeback_async_complete_packet: verify qh and qtd

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 13e9fd3..6d23af1 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -191,6 +191,7 @@ static int ehci_state_executing(EHCIQueue *q);
 static int ehci_state_writeback(EHCIQueue *q);
 static int ehci_state_advqueue(EHCIQueue *q);
 static int ehci_fill_queue(EHCIPacket *p);
+static void ehci_free_packet(EHCIPacket *p);
 
 static const char *nr2str(const char **n, size_t len, uint32_t nr)
 {
@@ -503,8 +504,21 @@ static bool ehci_verify_qtd(EHCIPacket *p, EHCIqtd *qtd)
 static void ehci_writeback_async_complete_packet(EHCIPacket *p)
 {
     EHCIQueue *q = p->queue;
+    EHCIqtd qtd;
+    EHCIqh qh;
     int state;
 
+    /* Verify the qh + qtd, like we do when going through fetchqh & fetchqtd */
+    get_dwords(q->ehci, NLPTR_GET(q->qhaddr),
+               (uint32_t *) &qh, sizeof(EHCIqh) >> 2);
+    get_dwords(q->ehci, NLPTR_GET(q->qtdaddr),
+               (uint32_t *) &qtd, sizeof(EHCIqtd) >> 2);
+    if (!ehci_verify_qh(q, &qh) || !ehci_verify_qtd(p, &qtd)) {
+        p->async = EHCI_ASYNC_INITIALIZED;
+        ehci_free_packet(p);
+        return;
+    }
+
     state = ehci_get_state(q->ehci, q->async);
     ehci_state_executing(q);
     ehci_state_writeback(q); /* Frees the packet! */
-- 
1.7.12.1

[Qemu-devel] [PATCH 10/10] ehci: Verify qtd for async completed packets

[Hans de Goede]

Remove the short-circuiting of fetchqtd in fetchqh, so that the
qtd gets properly verified before completing the transaction.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 6d23af1..da413fb 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1570,7 +1570,6 @@ out:
 static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
 {
     uint32_t entry;
-    EHCIPacket *p;
     EHCIQueue *q;
     EHCIqh qh;
 
@@ -1579,7 +1578,6 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     if (NULL == q) {
         q = ehci_alloc_queue(ehci, entry, async);
     }
-    p = QTAILQ_FIRST(&q->packets);
 
     q->seen++;
     if (q->seen > 1) {
@@ -1601,7 +1599,6 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
         if (ehci_reset_queue(q) > 0) {
             ehci_trace_guest_bug(ehci, "guest updated active QH");
         }
-        p = NULL;
     }
     q->qh = qh;
 
@@ -1615,13 +1612,6 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
                                   get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
     }
 
-    if (p && p->async == EHCI_ASYNC_FINISHED) {
-        /* I/O finished -- continue processing queue */
-        trace_usb_ehci_packet_action(p->queue, p, "complete");
-        ehci_set_state(ehci, async, EST_EXECUTING);
-        goto out;
-    }
-
     if (async && (q->qh.epchar & QH_EPCHAR_H)) {
 
         /*  EHCI spec version 1.0 Section 4.8.3 & 4.10.1 */
@@ -1792,10 +1782,7 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
             ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
             break;
         case EHCI_ASYNC_FINISHED:
-            /*
-             * We get here when advqueue moves to a packet which is already
-             * finished, which can happen with packets queued up by fill_queue
-             */
+            /* Complete executing of the packet */
             ehci_set_state(q->ehci, q->async, EST_EXECUTING);
             break;
         }
-- 
1.7.12.1

Re: [Qemu-devel] [PATCH 00/10] ehci: various fixes

[Gerd Hoffmann]

On 11/14/12 17:21, Hans de Goede wrote:
> While working on moving usb-redir and usb-hid over to using async packet
> handling for their interrupt input endpoints. I've found and fixed quite
> a few ehci bugs.
> 
> Unfortunately the moving to async for interrupt endpoints turns out to be a
> bad idea, as it causes issues for migration, an async completed packet will
> not getting written back to guest memory until the next poll time, and if a
> migration happens in between it will get lost!

Ok, so it looks like 1-3 are clear bugfixes which are needed in 1.3

I'd tend to schedule the other ones for the 1.4 devel cycle.  They are
clearly nice cleanups, but as we are not going the async-for-intr route
I think they are not important enougth to push them post-freeze.

Patch 8 doesn't pass checkpatch (yes, it just shuffles around existing
code, but it's still nice to fix style issues while touching those bits).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 00/10] ehci: various fixes

[Hans de Goede]

Hi,

On 11/15/2012 09:55 AM, Gerd Hoffmann wrote:
> On 11/14/12 17:21, Hans de Goede wrote:
>> While working on moving usb-redir and usb-hid over to using async packet
>> handling for their interrupt input endpoints. I've found and fixed quite
>> a few ehci bugs.
>>
>> Unfortunately the moving to async for interrupt endpoints turns out to be a
>> bad idea, as it causes issues for migration, an async completed packet will
>> not getting written back to guest memory until the next poll time, and if a
>> migration happens in between it will get lost!
>
> Ok, so it looks like 1-3 are clear bugfixes which are needed in 1.3
>

Ack, I think that "ehci: Don't verify the next pointer for periodic qh-s"
belongs in 1.3 too, since this could be hit with host-redirection which
does use async completion for interrupt endpoints.

Moving it forward in the patch-set will cause a conflict though, I'll go and
do the reshuffle myself, throw in 2 usb-redir fixes and then send a new set
targetting 1.3. Do you also want a separate set on top for 1.4 / usb-next, or
do you want me to keep the rest in my own tree for now ?

> I'd tend to schedule the other ones for the 1.4 devel cycle.  They are
> clearly nice cleanups, but as we are not going the async-for-intr route
> I think they are not important enougth to push them post-freeze.

Ok.

> Patch 8 doesn't pass checkpatch (yes, it just shuffles around existing
> code, but it's still nice to fix style issues while touching those bits).

Agreed, will fix.

Regards,

Hans

Re: [Qemu-devel] [PATCH 00/10] ehci: various fixes

[Gerd Hoffmann]

On 11/15/12 12:58, Hans de Goede wrote:
> Hi,
> 
> On 11/15/2012 09:55 AM, Gerd Hoffmann wrote:
>> On 11/14/12 17:21, Hans de Goede wrote:
>>> While working on moving usb-redir and usb-hid over to using async packet
>>> handling for their interrupt input endpoints. I've found and fixed quite
>>> a few ehci bugs.
>>>
>>> Unfortunately the moving to async for interrupt endpoints turns out
>>> to be a
>>> bad idea, as it causes issues for migration, an async completed
>>> packet will
>>> not getting written back to guest memory until the next poll time,
>>> and if a
>>> migration happens in between it will get lost!
>>
>> Ok, so it looks like 1-3 are clear bugfixes which are needed in 1.3
>>
> 
> Ack, I think that "ehci: Don't verify the next pointer for periodic qh-s"
> belongs in 1.3 too, since this could be hit with host-redirection which
> does use async completion for interrupt endpoints.
> 
> Moving it forward in the patch-set will cause a conflict though, I'll go
> and
> do the reshuffle myself, throw in 2 usb-redir fixes and then send a new set
> targetting 1.3.

Thanks.

> Do you also want a separate set on top for 1.4 /
> usb-next, or
> do you want me to keep the rest in my own tree for now ?

Keep it in your tree for now.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 00/10] ehci: various fixes

[Gerd Hoffmann]

  Hi,

>> Ok, so it looks like 1-3 are clear bugfixes which are needed in 1.3
>>

> Moving it forward in the patch-set will cause a conflict though, I'll go
> and
> do the reshuffle myself, throw in 2 usb-redir fixes and then send a new set

Oh, and I've picked 1-3 already, no need to resend them.

cheers,
  Gerd