[Qemu-devel] 2 issues with qemu-master / 1.2 ehci code

[Qemu-devel] 2 issues with qemu-master / 1.2 ehci code

[Hans de Goede]

Hi,

While testing qemu-master I've encountered 2 problems caused by the ehci changes
made since 1.1:

1) Newly plugged in devices don't get recognized by the guest
This seems to be a case of no interrupt getting generated while it should, doing lsusb in a linux
guest makes the device show up (after the lsusb, so its there in a second lsusb run)

2) I'm hitting:
qemu-system-x86_64: /home/hans/projects/qemu/hw/usb/hcd-ehci.c:2075: ehci_state_executing: Assertion `p->qtdaddr == q->qtdaddr' failed.
When trying to redirect a microsoft lifecam, since this is a device with multiple interfaces
(both uvc and usb-audio) I think it may be a case of multiple control requests getting submitted at the same time,
but that is just a wild guess.

Some gdb output:

(gdb) fr 4
#4  0x00005555556c33ce in ehci_state_executing (q=0x5555566c9590)
     at /home/hans/projects/qemu/hw/usb/hcd-ehci.c:2075
2075        assert(p->qtdaddr == q->qtdaddr);
(gdb) p q
$1 = (EHCIQueue *) 0x5555566c9590
(gdb) p *q
$2 = {ehci = 0x5555566e58b0, next = {tqe_next = 0x5555566c23e0, tqe_prev =
     0x5555566e7188}, seen = 1, ts = 500707440673, async = 1, revalidate = 0,
   qh = {next = 915959810, epchar = 1077960706, epcap = 1073741824,
     current_qtd = 915964192, next_qtd = 915964384, altnext_qtd = 1, token =
     2147483720, bufptr = {865509240, 0, 0, 0, 0}}, qhaddr = 915959906,
   qtdaddr = 915964192, dev = 0x555556710e10, packets = {tqh_first =
     0x55555676a440, tqh_last = 0x55555676aca8}}
(gdb) p *p
$3 = {queue = 0x5555566c9590, next = {tqe_next = 0x55555676aca0, tqe_prev =
     0x5555566c9600}, qtd = {next = 915964288, altnext = 1, token = 2147683712,
     bufptr = {865509232, 0, 0, 0, 0}}, qtdaddr = 915964480, packet = {pid =
     105, ep = 0x555556711f08, iov = {iov = 0x55555676a960, niov = 1, nalloc =
     1, size = 3}, parameter = 0, result = -3, state = USB_PACKET_COMPLETE,
     queue = {tqe_next = 0x55555676ace0, tqe_prev = 0x555556711f20}}, sgl = {
     sg = 0x555556769940, nsg = 1, nalloc = 5, size = 3, dma = 0x0}, pid = 105,
   tbytes = 3, async = EHCI_ASYNC_FINISHED, usb_status = -3}

Let me know what else you need to debug this.

Regards,

Hans

Re: [Qemu-devel] 2 issues with qemu-master / 1.2 ehci code

[Hans de Goede]

Hi,

On 08/14/2012 06:12 PM, Hans de Goede wrote:
> Hi,
>
> While testing qemu-master I've encountered 2 problems caused by the ehci changes
> made since 1.1:
>
> 1) Newly plugged in devices don't get recognized by the guest
> This seems to be a case of no interrupt getting generated while it should, doing lsusb in a linux
> guest makes the device show up (after the lsusb, so its there in a second lsusb run)

I've been looking into this one and I think I know what is going on, the problem is caused by
the "ehci: implement Interrupt Threshold Control support" patch:
http://www.kraxel.org/cgit/qemu/commit/?h=usb.57&id=7efc17af9a08839a05771541959696875e06cf99

What happens is that since neither the async, nor the periodic schedule are enabled the
frame-timer is not running, and during the attaching of the device the Port Change Events
(PCD) status bit should get raised multiple times due to port resets, etc. But after the
first call to commit_irq, usbsts_frindex contains frindex + 1 (in case of a linux guest),
so commit_irq turns into a nop and the guest never sees any of the PCD interrupts other then
the first.

Part of the problem is that the Interrupt Threshold Control support patch delays all
interrupts, which it should not, according to the EHCI spec section 4.15 (ehci-r10.pdf
page 115), the following irqs should not be delayed: Port Change Events (PCD),
Frame List Rollover (FLR), and Host System Error (HSE). So we need to change the code
to not delay these. Once that is done I expect the attach problem I've been seeing
to magically go away.

I'll go work on a patch for this after lunch.

> 2) I'm hitting:
> qemu-system-x86_64: /home/hans/projects/qemu/hw/usb/hcd-ehci.c:2075: ehci_state_executing: Assertion `p->qtdaddr == q->qtdaddr' failed.
> When trying to redirect a microsoft lifecam, since this is a device with multiple interfaces
> (both uvc and usb-audio) I think it may be a case of multiple control requests getting submitted at the same time,
> but that is just a wild guess.
>

I've not made any progress with this one yet. kraxel, any chance you could join #spice
on gimpnet so we can discuss this one interactively ?

(note I'm of to lunch for an hour first)

Regards,

Hans

Re: [Qemu-devel] 2 issues with qemu-master / 1.2 ehci code

[Gerd Hoffmann]

On 08/15/12 13:22, Hans de Goede wrote:
> Hi,
> 
> On 08/14/2012 06:12 PM, Hans de Goede wrote:
>> Hi,
>>
>> While testing qemu-master I've encountered 2 problems caused by the
>> ehci changes
>> made since 1.1:
>>
>> 1) Newly plugged in devices don't get recognized by the guest
>> This seems to be a case of no interrupt getting generated while it
>> should, doing lsusb in a linux
>> guest makes the device show up (after the lsusb, so its there in a
>> second lsusb run)
> 
> I've been looking into this one and I think I know what is going on, the
> problem is caused by
> the "ehci: implement Interrupt Threshold Control support" patch:
> http://www.kraxel.org/cgit/qemu/commit/?h=usb.57&id=7efc17af9a08839a05771541959696875e06cf99
> 
> 
> What happens is that since neither the async, nor the periodic schedule
> are enabled the
> frame-timer is not running, and during the attaching of the device the
> Port Change Events
> (PCD) status bit should get raised multiple times due to port resets,
> etc. But after the
> first call to commit_irq, usbsts_frindex contains frindex + 1 (in case
> of a linux guest),
> so commit_irq turns into a nop and the guest never sees any of the PCD
> interrupts other then
> the first.
> 
> Part of the problem is that the Interrupt Threshold Control support
> patch delays all
> interrupts, which it should not, according to the EHCI spec section 4.15
> (ehci-r10.pdf
> page 115), the following irqs should not be delayed: Port Change Events
> (PCD),
> Frame List Rollover (FLR), and Host System Error (HSE). So we need to
> change the code
> to not delay these. Once that is done I expect the attach problem I've
> been seeing
> to magically go away.

Makes sense.

>> 2) I'm hitting:
>> qemu-system-x86_64: /home/hans/projects/qemu/hw/usb/hcd-ehci.c:2075:
>> ehci_state_executing: Assertion `p->qtdaddr == q->qtdaddr' failed.
>> When trying to redirect a microsoft lifecam, since this is a device
>> with multiple interfaces
>> (both uvc and usb-audio) I think it may be a case of multiple control
>> requests getting submitted at the same time,
>> but that is just a wild guess.

Could be QH revalidation not being careful enougth (see commit
9bc3a3a216e2689bfcdd36c3e079333bbdbf3ba0)

> I've not made any progress with this one yet. kraxel, any chance you
> could join #spice
> on gimpnet so we can discuss this one interactively ?

Busy debugging ehci live migration issues and I'd prefer to finish that
first.

cheers,
  Gerd

Re: [Qemu-devel] 2 issues with qemu-master / 1.2 ehci code

[Hans de Goede]

Hi,

On 08/14/2012 06:12 PM, Hans de Goede wrote:
> Hi,
>

<snip>

> 2) I'm hitting:
> qemu-system-x86_64: /home/hans/projects/qemu/hw/usb/hcd-ehci.c:2075: ehci_state_executing: Assertion `p->qtdaddr == q->qtdaddr' failed.
> When trying to redirect a microsoft lifecam, since this is a device with multiple interfaces
> (both uvc and usb-audio) I think it may be a case of multiple control requests getting submitted at the same time,
> but that is just a wild guess.
>
> Some gdb output:
>
> (gdb) fr 4
> #4  0x00005555556c33ce in ehci_state_executing (q=0x5555566c9590)
>      at /home/hans/projects/qemu/hw/usb/hcd-ehci.c:2075
> 2075        assert(p->qtdaddr == q->qtdaddr);
> (gdb) p q
> $1 = (EHCIQueue *) 0x5555566c9590
> (gdb) p *q
> $2 = {ehci = 0x5555566e58b0, next = {tqe_next = 0x5555566c23e0, tqe_prev =
>      0x5555566e7188}, seen = 1, ts = 500707440673, async = 1, revalidate = 0,
>    qh = {next = 915959810, epchar = 1077960706, epcap = 1073741824,
>      current_qtd = 915964192, next_qtd = 915964384, altnext_qtd = 1, token =
>      2147483720, bufptr = {865509240, 0, 0, 0, 0}}, qhaddr = 915959906,
>    qtdaddr = 915964192, dev = 0x555556710e10, packets = {tqh_first =
>      0x55555676a440, tqh_last = 0x55555676aca8}}
> (gdb) p *p
> $3 = {queue = 0x5555566c9590, next = {tqe_next = 0x55555676aca0, tqe_prev =
>      0x5555566c9600}, qtd = {next = 915964288, altnext = 1, token = 2147683712,
>      bufptr = {865509232, 0, 0, 0, 0}}, qtdaddr = 915964480, packet = {pid =
>      105, ep = 0x555556711f08, iov = {iov = 0x55555676a960, niov = 1, nalloc =
>      1, size = 3}, parameter = 0, result = -3, state = USB_PACKET_COMPLETE,
>      queue = {tqe_next = 0x55555676ace0, tqe_prev = 0x555556711f20}}, sgl = {
>      sg = 0x555556769940, nsg = 1, nalloc = 5, size = 3, dma = 0x0}, pid = 105,
>    tbytes = 3, async = EHCI_ASYNC_FINISHED, usb_status = -3}

Ok, I've managed to significantly narrow this down, this is caused by
ehci_fill_queue() adding packets to the queue with different qtdaddr values
then the first one already queued up, this is of course more or less fully
expected, but does trigger the assert in question ...

I can get things working by turning ehci_fill_queue() into a nop... Investigating
this further. But if you've any insights they would be appreciated. I'm thinking
this may be caused by packets completing out of order, which could be caused
by the special handling of some ctrl commands ...

Regards,

Hans

Re: [Qemu-devel] 2 issues with qemu-master / 1.2 ehci code

[Gerd Hoffmann]

  Hi,

> Ok, I've managed to significantly narrow this down, this is caused
> by ehci_fill_queue() adding packets to the queue with different
> qtdaddr values then the first one already queued up, this is of
> course more or less fully expected,

Yes.

> but does trigger the assert in question ...

Hmm.  ehci_fill_queue() should neither change EHCIQueue->qtdaddr nor
EHCIPacket->qtdaddr of the first packet queued up.

Also processing the transfer descriptors later on should happen in the
same order the EHCIPacket's are queued up, so there should be no
mismatch either.  I can see how a buggy guest could trigger this
assert() by mucking with the TD queue after setting the active flag on
the TDs, but I think it is unlikely to be the cause as this behavior
violates the ehci specs.

> I can get things working by turning ehci_fill_queue() into a nop... 
> Investigating this further. But if you've any insights they would be
> appreciated. I'm thinking this may be caused by packets completing
> out of order, which could be caused by the special handling of some
> ctrl commands ...

usbredir wouldn't see multiple parallel inflight requests per endpoint
unless you explicitly enable this using usb_ep_set_pipeline().  Doing
that on bulk endpoints should give you a nice performance boost for usb
sticks.  Doing it on the control endpoint is asking for trouble ;)

Can you turn on all ehci tracepoints & send me a log?

cheers,
  Gerd

Re: [Qemu-devel] 2 issues with qemu-master / 1.2 ehci code

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 1881 bytes --]

Hi,

On 08/16/2012 09:26 PM, Gerd Hoffmann wrote:

<snip>

>> I can get things working by turning ehci_fill_queue() into a nop...
>> Investigating this further. But if you've any insights they would be
>> appreciated. I'm thinking this may be caused by packets completing
>> out of order, which could be caused by the special handling of some
>> ctrl commands ...
>
> usbredir wouldn't see multiple parallel inflight requests per endpoint
> unless you explicitly enable this using usb_ep_set_pipeline().  Doing
> that on bulk endpoints should give you a nice performance boost for usb
> sticks.  Doing it on the control endpoint is asking for trouble ;)
>
> Can you turn on all ehci tracepoints & send me a log?

No need for that, I've been debugging this almost the entire day yesterday
and I've significantly narrowed it down further, the problem is that
the assert triggers when:
1) There are more packets then 1 in the queue (iow fill_queue did something)
2) A packet completion is not followed by an advqueue call

2) happens when a packet fails, and the queue should be halted, in this case
ehci_state_writeback() correctly does not call advqueue, bot does a horizqh
instead. The problem is that once this happens p->qtdaddr == q->qtdaddr is no
longer true ...

I've already come up with multiple approaches to try and fix this, but none
work sofar :|


Another problem with failing packets is that hw/usb/core.c will happily execute
the next packet in the ep queue, even though the spec says the ep-queue should
be halted, giving the guest a chance to cancel transfers after the failed one
without them ever executing. I've a poc patch fixing this too.

I've attached a wip patch with my work on this so-far. Note that currently
the halted bit at the ehci level never gets cleared (since we've queues at 2
levels, we need to track halting at 2 levels too)

Regards,

Hans



[-- Attachment #2: wip --]
[-- Type: text/plain, Size: 9887 bytes --]

diff --git a/hw/usb.h b/hw/usb.h
index 432ccae..6fa3088 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;
 };
@@ -375,6 +376,8 @@ void usb_ep_set_max_packet_size(USBDevice *dev, int pid, int ep,
                                 uint16_t raw);
 int usb_ep_get_max_packet_size(USBDevice *dev, int pid, int ep);
 void usb_ep_set_pipeline(USBDevice *dev, int pid, int ep, bool enabled);
+void usb_ep_process_queue(USBEndpoint *ep);
+void usb_ep_clear_halt(USBEndpoint *ep);
 
 void usb_attach(USBPort *port);
 void usb_detach(USBPort *port);
diff --git a/hw/usb/core.c b/hw/usb/core.c
index c7e5bc0..c9ad57e 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -382,12 +382,20 @@ 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 */
+    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);
         }
@@ -402,33 +410,26 @@ int usb_handle_packet(USBDevice *dev, USBPacket *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. */
-void usb_packet_complete(USBDevice *dev, USBPacket *p)
+static void __usb_packet_complete(USBDevice *dev, USBPacket *p)
 {
     USBEndpoint *ep = p->ep;
-    int ret;
 
-    usb_packet_check_state(p, USB_PACKET_ASYNC);
-    assert(QTAILQ_FIRST(&ep->queue) == p);
+    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);
+}
 
-    while (!QTAILQ_EMPTY(&ep->queue)) {
-        p = QTAILQ_FIRST(&ep->queue);
-        if (p->state == USB_PACKET_ASYNC) {
-            break;
-        }
-        usb_packet_check_state(p, USB_PACKET_QUEUED);
-        ret = usb_process_one(p);
-        if (ret == USB_RET_ASYNC) {
-            usb_packet_set_state(p, USB_PACKET_ASYNC);
-            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);
-    }
+void usb_packet_complete(USBDevice *dev, USBPacket *p)
+{
+    USBEndpoint *ep = p->ep;
+
+    usb_packet_check_state(p, USB_PACKET_ASYNC);
+    assert(QTAILQ_FIRST(&ep->queue) == p);
+    __usb_packet_complete(dev, p);
+    usb_ep_process_queue(ep);
 }
 
 /* Cancel an active packet.  The packed must have been deferred by
@@ -702,3 +703,30 @@ void usb_ep_set_pipeline(USBDevice *dev, int pid, int ep, bool enabled)
     struct USBEndpoint *uep = usb_ep_get(dev, pid, ep);
     uep->pipeline = enabled;
 }
+
+void usb_ep_process_queue(USBEndpoint *ep)
+{
+    USBPacket *p;
+    int ret;
+
+    while (!ep->halted && !QTAILQ_EMPTY(&ep->queue)) {
+        p = QTAILQ_FIRST(&ep->queue);
+        if (p->state == USB_PACKET_ASYNC) {
+            break;
+        }
+        usb_packet_check_state(p, USB_PACKET_QUEUED);
+        ret = usb_process_one(p);
+        if (ret == USB_RET_ASYNC) {
+            usb_packet_set_state(p, USB_PACKET_ASYNC);
+            break;
+        }
+        p->result = ret;
+        __usb_packet_complete(ep->dev, p);
+    }
+}
+
+void usb_ep_clear_halt(USBEndpoint *ep)
+{
+    ep->halted = false;
+    usb_ep_process_queue(ep);
+}
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 41d663d..c8d2d96 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -366,6 +366,7 @@ struct EHCIQueue {
     uint64_t ts;
     int async;
     int revalidate;
+    bool halted;
 
     /* cached data from guest - needs to be flushed
      * when guest removes an entry (doorbell, handshake sequence)
@@ -740,6 +741,7 @@ static EHCIPacket *ehci_alloc_packet(EHCIQueue *q)
 
 static void ehci_free_packet(EHCIPacket *p)
 {
+    printf("queue %p packet %p free\n", p->queue, p);
     trace_usb_ehci_packet_action(p->queue, p, "free");
     if (p->async == EHCI_ASYNC_INFLIGHT) {
         usb_cancel_packet(&p->packet);
@@ -1773,6 +1775,7 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     if (q->revalidate && (q->qh.epchar      != qh.epchar ||
                           q->qh.epcap       != qh.epcap  ||
                           q->qh.current_qtd != qh.current_qtd)) {
+        printf("queue %p packet %p qtdaddr %08x freeing because of cancel\n", q, NULL, q->qtdaddr);
         ehci_free_queue(q);
         q = ehci_alloc_queue(ehci, entry, async);
         q->seen++;
@@ -1786,6 +1789,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) */
+            printf("guest bug 2!!!\n");
             while ((p = QTAILQ_FIRST(&q->packets)) != NULL) {
                 ehci_free_packet(p);
             }
@@ -1796,18 +1800,22 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
         q->dev = ehci_find_device(q->ehci, devaddr);
     }
 
-    if (p && p->async == EHCI_ASYNC_INFLIGHT) {
+    if (!q->halted && p && p->async == EHCI_ASYNC_INFLIGHT) {
+//        if (p->packet.ep->halted && !(q->qh.token & QTD_TOKEN_HALT)) {
+//            usb_ep_clear_halt(p->packet.ep);
+//        }
         /* I/O still in progress -- skip queue */
         ehci_set_state(ehci, async, EST_HORIZONTALQH);
         goto out;
     }
-    if (p && p->async == EHCI_ASYNC_FINISHED) {
+    if (!q->halted && p && p->async == EHCI_ASYNC_FINISHED) {
         /* I/O finished -- continue processing queue */
         trace_usb_ehci_packet_action(p->queue, p, "complete");
+        if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+             printf("queue %p packet %p qtdaddr %08x completing from fetchqh\n", q, p, p->qtdaddr);
         ehci_set_state(ehci, async, EST_EXECUTING);
         goto out;
     }
-
     if (async && (q->qh.epchar & QH_EPCHAR_H)) {
 
         /*  EHCI spec version 1.0 Section 4.8.3 & 4.10.1 */
@@ -1842,6 +1850,8 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
         ehci_set_state(ehci, async, EST_FETCHQTD);
 
     } else {
+        if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+            printf("queue %p packet %p qtdaddr %08x advqueue fetchqh\n", q, NULL, q->qtdaddr);
         /*  EHCI spec version 1.0 Section 4.10.2 */
         ehci_set_state(ehci, async, EST_ADVANCEQUEUE);
     }
@@ -1951,19 +1961,31 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
     p = QTAILQ_FIRST(&q->packets);
     while (p != NULL && p->qtdaddr != q->qtdaddr) {
         /* should not happen (guest bug) */
+        printf("guest bug!!!\n");
         ehci_free_packet(p);
         p = QTAILQ_FIRST(&q->packets);
     }
     if (p != NULL) {
         ehci_qh_do_overlay(q);
-        if (p->async == EHCI_ASYNC_INFLIGHT) {
+        switch (p->async) {
+        case EHCI_ASYNC_NONE:
+            ehci_set_state(q->ehci, q->async, EST_EXECUTING);
+            break;
+        case EHCI_ASYNC_INFLIGHT:
             ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
-        } else {
+            break;
+        case EHCI_ASYNC_FINISHED:
+            trace_usb_ehci_packet_action(q, p, "complete");
+            if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+                printf("queue %p packet %p qtdaddr %08x completing from fetchqtd\n", q, p, p->qtdaddr);
             ehci_set_state(q->ehci, q->async, EST_EXECUTING);
+            break;
         }
         again = 1;
     } else if (qtd.token & QTD_TOKEN_ACTIVE) {
         p = ehci_alloc_packet(q);
+        if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+            printf("queue %p packet %p qtdaddr %08x ep %d added (fetchqtd)\n", q, p, q->qtdaddr, (q->qh.epchar & QH_EPCHAR_EP_MASK) >> QH_EPCHAR_EP_SH);
         p->qtdaddr = q->qtdaddr;
         p->qtd = qtd;
         ehci_set_state(q->ehci, q->async, EST_EXECUTE);
@@ -2012,6 +2034,8 @@ static void ehci_fill_queue(EHCIPacket *p)
             break;
         }
         p = ehci_alloc_packet(q);
+        if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+            printf("queue %p packet %p qtdaddr %08x added (fill_queue)\n", q, p, qtdaddr);
         p->qtdaddr = qtdaddr;
         p->qtd = qtd;
         p->usb_status = ehci_execute(p, "queue");
@@ -2076,10 +2100,15 @@ static int ehci_state_executing(EHCIQueue *q)
     EHCIPacket *p = QTAILQ_FIRST(&q->packets);
     int again = 0;
 
+    if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+        printf("queue %p packet %p qtdaddr %08x completing, async %d\n", q, p, p->qtdaddr, p->async);
+
     assert(p != NULL);
     assert(p->qtdaddr == q->qtdaddr);
 
     ehci_execute_complete(q);
+    if ((q->qh.epchar & QH_EPCHAR_EP_MASK) == 0)
+        printf("queue %p packet %p qtdaddr %08x completed, status %d\n", q, p, p->qtdaddr, p->usb_status);
     if (p->usb_status == USB_RET_ASYNC) {
         goto out;
     }
@@ -2137,6 +2166,7 @@ static int ehci_state_writeback(EHCIQueue *q)
      * bit is clear.
      */
     if (q->qh.token & QTD_TOKEN_HALT) {
+        q->halted = true;
         ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
         again = 1;
     } else {

Re: [Qemu-devel] 2 issues with qemu-master / 1.2 ehci code

[Gerd Hoffmann]

  Hi,

> 2) happens when a packet fails, and the queue should be halted, in
> this case

Should we just cancel all queued packets on endpoint halts then?  If the
guest decides to go on we'll easily re-queue everything (with the
existing code).  If the guest does something else we don't have to do
anything special.

Not canceling, then trying to figure what the new state of the already
queued packets is could become tricky ...

> Another problem with failing packets is that hw/usb/core.c will
> happily execute the next packet in the ep queue, even though the spec
> says the ep-queue should be halted, giving the guest a chance to
> cancel transfers after the failed one without them ever executing.
> I've a poc patch fixing this too.

Indeed, the core should stop processing them.  Question is what to do
then.  If the host controller cancels all packets anyway we don't have
to do much extra work on the core.  Just stop processing on error and
implicitly un-halt the endpoint when the queue becomes empty.  Maybe
some extra state tracking and asserts() to catch bugs.

cheers,
  Gerd

Re: [Qemu-devel] 2 issues with qemu-master / 1.2 ehci code

[Hans de Goede]

Hi,

On 08/17/2012 09:07 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> 2) happens when a packet fails, and the queue should be halted, in
>> this case
>
> Should we just cancel all queued packets on endpoint halts then?  If the
> guest decides to go on we'll easily re-queue everything (with the
> existing code).  If the guest does something else we don't have to do
> anything special.
>
> Not canceling, then trying to figure what the new state of the already
> queued packets is could become tricky ...
>

Yeah, actually I've come to the same conclusion, after wasting almost a day
trying to get things to work the "figure what the new state of the already
queued packets is" way, and that indeed is not the way to go!

Spo I've just written a patch cancelling all the queued up packets, testing
that now :)

>> Another problem with failing packets is that hw/usb/core.c will
>> happily execute the next packet in the ep queue, even though the spec
>> says the ep-queue should be halted, giving the guest a chance to
>> cancel transfers after the failed one without them ever executing.
>> I've a poc patch fixing this too.
>
> Indeed, the core should stop processing them.  Question is what to do
> then.  If the host controller cancels all packets anyway we don't have
> to do much extra work on the core.  Just stop processing on error and
> implicitly un-halt the endpoint when the queue becomes empty.  Maybe
> some extra state tracking and asserts() to catch bugs.

Right, also done in my wip patch. I'll go test it, then split it up in
multiple patches and then submit. We really should get this in 1.2 btw!