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

[Blue Swirl]

On Thu, Sep 20, 2012 at 3:38 PM, Hans de Goede <address@hidden> 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 <address@hidden>
> Signed-off-by: Hans de Goede <address@hidden>
> ---
>  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
>
>