[Qemu-devel] [PULL for-1.2 00/11] usb patch queue

[Qemu-devel] [PULL for-1.2 00/11] usb patch queue

[Gerd Hoffmann]

  Hi,

This patch series brings a some last-minute usb bugfixes for 1.2.  The
new usb packet queuing code added in the 1.2 devel cycle fails to handle
some corner cases correctly and some regressions sneaked in.  Most of
this is in the ehci emulation.

Also included is a 'info qtree' fix which is strictly speaking not usb
related, but it is need to make usb-storage tests in autotest work.

please pull,
  Gerd

PS: complete usb patch queue, including all post-1.2 bits, is now 
    available from git://git.kraxel.org/qemu rebase/usb-next

The following changes since commit b834b5081d6266cc0789454905f3b7d622d4d096:

  w32: Fix broken build (2012-08-30 16:36:21 -0500)

are available in the git repository at:
  git://git.kraxel.org/qemu usb.61

Gerd Hoffmann (5):
      fix info qtree indention
      usb: unique packet ids
      ehci: add ehci_cancel_queue()
      ehci: handle TD deactivation of inflight packets
      uas: move transfer kickoff

Hans de Goede (6):
      usb: Halt ep queue en cancel pending packets on a packet error
      ehci: Fix NULL ptr deref when unplugging an USB dev with an iso stream active
      ehci: Schedule async-bh when IAAD bit gets set
      ehci: Remove unnecessary ehci_flush_qh call
      ehci: simplify ehci_state_executing
      ehci: Fix interrupt endpoints no longer working

 hw/qdev-monitor.c |    2 +-
 hw/usb.h          |    4 +-
 hw/usb/core.c     |   38 +++++++++++++----
 hw/usb/dev-uas.c  |    3 +-
 hw/usb/hcd-ehci.c |  122 ++++++++++++++++++++++++++++++++++------------------
 hw/usb/hcd-musb.c |    3 +-
 hw/usb/hcd-ohci.c |    4 +-
 hw/usb/hcd-uhci.c |   20 ++++++++-
 hw/usb/hcd-xhci.c |    2 +-
 9 files changed, 138 insertions(+), 60 deletions(-)

[Qemu-devel] [PATCH for-1.2 01/11] fix info qtree indention

[Gerd Hoffmann]

Without the patch bus properties are are not in line with the other
properties:

[ ... ]
  dev: fw_cfg, id ""
    ctl_iobase = 0x510
    data_iobase = 0x511
      irq 0
      mmio ffffffffffffffff/0000000000000002
      mmio ffffffffffffffff/0000000000000001
[ ... ]

With the patch applied everything is lined up properly:

[ ... ]
  dev: fw_cfg, id ""
    ctl_iobase = 0x510
    data_iobase = 0x511
    irq 0
    mmio ffffffffffffffff/0000000000000002
    mmio ffffffffffffffff/0000000000000001
[ ... ]

Needed to make the autotest qtree parser happy.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev-monitor.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index 018b386..33b7f79 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -543,7 +543,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
         qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent);
         class = object_class_get_parent(class);
     } while (class != object_class_by_name(TYPE_DEVICE));
-    bus_print_dev(dev->parent_bus, mon, dev, indent + 2);
+    bus_print_dev(dev->parent_bus, mon, dev, indent);
     QLIST_FOREACH(child, &dev->child_bus, sibling) {
         qbus_print(mon, child, indent);
     }
-- 
1.7.1

[Qemu-devel] [PATCH for-1.2 02/11] usb: Halt ep queue en cancel pending packets on a packet error

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

For controllers which queue up more then 1 packet at a time, we must halt the
ep queue, and inside the controller code cancel all pending packets on an
error.

There are multiple reasons for this:
1) Guests expect the controllers to halt ep queues on error, so that they
get the opportunity to cancel transfers which the scheduled after the failing
one, before processing continues

2) Not cancelling queued up packets after a failed transfer also messes up
the controller state machine, in the case of EHCI causing the following
assert to trigger: "assert(p->qtdaddr == q->qtdaddr)" at hcd-ehci.c:2075

3) For bulk endpoints with pipelining enabled (redirection to a real USB
device), we must cancel all the transfers after this a failed one so that:
a) If they've completed already, they are not processed further causing more
   stalls to be reported, originating from the same failed transfer
b) If still in flight, they are cancelled before the guest does
   a clear stall, otherwise the guest and device can loose sync!

Note this patch only touches the ehci and uhci controller changes, since AFAIK
no other controllers actually queue up multiple transfer. If I'm wrong on this
other controllers need to be updated too!

Also note that this patch was heavily tested with the ehci code, where I had
a reproducer for a device causing a transfer to fail. The uhci code is not
tested with actually failing transfers and could do with a thorough review!

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb.h          |    1 +
 hw/usb/core.c     |   35 ++++++++++++++++++++++++++++-------
 hw/usb/hcd-ehci.c |   13 +++++++++++++
 hw/usb/hcd-uhci.c |   16 ++++++++++++++++
 4 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/hw/usb.h b/hw/usb.h
index 432ccae..e574477 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -179,6 +179,7 @@ struct USBEndpoint {
     uint8_t ifnum;
     int max_packet_size;
     bool pipeline;
+    bool halted;
     USBDevice *dev;
     QTAILQ_HEAD(, USBPacket) queue;
 };
diff --git a/hw/usb/core.c b/hw/usb/core.c
index c7e5bc0..28b840e 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -382,12 +382,23 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
     usb_packet_check_state(p, USB_PACKET_SETUP);
     assert(p->ep != NULL);
 
+    /* Submitting a new packet clears halt */
+    if (p->ep->halted) {
+        assert(QTAILQ_EMPTY(&p->ep->queue));
+        p->ep->halted = false;
+    }
+
     if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
         ret = usb_process_one(p);
         if (ret == USB_RET_ASYNC) {
             usb_packet_set_state(p, USB_PACKET_ASYNC);
             QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
         } else {
+            /*
+             * When pipelining is enabled usb-devices must always return async,
+             * otherwise packets can complete out of order!
+             */
+            assert(!p->ep->pipeline);
             p->result = ret;
             usb_packet_set_state(p, USB_PACKET_COMPLETE);
         }
@@ -399,6 +410,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
     return ret;
 }
 
+static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
+{
+    USBEndpoint *ep = p->ep;
+
+    assert(p->result != USB_RET_ASYNC && p->result != USB_RET_NAK);
+
+    if (p->result < 0) {
+        ep->halted = true;
+    }
+    usb_packet_set_state(p, USB_PACKET_COMPLETE);
+    QTAILQ_REMOVE(&ep->queue, p, queue);
+    dev->port->ops->complete(dev->port, p);
+}
+
 /* Notify the controller that an async packet is complete.  This should only
    be called for packets previously deferred by returning USB_RET_ASYNC from
    handle_packet. */
@@ -409,11 +434,9 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
 
     usb_packet_check_state(p, USB_PACKET_ASYNC);
     assert(QTAILQ_FIRST(&ep->queue) == p);
-    usb_packet_set_state(p, USB_PACKET_COMPLETE);
-    QTAILQ_REMOVE(&ep->queue, p, queue);
-    dev->port->ops->complete(dev->port, p);
+    __usb_packet_complete(dev, p);
 
-    while (!QTAILQ_EMPTY(&ep->queue)) {
+    while (!ep->halted && !QTAILQ_EMPTY(&ep->queue)) {
         p = QTAILQ_FIRST(&ep->queue);
         if (p->state == USB_PACKET_ASYNC) {
             break;
@@ -425,9 +448,7 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
             break;
         }
         p->result = ret;
-        usb_packet_set_state(p, USB_PACKET_COMPLETE);
-        QTAILQ_REMOVE(&ep->queue, p, queue);
-        dev->port->ops->complete(dev->port, p);
+        __usb_packet_complete(ep->dev, p);
     }
 }
 
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 8b94b17..8504a6a 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2138,6 +2138,19 @@ static int ehci_state_writeback(EHCIQueue *q)
      * bit is clear.
      */
     if (q->qh.token & QTD_TOKEN_HALT) {
+        /*
+         * We should not do any further processing on a halted queue!
+         * This is esp. important for bulk endpoints with pipelining enabled
+         * (redirection to a real USB device), where we must cancel all the
+         * transfers after this one so that:
+         * 1) If they've completed already, they are not processed further
+         *    causing more stalls, originating from the same failed transfer
+         * 2) If still in flight, they are cancelled before the guest does
+         *    a clear stall, otherwise the guest and device can loose sync!
+         */
+        while ((p = QTAILQ_FIRST(&q->packets)) != NULL) {
+            ehci_free_packet(p);
+        }
         ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
         again = 1;
     } else {
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 1ace2a4..8987734 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -748,6 +748,22 @@ static int uhci_complete_td(UHCIState *s, UHCI_TD *td, UHCIAsync *async, uint32_
     return TD_RESULT_COMPLETE;
 
 out:
+    /*
+     * We should not do any further processing on a queue with errors!
+     * This is esp. important for bulk endpoints with pipelining enabled
+     * (redirection to a real USB device), where we must cancel all the
+     * transfers after this one so that:
+     * 1) If they've completed already, they are not processed further
+     *    causing more stalls, originating from the same failed transfer
+     * 2) If still in flight, they are cancelled before the guest does
+     *    a clear stall, otherwise the guest and device can loose sync!
+     */
+    while (!QTAILQ_EMPTY(&async->queue->asyncs)) {
+        UHCIAsync *as = QTAILQ_FIRST(&async->queue->asyncs);
+        uhci_async_unlink(as);
+        uhci_async_cancel(as);
+    }
+
     switch(ret) {
     case USB_RET_STALL:
         td->ctrl |= TD_CTRL_STALL;
-- 
1.7.1

[Qemu-devel] [PATCH for-1.2 03/11] usb: unique packet ids

[Gerd Hoffmann]

This patch adds IDs to usb packets.  Those IDs are (a) supposed to be
unique for the lifecycle of a packet (from packet setup until the packet
is either completed or canceled) and (b) stable across migration.

uhci, ohci, ehci and xhci use the guest physical address of the transfer
descriptor for this.

musb needs a different approach because there is no transfer descriptor.
But musb also doesn't support pipelining, so we have never more than one
packet per endpoint in flight.  So we go create an ID based on endpoint
and device address.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb.h          |    3 ++-
 hw/usb/core.c     |    3 ++-
 hw/usb/hcd-ehci.c |    9 +++++----
 hw/usb/hcd-musb.c |    3 ++-
 hw/usb/hcd-ohci.c |    4 ++--
 hw/usb/hcd-uhci.c |    4 ++--
 hw/usb/hcd-xhci.c |    2 +-
 7 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/usb.h b/hw/usb.h
index e574477..b8fceec 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -332,6 +332,7 @@ typedef enum USBPacketState {
 struct USBPacket {
     /* Data fields for use by the driver.  */
     int pid;
+    uint64_t id;
     USBEndpoint *ep;
     QEMUIOVector iov;
     uint64_t parameter; /* control transfers */
@@ -344,7 +345,7 @@ struct USBPacket {
 void usb_packet_init(USBPacket *p);
 void usb_packet_set_state(USBPacket *p, USBPacketState state);
 void usb_packet_check_state(USBPacket *p, USBPacketState expected);
-void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep);
+void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id);
 void usb_packet_addbuf(USBPacket *p, void *ptr, size_t len);
 int usb_packet_map(USBPacket *p, QEMUSGList *sgl);
 void usb_packet_unmap(USBPacket *p, QEMUSGList *sgl);
diff --git a/hw/usb/core.c b/hw/usb/core.c
index 28b840e..2da38e7 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -520,10 +520,11 @@ void usb_packet_set_state(USBPacket *p, USBPacketState state)
     p->state = state;
 }
 
-void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep)
+void usb_packet_setup(USBPacket *p, int pid, USBEndpoint *ep, uint64_t id)
 {
     assert(!usb_packet_is_inflight(p));
     assert(p->iov.iov != NULL);
+    p->id = id;
     p->pid = pid;
     p->ep = ep;
     p->result = 0;
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 8504a6a..f43d690 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1530,7 +1530,7 @@ static int ehci_execute(EHCIPacket *p, const char *action)
     endp = get_field(p->queue->qh.epchar, QH_EPCHAR_EP);
     ep = usb_ep_get(p->queue->dev, p->pid, endp);
 
-    usb_packet_setup(&p->packet, p->pid, ep);
+    usb_packet_setup(&p->packet, p->pid, ep, p->qtdaddr);
     usb_packet_map(&p->packet, &p->sgl);
 
     trace_usb_ehci_packet_action(p->queue, p, action);
@@ -1552,7 +1552,8 @@ static int ehci_execute(EHCIPacket *p, const char *action)
  */
 
 static int ehci_process_itd(EHCIState *ehci,
-                            EHCIitd *itd)
+                            EHCIitd *itd,
+                            uint32_t addr)
 {
     USBDevice *dev;
     USBEndpoint *ep;
@@ -1598,7 +1599,7 @@ static int ehci_process_itd(EHCIState *ehci,
             dev = ehci_find_device(ehci, devaddr);
             ep = usb_ep_get(dev, pid, endp);
             if (ep->type == USB_ENDPOINT_XFER_ISOC) {
-                usb_packet_setup(&ehci->ipacket, pid, ep);
+                usb_packet_setup(&ehci->ipacket, pid, ep, addr);
                 usb_packet_map(&ehci->ipacket, &ehci->isgl);
                 ret = usb_handle_packet(dev, &ehci->ipacket);
                 assert(ret != USB_RET_ASYNC);
@@ -1862,7 +1863,7 @@ static int ehci_state_fetchitd(EHCIState *ehci, int async)
                sizeof(EHCIitd) >> 2);
     ehci_trace_itd(ehci, entry, &itd);
 
-    if (ehci_process_itd(ehci, &itd) != 0) {
+    if (ehci_process_itd(ehci, &itd, entry) != 0) {
         return -1;
     }
 
diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
index fa9385e..0bb5c7b 100644
--- a/hw/usb/hcd-musb.c
+++ b/hw/usb/hcd-musb.c
@@ -626,7 +626,8 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
     /* A wild guess on the FADDR semantics... */
     dev = usb_find_device(&s->port, ep->faddr[idx]);
     uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
-    usb_packet_setup(&ep->packey[dir].p, pid, uep);
+    usb_packet_setup(&ep->packey[dir].p, pid, uep,
+                     (dev->addr << 16) | (uep->nr << 8) | pid);
     usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len);
     ep->packey[dir].ep = ep;
     ep->packey[dir].dir = dir;
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 844e7ed..c36184a 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -812,7 +812,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     } else {
         dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
         ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
-        usb_packet_setup(&ohci->usb_packet, pid, ep);
+        usb_packet_setup(&ohci->usb_packet, pid, ep, addr);
         usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, len);
         ret = usb_handle_packet(dev, &ohci->usb_packet);
         if (ret == USB_RET_ASYNC) {
@@ -1011,7 +1011,7 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
         }
         dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
         ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
-        usb_packet_setup(&ohci->usb_packet, pid, ep);
+        usb_packet_setup(&ohci->usb_packet, pid, ep, addr);
         usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, pktlen);
         ret = usb_handle_packet(dev, &ohci->usb_packet);
 #ifdef DEBUG_PACKET
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 8987734..b0db921 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -859,14 +859,14 @@ static int uhci_handle_td(UHCIState *s, uint32_t addr, UHCI_TD *td,
      * for initial isochronous requests
      */
     async->queue->valid = 32;
-    async->isoc  = td->ctrl & TD_CTRL_IOS;
+    async->isoc = td->ctrl & TD_CTRL_IOS;
 
     max_len = ((td->token >> 21) + 1) & 0x7ff;
     pid = td->token & 0xff;
 
     dev = uhci_find_device(s, (td->token >> 8) & 0x7f);
     ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf);
-    usb_packet_setup(&async->packet, pid, ep);
+    usb_packet_setup(&async->packet, pid, ep, addr);
     qemu_sglist_add(&async->sgl, td->buffer, max_len);
     usb_packet_map(&async->packet, &async->sgl);
 
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 6c2ff02..3eb27fa 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1392,7 +1392,7 @@ static int xhci_setup_packet(XHCITransfer *xfer, USBDevice *dev)
 
     dir = xfer->in_xfer ? USB_TOKEN_IN : USB_TOKEN_OUT;
     ep = usb_ep_get(dev, dir, xfer->epid >> 1);
-    usb_packet_setup(&xfer->packet, dir, ep);
+    usb_packet_setup(&xfer->packet, dir, ep, xfer->trbs[0].addr);
     usb_packet_addbuf(&xfer->packet, xfer->data, xfer->data_length);
     DPRINTF("xhci: setup packet pid 0x%x addr %d ep %d\n",
             xfer->packet.pid, dev->addr, ep->nr);
-- 
1.7.1

[Qemu-devel] [PATCH for-1.2 04/11] ehci: Fix NULL ptr deref when unplugging an USB dev with an iso stream active

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

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

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index f43d690..a3aea6d 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1598,7 +1598,7 @@ static int ehci_process_itd(EHCIState *ehci,
 
             dev = ehci_find_device(ehci, devaddr);
             ep = usb_ep_get(dev, pid, endp);
-            if (ep->type == USB_ENDPOINT_XFER_ISOC) {
+            if (ep && ep->type == USB_ENDPOINT_XFER_ISOC) {
                 usb_packet_setup(&ehci->ipacket, pid, ep, addr);
                 usb_packet_map(&ehci->ipacket, &ehci->isgl);
                 ret = usb_handle_packet(dev, &ehci->ipacket);
-- 
1.7.1

[Qemu-devel] [PATCH for-1.2 05/11] ehci: Schedule async-bh when IAAD bit gets set

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

After the "ehci: Print a warning when a queue unexpectedly contains packets
on cancel" commit. Under certain reproducable conditions I was getting the
following message: "EHCI: Warning queue not empty on queue reset".

After aprox. 8 hours of debugging I've finally found the cause. The Linux EHCI
driver has an IAAD watchdog, to work around certain EHCI hardware sometimes
not acknowledging the doorbell at all. This watchdog has a timeout of 10 ms,
which is less then the time between 2 runs through the async schedule when
async_stepdown is at its highest value.

Thus the watchdog can trigger, after which Linux clears the IAAD bit and
re-uses the QH. IOW we were not properly detecting the unlink of the qh, due
to us missing (ignoring for more then 10 ms) the IAAD command, which triggered
the warning.

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

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index a3aea6d..3e10977 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1194,6 +1194,15 @@ static void ehci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
             val &= ~USBCMD_FLS;
         }
 
+        if (val & USBCMD_IAAD) {
+            /*
+             * Process IAAD immediately, otherwise the Linux IAAD watchdog may
+             * trigger and re-use a qh without us seeing the unlink.
+             */
+            s->async_stepdown = 0;
+            qemu_bh_schedule(s->async_bh);
+        }
+
         if (((USBCMD_RUNSTOP | USBCMD_PSE | USBCMD_ASE) & val) !=
             ((USBCMD_RUNSTOP | USBCMD_PSE | USBCMD_ASE) & s->usbcmd)) {
             if (s->pstate == EST_INACTIVE) {
-- 
1.7.1

[Qemu-devel] [PATCH for-1.2 06/11] ehci: Remove unnecessary ehci_flush_qh call

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

ehci_qh_do_overlay() already calls ehci_flush_qh() before it returns, calling
it twice is useless.

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

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 3e10977..923a949 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1966,7 +1966,6 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
     }
     if (p != NULL) {
         ehci_qh_do_overlay(q);
-        ehci_flush_qh(q);
         if (p->async == EHCI_ASYNC_INFLIGHT) {
             ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
         } else {
-- 
1.7.1

[Qemu-devel] [PATCH for-1.2 07/11] ehci: simplify ehci_state_executing

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

ehci_state_executing does not need to check for p->usb_status == USB_RET_ASYNC
or USB_RET_PROCERR, since ehci_execute_complete already does a similar check
and will trigger an assert if either value is encountered.

USB_RET_ASYNC should never be the packet status when execute_complete runs
for obvious reasons, and USB_RET_PROCERR is only used by ehci_state_execute /
ehci_execute not by ehci_state_executing / ehci_execute_complete.

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

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 923a949..b6169ce 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2084,19 +2084,11 @@ out:
 static int ehci_state_executing(EHCIQueue *q)
 {
     EHCIPacket *p = QTAILQ_FIRST(&q->packets);
-    int again = 0;
 
     assert(p != NULL);
     assert(p->qtdaddr == q->qtdaddr);
 
     ehci_execute_complete(q);
-    if (p->usb_status == USB_RET_ASYNC) {
-        goto out;
-    }
-    if (p->usb_status == USB_RET_PROCERR) {
-        again = -1;
-        goto out;
-    }
 
     // 4.10.3
     if (!q->async) {
@@ -2114,11 +2106,8 @@ static int ehci_state_executing(EHCIQueue *q)
         ehci_set_state(q->ehci, q->async, EST_WRITEBACK);
     }
 
-    again = 1;
-
-out:
     ehci_flush_qh(q);
-    return again;
+    return 1;
 }
 
 
-- 
1.7.1

[Qemu-devel] [PATCH for-1.2 08/11] ehci: add ehci_cancel_queue()

[Gerd Hoffmann]

Factor out function to cancel all packets of a queue.
No behavior change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ehci.c |   30 ++++++++++++++++++++----------
 1 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index b6169ce..30e5e8f 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -766,15 +766,27 @@ static EHCIQueue *ehci_alloc_queue(EHCIState *ehci, uint32_t addr, int async)
     return q;
 }
 
+static void ehci_cancel_queue(EHCIQueue *q)
+{
+    EHCIPacket *p;
+
+    p = QTAILQ_FIRST(&q->packets);
+    if (p == NULL) {
+        return;
+    }
+
+    trace_usb_ehci_queue_action(q, "cancel");
+    do {
+        ehci_free_packet(p);
+    } while ((p = QTAILQ_FIRST(&q->packets)) != NULL);
+}
+
 static void ehci_free_queue(EHCIQueue *q)
 {
     EHCIQueueHead *head = q->async ? &q->ehci->aqueues : &q->ehci->pqueues;
-    EHCIPacket *p;
 
     trace_usb_ehci_queue_action(q, "free");
-    while ((p = QTAILQ_FIRST(&q->packets)) != NULL) {
-        ehci_free_packet(p);
-    }
+    ehci_cancel_queue(q);
     QTAILQ_REMOVE(head, q, next);
     g_free(q);
 }
@@ -1796,9 +1808,7 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     if (q->dev != NULL && q->dev->addr != devaddr) {
         if (!QTAILQ_EMPTY(&q->packets)) {
             /* should not happen (guest bug) */
-            while ((p = QTAILQ_FIRST(&q->packets)) != NULL) {
-                ehci_free_packet(p);
-            }
+            ehci_cancel_queue(q);
         }
         q->dev = NULL;
     }
@@ -1959,10 +1969,10 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
     ehci_trace_qtd(q, NLPTR_GET(q->qtdaddr), &qtd);
 
     p = QTAILQ_FIRST(&q->packets);
-    while (p != NULL && p->qtdaddr != q->qtdaddr) {
+    if (p != NULL && p->qtdaddr != q->qtdaddr) {
         /* should not happen (guest bug) */
-        ehci_free_packet(p);
-        p = QTAILQ_FIRST(&q->packets);
+        ehci_cancel_queue(q);
+        p = NULL;
     }
     if (p != NULL) {
         ehci_qh_do_overlay(q);
-- 
1.7.1

[Qemu-devel] [PATCH for-1.2 09/11] ehci: handle TD deactivation of inflight packets

[Gerd Hoffmann]

Check the TDs of inflight packets, cancel
packets in case the guest clears the active bit.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ehci.c |   38 +++++++++++++++++++++++---------------
 1 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 30e5e8f..eca1431 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1816,11 +1816,6 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
         q->dev = ehci_find_device(q->ehci, devaddr);
     }
 
-    if (p && p->async == EHCI_ASYNC_INFLIGHT) {
-        /* I/O still in progress -- skip queue */
-        ehci_set_state(ehci, async, EST_HORIZONTALQH);
-        goto out;
-    }
     if (p && p->async == EHCI_ASYNC_FINISHED) {
         /* I/O finished -- continue processing queue */
         trace_usb_ehci_packet_action(p->queue, p, "complete");
@@ -1969,28 +1964,41 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
     ehci_trace_qtd(q, NLPTR_GET(q->qtdaddr), &qtd);
 
     p = QTAILQ_FIRST(&q->packets);
-    if (p != NULL && p->qtdaddr != q->qtdaddr) {
-        /* should not happen (guest bug) */
-        ehci_cancel_queue(q);
-        p = NULL;
-    }
     if (p != NULL) {
-        ehci_qh_do_overlay(q);
+        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]) {
+            /* guest bug: guest updated active QH or qTD underneath us */
+            ehci_cancel_queue(q);
+            p = NULL;
+        } else {
+            p->qtd = qtd;
+            ehci_qh_do_overlay(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);
+        again = 1;
+    } else if (p != NULL) {
         if (p->async == EHCI_ASYNC_INFLIGHT) {
             ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
         } else {
             ehci_set_state(q->ehci, q->async, EST_EXECUTING);
         }
         again = 1;
-    } else if (qtd.token & QTD_TOKEN_ACTIVE) {
+    } else {
         p = ehci_alloc_packet(q);
         p->qtdaddr = q->qtdaddr;
         p->qtd = qtd;
         ehci_set_state(q->ehci, q->async, EST_EXECUTE);
         again = 1;
-    } else {
-        ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
-        again = 1;
     }
 
     return again;
-- 
1.7.1

[Qemu-devel] [PATCH for-1.2 10/11] ehci: Fix interrupt endpoints no longer working

[Gerd Hoffmann]

From: Hans de Goede <hdegoede@redhat.com>

One of the recent changes (likely the addition of queuing support) has broken
interrupt endpoints, this patch fixes this.

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

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index eca1431..017342b 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1987,10 +1987,19 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
         ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
         again = 1;
     } else if (p != NULL) {
-        if (p->async == EHCI_ASYNC_INFLIGHT) {
+        switch (p->async) {
+        case EHCI_ASYNC_NONE:
+            /* Previously nacked packet (likely interrupt ep) */
+           ehci_set_state(q->ehci, q->async, EST_EXECUTE);
+           break;
+        case EHCI_ASYNC_INFLIGHT:
+            /* Unfinyshed async handled packet, go horizontal */
             ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
-        } else {
+            break;
+        case EHCI_ASYNC_FINISHED:
+            /* Should never happen, as this case is caught by fetchqh */
             ehci_set_state(q->ehci, q->async, EST_EXECUTING);
+            break;
         }
         again = 1;
     } else {
-- 
1.7.1

[Qemu-devel] [PATCH for-1.2 11/11] uas: move transfer kickoff

[Gerd Hoffmann]

Kick next scsi transfer from request release callback instead of command
completion callback, otherwise we might get stuck in case scsi_req_unref()
doesn't release the request instantly due to someone else holding a
reference too.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-uas.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index b13eeba..5a0057a 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -424,6 +424,7 @@ static void usb_uas_scsi_free_request(SCSIBus *bus, void *priv)
     }
     QTAILQ_REMOVE(&uas->requests, req, next);
     g_free(req);
+    usb_uas_start_next_transfer(uas);
 }
 
 static UASRequest *usb_uas_find_request(UASDevice *uas, uint16_t tag)
@@ -456,7 +457,6 @@ static void usb_uas_scsi_command_complete(SCSIRequest *r,
                                           uint32_t status, size_t resid)
 {
     UASRequest *req = r->hba_private;
-    UASDevice *uas = req->uas;
 
     trace_usb_uas_scsi_complete(req->uas->dev.addr, req->tag, status, resid);
     req->complete = true;
@@ -465,7 +465,6 @@ static void usb_uas_scsi_command_complete(SCSIRequest *r,
     }
     usb_uas_queue_sense(req, status);
     scsi_req_unref(req->req);
-    usb_uas_start_next_transfer(uas);
 }
 
 static void usb_uas_scsi_request_cancelled(SCSIRequest *r)
-- 
1.7.1

Re: [Qemu-devel] [PATCH for-1.2 02/11] usb: Halt ep queue en cancel pending packets on a packet error

[Blue Swirl]

On Fri, Aug 31, 2012 at 2:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> From: Hans de Goede <hdegoede@redhat.com>
>
> For controllers which queue up more then 1 packet at a time, we must halt the
> ep queue, and inside the controller code cancel all pending packets on an
> error.
>
> There are multiple reasons for this:
> 1) Guests expect the controllers to halt ep queues on error, so that they
> get the opportunity to cancel transfers which the scheduled after the failing
> one, before processing continues
>
> 2) Not cancelling queued up packets after a failed transfer also messes up
> the controller state machine, in the case of EHCI causing the following
> assert to trigger: "assert(p->qtdaddr == q->qtdaddr)" at hcd-ehci.c:2075
>
> 3) For bulk endpoints with pipelining enabled (redirection to a real USB
> device), we must cancel all the transfers after this a failed one so that:
> a) If they've completed already, they are not processed further causing more
>    stalls to be reported, originating from the same failed transfer
> b) If still in flight, they are cancelled before the guest does
>    a clear stall, otherwise the guest and device can loose sync!
>
> Note this patch only touches the ehci and uhci controller changes, since AFAIK
> no other controllers actually queue up multiple transfer. If I'm wrong on this
> other controllers need to be updated too!
>
> Also note that this patch was heavily tested with the ehci code, where I had
> a reproducer for a device causing a transfer to fail. The uhci code is not
> tested with actually failing transfers and could do with a thorough review!
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb.h          |    1 +
>  hw/usb/core.c     |   35 ++++++++++++++++++++++++++++-------
>  hw/usb/hcd-ehci.c |   13 +++++++++++++
>  hw/usb/hcd-uhci.c |   16 ++++++++++++++++
>  4 files changed, 58 insertions(+), 7 deletions(-)
>
> diff --git a/hw/usb.h b/hw/usb.h
> index 432ccae..e574477 100644
> --- a/hw/usb.h
> +++ b/hw/usb.h
> @@ -179,6 +179,7 @@ struct USBEndpoint {
>      uint8_t ifnum;
>      int max_packet_size;
>      bool pipeline;
> +    bool halted;
>      USBDevice *dev;
>      QTAILQ_HEAD(, USBPacket) queue;
>  };
> diff --git a/hw/usb/core.c b/hw/usb/core.c
> index c7e5bc0..28b840e 100644
> --- a/hw/usb/core.c
> +++ b/hw/usb/core.c
> @@ -382,12 +382,23 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
>      usb_packet_check_state(p, USB_PACKET_SETUP);
>      assert(p->ep != NULL);
>
> +    /* Submitting a new packet clears halt */
> +    if (p->ep->halted) {
> +        assert(QTAILQ_EMPTY(&p->ep->queue));
> +        p->ep->halted = false;
> +    }
> +
>      if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
>          ret = usb_process_one(p);
>          if (ret == USB_RET_ASYNC) {
>              usb_packet_set_state(p, USB_PACKET_ASYNC);
>              QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
>          } else {
> +            /*
> +             * When pipelining is enabled usb-devices must always return async,
> +             * otherwise packets can complete out of order!
> +             */
> +            assert(!p->ep->pipeline);
>              p->result = ret;
>              usb_packet_set_state(p, USB_PACKET_COMPLETE);
>          }
> @@ -399,6 +410,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
>      return ret;
>  }
>
> +static void __usb_packet_complete(USBDevice *dev, USBPacket *p)

Please check reserved namespaces in HACKING.

> +{
> +    USBEndpoint *ep = p->ep;
> +
> +    assert(p->result != USB_RET_ASYNC && p->result != USB_RET_NAK);
> +
> +    if (p->result < 0) {
> +        ep->halted = true;
> +    }
> +    usb_packet_set_state(p, USB_PACKET_COMPLETE);
> +    QTAILQ_REMOVE(&ep->queue, p, queue);
> +    dev->port->ops->complete(dev->port, p);
> +}
> +
>  /* Notify the controller that an async packet is complete.  This should only
>     be called for packets previously deferred by returning USB_RET_ASYNC from
>     handle_packet. */
> @@ -409,11 +434,9 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
>
>      usb_packet_check_state(p, USB_PACKET_ASYNC);
>      assert(QTAILQ_FIRST(&ep->queue) == p);
> -    usb_packet_set_state(p, USB_PACKET_COMPLETE);
> -    QTAILQ_REMOVE(&ep->queue, p, queue);
> -    dev->port->ops->complete(dev->port, p);
> +    __usb_packet_complete(dev, p);
>
> -    while (!QTAILQ_EMPTY(&ep->queue)) {
> +    while (!ep->halted && !QTAILQ_EMPTY(&ep->queue)) {
>          p = QTAILQ_FIRST(&ep->queue);
>          if (p->state == USB_PACKET_ASYNC) {
>              break;
> @@ -425,9 +448,7 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
>              break;
>          }
>          p->result = ret;
> -        usb_packet_set_state(p, USB_PACKET_COMPLETE);
> -        QTAILQ_REMOVE(&ep->queue, p, queue);
> -        dev->port->ops->complete(dev->port, p);
> +        __usb_packet_complete(ep->dev, p);
>      }
>  }
>
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 8b94b17..8504a6a 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -2138,6 +2138,19 @@ static int ehci_state_writeback(EHCIQueue *q)
>       * bit is clear.
>       */
>      if (q->qh.token & QTD_TOKEN_HALT) {
> +        /*
> +         * We should not do any further processing on a halted queue!
> +         * This is esp. important for bulk endpoints with pipelining enabled
> +         * (redirection to a real USB device), where we must cancel all the
> +         * transfers after this one so that:
> +         * 1) If they've completed already, they are not processed further
> +         *    causing more stalls, originating from the same failed transfer
> +         * 2) If still in flight, they are cancelled before the guest does
> +         *    a clear stall, otherwise the guest and device can loose sync!
> +         */
> +        while ((p = QTAILQ_FIRST(&q->packets)) != NULL) {
> +            ehci_free_packet(p);
> +        }
>          ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
>          again = 1;
>      } else {
> diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
> index 1ace2a4..8987734 100644
> --- a/hw/usb/hcd-uhci.c
> +++ b/hw/usb/hcd-uhci.c
> @@ -748,6 +748,22 @@ static int uhci_complete_td(UHCIState *s, UHCI_TD *td, UHCIAsync *async, uint32_
>      return TD_RESULT_COMPLETE;
>
>  out:
> +    /*
> +     * We should not do any further processing on a queue with errors!
> +     * This is esp. important for bulk endpoints with pipelining enabled
> +     * (redirection to a real USB device), where we must cancel all the
> +     * transfers after this one so that:
> +     * 1) If they've completed already, they are not processed further
> +     *    causing more stalls, originating from the same failed transfer
> +     * 2) If still in flight, they are cancelled before the guest does
> +     *    a clear stall, otherwise the guest and device can loose sync!
> +     */
> +    while (!QTAILQ_EMPTY(&async->queue->asyncs)) {
> +        UHCIAsync *as = QTAILQ_FIRST(&async->queue->asyncs);
> +        uhci_async_unlink(as);
> +        uhci_async_cancel(as);
> +    }
> +
>      switch(ret) {
>      case USB_RET_STALL:
>          td->ctrl |= TD_CTRL_STALL;
> --
> 1.7.1
>
>

Re: [Qemu-devel] [PATCH for-1.2 02/11] usb: Halt ep queue en cancel pending packets on a packet error

[Hans de Goede]

Hi,

On 09/01/2012 12:42 PM, Blue Swirl wrote:
> On Fri, Aug 31, 2012 at 2:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> From: Hans de Goede <hdegoede@redhat.com>
>>
>> For controllers which queue up more then 1 packet at a time, we must halt the
>> ep queue, and inside the controller code cancel all pending packets on an
>> error.
>>
>> There are multiple reasons for this:
>> 1) Guests expect the controllers to halt ep queues on error, so that they
>> get the opportunity to cancel transfers which the scheduled after the failing
>> one, before processing continues
>>
>> 2) Not cancelling queued up packets after a failed transfer also messes up
>> the controller state machine, in the case of EHCI causing the following
>> assert to trigger: "assert(p->qtdaddr == q->qtdaddr)" at hcd-ehci.c:2075
>>
>> 3) For bulk endpoints with pipelining enabled (redirection to a real USB
>> device), we must cancel all the transfers after this a failed one so that:
>> a) If they've completed already, they are not processed further causing more
>>     stalls to be reported, originating from the same failed transfer
>> b) If still in flight, they are cancelled before the guest does
>>     a clear stall, otherwise the guest and device can loose sync!
>>
>> Note this patch only touches the ehci and uhci controller changes, since AFAIK
>> no other controllers actually queue up multiple transfer. If I'm wrong on this
>> other controllers need to be updated too!
>>
>> Also note that this patch was heavily tested with the ehci code, where I had
>> a reproducer for a device causing a transfer to fail. The uhci code is not
>> tested with actually failing transfers and could do with a thorough review!
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>   hw/usb.h          |    1 +
>>   hw/usb/core.c     |   35 ++++++++++++++++++++++++++++-------
>>   hw/usb/hcd-ehci.c |   13 +++++++++++++
>>   hw/usb/hcd-uhci.c |   16 ++++++++++++++++
>>   4 files changed, 58 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/usb.h b/hw/usb.h
>> index 432ccae..e574477 100644
>> --- a/hw/usb.h
>> +++ b/hw/usb.h
>> @@ -179,6 +179,7 @@ struct USBEndpoint {
>>       uint8_t ifnum;
>>       int max_packet_size;
>>       bool pipeline;
>> +    bool halted;
>>       USBDevice *dev;
>>       QTAILQ_HEAD(, USBPacket) queue;
>>   };
>> diff --git a/hw/usb/core.c b/hw/usb/core.c
>> index c7e5bc0..28b840e 100644
>> --- a/hw/usb/core.c
>> +++ b/hw/usb/core.c
>> @@ -382,12 +382,23 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
>>       usb_packet_check_state(p, USB_PACKET_SETUP);
>>       assert(p->ep != NULL);
>>
>> +    /* Submitting a new packet clears halt */
>> +    if (p->ep->halted) {
>> +        assert(QTAILQ_EMPTY(&p->ep->queue));
>> +        p->ep->halted = false;
>> +    }
>> +
>>       if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
>>           ret = usb_process_one(p);
>>           if (ret == USB_RET_ASYNC) {
>>               usb_packet_set_state(p, USB_PACKET_ASYNC);
>>               QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
>>           } else {
>> +            /*
>> +             * When pipelining is enabled usb-devices must always return async,
>> +             * otherwise packets can complete out of order!
>> +             */
>> +            assert(!p->ep->pipeline);
>>               p->result = ret;
>>               usb_packet_set_state(p, USB_PACKET_COMPLETE);
>>           }
>> @@ -399,6 +410,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
>>       return ret;
>>   }
>>
>> +static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
>
> Please check reserved namespaces in HACKING.

That talks about suffixes not prefixes.

Regards,

Hans

Re: [Qemu-devel] [PATCH for-1.2 02/11] usb: Halt ep queue en cancel pending packets on a packet error

[Michael Roth]

On Sat, Sep 01, 2012 at 03:37:03PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/01/2012 12:42 PM, Blue Swirl wrote:
> >On Fri, Aug 31, 2012 at 2:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >>From: Hans de Goede <hdegoede@redhat.com>
> >>
> >>For controllers which queue up more then 1 packet at a time, we must halt the
> >>ep queue, and inside the controller code cancel all pending packets on an
> >>error.
> >>
> >>There are multiple reasons for this:
> >>1) Guests expect the controllers to halt ep queues on error, so that they
> >>get the opportunity to cancel transfers which the scheduled after the failing
> >>one, before processing continues
> >>
> >>2) Not cancelling queued up packets after a failed transfer also messes up
> >>the controller state machine, in the case of EHCI causing the following
> >>assert to trigger: "assert(p->qtdaddr == q->qtdaddr)" at hcd-ehci.c:2075
> >>
> >>3) For bulk endpoints with pipelining enabled (redirection to a real USB
> >>device), we must cancel all the transfers after this a failed one so that:
> >>a) If they've completed already, they are not processed further causing more
> >>    stalls to be reported, originating from the same failed transfer
> >>b) If still in flight, they are cancelled before the guest does
> >>    a clear stall, otherwise the guest and device can loose sync!
> >>
> >>Note this patch only touches the ehci and uhci controller changes, since AFAIK
> >>no other controllers actually queue up multiple transfer. If I'm wrong on this
> >>other controllers need to be updated too!
> >>
> >>Also note that this patch was heavily tested with the ehci code, where I had
> >>a reproducer for a device causing a transfer to fail. The uhci code is not
> >>tested with actually failing transfers and could do with a thorough review!
> >>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >>---
> >>  hw/usb.h          |    1 +
> >>  hw/usb/core.c     |   35 ++++++++++++++++++++++++++++-------
> >>  hw/usb/hcd-ehci.c |   13 +++++++++++++
> >>  hw/usb/hcd-uhci.c |   16 ++++++++++++++++
> >>  4 files changed, 58 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/hw/usb.h b/hw/usb.h
> >>index 432ccae..e574477 100644
> >>--- a/hw/usb.h
> >>+++ b/hw/usb.h
> >>@@ -179,6 +179,7 @@ struct USBEndpoint {
> >>      uint8_t ifnum;
> >>      int max_packet_size;
> >>      bool pipeline;
> >>+    bool halted;
> >>      USBDevice *dev;
> >>      QTAILQ_HEAD(, USBPacket) queue;
> >>  };
> >>diff --git a/hw/usb/core.c b/hw/usb/core.c
> >>index c7e5bc0..28b840e 100644
> >>--- a/hw/usb/core.c
> >>+++ b/hw/usb/core.c
> >>@@ -382,12 +382,23 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
> >>      usb_packet_check_state(p, USB_PACKET_SETUP);
> >>      assert(p->ep != NULL);
> >>
> >>+    /* Submitting a new packet clears halt */
> >>+    if (p->ep->halted) {
> >>+        assert(QTAILQ_EMPTY(&p->ep->queue));
> >>+        p->ep->halted = false;
> >>+    }
> >>+
> >>      if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
> >>          ret = usb_process_one(p);
> >>          if (ret == USB_RET_ASYNC) {
> >>              usb_packet_set_state(p, USB_PACKET_ASYNC);
> >>              QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
> >>          } else {
> >>+            /*
> >>+             * When pipelining is enabled usb-devices must always return async,
> >>+             * otherwise packets can complete out of order!
> >>+             */
> >>+            assert(!p->ep->pipeline);
> >>              p->result = ret;
> >>              usb_packet_set_state(p, USB_PACKET_COMPLETE);
> >>          }
> >>@@ -399,6 +410,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
> >>      return ret;
> >>  }
> >>
> >>+static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
> >
> >Please check reserved namespaces in HACKING.
> 
> That talks about suffixes not prefixes.

I think it's just poorly wordly. At least, recent discussions on the
list assume it's referencing __ prefixes:

http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg04781.html

> 
> Regards,
> 
> Hans
> 

Re: [Qemu-devel] [PATCH for-1.2 02/11] usb: Halt ep queue en cancel pending packets on a packet error

[Hans de Goede]

Hi,

On 09/01/2012 04:12 PM, Michael Roth wrote:
> On Sat, Sep 01, 2012 at 03:37:03PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 09/01/2012 12:42 PM, Blue Swirl wrote:
>>> On Fri, Aug 31, 2012 at 2:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>> From: Hans de Goede <hdegoede@redhat.com>
>>>>
>>>> For controllers which queue up more then 1 packet at a time, we must halt the
>>>> ep queue, and inside the controller code cancel all pending packets on an
>>>> error.
>>>>
>>>> There are multiple reasons for this:
>>>> 1) Guests expect the controllers to halt ep queues on error, so that they
>>>> get the opportunity to cancel transfers which the scheduled after the failing
>>>> one, before processing continues
>>>>
>>>> 2) Not cancelling queued up packets after a failed transfer also messes up
>>>> the controller state machine, in the case of EHCI causing the following
>>>> assert to trigger: "assert(p->qtdaddr == q->qtdaddr)" at hcd-ehci.c:2075
>>>>
>>>> 3) For bulk endpoints with pipelining enabled (redirection to a real USB
>>>> device), we must cancel all the transfers after this a failed one so that:
>>>> a) If they've completed already, they are not processed further causing more
>>>>     stalls to be reported, originating from the same failed transfer
>>>> b) If still in flight, they are cancelled before the guest does
>>>>     a clear stall, otherwise the guest and device can loose sync!
>>>>
>>>> Note this patch only touches the ehci and uhci controller changes, since AFAIK
>>>> no other controllers actually queue up multiple transfer. If I'm wrong on this
>>>> other controllers need to be updated too!
>>>>
>>>> Also note that this patch was heavily tested with the ehci code, where I had
>>>> a reproducer for a device causing a transfer to fail. The uhci code is not
>>>> tested with actually failing transfers and could do with a thorough review!
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>> ---
>>>>   hw/usb.h          |    1 +
>>>>   hw/usb/core.c     |   35 ++++++++++++++++++++++++++++-------
>>>>   hw/usb/hcd-ehci.c |   13 +++++++++++++
>>>>   hw/usb/hcd-uhci.c |   16 ++++++++++++++++
>>>>   4 files changed, 58 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/hw/usb.h b/hw/usb.h
>>>> index 432ccae..e574477 100644
>>>> --- a/hw/usb.h
>>>> +++ b/hw/usb.h
>>>> @@ -179,6 +179,7 @@ struct USBEndpoint {
>>>>       uint8_t ifnum;
>>>>       int max_packet_size;
>>>>       bool pipeline;
>>>> +    bool halted;
>>>>       USBDevice *dev;
>>>>       QTAILQ_HEAD(, USBPacket) queue;
>>>>   };
>>>> diff --git a/hw/usb/core.c b/hw/usb/core.c
>>>> index c7e5bc0..28b840e 100644
>>>> --- a/hw/usb/core.c
>>>> +++ b/hw/usb/core.c
>>>> @@ -382,12 +382,23 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
>>>>       usb_packet_check_state(p, USB_PACKET_SETUP);
>>>>       assert(p->ep != NULL);
>>>>
>>>> +    /* Submitting a new packet clears halt */
>>>> +    if (p->ep->halted) {
>>>> +        assert(QTAILQ_EMPTY(&p->ep->queue));
>>>> +        p->ep->halted = false;
>>>> +    }
>>>> +
>>>>       if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
>>>>           ret = usb_process_one(p);
>>>>           if (ret == USB_RET_ASYNC) {
>>>>               usb_packet_set_state(p, USB_PACKET_ASYNC);
>>>>               QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
>>>>           } else {
>>>> +            /*
>>>> +             * When pipelining is enabled usb-devices must always return async,
>>>> +             * otherwise packets can complete out of order!
>>>> +             */
>>>> +            assert(!p->ep->pipeline);
>>>>               p->result = ret;
>>>>               usb_packet_set_state(p, USB_PACKET_COMPLETE);
>>>>           }
>>>> @@ -399,6 +410,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
>>>>       return ret;
>>>>   }
>>>>
>>>> +static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
>>>
>>> Please check reserved namespaces in HACKING.
>>
>> That talks about suffixes not prefixes.
>
> I think it's just poorly wordly. At least, recent discussions on the
> list assume it's referencing __ prefixes:
>
> http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg04781.html

Ok, so lets change it to a single underscore if people prefer that.

Gerd can you make that change in your tree, or do you want me to
resend the (corrected) patch ?

Regards,

Hans

Re: [Qemu-devel] [PATCH for-1.2 02/11] usb: Halt ep queue en cancel pending packets on a packet error

[Peter Maydell]

On 1 September 2012 19:47, Hans de Goede <hdegoede@redhat.com> wrote:
> Ok, so lets change it to a single underscore if people prefer that.

Why does this function have any kind of starting-with-underscore
name at all? The usual reason for a leading underscore is functions
in header files or macros where you don't want them to clash with
names used by the rest of the program. This is a simple static
helper function in one C file -- there's no reason for it not to
just have a plain name like any other function. Maybe
usb_complete_one_packet() or something.

-- PMM

Re: [Qemu-devel] [PATCH for-1.2 02/11] usb: Halt ep queue en cancel pending packets on a packet error

[Michael Roth]

On Sat, Sep 01, 2012 at 08:47:28PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/01/2012 04:12 PM, Michael Roth wrote:
> >On Sat, Sep 01, 2012 at 03:37:03PM +0200, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 09/01/2012 12:42 PM, Blue Swirl wrote:
> >>>On Fri, Aug 31, 2012 at 2:19 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >>>>From: Hans de Goede <hdegoede@redhat.com>
> >>>>
> >>>>For controllers which queue up more then 1 packet at a time, we must halt the
> >>>>ep queue, and inside the controller code cancel all pending packets on an
> >>>>error.
> >>>>
> >>>>There are multiple reasons for this:
> >>>>1) Guests expect the controllers to halt ep queues on error, so that they
> >>>>get the opportunity to cancel transfers which the scheduled after the failing
> >>>>one, before processing continues
> >>>>
> >>>>2) Not cancelling queued up packets after a failed transfer also messes up
> >>>>the controller state machine, in the case of EHCI causing the following
> >>>>assert to trigger: "assert(p->qtdaddr == q->qtdaddr)" at hcd-ehci.c:2075
> >>>>
> >>>>3) For bulk endpoints with pipelining enabled (redirection to a real USB
> >>>>device), we must cancel all the transfers after this a failed one so that:
> >>>>a) If they've completed already, they are not processed further causing more
> >>>>    stalls to be reported, originating from the same failed transfer
> >>>>b) If still in flight, they are cancelled before the guest does
> >>>>    a clear stall, otherwise the guest and device can loose sync!
> >>>>
> >>>>Note this patch only touches the ehci and uhci controller changes, since AFAIK
> >>>>no other controllers actually queue up multiple transfer. If I'm wrong on this
> >>>>other controllers need to be updated too!
> >>>>
> >>>>Also note that this patch was heavily tested with the ehci code, where I had
> >>>>a reproducer for a device causing a transfer to fail. The uhci code is not
> >>>>tested with actually failing transfers and could do with a thorough review!
> >>>>
> >>>>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >>>>---
> >>>>  hw/usb.h          |    1 +
> >>>>  hw/usb/core.c     |   35 ++++++++++++++++++++++++++++-------
> >>>>  hw/usb/hcd-ehci.c |   13 +++++++++++++
> >>>>  hw/usb/hcd-uhci.c |   16 ++++++++++++++++
> >>>>  4 files changed, 58 insertions(+), 7 deletions(-)
> >>>>
> >>>>diff --git a/hw/usb.h b/hw/usb.h
> >>>>index 432ccae..e574477 100644
> >>>>--- a/hw/usb.h
> >>>>+++ b/hw/usb.h
> >>>>@@ -179,6 +179,7 @@ struct USBEndpoint {
> >>>>      uint8_t ifnum;
> >>>>      int max_packet_size;
> >>>>      bool pipeline;
> >>>>+    bool halted;
> >>>>      USBDevice *dev;
> >>>>      QTAILQ_HEAD(, USBPacket) queue;
> >>>>  };
> >>>>diff --git a/hw/usb/core.c b/hw/usb/core.c
> >>>>index c7e5bc0..28b840e 100644
> >>>>--- a/hw/usb/core.c
> >>>>+++ b/hw/usb/core.c
> >>>>@@ -382,12 +382,23 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
> >>>>      usb_packet_check_state(p, USB_PACKET_SETUP);
> >>>>      assert(p->ep != NULL);
> >>>>
> >>>>+    /* Submitting a new packet clears halt */
> >>>>+    if (p->ep->halted) {
> >>>>+        assert(QTAILQ_EMPTY(&p->ep->queue));
> >>>>+        p->ep->halted = false;
> >>>>+    }
> >>>>+
> >>>>      if (QTAILQ_EMPTY(&p->ep->queue) || p->ep->pipeline) {
> >>>>          ret = usb_process_one(p);
> >>>>          if (ret == USB_RET_ASYNC) {
> >>>>              usb_packet_set_state(p, USB_PACKET_ASYNC);
> >>>>              QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);
> >>>>          } else {
> >>>>+            /*
> >>>>+             * When pipelining is enabled usb-devices must always return async,
> >>>>+             * otherwise packets can complete out of order!
> >>>>+             */
> >>>>+            assert(!p->ep->pipeline);
> >>>>              p->result = ret;
> >>>>              usb_packet_set_state(p, USB_PACKET_COMPLETE);
> >>>>          }
> >>>>@@ -399,6 +410,20 @@ int usb_handle_packet(USBDevice *dev, USBPacket *p)
> >>>>      return ret;
> >>>>  }
> >>>>
> >>>>+static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
> >>>
> >>>Please check reserved namespaces in HACKING.
> >>
> >>That talks about suffixes not prefixes.
> >
> >I think it's just poorly wordly. At least, recent discussions on the
> >list assume it's referencing __ prefixes:
> >
> >http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg04781.html
> 
> Ok, so lets change it to a single underscore if people prefer that.
> 
> Gerd can you make that change in your tree, or do you want me to
> resend the (corrected) patch ?

Looks like it's in master already. Can probably fix it up in a seperate
patch for 1.3 if it's not causing any issues currently.

> 
> Regards,
> 
> Hans
>