[Qemu-devel] usb patches [v2]

[Qemu-devel] usb patches [v2]

[Hans de Goede]

Hi,

Here is a series of usb patches against current qemu master, mostly
consisting of a whole bunch of small ehci fixes, plus some redirection
fixes.

Changes in v2:
Due to some reshuffling of patches before sending [PATCH 04/13] "usb-ehci:
always call ehci_queues_rip_unused for period queues" ended up passing in the
flush flag of ehci_queues_rip_unused, which does not get added to it until
the next patch. This version fixes this so that all intermediate steps
compile.

Regards,

Hans

[Qemu-devel] [PATCH 01/13] usb-redir: Set ep type and interface

[Hans de Goede]

Since we don't use usb_desc.c we need to do this ourselves. This fixes
iso transfers no longer working for USB 2 devices due to the ep->type
check in ehci.c

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index fc58a0e..ef9fa96 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -1149,6 +1149,7 @@ static void usbredir_device_disconnect(void *priv)
     for (i = 0; i < MAX_ENDPOINTS; i++) {
         QTAILQ_INIT(&dev->endpoint[i].bufpq);
     }
+    usb_ep_init(&dev->dev);
     dev->interface_info.interface_count = 0;
 }
 
@@ -1175,6 +1176,7 @@ static void usbredir_ep_info(void *priv,
     struct usb_redir_ep_info_header *ep_info)
 {
     USBRedirDevice *dev = priv;
+    struct USBEndpoint *usb_ep;
     int i;
 
     for (i = 0; i < MAX_ENDPOINTS; i++) {
@@ -1199,7 +1201,13 @@ static void usbredir_ep_info(void *priv,
         default:
             ERROR("Received invalid endpoint type\n");
             usbredir_device_disconnect(dev);
+            return;
         }
+        usb_ep = usb_ep_get(&dev->dev,
+                            (i & 0x10) ? USB_TOKEN_IN : USB_TOKEN_OUT,
+                            i & 0x0f);
+        usb_ep->type = dev->endpoint[i].type;
+        usb_ep->ifnum = dev->endpoint[i].interface;
     }
 }
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 02/13] usb-ehci: Never follow table entries with the T-bit set

[Hans de Goede]

Before this patch the T-bit was not checked in 2 places, while it should be.

Once we properly check the T-bit everywhere we no longer need the weird
entry < 0x1000 and entry > 0x1000 checks, so this patch removes them.

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index afc8ccf..d41b80e 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -1568,8 +1568,7 @@ static int ehci_state_fetchentry(EHCIState *ehci, int async)
     int again = 0;
     uint32_t entry = ehci_get_fetch_addr(ehci, async);
 
-    if (entry < 0x1000) {
-        DPRINTF("fetchentry: entry invalid (0x%08x)\n", entry);
+    if (NLPTR_TBIT(entry)) {
         ehci_set_state(ehci, async, EST_ACTIVE);
         goto out;
     }
@@ -1677,7 +1676,8 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     if (q->qh.token & QTD_TOKEN_HALT) {
         ehci_set_state(ehci, async, EST_HORIZONTALQH);
 
-    } else if ((q->qh.token & QTD_TOKEN_ACTIVE) && (q->qh.current_qtd > 0x1000)) {
+    } else if ((q->qh.token & QTD_TOKEN_ACTIVE) &&
+               (NLPTR_TBIT(q->qh.current_qtd) == 0)) {
         q->qtdaddr = q->qh.current_qtd;
         ehci_set_state(ehci, async, EST_FETCHQTD);
 
@@ -1756,7 +1756,6 @@ static int ehci_state_advqueue(EHCIQueue *q, int async)
      * want data and alt-next qTD is valid
      */
     if (((q->qh.token & QTD_TOKEN_TBYTES_MASK) != 0) &&
-        (q->qh.altnext_qtd > 0x1000) &&
         (NLPTR_TBIT(q->qh.altnext_qtd) == 0)) {
         q->qtdaddr = q->qh.altnext_qtd;
         ehci_set_state(q->ehci, async, EST_FETCHQTD);
@@ -1764,8 +1763,7 @@ static int ehci_state_advqueue(EHCIQueue *q, int async)
     /*
      *  next qTD is valid
      */
-    } else if ((q->qh.next_qtd > 0x1000) &&
-               (NLPTR_TBIT(q->qh.next_qtd) == 0)) {
+    } else if (NLPTR_TBIT(q->qh.next_qtd) == 0) {
         q->qtdaddr = q->qh.next_qtd;
         ehci_set_state(q->ehci, async, EST_FETCHQTD);
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 03/13] usb-ehci: split our qh queue into async and periodic queues

[Hans de Goede]

qhs can be part of both the async and the periodic schedule, as is shown
in later patches in this series it is useful to keep track of the qhs on
a per schedule basis.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-ehci.c |   62 ++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 37 insertions(+), 25 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index d41b80e..840022d 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -347,7 +347,6 @@ enum async_state {
 struct EHCIQueue {
     EHCIState *ehci;
     QTAILQ_ENTRY(EHCIQueue) next;
-    bool async_schedule;
     uint32_t seen;
     uint64_t ts;
 
@@ -367,6 +366,8 @@ struct EHCIQueue {
     int usb_status;
 };
 
+typedef QTAILQ_HEAD(EHCIQueueHead, EHCIQueue) EHCIQueueHead;
+
 struct EHCIState {
     PCIDevice dev;
     USBBus bus;
@@ -410,7 +411,8 @@ struct EHCIState {
     USBPort ports[NB_PORTS];
     USBPort *companion_ports[NB_PORTS];
     uint32_t usbsts_pending;
-    QTAILQ_HEAD(, EHCIQueue) queues;
+    EHCIQueueHead aqueues;
+    EHCIQueueHead pqueues;
 
     uint32_t a_fetch_addr;   // which address to look at next
     uint32_t p_fetch_addr;   // which address to look at next
@@ -660,31 +662,34 @@ static void ehci_trace_sitd(EHCIState *s, target_phys_addr_t addr,
 
 static EHCIQueue *ehci_alloc_queue(EHCIState *ehci, int async)
 {
+    EHCIQueueHead *head = async ? &ehci->aqueues : &ehci->pqueues;
     EHCIQueue *q;
 
     q = g_malloc0(sizeof(*q));
     q->ehci = ehci;
-    q->async_schedule = async;
-    QTAILQ_INSERT_HEAD(&ehci->queues, q, next);
+    QTAILQ_INSERT_HEAD(head, q, next);
     trace_usb_ehci_queue_action(q, "alloc");
     return q;
 }
 
-static void ehci_free_queue(EHCIQueue *q)
+static void ehci_free_queue(EHCIQueue *q, int async)
 {
+    EHCIQueueHead *head = async ? &q->ehci->aqueues : &q->ehci->pqueues;
     trace_usb_ehci_queue_action(q, "free");
     if (q->async == EHCI_ASYNC_INFLIGHT) {
         usb_cancel_packet(&q->packet);
     }
-    QTAILQ_REMOVE(&q->ehci->queues, q, next);
+    QTAILQ_REMOVE(head, q, next);
     g_free(q);
 }
 
-static EHCIQueue *ehci_find_queue_by_qh(EHCIState *ehci, uint32_t addr)
+static EHCIQueue *ehci_find_queue_by_qh(EHCIState *ehci, uint32_t addr,
+                                        int async)
 {
+    EHCIQueueHead *head = async ? &ehci->aqueues : &ehci->pqueues;
     EHCIQueue *q;
 
-    QTAILQ_FOREACH(q, &ehci->queues, next) {
+    QTAILQ_FOREACH(q, head, next) {
         if (addr == q->qhaddr) {
             return q;
         }
@@ -692,11 +697,12 @@ static EHCIQueue *ehci_find_queue_by_qh(EHCIState *ehci, uint32_t addr)
     return NULL;
 }
 
-static void ehci_queues_rip_unused(EHCIState *ehci)
+static void ehci_queues_rip_unused(EHCIState *ehci, int async)
 {
+    EHCIQueueHead *head = async ? &ehci->aqueues : &ehci->pqueues;
     EHCIQueue *q, *tmp;
 
-    QTAILQ_FOREACH_SAFE(q, &ehci->queues, next, tmp) {
+    QTAILQ_FOREACH_SAFE(q, head, next, tmp) {
         if (q->seen) {
             q->seen = 0;
             q->ts = ehci->last_run_ns;
@@ -706,29 +712,31 @@ static void ehci_queues_rip_unused(EHCIState *ehci)
             /* allow 0.25 sec idle */
             continue;
         }
-        ehci_free_queue(q);
+        ehci_free_queue(q, async);
     }
 }
 
-static void ehci_queues_rip_device(EHCIState *ehci, USBDevice *dev)
+static void ehci_queues_rip_device(EHCIState *ehci, USBDevice *dev, int async)
 {
+    EHCIQueueHead *head = async ? &ehci->aqueues : &ehci->pqueues;
     EHCIQueue *q, *tmp;
 
-    QTAILQ_FOREACH_SAFE(q, &ehci->queues, next, tmp) {
+    QTAILQ_FOREACH_SAFE(q, head, next, tmp) {
         if (!usb_packet_is_inflight(&q->packet) ||
             q->packet.ep->dev != dev) {
             continue;
         }
-        ehci_free_queue(q);
+        ehci_free_queue(q, async);
     }
 }
 
-static void ehci_queues_rip_all(EHCIState *ehci)
+static void ehci_queues_rip_all(EHCIState *ehci, int async)
 {
+    EHCIQueueHead *head = async ? &ehci->aqueues : &ehci->pqueues;
     EHCIQueue *q, *tmp;
 
-    QTAILQ_FOREACH_SAFE(q, &ehci->queues, next, tmp) {
-        ehci_free_queue(q);
+    QTAILQ_FOREACH_SAFE(q, head, next, tmp) {
+        ehci_free_queue(q, async);
     }
 }
 
@@ -773,7 +781,8 @@ static void ehci_detach(USBPort *port)
         return;
     }
 
-    ehci_queues_rip_device(s, port->dev);
+    ehci_queues_rip_device(s, port->dev, 0);
+    ehci_queues_rip_device(s, port->dev, 1);
 
     *portsc &= ~(PORTSC_CONNECT|PORTSC_PED);
     *portsc |= PORTSC_CSC;
@@ -793,7 +802,8 @@ static void ehci_child_detach(USBPort *port, USBDevice *child)
         return;
     }
 
-    ehci_queues_rip_device(s, child);
+    ehci_queues_rip_device(s, child, 0);
+    ehci_queues_rip_device(s, child, 1);
 }
 
 static void ehci_wakeup(USBPort *port)
@@ -911,7 +921,8 @@ static void ehci_reset(void *opaque)
             usb_device_reset(devs[i]);
         }
     }
-    ehci_queues_rip_all(s);
+    ehci_queues_rip_all(s, 0);
+    ehci_queues_rip_all(s, 1);
     qemu_del_timer(s->frame_timer);
 }
 
@@ -1526,7 +1537,7 @@ static int ehci_state_waitlisthead(EHCIState *ehci,  int async)
         ehci_set_usbsts(ehci, USBSTS_REC);
     }
 
-    ehci_queues_rip_unused(ehci);
+    ehci_queues_rip_unused(ehci, async);
 
     /*  Find the head of the list (4.9.1.1) */
     for(i = 0; i < MAX_QH; i++) {
@@ -1613,7 +1624,7 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     int reload;
 
     entry = ehci_get_fetch_addr(ehci, async);
-    q = ehci_find_queue_by_qh(ehci, entry);
+    q = ehci_find_queue_by_qh(ehci, entry, async);
     if (NULL == q) {
         q = ehci_alloc_queue(ehci, async);
     }
@@ -2064,7 +2075,7 @@ static void ehci_advance_state(EHCIState *ehci,
 
 static void ehci_advance_async_state(EHCIState *ehci)
 {
-    int async = 1;
+    const int async = 1;
 
     switch(ehci_get_state(ehci, async)) {
     case EST_INACTIVE:
@@ -2121,7 +2132,7 @@ static void ehci_advance_periodic_state(EHCIState *ehci)
 {
     uint32_t entry;
     uint32_t list;
-    int async = 0;
+    const int async = 0;
 
     // 4.6
 
@@ -2354,7 +2365,8 @@ static int usb_ehci_initfn(PCIDevice *dev)
     }
 
     s->frame_timer = qemu_new_timer_ns(vm_clock, ehci_frame_timer, s);
-    QTAILQ_INIT(&s->queues);
+    QTAILQ_INIT(&s->aqueues);
+    QTAILQ_INIT(&s->pqueues);
 
     qemu_register_reset(ehci_reset, s);
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 04/13] usb-ehci: always call ehci_queues_rip_unused for period queues

[Hans de Goede]

Before this patch USB 2 devices with interrupt endpoints were not working
properly. The problem is that to avoid loops we stop processing as soon
as we encounter a queue-head (qh) we've already seen since qhs can be linked
in a circular fashion, this is tracked by the seen flag in our qh struct.

The resetting of the seen flag is done from ehci_queues_rip_unused which
before this patch was only called when executing the statemachine for the
async schedule.

But packets for interrupt endpoints are part of the periodic schedule! So what
would happen is that when there were no ctrl or bulk packets for a USB 2
device with an interrupt endpoint, the async schedule would become non
active, then ehci_queues_rip_unused would no longer get called and when
processing the qhs for the interrupt endpoints from the periodic schedule
their seen bit would still be 1 and they would be skipped.

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 840022d..d384fcc 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -2167,6 +2167,7 @@ static void ehci_advance_periodic_state(EHCIState *ehci)
         ehci_set_fetch_addr(ehci, async,entry);
         ehci_set_state(ehci, async, EST_FETCHENTRY);
         ehci_advance_state(ehci, async);
+        ehci_queues_rip_unused(ehci, async);
         break;
 
     default:
-- 
1.7.7.6

[Qemu-devel] [PATCH 05/13] usb-ehci: Drop cached qhs when the doorbell gets rung

[Hans de Goede]

The purpose of the IAAD bit / the doorbell is to make the ehci controller
forget about cached qhs, this is mainly used when cancelling transactions,
the qh is unlinked from the async schedule and then the doorbell gets rung,
once the doorbell is acked by the controller the hcd knows that the qh is
no longer in use and that it can do something else with the memory, such
as re-use it for a new qh! But we keep our struct representing this qh around
for circa 250 ms. This allows for a (mightily large) race window where the
following could happen:
-hcd submits a qh at address 0xdeadbeef
-our ehci code sees the qh, sends a request to a usb-device, gets a result
 of USB_RET_ASYNC, sets the async_state of the qh to EHCI_ASYNC_INFLIGHT
-hcd unlinks the qh at address 0xdeadbeef
-hcd rings the doorbell, wait for us to ack it
-hcd re-uses the qh at address 0xdeadbeef
-our ehci code sees the qh, looks in the async_queue, sees there already is
 a qh at address 0xdeadbeef there with async_state of EHCI_ASYNC_INFLIGHT,
 does nothing
-the *original* (which the hcd thinks it has cancelled) transaction finishes
-our ehci code sees the qh on yet another pass through the async list,
 looks in the async_queue, sees there already is a qh at address 0xdeadbeef
 there with async_state of EHCI_ASYNC_COMPLETED, and finished the transaction
 with the results of the *original* transaction.

Not good (tm), this patch fixes this race by removing all qhs which have not
been seen during the last cycle through the async list immidiately when the
doorbell is rung.

Note this patch does not fix any actually observed problem, but upon
reading of the EHCI spec it became apparent to me that the above race could
happen and the usb-ehci behavior from before this patch is not good.

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index d384fcc..b349003 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -697,7 +697,7 @@ static EHCIQueue *ehci_find_queue_by_qh(EHCIState *ehci, uint32_t addr,
     return NULL;
 }
 
-static void ehci_queues_rip_unused(EHCIState *ehci, int async)
+static void ehci_queues_rip_unused(EHCIState *ehci, int async, int flush)
 {
     EHCIQueueHead *head = async ? &ehci->aqueues : &ehci->pqueues;
     EHCIQueue *q, *tmp;
@@ -708,7 +708,7 @@ static void ehci_queues_rip_unused(EHCIState *ehci, int async)
             q->ts = ehci->last_run_ns;
             continue;
         }
-        if (ehci->last_run_ns < q->ts + 250000000) {
+        if (!flush && ehci->last_run_ns < q->ts + 250000000) {
             /* allow 0.25 sec idle */
             continue;
         }
@@ -1537,7 +1537,7 @@ static int ehci_state_waitlisthead(EHCIState *ehci,  int async)
         ehci_set_usbsts(ehci, USBSTS_REC);
     }
 
-    ehci_queues_rip_unused(ehci, async);
+    ehci_queues_rip_unused(ehci, async, 0);
 
     /*  Find the head of the list (4.9.1.1) */
     for(i = 0; i < MAX_QH; i++) {
@@ -2093,18 +2093,7 @@ static void ehci_advance_async_state(EHCIState *ehci)
             break;
         }
 
-        /* If the doorbell is set, the guest wants to make a change to the
-         * schedule. The host controller needs to release cached data.
-         * (section 4.8.2)
-         */
-        if (ehci->usbcmd & USBCMD_IAAD) {
-            DPRINTF("ASYNC: doorbell request acknowledged\n");
-            ehci->usbcmd &= ~USBCMD_IAAD;
-            ehci_set_interrupt(ehci, USBSTS_IAA);
-            break;
-        }
-
-        /* make sure guest has acknowledged */
+        /* make sure guest has acknowledged the doorbell interrupt */
         /* TO-DO: is this really needed? */
         if (ehci->usbsts & USBSTS_IAA) {
             DPRINTF("IAA status bit still set.\n");
@@ -2118,6 +2107,18 @@ static void ehci_advance_async_state(EHCIState *ehci)
 
         ehci_set_state(ehci, async, EST_WAITLISTHEAD);
         ehci_advance_state(ehci, async);
+
+        /* If the doorbell is set, the guest wants to make a change to the
+         * schedule. The host controller needs to release cached data.
+         * (section 4.8.2)
+         */
+        if (ehci->usbcmd & USBCMD_IAAD) {
+            /* Remove all unseen qhs from the async qhs queue */
+            ehci_queues_rip_unused(ehci, async, 1);
+            DPRINTF("ASYNC: doorbell request acknowledged\n");
+            ehci->usbcmd &= ~USBCMD_IAAD;
+            ehci_set_interrupt(ehci, USBSTS_IAA);
+        }
         break;
 
     default:
@@ -2167,7 +2168,7 @@ static void ehci_advance_periodic_state(EHCIState *ehci)
         ehci_set_fetch_addr(ehci, async,entry);
         ehci_set_state(ehci, async, EST_FETCHENTRY);
         ehci_advance_state(ehci, async);
-        ehci_queues_rip_unused(ehci, async);
+        ehci_queues_rip_unused(ehci, async, 0);
         break;
 
     default:
-- 
1.7.7.6

[Qemu-devel] [PATCH 06/13] usb-ehci: Rip the queues when the async or period schedule is halted

[Hans de Goede]

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index b349003..d386b84 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -1076,7 +1076,8 @@ static void ehci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
 
         if (!(val & USBCMD_RUNSTOP) && (s->usbcmd & USBCMD_RUNSTOP)) {
             qemu_del_timer(s->frame_timer);
-            // TODO - should finish out some stuff before setting halt
+            ehci_queues_rip_all(s, 0);
+            ehci_queues_rip_all(s, 1);
             ehci_set_usbsts(s, USBSTS_HALT);
         }
 
@@ -2088,6 +2089,7 @@ static void ehci_advance_async_state(EHCIState *ehci)
 
     case EST_ACTIVE:
         if ( !(ehci->usbcmd & USBCMD_ASE)) {
+            ehci_queues_rip_all(ehci, async);
             ehci_clear_usbsts(ehci, USBSTS_ASS);
             ehci_set_state(ehci, async, EST_INACTIVE);
             break;
@@ -2148,6 +2150,7 @@ static void ehci_advance_periodic_state(EHCIState *ehci)
 
     case EST_ACTIVE:
         if ( !(ehci->frindex & 7) && !(ehci->usbcmd & USBCMD_PSE)) {
+            ehci_queues_rip_all(ehci, async);
             ehci_clear_usbsts(ehci, USBSTS_PSS);
             ehci_set_state(ehci, async, EST_INACTIVE);
             break;
-- 
1.7.7.6

[Qemu-devel] [PATCH 07/13] usb-ehci: Any packet completion except for NAK should set the interrupt

[Hans de Goede]

As clearly stated in the 2.3.2 of the EHCI spec, any time USBERRINT get
sets then if the td has its IOC bit set USBINT should be set as well.

This means that for any status except for USB_RET_NAK we should set
USBINT if the IOC bit is set.

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index d386b84..507e4a8 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -1360,7 +1360,7 @@ err:
     q->qh.token ^= QTD_TOKEN_DTOGGLE;
     q->qh.token &= ~QTD_TOKEN_ACTIVE;
 
-    if ((q->usb_status >= 0) && (q->qh.token & QTD_TOKEN_IOC)) {
+    if ((q->usb_status != USB_RET_NAK) && (q->qh.token & QTD_TOKEN_IOC)) {
         ehci_record_interrupt(q->ehci, USBSTS_INT);
     }
 }
-- 
1.7.7.6

[Qemu-devel] [PATCH 08/13] usb-ehci: Fix cerr tracking

[Hans de Goede]

cerr should only be decremented on errors which cause XactErr to be set, and
when that happens the failing transaction should be retried until cerr reaches
0 and only then should USBSTS_ERRINT be set (and inactive cleared and
USBSTS_INT set if requested).

Since we don't have any hardware level errors (and in case of redirection
the real hardware has already retried), re-trying makes no sense, so
immediately set cerr to 0 on errors which set XactErr.

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 507e4a8..2685adc 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -1291,7 +1291,7 @@ static void ehci_async_complete_packet(USBPort *port, USBPacket *packet)
 
 static void ehci_execute_complete(EHCIQueue *q)
 {
-    int c_err, reload;
+    int reload;
 
     assert(q->async != EHCI_ASYNC_INFLIGHT);
     q->async = EHCI_ASYNC_NONE;
@@ -1300,15 +1300,10 @@ static void ehci_execute_complete(EHCIQueue *q)
             q->qhaddr, q->qh.next, q->qtdaddr, q->usb_status);
 
     if (q->usb_status < 0) {
-err:
-        /* TO-DO: put this is in a function that can be invoked below as well */
-        c_err = get_field(q->qh.token, QTD_TOKEN_CERR);
-        c_err--;
-        set_field(&q->qh.token, c_err, QTD_TOKEN_CERR);
-
         switch(q->usb_status) {
         case USB_RET_NODEV:
             q->qh.token |= (QTD_TOKEN_HALT | QTD_TOKEN_XACTERR);
+            set_field(&q->qh.token, 0, QTD_TOKEN_CERR);
             ehci_record_interrupt(q->ehci, USBSTS_ERRINT);
             break;
         case USB_RET_STALL:
@@ -1336,15 +1331,13 @@ err:
             assert(0);
             break;
         }
+    } else if ((q->usb_status > q->tbytes) && (q->pid == USB_TOKEN_IN)) {
+        q->usb_status = USB_RET_BABBLE;
+        q->qh.token |= (QTD_TOKEN_HALT | QTD_TOKEN_BABBLE);
+        ehci_record_interrupt(q->ehci, USBSTS_ERRINT);
     } else {
-        // DPRINTF("Short packet condition\n");
         // TODO check 4.12 for splits
 
-        if ((q->usb_status > q->tbytes) && (q->pid == USB_TOKEN_IN)) {
-            q->usb_status = USB_RET_BABBLE;
-            goto err;
-        }
-
         if (q->tbytes && q->pid == USB_TOKEN_IN) {
             q->tbytes -= q->usb_status;
         } else {
-- 
1.7.7.6

[Qemu-devel] [PATCH 09/13] usb-ehci: Remove dead nakcnt code

[Hans de Goede]

This patch removes 2 bits of dead nakcnt code:

1) usb_ehci_execute calls ehci_qh_do_overlay which does:
nakcnt = reload;
and then has a block of code which is conditional on:
if (reload && !nakcnt) {
which ofcourse is never true now as nakcnt == reload.

2) ehci_state_fetchqh does:
nakcnt = reload;
but before nakcnt is ever used ehci_state_fetchqh is always followed
by a ehci_qh_do_overlay call which also does:
nakcnt = reload;
So doing this from ehci_state_fetchqh is redundant.

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 2685adc..07bcd1f 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -1615,7 +1615,6 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
 {
     uint32_t entry;
     EHCIQueue *q;
-    int reload;
 
     entry = ehci_get_fetch_addr(ehci, async);
     q = ehci_find_queue_by_qh(ehci, entry, async);
@@ -1673,11 +1672,6 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     }
 #endif
 
-    reload = get_field(q->qh.epchar, QH_EPCHAR_RL);
-    if (reload) {
-        set_field(&q->qh.altnext_qtd, reload, QH_ALTNEXT_NAKCNT);
-    }
-
     if (q->qh.token & QTD_TOKEN_HALT) {
         ehci_set_state(ehci, async, EST_HORIZONTALQH);
 
@@ -1837,25 +1831,11 @@ static void ehci_flush_qh(EHCIQueue *q)
 static int ehci_state_execute(EHCIQueue *q, int async)
 {
     int again = 0;
-    int reload, nakcnt;
-    int smask;
 
     if (ehci_qh_do_overlay(q) != 0) {
         return -1;
     }
 
-    smask = get_field(q->qh.epcap, QH_EPCAP_SMASK);
-
-    if (!smask) {
-        reload = get_field(q->qh.epchar, QH_EPCHAR_RL);
-        nakcnt = get_field(q->qh.altnext_qtd, QH_ALTNEXT_NAKCNT);
-        if (reload && !nakcnt) {
-            ehci_set_state(q->ehci, async, EST_HORIZONTALQH);
-            again = 1;
-            goto out;
-        }
-    }
-
     // TODO verify enough time remains in the uframe as in 4.4.1.1
     // TODO write back ptr to async list when done or out of time
     // TODO Windows does not seem to ever set the MULT field
-- 
1.7.7.6

[Qemu-devel] [PATCH 10/13] usb-ehci: Fix and simplify nakcnt handling

[Hans de Goede]

The nakcnt code in ehci_execute_complete() marked transactions as finished
when a packet completed with a result of USB_RET_NAK, but USB_RET_NAK
means that the device cannot receive / send data at that time and that
the transaction should be retried later, which is also what the usb-uhci
and usb-ohci code does.

Note that there already was some special code in place to handle this
for interrupt endpoints in the form of doing a return from
ehci_execute_complete() when reload == 0, but that for bulk transactions
this was not handled correctly (where as for example the usb-ccid device does
return USB_RET_NAK for bulk packets).

Besides that the code in ehci_execute_complete() decrement nakcnt by 1
on a packet result of USB_RET_NAK, but
-since the transaction got marked as finished,
 nakcnt would never be decremented again
-there is no code checking for nakcnt becoming 0
-there is no use in re-trying the transaction within the same usb frame /
 usb-ehci frame-timer call, since the status of emulated devices won't change
 as long as the usb-ehci frame-timer is running
So we should simply set the nakcnt to 0 when we get a USB_RET_NAK, thus
claiming that we've tried reload times (or as many times as possible if
reload is 0).

Besides the code in ehci_execute_complete() handling USB_RET_NAK there
was also code handling it in ehci_state_executing(), which calls
ehci_execute_complete(), and then does its own handling on top of the handling
in ehci_execute_complete(), this code would decrement nakcnt *again* (if not
already 0), or restore the reload value (which was never changed) on success.

Since the double decrement was wrong to begin with, and is no longer needed
now that we set nakcnt directly to 0 on USB_RET_NAK, and the restore of reload
is not needed either, this patch simply removes all nakcnt handling from
ehci_state_executing().

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 07bcd1f..9197298 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -1291,8 +1291,6 @@ static void ehci_async_complete_packet(USBPort *port, USBPacket *packet)
 
 static void ehci_execute_complete(EHCIQueue *q)
 {
-    int reload;
-
     assert(q->async != EHCI_ASYNC_INFLIGHT);
     q->async = EHCI_ASYNC_NONE;
 
@@ -1311,16 +1309,8 @@ static void ehci_execute_complete(EHCIQueue *q)
             ehci_record_interrupt(q->ehci, USBSTS_ERRINT);
             break;
         case USB_RET_NAK:
-            /* 4.10.3 */
-            reload = get_field(q->qh.epchar, QH_EPCHAR_RL);
-            if ((q->pid == USB_TOKEN_IN) && reload) {
-                int nakcnt = get_field(q->qh.altnext_qtd, QH_ALTNEXT_NAKCNT);
-                nakcnt--;
-                set_field(&q->qh.altnext_qtd, nakcnt, QH_ALTNEXT_NAKCNT);
-            } else if (!reload) {
-                return;
-            }
-            break;
+            set_field(&q->qh.altnext_qtd, 0, QH_ALTNEXT_NAKCNT);
+            return; /* We're not done yet with this transaction */
         case USB_RET_BABBLE:
             q->qh.token |= (QTD_TOKEN_HALT | QTD_TOKEN_BABBLE);
             ehci_record_interrupt(q->ehci, USBSTS_ERRINT);
@@ -1353,7 +1343,7 @@ static void ehci_execute_complete(EHCIQueue *q)
     q->qh.token ^= QTD_TOKEN_DTOGGLE;
     q->qh.token &= ~QTD_TOKEN_ACTIVE;
 
-    if ((q->usb_status != USB_RET_NAK) && (q->qh.token & QTD_TOKEN_IOC)) {
+    if (q->qh.token & QTD_TOKEN_IOC) {
         ehci_record_interrupt(q->ehci, USBSTS_INT);
     }
 }
@@ -1877,7 +1867,6 @@ out:
 static int ehci_state_executing(EHCIQueue *q, int async)
 {
     int again = 0;
-    int reload, nakcnt;
 
     ehci_execute_complete(q);
     if (q->usb_status == USB_RET_ASYNC) {
@@ -1897,21 +1886,8 @@ static int ehci_state_executing(EHCIQueue *q, int async)
         // counter decrements to 0
     }
 
-    reload = get_field(q->qh.epchar, QH_EPCHAR_RL);
-    if (reload) {
-        nakcnt = get_field(q->qh.altnext_qtd, QH_ALTNEXT_NAKCNT);
-        if (q->usb_status == USB_RET_NAK) {
-            if (nakcnt) {
-                nakcnt--;
-            }
-        } else {
-            nakcnt = reload;
-        }
-        set_field(&q->qh.altnext_qtd, nakcnt, QH_ALTNEXT_NAKCNT);
-    }
-
     /* 4.10.5 */
-    if ((q->usb_status == USB_RET_NAK) || (q->qh.token & QTD_TOKEN_ACTIVE)) {
+    if (q->usb_status == USB_RET_NAK) {
         ehci_set_state(q->ehci, async, EST_HORIZONTALQH);
     } else {
         ehci_set_state(q->ehci, async, EST_WRITEBACK);
-- 
1.7.7.6

[Qemu-devel] [PATCH 11/13] usb-ehci: Cleanup itd error handling

[Hans de Goede]

All error statuses except for NAK are handled in a switch case, move the
handling of NAK into the same switch case.

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

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 9197298..825fcc0 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -1466,20 +1466,7 @@ static int ehci_process_itd(EHCIState *ehci,
             }
             qemu_sglist_destroy(&ehci->isgl);
 
-            if (ret == USB_RET_NAK) {
-                /* no data for us, so do a zero-length transfer */
-                ret = 0;
-            }
-
-            if (ret >= 0) {
-                if (!dir) {
-                    /* OUT */
-                    set_field(&itd->transact[i], len - ret, ITD_XACT_LENGTH);
-                } else {
-                    /* IN */
-                    set_field(&itd->transact[i], ret, ITD_XACT_LENGTH);
-                }
-            } else {
+            if (ret < 0) {
                 switch (ret) {
                 default:
                     fprintf(stderr, "Unexpected iso usb result: %d\n", ret);
@@ -1495,6 +1482,19 @@ static int ehci_process_itd(EHCIState *ehci,
                     itd->transact[i] |= ITD_XACT_BABBLE;
                     ehci_record_interrupt(ehci, USBSTS_ERRINT);
                     break;
+                case USB_RET_NAK:
+                    /* no data for us, so do a zero-length transfer */
+                    ret = 0;
+                    break;
+                }
+            }
+            if (ret >= 0) {
+                if (!dir) {
+                    /* OUT */
+                    set_field(&itd->transact[i], len - ret, ITD_XACT_LENGTH);
+                } else {
+                    /* IN */
+                    set_field(&itd->transact[i], ret, ITD_XACT_LENGTH);
                 }
             }
             if (itd->transact[i] & ITD_XACT_IOC) {
-- 
1.7.7.6

[Qemu-devel] [PATCH 12/13] usb: return BABBLE rather then NAK when we receive too much data

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-linux.c |    8 +++++++-
 usb-redir.c |    4 ++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 47994f3..38df9e6 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -364,6 +364,10 @@ static void async_complete(void *opaque)
                 p->result = USB_RET_STALL;
                 break;
 
+            case -EOVERFLOW:
+                p->result = USB_RET_BABBLE;
+                break;
+
             default:
                 p->result = USB_RET_NAK;
                 break;
@@ -722,6 +726,8 @@ static int urb_status_to_usb_ret(int status)
     switch (status) {
     case -EPIPE:
         return USB_RET_STALL;
+    case -EOVERFLOW:
+        return USB_RET_BABBLE;
     default:
         return USB_RET_NAK;
     }
@@ -759,7 +765,7 @@ static int usb_host_handle_iso_data(USBHostDevice *s, USBPacket *p, int in)
             } else if (aurb[i].urb.iso_frame_desc[j].actual_length
                        > p->iov.size) {
                 printf("husb: received iso data is larger then packet\n");
-                len = USB_RET_NAK;
+                len = USB_RET_BABBLE;
             /* All good copy data over */
             } else {
                 len = aurb[i].urb.iso_frame_desc[j].actual_length;
diff --git a/usb-redir.c b/usb-redir.c
index ef9fa96..4c8e3e3 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -457,7 +457,7 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
             ERROR("received iso data is larger then packet ep %02X (%d > %d)\n",
                   ep, len, (int)p->iov.size);
             bufp_free(dev, isop, ep);
-            return USB_RET_NAK;
+            return USB_RET_BABBLE;
         }
         usb_packet_copy(p, isop->data, len);
         bufp_free(dev, isop, ep);
@@ -576,7 +576,7 @@ static int usbredir_handle_interrupt_data(USBRedirDevice *dev,
         if (len > p->iov.size) {
             ERROR("received int data is larger then packet ep %02X\n", ep);
             bufp_free(dev, intp, ep);
-            return USB_RET_NAK;
+            return USB_RET_BABBLE;
         }
         usb_packet_copy(p, intp->data, len);
         bufp_free(dev, intp, ep);
-- 
1.7.7.6

[Qemu-devel] [PATCH 13/13] usb: add USB_RET_IOERROR

[Hans de Goede]

We already have USB_RET_NAK, but that means that a device does not want
to send/receive right now. But with host / network redirection we can
actually have a transaction fail due to some io error, rather then ie
the device just not having any data atm.

This patch adds a new error code named USB_RET_IOERROR for this, and uses
it were appropriate.

Notes:
-Currently all usb-controllers handle this the same as NODEV, but that
 may change in the future, OHCI could indicate a CRC error instead for example.
-This patch does not touch hw/usb-musb.c, that is because the code in there
 handles STALL and NAK specially and has a if status < 0 generic catch all
 for all other errors

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb-ehci.c |    2 ++
 hw/usb-ohci.c |    2 ++
 hw/usb-uhci.c |    1 +
 hw/usb.h      |   11 ++++++-----
 usb-linux.c   |    4 ++--
 usb-redir.c   |    9 ++++++---
 6 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index 825fcc0..df742f7 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -1299,6 +1299,7 @@ static void ehci_execute_complete(EHCIQueue *q)
 
     if (q->usb_status < 0) {
         switch(q->usb_status) {
+        case USB_RET_IOERROR:
         case USB_RET_NODEV:
             q->qh.token |= (QTD_TOKEN_HALT | QTD_TOKEN_XACTERR);
             set_field(&q->qh.token, 0, QTD_TOKEN_CERR);
@@ -1471,6 +1472,7 @@ static int ehci_process_itd(EHCIState *ehci,
                 default:
                     fprintf(stderr, "Unexpected iso usb result: %d\n", ret);
                     /* Fall through */
+                case USB_RET_IOERROR:
                 case USB_RET_NODEV:
                     /* 3.3.2: XACTERR is only allowed on IN transactions */
                     if (dir) {
diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index 7aa19fe..20aaa74 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -837,6 +837,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
                         OHCI_CC_DATAUNDERRUN);
         } else {
             switch (ret) {
+            case USB_RET_IOERROR:
             case USB_RET_NODEV:
                 OHCI_SET_BM(iso_td.offset[relative_frame_number], TD_PSW_CC,
                             OHCI_CC_DEVICENOTRESPONDING);
@@ -1052,6 +1053,7 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
             OHCI_SET_BM(td.flags, TD_CC, OHCI_CC_DATAUNDERRUN);
         } else {
             switch (ret) {
+            case USB_RET_IOERROR:
             case USB_RET_NODEV:
                 OHCI_SET_BM(td.flags, TD_CC, OHCI_CC_DEVICENOTRESPONDING);
             case USB_RET_NAK:
diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index 70e3881..2c6ed38 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -765,6 +765,7 @@ out:
             break;
 	return 1;
 
+    case USB_RET_IOERROR:
     case USB_RET_NODEV:
     default:
 	break;
diff --git a/hw/usb.h b/hw/usb.h
index 8e83697..1a30ebb 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -39,11 +39,12 @@
 #define USB_TOKEN_IN    0x69 /* device -> host */
 #define USB_TOKEN_OUT   0xe1 /* host -> device */
 
-#define USB_RET_NODEV  (-1)
-#define USB_RET_NAK    (-2)
-#define USB_RET_STALL  (-3)
-#define USB_RET_BABBLE (-4)
-#define USB_RET_ASYNC  (-5)
+#define USB_RET_NODEV   (-1)
+#define USB_RET_NAK     (-2)
+#define USB_RET_STALL   (-3)
+#define USB_RET_BABBLE  (-4)
+#define USB_RET_IOERROR (-5)
+#define USB_RET_ASYNC   (-6)
 
 #define USB_SPEED_LOW   0
 #define USB_SPEED_FULL  1
diff --git a/usb-linux.c b/usb-linux.c
index 38df9e6..050ea7a 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -369,7 +369,7 @@ static void async_complete(void *opaque)
                 break;
 
             default:
-                p->result = USB_RET_NAK;
+                p->result = USB_RET_IOERROR;
                 break;
             }
 
@@ -729,7 +729,7 @@ static int urb_status_to_usb_ret(int status)
     case -EOVERFLOW:
         return USB_RET_BABBLE;
     default:
-        return USB_RET_NAK;
+        return USB_RET_IOERROR;
     }
 }
 
diff --git a/usb-redir.c b/usb-redir.c
index 4c8e3e3..59e9327 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -441,7 +441,7 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
             /* Check iso_error for stream errors, otherwise its an underrun */
             status = dev->endpoint[EP2I(ep)].iso_error;
             dev->endpoint[EP2I(ep)].iso_error = 0;
-            return status ? USB_RET_NAK : 0;
+            return status ? USB_RET_IOERROR : 0;
         }
         DPRINTF2("iso-token-in ep %02X status %d len %d queue-size: %d\n", ep,
                  isop->status, isop->len, dev->endpoint[EP2I(ep)].bufpq_size);
@@ -449,7 +449,7 @@ static int usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
         status = isop->status;
         if (status != usb_redir_success) {
             bufp_free(dev, isop, ep);
-            return USB_RET_NAK;
+            return USB_RET_IOERROR;
         }
 
         len = isop->len;
@@ -1045,11 +1045,14 @@ static int usbredir_handle_status(USBRedirDevice *dev,
         return USB_RET_STALL;
     case usb_redir_cancelled:
         WARNING("returning cancelled packet to HC?\n");
+        return USB_RET_NAK;
     case usb_redir_inval:
+        WARNING("got invalid param error from usb-host?\n");
+        return USB_RET_NAK;
     case usb_redir_ioerror:
     case usb_redir_timeout:
     default:
-        return USB_RET_NAK;
+        return USB_RET_IOERROR;
     }
 }
 
-- 
1.7.7.6

Re: [Qemu-devel] usb patches [v2]

[Gerd Hoffmann]

On 03/02/12 21:27, Hans de Goede wrote:
> Here is a series of usb patches against current qemu master, mostly
> consisting of a whole bunch of small ehci fixes, plus some redirection
> fixes.

Patch added to usb patch queue.

thanks,
  Gerd