Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-04-15 Thread Hans de Goede 

Hi,

On 04/15/2014 09:42 PM, Julius Werner wrote:

+hdegoede


I tried to apply this patch on top of 3.15-rc1, but it fails because of the
streams support added to xhci_find_new_dequeue_state()

After some manual editing the interesting parts of
xhci_find_new_dequeue_state() looks like this:

@@ -577,46 +568,57 @@ void xhci_find_new_dequeue_state(struct xhci_hcd
*xhci,
 if (ep->ep_state & EP_HAS_STREAMS) {
 struct xhci_stream_ctx *ctx =
 >stream_info->stream_ctx_array[stream_id];
-   state->new_cycle_state = 0x1 &
le64_to_cpu(ctx->stream_ring);
+   hw_dequeue = le64_to_cpu(ctx->stream_ring);
 } else {
 struct xhci_ep_ctx *ep_ctx

 = xhci_get_ep_ctx(xhci, dev->out_ctx, ep_index);
-   state->new_cycle_state = 0x1 & le64_to_cpu(ep_ctx->deq);
+   hw_dequeue = le64_to_cpu(ep_ctx->deq);
 }

+   /* Find virtual address and segment of hardware dequeue pointer */

+   state->new_deq_seg = ep_ring->deq_seg;
+   state->new_deq_ptr = ep_ring->dequeue;
+   while (xhci_trb_virt_to_dma(state->new_deq_seg, state->new_deq_ptr)
+   != (dma_addr_t)(hw_dequeue & ~0x1)) {
+   next_trb(xhci, ep_ring, >new_deq_seg,
+   >new_deq_ptr);
+   if (state->new_deq_ptr == ep_ring->dequeue) {
+   WARN_ON(1);
+   return;
+   }
+   }

Also the comparison of the dequeue pointers, using (hw_dequeue & ~0x1) might
have some troubles with streams. Endpoint context TR dequeue pointer LO
field has bits 3:1 reserved (probably zero) but stream context uses those
bits. Would it make sense to use (hw_dequeue & ~0xf) here instead?


Ah, yes, looks like that patch wasn't in Linus' tree yet back when I
wrote this. I think your merge looks pretty good... just use
(hw_dequeue & ~0xf) instead of (hw_dequeue & ~0x1) to get the pointer
as you said, and this should work fine.


But I'm still concerned about the dequeue pointer in the streams case.
streams may be nested, we might be pointing at another stream context
instead of the dequeue pointer.


Since I've not followed the entire discussion previously to this I cannot
really provide any useful feedback on this patch. Other then 2 remarks:

1) We don't use nested streams, so no need to worry about those
2) You're right that for streams to get the dequeue address you need
to mask with ~0xf

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-04-15 Thread Julius Werner 
+hdegoede

> I tried to apply this patch on top of 3.15-rc1, but it fails because of the
> streams support added to xhci_find_new_dequeue_state()
>
> After some manual editing the interesting parts of
> xhci_find_new_dequeue_state() looks like this:
>
> @@ -577,46 +568,57 @@ void xhci_find_new_dequeue_state(struct xhci_hcd
> *xhci,
> if (ep->ep_state & EP_HAS_STREAMS) {
> struct xhci_stream_ctx *ctx =
> >stream_info->stream_ctx_array[stream_id];
> -   state->new_cycle_state = 0x1 &
> le64_to_cpu(ctx->stream_ring);
> +   hw_dequeue = le64_to_cpu(ctx->stream_ring);
> } else {
> struct xhci_ep_ctx *ep_ctx
>
> = xhci_get_ep_ctx(xhci, dev->out_ctx, ep_index);
> -   state->new_cycle_state = 0x1 & le64_to_cpu(ep_ctx->deq);
> +   hw_dequeue = le64_to_cpu(ep_ctx->deq);
> }
>
> +   /* Find virtual address and segment of hardware dequeue pointer */
>
> +   state->new_deq_seg = ep_ring->deq_seg;
> +   state->new_deq_ptr = ep_ring->dequeue;
> +   while (xhci_trb_virt_to_dma(state->new_deq_seg, state->new_deq_ptr)
> +   != (dma_addr_t)(hw_dequeue & ~0x1)) {
> +   next_trb(xhci, ep_ring, >new_deq_seg,
> +   >new_deq_ptr);
> +   if (state->new_deq_ptr == ep_ring->dequeue) {
> +   WARN_ON(1);
> +   return;
> +   }
> +   }
>
> Also the comparison of the dequeue pointers, using (hw_dequeue & ~0x1) might
> have some troubles with streams. Endpoint context TR dequeue pointer LO
> field has bits 3:1 reserved (probably zero) but stream context uses those
> bits. Would it make sense to use (hw_dequeue & ~0xf) here instead?

Ah, yes, looks like that patch wasn't in Linus' tree yet back when I
wrote this. I think your merge looks pretty good... just use
(hw_dequeue & ~0xf) instead of (hw_dequeue & ~0x1) to get the pointer
as you said, and this should work fine.

> But I'm still concerned about the dequeue pointer in the streams case.
> streams may be nested, we might be pointing at another stream context
> instead of the dequeue pointer.
>
> So there's still some work needed. Are you interested in re-working this to
> fit on top of 3.15-rc1 or should I add it to my todo list?

Hmmm... maybe the stream_id parameter is already pointing to the
correct secondary stream (if applicable) so you can rely on having a
normal dequeue pointer there? The xhci_triad_to_transfer_ring()
function seems to make the same assumption... At any rate, if there is
a problem here it would also be in the original c4bedb77e ("xhci: For
streams the css flag most be read from the stream-ctx on ep stop")
already, so I think you should follow up with that patch's author and
fix it in a separate commit if necessary.

I unfortunately don't have a device using streams to test with, so I
couldn't do more for this patch than you've already done. I think if
you change the hw_dequeue masking as you said that should guarantee
that the patch at least won't make things worse for streams, and that
should be good enough.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-04-15 Thread Mathias Nyman 

On 04/03/2014 12:29 AM, Julius Werner wrote:

Hi Mathias,


The patch looks fine.  Mathias is taking over for xHCI driver
maintainership in 3.15.  He's currently handling queuing bug fix patches
for 3.14 while I finish queueing feature patches for 3.15.  Mathias,
will you test and queue this up for 3.14?


Did you pick this patch up anywhere yet or are there still issues with
it? I just want to make sure it doesn't slip through the cracks.
(Maybe I just didn't see it yet... are you still queueing patches in
sarah/xhci.git or do you have your own repository somewhere?)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



I tried to apply this patch on top of 3.15-rc1, but it fails because of 
the streams support added to xhci_find_new_dequeue_state()


After some manual editing the interesting parts of 
xhci_find_new_dequeue_state() looks like this:


@@ -577,46 +568,57 @@ void xhci_find_new_dequeue_state(struct xhci_hcd 
*xhci,

if (ep->ep_state & EP_HAS_STREAMS) {
struct xhci_stream_ctx *ctx =
>stream_info->stream_ctx_array[stream_id];
-   state->new_cycle_state = 0x1 & 
le64_to_cpu(ctx->stream_ring);

+   hw_dequeue = le64_to_cpu(ctx->stream_ring);
} else {
struct xhci_ep_ctx *ep_ctx
= xhci_get_ep_ctx(xhci, dev->out_ctx, ep_index);
-   state->new_cycle_state = 0x1 & le64_to_cpu(ep_ctx->deq);
+   hw_dequeue = le64_to_cpu(ep_ctx->deq);
}

+   /* Find virtual address and segment of hardware dequeue pointer */
+   state->new_deq_seg = ep_ring->deq_seg;
+   state->new_deq_ptr = ep_ring->dequeue;
+   while (xhci_trb_virt_to_dma(state->new_deq_seg, state->new_deq_ptr)
+   != (dma_addr_t)(hw_dequeue & ~0x1)) {
+   next_trb(xhci, ep_ring, >new_deq_seg,
+   >new_deq_ptr);
+   if (state->new_deq_ptr == ep_ring->dequeue) {
+   WARN_ON(1);
+   return;
+   }
+   }


But I'm still concerned about the dequeue pointer in the streams case. 
streams may be nested, we might be pointing at another stream context 
instead of the dequeue pointer.


Also the comparison of the dequeue pointers, using (hw_dequeue & ~0x1) 
might have some troubles with streams. Endpoint context TR dequeue 
pointer LO field has bits 3:1 reserved (probably zero) but stream 
context uses those bits. Would it make sense to use (hw_dequeue & ~0xf) 
here instead?


So there's still some work needed. Are you interested in re-working this 
to fit on top of 3.15-rc1 or should I add it to my todo list?


-Mathias



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-04-15 Thread Mathias Nyman 

On 04/03/2014 12:29 AM, Julius Werner wrote:

Hi Mathias,


The patch looks fine.  Mathias is taking over for xHCI driver
maintainership in 3.15.  He's currently handling queuing bug fix patches
for 3.14 while I finish queueing feature patches for 3.15.  Mathias,
will you test and queue this up for 3.14?


Did you pick this patch up anywhere yet or are there still issues with
it? I just want to make sure it doesn't slip through the cracks.
(Maybe I just didn't see it yet... are you still queueing patches in
sarah/xhci.git or do you have your own repository somewhere?)
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



I tried to apply this patch on top of 3.15-rc1, but it fails because of 
the streams support added to xhci_find_new_dequeue_state()


After some manual editing the interesting parts of 
xhci_find_new_dequeue_state() looks like this:


@@ -577,46 +568,57 @@ void xhci_find_new_dequeue_state(struct xhci_hcd 
*xhci,

if (ep-ep_state  EP_HAS_STREAMS) {
struct xhci_stream_ctx *ctx =
ep-stream_info-stream_ctx_array[stream_id];
-   state-new_cycle_state = 0x1  
le64_to_cpu(ctx-stream_ring);

+   hw_dequeue = le64_to_cpu(ctx-stream_ring);
} else {
struct xhci_ep_ctx *ep_ctx
= xhci_get_ep_ctx(xhci, dev-out_ctx, ep_index);
-   state-new_cycle_state = 0x1  le64_to_cpu(ep_ctx-deq);
+   hw_dequeue = le64_to_cpu(ep_ctx-deq);
}

+   /* Find virtual address and segment of hardware dequeue pointer */
+   state-new_deq_seg = ep_ring-deq_seg;
+   state-new_deq_ptr = ep_ring-dequeue;
+   while (xhci_trb_virt_to_dma(state-new_deq_seg, state-new_deq_ptr)
+   != (dma_addr_t)(hw_dequeue  ~0x1)) {
+   next_trb(xhci, ep_ring, state-new_deq_seg,
+   state-new_deq_ptr);
+   if (state-new_deq_ptr == ep_ring-dequeue) {
+   WARN_ON(1);
+   return;
+   }
+   }


But I'm still concerned about the dequeue pointer in the streams case. 
streams may be nested, we might be pointing at another stream context 
instead of the dequeue pointer.


Also the comparison of the dequeue pointers, using (hw_dequeue  ~0x1) 
might have some troubles with streams. Endpoint context TR dequeue 
pointer LO field has bits 3:1 reserved (probably zero) but stream 
context uses those bits. Would it make sense to use (hw_dequeue  ~0xf) 
here instead?


So there's still some work needed. Are you interested in re-working this 
to fit on top of 3.15-rc1 or should I add it to my todo list?


-Mathias



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-04-15 Thread Julius Werner 
+hdegoede

 I tried to apply this patch on top of 3.15-rc1, but it fails because of the
 streams support added to xhci_find_new_dequeue_state()

 After some manual editing the interesting parts of
 xhci_find_new_dequeue_state() looks like this:

 @@ -577,46 +568,57 @@ void xhci_find_new_dequeue_state(struct xhci_hcd
 *xhci,
 if (ep-ep_state  EP_HAS_STREAMS) {
 struct xhci_stream_ctx *ctx =
 ep-stream_info-stream_ctx_array[stream_id];
 -   state-new_cycle_state = 0x1 
 le64_to_cpu(ctx-stream_ring);
 +   hw_dequeue = le64_to_cpu(ctx-stream_ring);
 } else {
 struct xhci_ep_ctx *ep_ctx

 = xhci_get_ep_ctx(xhci, dev-out_ctx, ep_index);
 -   state-new_cycle_state = 0x1  le64_to_cpu(ep_ctx-deq);
 +   hw_dequeue = le64_to_cpu(ep_ctx-deq);
 }

 +   /* Find virtual address and segment of hardware dequeue pointer */

 +   state-new_deq_seg = ep_ring-deq_seg;
 +   state-new_deq_ptr = ep_ring-dequeue;
 +   while (xhci_trb_virt_to_dma(state-new_deq_seg, state-new_deq_ptr)
 +   != (dma_addr_t)(hw_dequeue  ~0x1)) {
 +   next_trb(xhci, ep_ring, state-new_deq_seg,
 +   state-new_deq_ptr);
 +   if (state-new_deq_ptr == ep_ring-dequeue) {
 +   WARN_ON(1);
 +   return;
 +   }
 +   }

 Also the comparison of the dequeue pointers, using (hw_dequeue  ~0x1) might
 have some troubles with streams. Endpoint context TR dequeue pointer LO
 field has bits 3:1 reserved (probably zero) but stream context uses those
 bits. Would it make sense to use (hw_dequeue  ~0xf) here instead?

Ah, yes, looks like that patch wasn't in Linus' tree yet back when I
wrote this. I think your merge looks pretty good... just use
(hw_dequeue  ~0xf) instead of (hw_dequeue  ~0x1) to get the pointer
as you said, and this should work fine.

 But I'm still concerned about the dequeue pointer in the streams case.
 streams may be nested, we might be pointing at another stream context
 instead of the dequeue pointer.

 So there's still some work needed. Are you interested in re-working this to
 fit on top of 3.15-rc1 or should I add it to my todo list?

Hmmm... maybe the stream_id parameter is already pointing to the
correct secondary stream (if applicable) so you can rely on having a
normal dequeue pointer there? The xhci_triad_to_transfer_ring()
function seems to make the same assumption... At any rate, if there is
a problem here it would also be in the original c4bedb77e (xhci: For
streams the css flag most be read from the stream-ctx on ep stop)
already, so I think you should follow up with that patch's author and
fix it in a separate commit if necessary.

I unfortunately don't have a device using streams to test with, so I
couldn't do more for this patch than you've already done. I think if
you change the hw_dequeue masking as you said that should guarantee
that the patch at least won't make things worse for streams, and that
should be good enough.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-04-15 Thread Hans de Goede 

Hi,

On 04/15/2014 09:42 PM, Julius Werner wrote:

+hdegoede


I tried to apply this patch on top of 3.15-rc1, but it fails because of the
streams support added to xhci_find_new_dequeue_state()

After some manual editing the interesting parts of
xhci_find_new_dequeue_state() looks like this:

@@ -577,46 +568,57 @@ void xhci_find_new_dequeue_state(struct xhci_hcd
*xhci,
 if (ep-ep_state  EP_HAS_STREAMS) {
 struct xhci_stream_ctx *ctx =
 ep-stream_info-stream_ctx_array[stream_id];
-   state-new_cycle_state = 0x1 
le64_to_cpu(ctx-stream_ring);
+   hw_dequeue = le64_to_cpu(ctx-stream_ring);
 } else {
 struct xhci_ep_ctx *ep_ctx

 = xhci_get_ep_ctx(xhci, dev-out_ctx, ep_index);
-   state-new_cycle_state = 0x1  le64_to_cpu(ep_ctx-deq);
+   hw_dequeue = le64_to_cpu(ep_ctx-deq);
 }

+   /* Find virtual address and segment of hardware dequeue pointer */

+   state-new_deq_seg = ep_ring-deq_seg;
+   state-new_deq_ptr = ep_ring-dequeue;
+   while (xhci_trb_virt_to_dma(state-new_deq_seg, state-new_deq_ptr)
+   != (dma_addr_t)(hw_dequeue  ~0x1)) {
+   next_trb(xhci, ep_ring, state-new_deq_seg,
+   state-new_deq_ptr);
+   if (state-new_deq_ptr == ep_ring-dequeue) {
+   WARN_ON(1);
+   return;
+   }
+   }

Also the comparison of the dequeue pointers, using (hw_dequeue  ~0x1) might
have some troubles with streams. Endpoint context TR dequeue pointer LO
field has bits 3:1 reserved (probably zero) but stream context uses those
bits. Would it make sense to use (hw_dequeue  ~0xf) here instead?


Ah, yes, looks like that patch wasn't in Linus' tree yet back when I
wrote this. I think your merge looks pretty good... just use
(hw_dequeue  ~0xf) instead of (hw_dequeue  ~0x1) to get the pointer
as you said, and this should work fine.


But I'm still concerned about the dequeue pointer in the streams case.
streams may be nested, we might be pointing at another stream context
instead of the dequeue pointer.


Since I've not followed the entire discussion previously to this I cannot
really provide any useful feedback on this patch. Other then 2 remarks:

1) We don't use nested streams, so no need to worry about those
2) You're right that for streams to get the dequeue address you need
to mask with ~0xf

Regards,

Hans
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-04-03 Thread Mathias Nyman 

Hi

On 04/03/2014 12:29 AM, Julius Werner wrote:

Hi Mathias,


The patch looks fine.  Mathias is taking over for xHCI driver
maintainership in 3.15.  He's currently handling queuing bug fix patches
for 3.14 while I finish queueing feature patches for 3.15.  Mathias,
will you test and queue this up for 3.14?


Did you pick this patch up anywhere yet or are there still issues with
it? I just want to make sure it doesn't slip through the cracks.
(Maybe I just didn't see it yet... are you still queueing patches in
sarah/xhci.git or do you have your own repository somewhere?)


Patch applies fine and I'll send it forward to Greg once 3.15-rc1 is out.

I'm not using Sarah's tree, but setting up my own tree on kernel.org 
(Just got access). I got a temporary one on github if you want to take a 
look, but it's a short term thing and it may change without warning.


g...@github.com:matnyman/xhci.git

-Mathias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-04-03 Thread Mathias Nyman 

Hi

On 04/03/2014 12:29 AM, Julius Werner wrote:

Hi Mathias,


The patch looks fine.  Mathias is taking over for xHCI driver
maintainership in 3.15.  He's currently handling queuing bug fix patches
for 3.14 while I finish queueing feature patches for 3.15.  Mathias,
will you test and queue this up for 3.14?


Did you pick this patch up anywhere yet or are there still issues with
it? I just want to make sure it doesn't slip through the cracks.
(Maybe I just didn't see it yet... are you still queueing patches in
sarah/xhci.git or do you have your own repository somewhere?)


Patch applies fine and I'll send it forward to Greg once 3.15-rc1 is out.

I'm not using Sarah's tree, but setting up my own tree on kernel.org 
(Just got access). I got a temporary one on github if you want to take a 
look, but it's a short term thing and it may change without warning.


g...@github.com:matnyman/xhci.git

-Mathias
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-04-02 Thread Julius Werner 
Hi Mathias,

> The patch looks fine.  Mathias is taking over for xHCI driver
> maintainership in 3.15.  He's currently handling queuing bug fix patches
> for 3.14 while I finish queueing feature patches for 3.15.  Mathias,
> will you test and queue this up for 3.14?

Did you pick this patch up anywhere yet or are there still issues with
it? I just want to make sure it doesn't slip through the cracks.
(Maybe I just didn't see it yet... are you still queueing patches in
sarah/xhci.git or do you have your own repository somewhere?)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-04-02 Thread Julius Werner 
Hi Mathias,

 The patch looks fine.  Mathias is taking over for xHCI driver
 maintainership in 3.15.  He's currently handling queuing bug fix patches
 for 3.14 while I finish queueing feature patches for 3.15.  Mathias,
 will you test and queue this up for 3.14?

Did you pick this patch up anywhere yet or are there still issues with
it? I just want to make sure it doesn't slip through the cracks.
(Maybe I just didn't see it yet... are you still queueing patches in
sarah/xhci.git or do you have your own repository somewhere?)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-02-28 Thread Sarah Sharp 
On Thu, Feb 20, 2014 at 09:12:15PM -0800, Julius Werner wrote:
> We have observed a rare cycle state desync bug after Set TR Dequeue
> Pointer commands on Intel LynxPoint xHCs (resulting in an endpoint that
> doesn't fetch new TRBs and thus an unresponsive USB device). It always
> triggers when a previous Set TR Dequeue Pointer command has set the
> pointer to the final Link TRB of a segment, and then another URB gets
> enqueued and cancelled again before it can be completed. Further
> investigation showed that the xHC had returned the Link TRB in the TRB
> Pointer field of the Transfer Event (CC == Stopped -- Length Invalid),
> but when xhci_find_new_dequeue_state() later accesses the Endpoint
> Context's TR Dequeue Pointer field it is set to the first TRB of the
> next segment.
> 
> The driver expects those two values to be the same in this situation,
> and uses the cycle state of the latter together with the address of the
> former. This should be fine according to the XHCI specification, since
> the endpoint ring should be stopped when returning the Transfer Event
> and thus should not advance over the Link TRB before it gets restarted.
> However, real-world XHCI implementations apparently don't really care
> that much about these details, so the driver should follow a more
> defensive approach to try to work around HC spec violations.

Length Invalid can actually be returned when the host is "in between"
processing TRBs.  Perhaps it had processed the link TRB, but hadn't
started processing the TRB at the top of the ring.  So it's not a spec
violation per say, but definitely a spec ambiguity.

The patch looks fine.  Mathias is taking over for xHCI driver
maintainership in 3.15.  He's currently handling queuing bug fix patches
for 3.14 while I finish queueing feature patches for 3.15.  Mathias,
will you test and queue this up for 3.14?

Signed-off-by: Sarah Sharp 

> This patch removes the stopped_trb variable that had been used to store
> the TRB Pointer from the last Transfer Event of a stopped TRB. Instead,
> xhci_find_new_dequeue_state() now relies only on the Endpoint Context,
> requiring a small amount of additional processing to find the virtual
> address corresponding to the TR Dequeue Pointer. Some other parts of the
> function were slightly rearranged to better fit into this model.
> 
> This patch should be backported to kernels as old as 2.6.31 that contain
> the commit ae636747146ea97efa18e04576acd3416e2514f5 "USB: xhci: URB
> cancellation support."
> 
> Signed-off-by: Julius Werner 
> ---
>  drivers/usb/host/xhci-ring.c | 66 
> 
>  drivers/usb/host/xhci.c  |  1 -
>  drivers/usb/host/xhci.h  |  2 --
>  3 files changed, 30 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index a0b248c..b8277c7 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -549,6 +549,7 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
>   struct xhci_generic_trb *trb;
>   struct xhci_ep_ctx *ep_ctx;
>   dma_addr_t addr;
> + u64 hw_dequeue;
>  
>   ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
>   ep_index, stream_id);
> @@ -558,56 +559,56 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
>   stream_id);
>   return;
>   }
> - state->new_cycle_state = 0;
> - xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
> - "Finding segment containing stopped TRB.");
> - state->new_deq_seg = find_trb_seg(cur_td->start_seg,
> - dev->eps[ep_index].stopped_trb,
> - >new_cycle_state);
> - if (!state->new_deq_seg) {
> - WARN_ON(1);
> - return;
> - }
>  
>   /* Dig out the cycle state saved by the xHC during the stop ep cmd */
>   xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
>   "Finding endpoint context");
>   ep_ctx = xhci_get_ep_ctx(xhci, dev->out_ctx, ep_index);
> - state->new_cycle_state = 0x1 & le64_to_cpu(ep_ctx->deq);
> + hw_dequeue = le64_to_cpu(ep_ctx->deq);
> +
> + /* Find virtual address and segment of hardware dequeue pointer */
> + state->new_deq_seg = ep_ring->deq_seg;
> + state->new_deq_ptr = ep_ring->dequeue;
> + while (xhci_trb_virt_to_dma(state->new_deq_seg, state->new_deq_ptr)
> + != (dma_addr_t)(hw_dequeue & ~0x1)) {
> + next_trb(xhci, ep_ring, >new_deq_seg,
> + >new_deq_ptr);
> + if (state->new_deq_ptr == ep_ring->dequeue) {
> + WARN_ON(1);
> + return;
> + }
> + }
>  
> + /*
> +  * 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

Re: [PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-02-28 Thread Sarah Sharp 
On Thu, Feb 20, 2014 at 09:12:15PM -0800, Julius Werner wrote:
 We have observed a rare cycle state desync bug after Set TR Dequeue
 Pointer commands on Intel LynxPoint xHCs (resulting in an endpoint that
 doesn't fetch new TRBs and thus an unresponsive USB device). It always
 triggers when a previous Set TR Dequeue Pointer command has set the
 pointer to the final Link TRB of a segment, and then another URB gets
 enqueued and cancelled again before it can be completed. Further
 investigation showed that the xHC had returned the Link TRB in the TRB
 Pointer field of the Transfer Event (CC == Stopped -- Length Invalid),
 but when xhci_find_new_dequeue_state() later accesses the Endpoint
 Context's TR Dequeue Pointer field it is set to the first TRB of the
 next segment.
 
 The driver expects those two values to be the same in this situation,
 and uses the cycle state of the latter together with the address of the
 former. This should be fine according to the XHCI specification, since
 the endpoint ring should be stopped when returning the Transfer Event
 and thus should not advance over the Link TRB before it gets restarted.
 However, real-world XHCI implementations apparently don't really care
 that much about these details, so the driver should follow a more
 defensive approach to try to work around HC spec violations.

Length Invalid can actually be returned when the host is in between
processing TRBs.  Perhaps it had processed the link TRB, but hadn't
started processing the TRB at the top of the ring.  So it's not a spec
violation per say, but definitely a spec ambiguity.

The patch looks fine.  Mathias is taking over for xHCI driver
maintainership in 3.15.  He's currently handling queuing bug fix patches
for 3.14 while I finish queueing feature patches for 3.15.  Mathias,
will you test and queue this up for 3.14?

Signed-off-by: Sarah Sharp sarah.a.sh...@linux.intel.com

 This patch removes the stopped_trb variable that had been used to store
 the TRB Pointer from the last Transfer Event of a stopped TRB. Instead,
 xhci_find_new_dequeue_state() now relies only on the Endpoint Context,
 requiring a small amount of additional processing to find the virtual
 address corresponding to the TR Dequeue Pointer. Some other parts of the
 function were slightly rearranged to better fit into this model.
 
 This patch should be backported to kernels as old as 2.6.31 that contain
 the commit ae636747146ea97efa18e04576acd3416e2514f5 USB: xhci: URB
 cancellation support.
 
 Signed-off-by: Julius Werner jwer...@chromium.org
 ---
  drivers/usb/host/xhci-ring.c | 66 
 
  drivers/usb/host/xhci.c  |  1 -
  drivers/usb/host/xhci.h  |  2 --
  3 files changed, 30 insertions(+), 39 deletions(-)
 
 diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
 index a0b248c..b8277c7 100644
 --- a/drivers/usb/host/xhci-ring.c
 +++ b/drivers/usb/host/xhci-ring.c
 @@ -549,6 +549,7 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
   struct xhci_generic_trb *trb;
   struct xhci_ep_ctx *ep_ctx;
   dma_addr_t addr;
 + u64 hw_dequeue;
  
   ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
   ep_index, stream_id);
 @@ -558,56 +559,56 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
   stream_id);
   return;
   }
 - state-new_cycle_state = 0;
 - xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
 - Finding segment containing stopped TRB.);
 - state-new_deq_seg = find_trb_seg(cur_td-start_seg,
 - dev-eps[ep_index].stopped_trb,
 - state-new_cycle_state);
 - if (!state-new_deq_seg) {
 - WARN_ON(1);
 - return;
 - }
  
   /* Dig out the cycle state saved by the xHC during the stop ep cmd */
   xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
   Finding endpoint context);
   ep_ctx = xhci_get_ep_ctx(xhci, dev-out_ctx, ep_index);
 - state-new_cycle_state = 0x1  le64_to_cpu(ep_ctx-deq);
 + hw_dequeue = le64_to_cpu(ep_ctx-deq);
 +
 + /* Find virtual address and segment of hardware dequeue pointer */
 + state-new_deq_seg = ep_ring-deq_seg;
 + state-new_deq_ptr = ep_ring-dequeue;
 + while (xhci_trb_virt_to_dma(state-new_deq_seg, state-new_deq_ptr)
 + != (dma_addr_t)(hw_dequeue  ~0x1)) {
 + next_trb(xhci, ep_ring, state-new_deq_seg,
 + state-new_deq_ptr);
 + if (state-new_deq_ptr == ep_ring-dequeue) {
 + WARN_ON(1);
 + return;
 + }
 + }
  
 + /*
 +  * 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

[PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-02-20 Thread Julius Werner 
We have observed a rare cycle state desync bug after Set TR Dequeue
Pointer commands on Intel LynxPoint xHCs (resulting in an endpoint that
doesn't fetch new TRBs and thus an unresponsive USB device). It always
triggers when a previous Set TR Dequeue Pointer command has set the
pointer to the final Link TRB of a segment, and then another URB gets
enqueued and cancelled again before it can be completed. Further
investigation showed that the xHC had returned the Link TRB in the TRB
Pointer field of the Transfer Event (CC == Stopped -- Length Invalid),
but when xhci_find_new_dequeue_state() later accesses the Endpoint
Context's TR Dequeue Pointer field it is set to the first TRB of the
next segment.

The driver expects those two values to be the same in this situation,
and uses the cycle state of the latter together with the address of the
former. This should be fine according to the XHCI specification, since
the endpoint ring should be stopped when returning the Transfer Event
and thus should not advance over the Link TRB before it gets restarted.
However, real-world XHCI implementations apparently don't really care
that much about these details, so the driver should follow a more
defensive approach to try to work around HC spec violations.

This patch removes the stopped_trb variable that had been used to store
the TRB Pointer from the last Transfer Event of a stopped TRB. Instead,
xhci_find_new_dequeue_state() now relies only on the Endpoint Context,
requiring a small amount of additional processing to find the virtual
address corresponding to the TR Dequeue Pointer. Some other parts of the
function were slightly rearranged to better fit into this model.

This patch should be backported to kernels as old as 2.6.31 that contain
the commit ae636747146ea97efa18e04576acd3416e2514f5 "USB: xhci: URB
cancellation support."

Signed-off-by: Julius Werner 
---
 drivers/usb/host/xhci-ring.c | 66 
 drivers/usb/host/xhci.c  |  1 -
 drivers/usb/host/xhci.h  |  2 --
 3 files changed, 30 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a0b248c..b8277c7 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -549,6 +549,7 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
struct xhci_generic_trb *trb;
struct xhci_ep_ctx *ep_ctx;
dma_addr_t addr;
+   u64 hw_dequeue;
 
ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
ep_index, stream_id);
@@ -558,56 +559,56 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
stream_id);
return;
}
-   state->new_cycle_state = 0;
-   xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-   "Finding segment containing stopped TRB.");
-   state->new_deq_seg = find_trb_seg(cur_td->start_seg,
-   dev->eps[ep_index].stopped_trb,
-   >new_cycle_state);
-   if (!state->new_deq_seg) {
-   WARN_ON(1);
-   return;
-   }
 
/* Dig out the cycle state saved by the xHC during the stop ep cmd */
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"Finding endpoint context");
ep_ctx = xhci_get_ep_ctx(xhci, dev->out_ctx, ep_index);
-   state->new_cycle_state = 0x1 & le64_to_cpu(ep_ctx->deq);
+   hw_dequeue = le64_to_cpu(ep_ctx->deq);
+
+   /* Find virtual address and segment of hardware dequeue pointer */
+   state->new_deq_seg = ep_ring->deq_seg;
+   state->new_deq_ptr = ep_ring->dequeue;
+   while (xhci_trb_virt_to_dma(state->new_deq_seg, state->new_deq_ptr)
+   != (dma_addr_t)(hw_dequeue & ~0x1)) {
+   next_trb(xhci, ep_ring, >new_deq_seg,
+   >new_deq_ptr);
+   if (state->new_deq_ptr == ep_ring->dequeue) {
+   WARN_ON(1);
+   return;
+   }
+   }
 
+   /*
+* 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.
+*/
+   state->new_cycle_state = hw_dequeue & 0x1;
+   if (ep_ring->first_seg == ep_ring->first_seg->next &&
+   cur_td->last_trb < state->new_deq_ptr)
+   state->new_cycle_state ^= 0x1;
state->new_deq_ptr = cur_td->last_trb;
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"Finding segment containing last TRB in TD.");
state->new_deq_seg = find_trb_seg(state->new_deq_seg,
-   state->new_deq_ptr,
-   >new_cycle_state);
+

[PATCH] usb: xhci: Prefer endpoint context dequeue pointer over stopped_trb

2014-02-20 Thread Julius Werner 
We have observed a rare cycle state desync bug after Set TR Dequeue
Pointer commands on Intel LynxPoint xHCs (resulting in an endpoint that
doesn't fetch new TRBs and thus an unresponsive USB device). It always
triggers when a previous Set TR Dequeue Pointer command has set the
pointer to the final Link TRB of a segment, and then another URB gets
enqueued and cancelled again before it can be completed. Further
investigation showed that the xHC had returned the Link TRB in the TRB
Pointer field of the Transfer Event (CC == Stopped -- Length Invalid),
but when xhci_find_new_dequeue_state() later accesses the Endpoint
Context's TR Dequeue Pointer field it is set to the first TRB of the
next segment.

The driver expects those two values to be the same in this situation,
and uses the cycle state of the latter together with the address of the
former. This should be fine according to the XHCI specification, since
the endpoint ring should be stopped when returning the Transfer Event
and thus should not advance over the Link TRB before it gets restarted.
However, real-world XHCI implementations apparently don't really care
that much about these details, so the driver should follow a more
defensive approach to try to work around HC spec violations.

This patch removes the stopped_trb variable that had been used to store
the TRB Pointer from the last Transfer Event of a stopped TRB. Instead,
xhci_find_new_dequeue_state() now relies only on the Endpoint Context,
requiring a small amount of additional processing to find the virtual
address corresponding to the TR Dequeue Pointer. Some other parts of the
function were slightly rearranged to better fit into this model.

This patch should be backported to kernels as old as 2.6.31 that contain
the commit ae636747146ea97efa18e04576acd3416e2514f5 USB: xhci: URB
cancellation support.

Signed-off-by: Julius Werner jwer...@chromium.org
---
 drivers/usb/host/xhci-ring.c | 66 
 drivers/usb/host/xhci.c  |  1 -
 drivers/usb/host/xhci.h  |  2 --
 3 files changed, 30 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a0b248c..b8277c7 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -549,6 +549,7 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
struct xhci_generic_trb *trb;
struct xhci_ep_ctx *ep_ctx;
dma_addr_t addr;
+   u64 hw_dequeue;
 
ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
ep_index, stream_id);
@@ -558,56 +559,56 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
stream_id);
return;
}
-   state-new_cycle_state = 0;
-   xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-   Finding segment containing stopped TRB.);
-   state-new_deq_seg = find_trb_seg(cur_td-start_seg,
-   dev-eps[ep_index].stopped_trb,
-   state-new_cycle_state);
-   if (!state-new_deq_seg) {
-   WARN_ON(1);
-   return;
-   }
 
/* Dig out the cycle state saved by the xHC during the stop ep cmd */
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
Finding endpoint context);
ep_ctx = xhci_get_ep_ctx(xhci, dev-out_ctx, ep_index);
-   state-new_cycle_state = 0x1  le64_to_cpu(ep_ctx-deq);
+   hw_dequeue = le64_to_cpu(ep_ctx-deq);
+
+   /* Find virtual address and segment of hardware dequeue pointer */
+   state-new_deq_seg = ep_ring-deq_seg;
+   state-new_deq_ptr = ep_ring-dequeue;
+   while (xhci_trb_virt_to_dma(state-new_deq_seg, state-new_deq_ptr)
+   != (dma_addr_t)(hw_dequeue  ~0x1)) {
+   next_trb(xhci, ep_ring, state-new_deq_seg,
+   state-new_deq_ptr);
+   if (state-new_deq_ptr == ep_ring-dequeue) {
+   WARN_ON(1);
+   return;
+   }
+   }
 
+   /*
+* 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.
+*/
+   state-new_cycle_state = hw_dequeue  0x1;
+   if (ep_ring-first_seg == ep_ring-first_seg-next 
+   cur_td-last_trb  state-new_deq_ptr)
+   state-new_cycle_state ^= 0x1;
state-new_deq_ptr = cur_td-last_trb;
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
Finding segment containing last TRB in TD.);
state-new_deq_seg = find_trb_seg(state-new_deq_seg,
-   state-new_deq_ptr,
-   state-new_cycle_state);
+   state-new_deq_ptr,