* Re: [PATCH 3/3] xhci: rework cycle bit checking for new dequeue pointers
[not found] ` <1408450678-5653-4-git-send-email-mathias.nyman@linux.intel.com>
@ 2014-08-20 22:06 ` Joseph Salisbury
2014-08-27 11:07 ` Mathias Nyman
0 siblings, 1 reply; 3+ messages in thread
From: Joseph Salisbury @ 2014-08-20 22:06 UTC (permalink / raw
To: Mathias Nyman; +Cc: gregkh, linux-usb, stable, Dustin Kirkland, LKML
On 08/19/2014 08:17 AM, Mathias Nyman wrote:
> When we manually need to move the TR dequeue pointer we need to set the
> correct cycle bit as well. Previously we used the trb pointer from the
> last event received as a base, but this was changed in
> commit 1f81b6d22a59 ("usb: xhci: Prefer endpoint context dequeue pointer")
> to use the dequeue pointer from the endpoint context instead
>
> It turns out some Asmedia controllers advance the dequeue pointer
> stored in the endpoint context past the event triggering TRB, and
> this messed up the way the cycle bit was calculated.
>
> Instead of adding a quirk or complicating the already hard to follow cycle bit
> code, the whole cycle bit calculation is now simplified and adapted to handle
> event and endpoint context dequeue pointer differences.
>
> Fixes: 1f81b6d22a59 ("usb: xhci: Prefer endpoint context dequeue pointer")
> Reported-by: Maciej Puzio <mx34567@gmail.com>
> Reported-by: Evan Langlois <uudruid74@gmail.com>
> Reviewed-by: Julius Werner <jwerner@chromium.org>
> Tested-by: Maciej Puzio <mx34567@gmail.com>
> Tested-by: Evan Langlois <uudruid74@gmail.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
> drivers/usb/host/xhci-ring.c | 101 +++++++++++++++++--------------------------
> drivers/usb/host/xhci.c | 3 ++
> 2 files changed, 42 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index ac8cf23..abed30b 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -364,32 +364,6 @@ static void ring_doorbell_for_active_rings(struct xhci_hcd *xhci,
> }
> }
>
> -/*
> - * Find the segment that trb is in. Start searching in start_seg.
> - * If we must move past a segment that has a link TRB with a toggle cycle state
> - * bit set, then we will toggle the value pointed at by cycle_state.
> - */
> -static struct xhci_segment *find_trb_seg(
> - struct xhci_segment *start_seg,
> - union xhci_trb *trb, int *cycle_state)
> -{
> - struct xhci_segment *cur_seg = start_seg;
> - struct xhci_generic_trb *generic_trb;
> -
> - while (cur_seg->trbs > trb ||
> - &cur_seg->trbs[TRBS_PER_SEGMENT - 1] < trb) {
> - generic_trb = &cur_seg->trbs[TRBS_PER_SEGMENT - 1].generic;
> - if (generic_trb->field[3] & cpu_to_le32(LINK_TOGGLE))
> - *cycle_state ^= 0x1;
> - cur_seg = cur_seg->next;
> - if (cur_seg == start_seg)
> - /* Looped over the entire list. Oops! */
> - return NULL;
> - }
> - return cur_seg;
> -}
> -
> -
> static struct xhci_ring *xhci_triad_to_transfer_ring(struct xhci_hcd *xhci,
> unsigned int slot_id, unsigned int ep_index,
> unsigned int stream_id)
> @@ -459,9 +433,12 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
> struct xhci_virt_device *dev = xhci->devs[slot_id];
> struct xhci_virt_ep *ep = &dev->eps[ep_index];
> struct xhci_ring *ep_ring;
> - struct xhci_generic_trb *trb;
> + struct xhci_segment *new_seg;
> + union xhci_trb *new_deq;
> dma_addr_t addr;
> u64 hw_dequeue;
> + bool cycle_found = false;
> + bool td_last_trb_found = false;
>
> ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
> ep_index, stream_id);
> @@ -486,45 +463,45 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
> 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 & ~0xf)) {
> - 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;
> - }
> - }
> + new_seg = ep_ring->deq_seg;
> + new_deq = ep_ring->dequeue;
> + state->new_cycle_state = hw_dequeue & 0x1;
> +
> /*
> - * 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.
> + * We want to find the pointer, segment and cycle state of the new trb
> + * (the one after current TD's last_trb). We know the cycle state at
> + * hw_dequeue, so walk the ring until both hw_dequeue and last_trb are
> + * found.
> */
> - 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;
> + do {
> + if (!cycle_found && xhci_trb_virt_to_dma(new_seg, new_deq)
> + == (dma_addr_t)(hw_dequeue & ~0xf)) {
> + cycle_found = true;
> + if (td_last_trb_found)
> + break;
> + }
> + if (new_deq == cur_td->last_trb)
> + td_last_trb_found = true;
>
> - 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);
> - if (!state->new_deq_seg) {
> - WARN_ON(1);
> - return;
> - }
> + if (cycle_found &&
> + TRB_TYPE_LINK_LE32(new_deq->generic.field[3]) &&
> + new_deq->generic.field[3] & cpu_to_le32(LINK_TOGGLE))
> + state->new_cycle_state ^= 0x1;
> +
> + next_trb(xhci, ep_ring, &new_seg, &new_deq);
> +
> + /* Search wrapped around, bail out */
> + if (new_deq == ep->ring->dequeue) {
> + xhci_err(xhci, "Error: Failed finding new dequeue state\n");
> + state->new_deq_seg = NULL;
> + state->new_deq_ptr = NULL;
> + return;
> + }
> +
> + } while (!cycle_found || !td_last_trb_found);
>
> - /* Increment to find next TRB after last_trb. Cycle if appropriate. */
> - trb = &state->new_deq_ptr->generic;
> - if (TRB_TYPE_LINK_LE32(trb->field[3]) &&
> - (trb->field[3] & cpu_to_le32(LINK_TOGGLE)))
> - state->new_cycle_state ^= 0x1;
> - next_trb(xhci, ep_ring, &state->new_deq_seg, &state->new_deq_ptr);
> + state->new_deq_seg = new_seg;
> + state->new_deq_ptr = new_deq;
>
> /* Don't update the ring cycle state for the producer (us). */
> xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index b6f2117..c020b09 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -2880,6 +2880,9 @@ void xhci_cleanup_stalled_ring(struct xhci_hcd *xhci,
> ep_index, ep->stopped_stream, ep->stopped_td,
> &deq_state);
>
> + if (!deq_state.new_deq_ptr || !deq_state.new_deq_seg)
> + return;
> +
> /* HW with the reset endpoint quirk will use the saved dequeue state to
> * issue a configure endpoint command later.
> */
Hi Mathias,
Some of the stable kernel versions fail to build with this patch, 3.2.y
and 3.13.y for example. This is because the function 'find_trb_seg' is
still called by xhci_cmd_to_noop, which is removed from mainline but
still exists in the stable kernels. The patch removes the definition of
find_trb_seg, which is what causes the build to fail.
Should we leave the definition of find_trb_seg in your patch for the
stable kernels, or do you have other ideas?
Thanks,
Joe
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 3/3] xhci: rework cycle bit checking for new dequeue pointers
2014-08-20 22:06 ` [PATCH 3/3] xhci: rework cycle bit checking for new dequeue pointers Joseph Salisbury
@ 2014-08-27 11:07 ` Mathias Nyman
2014-08-27 14:22 ` Joseph Salisbury
0 siblings, 1 reply; 3+ messages in thread
From: Mathias Nyman @ 2014-08-27 11:07 UTC (permalink / raw
To: Joseph Salisbury; +Cc: gregkh, linux-usb, stable, Dustin Kirkland, LKML
On 08/21/2014 01:06 AM, Joseph Salisbury wrote:
> On 08/19/2014 08:17 AM, Mathias Nyman wrote:
>> When we manually need to move the TR dequeue pointer we need to set the
>> correct cycle bit as well. Previously we used the trb pointer from the
>> last event received as a base, but this was changed in
>> commit 1f81b6d22a59 ("usb: xhci: Prefer endpoint context dequeue pointer")
>> to use the dequeue pointer from the endpoint context instead
>>
>> It turns out some Asmedia controllers advance the dequeue pointer
>> stored in the endpoint context past the event triggering TRB, and
>> this messed up the way the cycle bit was calculated.
>>
>> Instead of adding a quirk or complicating the already hard to follow cycle bit
>> code, the whole cycle bit calculation is now simplified and adapted to handle
>> event and endpoint context dequeue pointer differences.
>>
>> Fixes: 1f81b6d22a59 ("usb: xhci: Prefer endpoint context dequeue pointer")
>> Reported-by: Maciej Puzio <mx34567@gmail.com>
>> Reported-by: Evan Langlois <uudruid74@gmail.com>
>> Reviewed-by: Julius Werner <jwerner@chromium.org>
>> Tested-by: Maciej Puzio <mx34567@gmail.com>
>> Tested-by: Evan Langlois <uudruid74@gmail.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Cc: stable@vger.kernel.org
>> ---
>> drivers/usb/host/xhci-ring.c | 101 +++++++++++++++++--------------------------
>> drivers/usb/host/xhci.c | 3 ++
>> 2 files changed, 42 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index ac8cf23..abed30b 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -364,32 +364,6 @@ static void ring_doorbell_for_active_rings(struct xhci_hcd *xhci,
>> }
>> }
>>
>> -/*
>> - * Find the segment that trb is in. Start searching in start_seg.
>> - * If we must move past a segment that has a link TRB with a toggle cycle state
>> - * bit set, then we will toggle the value pointed at by cycle_state.
>> - */
>> -static struct xhci_segment *find_trb_seg(
>> - struct xhci_segment *start_seg,
>> - union xhci_trb *trb, int *cycle_state)
>> -{
>> - struct xhci_segment *cur_seg = start_seg;
>> - struct xhci_generic_trb *generic_trb;
>> -
>> - while (cur_seg->trbs > trb ||
>> - &cur_seg->trbs[TRBS_PER_SEGMENT - 1] < trb) {
>> - generic_trb = &cur_seg->trbs[TRBS_PER_SEGMENT - 1].generic;
>> - if (generic_trb->field[3] & cpu_to_le32(LINK_TOGGLE))
>> - *cycle_state ^= 0x1;
>> - cur_seg = cur_seg->next;
>> - if (cur_seg == start_seg)
>> - /* Looped over the entire list. Oops! */
>> - return NULL;
>> - }
>> - return cur_seg;
>> -}
>> -
>> -
>> static struct xhci_ring *xhci_triad_to_transfer_ring(struct xhci_hcd *xhci,
>> unsigned int slot_id, unsigned int ep_index,
>> unsigned int stream_id)
>> @@ -459,9 +433,12 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
>> struct xhci_virt_device *dev = xhci->devs[slot_id];
>> struct xhci_virt_ep *ep = &dev->eps[ep_index];
>> struct xhci_ring *ep_ring;
>> - struct xhci_generic_trb *trb;
>> + struct xhci_segment *new_seg;
>> + union xhci_trb *new_deq;
>> dma_addr_t addr;
>> u64 hw_dequeue;
>> + bool cycle_found = false;
>> + bool td_last_trb_found = false;
>>
>> ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
>> ep_index, stream_id);
>> @@ -486,45 +463,45 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
>> 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 & ~0xf)) {
>> - 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;
>> - }
>> - }
>> + new_seg = ep_ring->deq_seg;
>> + new_deq = ep_ring->dequeue;
>> + state->new_cycle_state = hw_dequeue & 0x1;
>> +
>> /*
>> - * 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.
>> + * We want to find the pointer, segment and cycle state of the new trb
>> + * (the one after current TD's last_trb). We know the cycle state at
>> + * hw_dequeue, so walk the ring until both hw_dequeue and last_trb are
>> + * found.
>> */
>> - 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;
>> + do {
>> + if (!cycle_found && xhci_trb_virt_to_dma(new_seg, new_deq)
>> + == (dma_addr_t)(hw_dequeue & ~0xf)) {
>> + cycle_found = true;
>> + if (td_last_trb_found)
>> + break;
>> + }
>> + if (new_deq == cur_td->last_trb)
>> + td_last_trb_found = true;
>>
>> - 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);
>> - if (!state->new_deq_seg) {
>> - WARN_ON(1);
>> - return;
>> - }
>> + if (cycle_found &&
>> + TRB_TYPE_LINK_LE32(new_deq->generic.field[3]) &&
>> + new_deq->generic.field[3] & cpu_to_le32(LINK_TOGGLE))
>> + state->new_cycle_state ^= 0x1;
>> +
>> + next_trb(xhci, ep_ring, &new_seg, &new_deq);
>> +
>> + /* Search wrapped around, bail out */
>> + if (new_deq == ep->ring->dequeue) {
>> + xhci_err(xhci, "Error: Failed finding new dequeue state\n");
>> + state->new_deq_seg = NULL;
>> + state->new_deq_ptr = NULL;
>> + return;
>> + }
>> +
>> + } while (!cycle_found || !td_last_trb_found);
>>
>> - /* Increment to find next TRB after last_trb. Cycle if appropriate. */
>> - trb = &state->new_deq_ptr->generic;
>> - if (TRB_TYPE_LINK_LE32(trb->field[3]) &&
>> - (trb->field[3] & cpu_to_le32(LINK_TOGGLE)))
>> - state->new_cycle_state ^= 0x1;
>> - next_trb(xhci, ep_ring, &state->new_deq_seg, &state->new_deq_ptr);
>> + state->new_deq_seg = new_seg;
>> + state->new_deq_ptr = new_deq;
>>
>> /* Don't update the ring cycle state for the producer (us). */
>> xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index b6f2117..c020b09 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -2880,6 +2880,9 @@ void xhci_cleanup_stalled_ring(struct xhci_hcd *xhci,
>> ep_index, ep->stopped_stream, ep->stopped_td,
>> &deq_state);
>>
>> + if (!deq_state.new_deq_ptr || !deq_state.new_deq_seg)
>> + return;
>> +
>> /* HW with the reset endpoint quirk will use the saved dequeue state to
>> * issue a configure endpoint command later.
>> */
>
> Hi Mathias,
>
> Some of the stable kernel versions fail to build with this patch, 3.2.y
> and 3.13.y for example. This is because the function 'find_trb_seg' is
> still called by xhci_cmd_to_noop, which is removed from mainline but
> still exists in the stable kernels. The patch removes the definition of
> find_trb_seg, which is what causes the build to fail.
>
> Should we leave the definition of find_trb_seg in your patch for the
> stable kernels, or do you have other ideas?
>
You can leave the find_trb_seg() function there for xhci_cmd_to_noop() to use in older kernels
Should I backport it against 3.13.y for you?
-Mathias
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 3/3] xhci: rework cycle bit checking for new dequeue pointers
2014-08-27 11:07 ` Mathias Nyman
@ 2014-08-27 14:22 ` Joseph Salisbury
0 siblings, 0 replies; 3+ messages in thread
From: Joseph Salisbury @ 2014-08-27 14:22 UTC (permalink / raw
To: Mathias Nyman; +Cc: gregkh, linux-usb, stable, Dustin Kirkland, LKML
On 08/27/2014 07:07 AM, Mathias Nyman wrote:
> On 08/21/2014 01:06 AM, Joseph Salisbury wrote:
>> On 08/19/2014 08:17 AM, Mathias Nyman wrote:
>>> When we manually need to move the TR dequeue pointer we need to set the
>>> correct cycle bit as well. Previously we used the trb pointer from the
>>> last event received as a base, but this was changed in
>>> commit 1f81b6d22a59 ("usb: xhci: Prefer endpoint context dequeue pointer")
>>> to use the dequeue pointer from the endpoint context instead
>>>
>>> It turns out some Asmedia controllers advance the dequeue pointer
>>> stored in the endpoint context past the event triggering TRB, and
>>> this messed up the way the cycle bit was calculated.
>>>
>>> Instead of adding a quirk or complicating the already hard to follow cycle bit
>>> code, the whole cycle bit calculation is now simplified and adapted to handle
>>> event and endpoint context dequeue pointer differences.
>>>
>>> Fixes: 1f81b6d22a59 ("usb: xhci: Prefer endpoint context dequeue pointer")
>>> Reported-by: Maciej Puzio <mx34567@gmail.com>
>>> Reported-by: Evan Langlois <uudruid74@gmail.com>
>>> Reviewed-by: Julius Werner <jwerner@chromium.org>
>>> Tested-by: Maciej Puzio <mx34567@gmail.com>
>>> Tested-by: Evan Langlois <uudruid74@gmail.com>
>>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>> Cc: stable@vger.kernel.org
>>> ---
>>> drivers/usb/host/xhci-ring.c | 101 +++++++++++++++++--------------------------
>>> drivers/usb/host/xhci.c | 3 ++
>>> 2 files changed, 42 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>>> index ac8cf23..abed30b 100644
>>> --- a/drivers/usb/host/xhci-ring.c
>>> +++ b/drivers/usb/host/xhci-ring.c
>>> @@ -364,32 +364,6 @@ static void ring_doorbell_for_active_rings(struct xhci_hcd *xhci,
>>> }
>>> }
>>>
>>> -/*
>>> - * Find the segment that trb is in. Start searching in start_seg.
>>> - * If we must move past a segment that has a link TRB with a toggle cycle state
>>> - * bit set, then we will toggle the value pointed at by cycle_state.
>>> - */
>>> -static struct xhci_segment *find_trb_seg(
>>> - struct xhci_segment *start_seg,
>>> - union xhci_trb *trb, int *cycle_state)
>>> -{
>>> - struct xhci_segment *cur_seg = start_seg;
>>> - struct xhci_generic_trb *generic_trb;
>>> -
>>> - while (cur_seg->trbs > trb ||
>>> - &cur_seg->trbs[TRBS_PER_SEGMENT - 1] < trb) {
>>> - generic_trb = &cur_seg->trbs[TRBS_PER_SEGMENT - 1].generic;
>>> - if (generic_trb->field[3] & cpu_to_le32(LINK_TOGGLE))
>>> - *cycle_state ^= 0x1;
>>> - cur_seg = cur_seg->next;
>>> - if (cur_seg == start_seg)
>>> - /* Looped over the entire list. Oops! */
>>> - return NULL;
>>> - }
>>> - return cur_seg;
>>> -}
>>> -
>>> -
>>> static struct xhci_ring *xhci_triad_to_transfer_ring(struct xhci_hcd *xhci,
>>> unsigned int slot_id, unsigned int ep_index,
>>> unsigned int stream_id)
>>> @@ -459,9 +433,12 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
>>> struct xhci_virt_device *dev = xhci->devs[slot_id];
>>> struct xhci_virt_ep *ep = &dev->eps[ep_index];
>>> struct xhci_ring *ep_ring;
>>> - struct xhci_generic_trb *trb;
>>> + struct xhci_segment *new_seg;
>>> + union xhci_trb *new_deq;
>>> dma_addr_t addr;
>>> u64 hw_dequeue;
>>> + bool cycle_found = false;
>>> + bool td_last_trb_found = false;
>>>
>>> ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
>>> ep_index, stream_id);
>>> @@ -486,45 +463,45 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
>>> 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 & ~0xf)) {
>>> - 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;
>>> - }
>>> - }
>>> + new_seg = ep_ring->deq_seg;
>>> + new_deq = ep_ring->dequeue;
>>> + state->new_cycle_state = hw_dequeue & 0x1;
>>> +
>>> /*
>>> - * 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.
>>> + * We want to find the pointer, segment and cycle state of the new trb
>>> + * (the one after current TD's last_trb). We know the cycle state at
>>> + * hw_dequeue, so walk the ring until both hw_dequeue and last_trb are
>>> + * found.
>>> */
>>> - 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;
>>> + do {
>>> + if (!cycle_found && xhci_trb_virt_to_dma(new_seg, new_deq)
>>> + == (dma_addr_t)(hw_dequeue & ~0xf)) {
>>> + cycle_found = true;
>>> + if (td_last_trb_found)
>>> + break;
>>> + }
>>> + if (new_deq == cur_td->last_trb)
>>> + td_last_trb_found = true;
>>>
>>> - 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);
>>> - if (!state->new_deq_seg) {
>>> - WARN_ON(1);
>>> - return;
>>> - }
>>> + if (cycle_found &&
>>> + TRB_TYPE_LINK_LE32(new_deq->generic.field[3]) &&
>>> + new_deq->generic.field[3] & cpu_to_le32(LINK_TOGGLE))
>>> + state->new_cycle_state ^= 0x1;
>>> +
>>> + next_trb(xhci, ep_ring, &new_seg, &new_deq);
>>> +
>>> + /* Search wrapped around, bail out */
>>> + if (new_deq == ep->ring->dequeue) {
>>> + xhci_err(xhci, "Error: Failed finding new dequeue state\n");
>>> + state->new_deq_seg = NULL;
>>> + state->new_deq_ptr = NULL;
>>> + return;
>>> + }
>>> +
>>> + } while (!cycle_found || !td_last_trb_found);
>>>
>>> - /* Increment to find next TRB after last_trb. Cycle if appropriate. */
>>> - trb = &state->new_deq_ptr->generic;
>>> - if (TRB_TYPE_LINK_LE32(trb->field[3]) &&
>>> - (trb->field[3] & cpu_to_le32(LINK_TOGGLE)))
>>> - state->new_cycle_state ^= 0x1;
>>> - next_trb(xhci, ep_ring, &state->new_deq_seg, &state->new_deq_ptr);
>>> + state->new_deq_seg = new_seg;
>>> + state->new_deq_ptr = new_deq;
>>>
>>> /* Don't update the ring cycle state for the producer (us). */
>>> xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>> index b6f2117..c020b09 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>>> @@ -2880,6 +2880,9 @@ void xhci_cleanup_stalled_ring(struct xhci_hcd *xhci,
>>> ep_index, ep->stopped_stream, ep->stopped_td,
>>> &deq_state);
>>>
>>> + if (!deq_state.new_deq_ptr || !deq_state.new_deq_seg)
>>> + return;
>>> +
>>> /* HW with the reset endpoint quirk will use the saved dequeue state to
>>> * issue a configure endpoint command later.
>>> */
>>
>> Hi Mathias,
>>
>> Some of the stable kernel versions fail to build with this patch, 3.2.y
>> and 3.13.y for example. This is because the function 'find_trb_seg' is
>> still called by xhci_cmd_to_noop, which is removed from mainline but
>> still exists in the stable kernels. The patch removes the definition of
>> find_trb_seg, which is what causes the build to fail.
>>
>> Should we leave the definition of find_trb_seg in your patch for the
>> stable kernels, or do you have other ideas?
>>
> You can leave the find_trb_seg() function there for xhci_cmd_to_noop() to use in older kernels
>
> Should I backport it against 3.13.y for you?
No need for you to backport. I just wanted to confirm that leaving
find_trb_seg() was an ok approach.
>
> -Mathias
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-08-27 14:22 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1408450678-5653-1-git-send-email-mathias.nyman@linux.intel.com>
[not found] ` <1408450678-5653-4-git-send-email-mathias.nyman@linux.intel.com>
2014-08-20 22:06 ` [PATCH 3/3] xhci: rework cycle bit checking for new dequeue pointers Joseph Salisbury
2014-08-27 11:07 ` Mathias Nyman
2014-08-27 14:22 ` Joseph Salisbury
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.