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

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

[Julius Werner]

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@vger.kernel.org
Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 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

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

[Maciej Puzio]

Tested-by: Maciej Puzio <mx34567@gmail.com>

On Tue, Jul 8, 2014 at 2:01 PM, Julius Werner <jwerner@chromium.org> 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@vger.kernel.org
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  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
>

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

[Paul Zimmerman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4901 bytes --]

> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Maciej Puzio
> Sent: Thursday, July 17, 2014 11:25 AM
> 
> Tested-by: Maciej Puzio <mx34567@gmail.com>
> 
> On Tue, Jul 8, 2014 at 2:01 PM, Julius Werner <jwerner@chromium.org> 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@vger.kernel.org
> > Signed-off-by: Julius Werner <jwerner@chromium.org>
> > ---
> >  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

Hmm. Wouldn't it be safer to have a quirk for this, and only enable
the workaround if the Asmedia controller is detected? This code is so
complicated that it is difficult to see whether this could have a
harmful effect on controllers without the bug.

-- 
Paul

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Julius Werner]

> Hmm. Wouldn't it be safer to have a quirk for this, and only enable
> the workaround if the Asmedia controller is detected? This code is so
> complicated that it is difficult to see whether this could have a
> harmful effect on controllers without the bug.

Sorry for making it complicated, but it kinda has been like that
before already... I don't think the new patch adds much confusion on
its own. I would really advise against making this a quirk: checking
for it in every case essentially doesn't cost us anything (just one
more register compare that is negligible against all the
cache-coherent memory accesses of walking the ring), and hardcoding a
quirk would mean that we have to identify and add every host
controller that does this individually.

I still haven't seen anything in the XHCI spec that actually forbids
this behavior, so it might be a perfectly legal interpretation and who
knows how many host controllers chose to do it that way. Until we find
them all, we would have some really bad and hard to track down bugs on
those controllers (we really just got lucky this time that Maciej was
able to catch it in a bisect). I think it's better to program the
driver defensively and able to deal with unexpected situations in
general.

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

[Mathias Nyman]

On 07/17/2014 10:50 PM, Julius Werner wrote:
>> Hmm. Wouldn't it be safer to have a quirk for this, and only enable
>> the workaround if the Asmedia controller is detected? This code is so
>> complicated that it is difficult to see whether this could have a
>> harmful effect on controllers without the bug.
> 
> Sorry for making it complicated, but it kinda has been like that
> before already... I don't think the new patch adds much confusion on
> its own. I would really advise against making this a quirk: checking
> for it in every case essentially doesn't cost us anything (just one
> more register compare that is negligible against all the
> cache-coherent memory accesses of walking the ring), and hardcoding a
> quirk would mean that we have to identify and add every host
> controller that does this individually.
> 
> I still haven't seen anything in the XHCI spec that actually forbids
> this behavior, so it might be a perfectly legal interpretation and who
> knows how many host controllers chose to do it that way. Until we find
> them all, we would have some really bad and hard to track down bugs on
> those controllers (we really just got lucky this time that Maciej was
> able to catch it in a bisect). I think it's better to program the
> driver defensively and able to deal with unexpected situations in
> general.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

It's starting to get a bit too complicated.
xhci find_new_dequeue_state() now has four places where it can toggle the cycle bit
in addition to the cycle toggle in find_trb_seg().

In the end we really just want to do it max once.

I'll see if I can simplify the whole cycle bit code somehow.

If not, then we need to take this or revert the original patch

-Mathias

 

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

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 569 bytes --]

> It's starting to get a bit too complicated.
> xhci find_new_dequeue_state() now has four places where it can toggle the cycle bit
> in addition to the cycle toggle in find_trb_seg().
> 
> In the end we really just want to do it max once.
> 
> I'll see if I can simplify the whole cycle bit code somehow.

Have a look at the patches I submitted last year.
I remember simplifying a lot of that code.

	David

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥