Re: [PATCH] usb: xhci: Fix Set TR Dequeue Pointer cycle state for quirky xHCs

[Maciej Puzio]

Tested-by: Maciej Puzio <mx34567@xxxxxxxxx>

On Tue, Jul 8, 2014 at 2:01 PM, Julius Werner <jwerner@xxxxxxxxxxxx> wrote:
> Commit 1f81b6d22 "usb: xhci: Prefer endpoint context dequeue pointer
> over stopped_trb" changed the logic in xhci_find_new_dequeue_state() to
> only use the Endpoint Context's TR Dequeue Pointer instead of the last
> Event TRB's TRB Pointer as a basis from which to infer the host
> controller state. This has uncovered a bug with certain Asmedia xHCs
> which seem to advance their TR Dequeue Pointer one TRB behind the one
> that caused a Stall Error.
>
> This confuses the cycle state calculation since cur_td->last_trb is now
> behind hw_dequeue (which the algorithm interprets as a single huge TD
> that wraps around the whole transfer ring). This patch counteracts that
> by explicitly looking for last_trb when searching for hw_dequeue from
> the old software dequeue pointer. If it is found, we toggle the cycle
> state once more to balance out the incorrect toggle that will happen
> later.
>
> In order to make this work for both single and multi segment rings, this
> patch also expands the detection of wrap-around TDs to work on the
> latter ones (which cannot normally happen because the kernel prevents
> that case to allow for ring expansion, but it's how things appear to be
> in the quirky case and allowing the code to handle it makes it easier to
> generalize this fix across all kernels). The code will now toggle the
> cycle state whenever last_trb is before hw_dequeue on the same segment,
> regardless of how many segments there are in the ring.
>
> This patch should be backported to kernels as old as 2.6.31 that contain
> the commit ae636747146ea97efa18e04576acd3416e2514f5 "USB: xhci: URB
> cancellation support."
>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Julius Werner <jwerner@xxxxxxxxxxxx>
> ---
>  drivers/usb/host/xhci-ring.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 749fc68..50abc68 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -489,8 +489,17 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
>         /* Find virtual address and segment of hardware dequeue pointer */
>         state->new_deq_seg = ep_ring->deq_seg;
>         state->new_deq_ptr = ep_ring->dequeue;
> +       state->new_cycle_state = hw_dequeue & 0x1;
>         while (xhci_trb_virt_to_dma(state->new_deq_seg, state->new_deq_ptr)
>                         != (dma_addr_t)(hw_dequeue & ~0xf)) {
> +               /*
> +                * Quirky HCs can advance hw_dequeue behind cur_td->last_trb.
> +                * That violates the assumptions of the TD wrap around check
> +                * below, so toggle the cycle state once more to cancel it out.
> +                */
> +               if (state->new_deq_ptr == cur_td->last_trb)
> +                       state->new_cycle_state ^= 1;
> +
>                 next_trb(xhci, ep_ring, &state->new_deq_seg,
>                                         &state->new_deq_ptr);
>                 if (state->new_deq_ptr == ep_ring->dequeue) {
> @@ -500,12 +509,11 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
>         }
>         /*
>          * Find cycle state for last_trb, starting at old cycle state of
> -        * hw_dequeue. If there is only one segment ring, find_trb_seg() will
> -        * return immediately and cannot toggle the cycle state if this search
> -        * wraps around, so add one more toggle manually in that case.
> +        * hw_dequeue. If last_trb is on the current segment before hw_dequeue,
> +        * that means we wrap around the whole ring, but find_trb_seq() will
> +        * return immediately. Toggle the cycle state manually in that case.
>          */
> -       state->new_cycle_state = hw_dequeue & 0x1;
> -       if (ep_ring->first_seg == ep_ring->first_seg->next &&
> +       if (state->new_deq_seg->trbs < cur_td->last_trb &&
>                         cur_td->last_trb < state->new_deq_ptr)
>                 state->new_cycle_state ^= 0x1;
>
> --
> 1.8.3.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/