On Thu, Sep 20, 2012 at 3:38 PM, Hans de Goede <hdego...@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.st...@rogers.com>
> Signed-off-by: Hans de Goede <hdego...@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
>
>
