[Qemu-devel] [PATCH] ehci: Fix interrupt packet MULT handling

[Qemu-devel] [PATCH] ehci: Fix interrupt packet MULT handling

[Hans de Goede]

There are several issues with our handling of the MULT epcap field
of interrupt qhs, which this patch fixes.

1) When we don't execute a transaction because of the transaction counter
being 0, p->async stays EHCI_ASYNC_NONE, and the next time we process the
same qtd we hit an assert in ehci_state_fetchqtd because of this. Even though
I believe that this is caused by 3 below, this patch still removes the assert,
as that can still happen without 3, when multiple packets are queued for the
same interrupt ep.

2) We only *check* the transaction counter from ehci_state_execute, any
packets queued up by fill_queue bypass this check. This is fixed by not calling
fill_queue for interrupt packets.

3) Some versions of Windows set the MULT field of the qh to 0, which is a
clear violation of the EHCI spec, but still they do it. This means that we
will never execute a qtd for these, making interrupt ep-s on USB-2 devices
not work, and after recent changes, triggering 1).

So far we've stored the transaction counter in our copy of the mult field,
but with this beginnig at 0 already when dealing with these version of windows
this won't work. So this patch adds a transact_ctr field to our qh struct,
and sets this to the MULT field value on fetchqh. When the MULT field value
is 0, we set it to 4. Assuming that windows gets way with setting it to 0,
by the actual hardware going horizontal on a 1 -> 0 transition, which will
give it 4 transactions (MULT goes from 0 - 3).

Note that we cannot stop on detecting the 1 -> 0 transition, as our decrement
of the transaction counter, and checking for it are done in 2 different places.

Reported-by: Shawn Starr <shawn.starr@rogers.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 48a1b09..3acd881a 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -373,6 +373,7 @@ struct EHCIQueue {
     uint32_t seen;
     uint64_t ts;
     int async;
+    int transact_ctr;
 
     /* cached data from guest - needs to be flushed
      * when guest removes an entry (doorbell, handshake sequence)
@@ -1837,6 +1838,10 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     }
     q->qh = qh;
 
+    q->transact_ctr = get_field(q->qh.epcap, QH_EPCAP_MULT);
+    if (q->transact_ctr == 0) /* Guest bug in some versions of windows */
+        q->transact_ctr = 4;
+
     if (q->dev == NULL) {
         q->dev = ehci_find_device(q->ehci, devaddr);
     }
@@ -2014,11 +2019,8 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
     } else if (p != NULL) {
         switch (p->async) {
         case EHCI_ASYNC_NONE:
-            /* Should never happen packet should at least be initialized */
-            assert(0);
-            break;
         case EHCI_ASYNC_INITIALIZED:
-            /* Previously nacked packet (likely interrupt ep) */
+            /* Not yet executed (MULT), or previously nacked (int) packet */
             ehci_set_state(q->ehci, q->async, EST_EXECUTE);
             break;
         case EHCI_ASYNC_INFLIGHT:
@@ -2107,15 +2109,12 @@ static int ehci_state_execute(EHCIQueue *q)
 
     // 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
 
-    if (!q->async) {
-        int transactCtr = get_field(q->qh.epcap, QH_EPCAP_MULT);
-        if (!transactCtr) {
-            ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
-            again = 1;
-            goto out;
-        }
+    /* 4.10.3, bottom of page 82, go horizontal on transaction counter == 0 */
+    if (!q->async && q->transact_ctr == 0) {
+        ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
+        again = 1;
+        goto out;
     }
 
     if (q->async) {
@@ -2132,7 +2131,11 @@ static int ehci_state_execute(EHCIQueue *q)
         trace_usb_ehci_packet_action(p->queue, p, "async");
         p->async = EHCI_ASYNC_INFLIGHT;
         ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
-        again = (ehci_fill_queue(p) == USB_RET_PROCERR) ? -1 : 1;
+        if (q->async) {
+            again = (ehci_fill_queue(p) == USB_RET_PROCERR) ? -1 : 1;
+        } else {
+            again = 1;
+        }
         goto out;
     }
 
@@ -2152,13 +2155,9 @@ static int ehci_state_executing(EHCIQueue *q)
 
     ehci_execute_complete(q);
 
-    // 4.10.3
-    if (!q->async) {
-        int transactCtr = get_field(q->qh.epcap, QH_EPCAP_MULT);
-        transactCtr--;
-        set_field(&q->qh.epcap, transactCtr, QH_EPCAP_MULT);
-        // 4.10.3, bottom of page 82, should exit this state when transaction
-        // counter decrements to 0
+    /* 4.10.3 */
+    if (!q->async && q->transact_ctr > 0) {
+        q->transact_ctr--;
     }
 
     /* 4.10.5 */
-- 
1.7.12

Re: [Qemu-devel] [PATCH] ehci: Fix interrupt packet MULT handling

[Gerd Hoffmann]

On 09/20/12 17:38, Hans de Goede wrote:
> There are several issues with our handling of the MULT epcap field
> of interrupt qhs, which this patch fixes.

Patch added to usb patch queue.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH] ehci: Fix interrupt packet MULT handling

[Blue Swirl]

On Thu, Sep 20, 2012 at 3:38 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> There are several issues with our handling of the MULT epcap field
> of interrupt qhs, which this patch fixes.
>
> 1) When we don't execute a transaction because of the transaction counter
> being 0, p->async stays EHCI_ASYNC_NONE, and the next time we process the
> same qtd we hit an assert in ehci_state_fetchqtd because of this. Even though
> I believe that this is caused by 3 below, this patch still removes the assert,
> as that can still happen without 3, when multiple packets are queued for the
> same interrupt ep.
>
> 2) We only *check* the transaction counter from ehci_state_execute, any
> packets queued up by fill_queue bypass this check. This is fixed by not calling
> fill_queue for interrupt packets.
>
> 3) Some versions of Windows set the MULT field of the qh to 0, which is a
> clear violation of the EHCI spec, but still they do it. This means that we
> will never execute a qtd for these, making interrupt ep-s on USB-2 devices
> not work, and after recent changes, triggering 1).
>
> So far we've stored the transaction counter in our copy of the mult field,
> but with this beginnig at 0 already when dealing with these version of windows
> this won't work. So this patch adds a transact_ctr field to our qh struct,
> and sets this to the MULT field value on fetchqh. When the MULT field value
> is 0, we set it to 4. Assuming that windows gets way with setting it to 0,
> by the actual hardware going horizontal on a 1 -> 0 transition, which will
> give it 4 transactions (MULT goes from 0 - 3).
>
> Note that we cannot stop on detecting the 1 -> 0 transition, as our decrement
> of the transaction counter, and checking for it are done in 2 different places.
>
> Reported-by: Shawn Starr <shawn.starr@rogers.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  hw/usb/hcd-ehci.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 48a1b09..3acd881a 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -373,6 +373,7 @@ struct EHCIQueue {
>      uint32_t seen;
>      uint64_t ts;
>      int async;
> +    int transact_ctr;
>
>      /* cached data from guest - needs to be flushed
>       * when guest removes an entry (doorbell, handshake sequence)
> @@ -1837,6 +1838,10 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
>      }
>      q->qh = qh;
>
> +    q->transact_ctr = get_field(q->qh.epcap, QH_EPCAP_MULT);
> +    if (q->transact_ctr == 0) /* Guest bug in some versions of windows */
> +        q->transact_ctr = 4;

Missing braces, please use checkpatch.pl.

> +
>      if (q->dev == NULL) {
>          q->dev = ehci_find_device(q->ehci, devaddr);
>      }
> @@ -2014,11 +2019,8 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
>      } else if (p != NULL) {
>          switch (p->async) {
>          case EHCI_ASYNC_NONE:
> -            /* Should never happen packet should at least be initialized */
> -            assert(0);
> -            break;
>          case EHCI_ASYNC_INITIALIZED:
> -            /* Previously nacked packet (likely interrupt ep) */
> +            /* Not yet executed (MULT), or previously nacked (int) packet */
>              ehci_set_state(q->ehci, q->async, EST_EXECUTE);
>              break;
>          case EHCI_ASYNC_INFLIGHT:
> @@ -2107,15 +2109,12 @@ static int ehci_state_execute(EHCIQueue *q)
>
>      // 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
>
> -    if (!q->async) {
> -        int transactCtr = get_field(q->qh.epcap, QH_EPCAP_MULT);
> -        if (!transactCtr) {
> -            ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
> -            again = 1;
> -            goto out;
> -        }
> +    /* 4.10.3, bottom of page 82, go horizontal on transaction counter == 0 */
> +    if (!q->async && q->transact_ctr == 0) {
> +        ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
> +        again = 1;
> +        goto out;
>      }
>
>      if (q->async) {
> @@ -2132,7 +2131,11 @@ static int ehci_state_execute(EHCIQueue *q)
>          trace_usb_ehci_packet_action(p->queue, p, "async");
>          p->async = EHCI_ASYNC_INFLIGHT;
>          ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
> -        again = (ehci_fill_queue(p) == USB_RET_PROCERR) ? -1 : 1;
> +        if (q->async) {
> +            again = (ehci_fill_queue(p) == USB_RET_PROCERR) ? -1 : 1;
> +        } else {
> +            again = 1;
> +        }
>          goto out;
>      }
>
> @@ -2152,13 +2155,9 @@ static int ehci_state_executing(EHCIQueue *q)
>
>      ehci_execute_complete(q);
>
> -    // 4.10.3
> -    if (!q->async) {
> -        int transactCtr = get_field(q->qh.epcap, QH_EPCAP_MULT);
> -        transactCtr--;
> -        set_field(&q->qh.epcap, transactCtr, QH_EPCAP_MULT);
> -        // 4.10.3, bottom of page 82, should exit this state when transaction
> -        // counter decrements to 0
> +    /* 4.10.3 */
> +    if (!q->async && q->transact_ctr > 0) {
> +        q->transact_ctr--;
>      }
>
>      /* 4.10.5 */
> --
> 1.7.12
>
>

Re: [Qemu-devel] [PATCH] ehci: Fix interrupt packet MULT handling

[Hans de Goede]

Hi,

Sorry.

On 09/22/2012 03:16 PM, Blue Swirl wrote:
> On Thu, Sep 20, 2012 at 3:38 PM, Hans de Goede <hdegoede@redhat.com> wrote:

<snip>

>> @@ -1837,6 +1838,10 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
>>       }
>>       q->qh = qh;
>>
>> +    q->transact_ctr = get_field(q->qh.epcap, QH_EPCAP_MULT);
>> +    if (q->transact_ctr == 0) /* Guest bug in some versions of windows */
>> +        q->transact_ctr = 4;
>
> Missing braces, please use checkpatch.pl.
>

Oops, Gerd since this is already in your tree, can you fix this, or do you
want a new fixed version ?

Regards,

Hans

Re: [Qemu-devel] [PATCH] ehci: Fix interrupt packet MULT handling

[Gerd Hoffmann]

  Hi,

>>
>> Missing braces, please use checkpatch.pl.
>>
> 
> Oops, Gerd since this is already in your tree, can you fix this, or do you
> want a new fixed version ?

I've already fixed it.

You can wire checkpatch.pl into the git commit hook to avoid those slip
through unnoticed.

cheers,
  Gerd