On Fri, Jul 25, 2014 at 8:47 AM, Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> wrote:
> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/host/xhci-ring.c | 95 +++++++++++++++-----------------------------
> 1 file changed, 31 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 749fc68..35da18c 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,11 @@ 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, *tmp_trb;
> dma_addr_t addr;
> u64 hw_dequeue;
> + bool cycle_found = false;
>
> ep_ring = xhci_triad_to_transfer_ring(xhci, slot_id,
> ep_index, stream_id);
> @@ -486,45 +462,36 @@ 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 it's found.
> + * Once found we can jump a whole segments, but carefully cross them
> + * to check for cycle toggles.
> */
> - 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);
> - if (!state->new_deq_seg) {
> - WARN_ON(1);
> - return;
> - }
> -
> - /* 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);
> + do {
> + if (cycle_found) {
> + if (new_seg->trbs > cur_td->last_trb ||
> + &new_seg->trbs[TRBS_PER_SEGMENT - 1] < cur_td->last_trb)
> + new_deq = &new_seg->trbs[TRBS_PER_SEGMENT - 1];
> + } else if (xhci_trb_virt_to_dma(new_seg, new_deq)
> + == (dma_addr_t)(hw_dequeue & ~0xf))
> + cycle_found = true;
> + tmp_trb = new_deq;
> + next_trb(xhci, ep_ring, &new_seg, &new_deq);
> +
> + if (cycle_found &&
> + TRB_TYPE_LINK_LE32(tmp_trb->generic.field[3]) &&
> + tmp_trb->generic.field[3] & cpu_to_le32(LINK_TOGGLE))
> + state->new_cycle_state ^= 0x1;
> +
> + } while (tmp_trb != cur_td->last_trb && new_deq != ep->ring->dequeue);
What happens when new_deq == ep->ring->dequeue? I'm not sure if
there's anything good you could do (save for reinitializing the whole
ring), and it shouldn't happen anyway, but you should probably at
least print some kind of warning.
> +
> + 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,
> --
> 1.8.3.2
>
Looks pretty good! Merging both searches together like that really
does make it look much simpler.
But it makes the slightly dangerous assumption that hw_dequeue is at
most one TRB after last_trb (because once you found last_trb you
always exit even if cycle_found hasn't been set yet). I'm worried
about what Maciej's controller will do if it hits a Stall Error on the
last Normal TRB before a Link TRB with cycle change (a hard case to
test, unfortunately). Will it just put hw_dequeue on the Link TRB, or
will it keep advancing further to the first TRB of the next segment?
In the latter case, your cycle state would be wrong with this code
(because you set the dequeue pointer to the Link TRB but give it the
cycle state from the next segment).
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
