On 01/18/2012 07:01 AM, Sarah Sharp wrote:
> The cancellation and stall endpoint handlers will use a Set TR Dequeue
> Pointer command to move the dequeue pointers past the offending TD.
> Since link TRBs are not given to the hardware until a new TD is enqueued
> to the top of the next segment, the command may set the new dequeue
> pointer to point to the link TRB.
>
> This would cause the old code in handle_set_deq_completion() to skip
> over the link TRB without checking to see if it matched the new dequeue
> pointer. It would then go into an infinite loop, looking for the new
> dequeue pointer in every TRB in the ring, except for the link TRB.
>
> Andiry, perhaps you want to add a bit of code to store the initial ring
> dequeue pointer, and break out of this loop if we come back to it
> without finding the correct dequeue pointer? You could store the
> current number of freed TRB and restore it if that happens. Also, this
> particular chunk of code is indented way too far, so you should refactor
> it into a new function. I don't think you need a while loop for
> checking the last_trb(), since it's against the xHCI rules to have two
> link TRBs in a row.
>
> The USB 3.0 card reader I have will stall every SCSI command while it is
> empty, so it was only a matter of time before a TD was queued just
> before the link TRB. That's why my box would always hang with the ring
> expansion patchset and that device.
>
> Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
> Cc: Andiry Xu <andiry.xu@xxxxxxx>
> ---
>
> Hi Andiry,
>
> I didn't get around to applying your revert patch because I looked at
> the new Set TR dequeue pointer command code and found the bug.
> Feel free to integrate this fix into your next patchset version.
>
Thanks for the patch. I'll merge it. From my understanding, the enqueue
pointer may stay on the link TRB, while the dequeue pointer never does
so. Anyway, I'll add a check to make sure it does not fall into an
infinite loop.
Thanks,
Andiry
>
> drivers/usb/host/xhci-ring.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 4163de7..ccfbec2 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1080,8 +1080,11 @@ static void handle_set_deq_completion(struct xhci_hcd *xhci,
> /* We have more usable TRBs */
> ep_ring->num_trbs_free++;
> ep_ring->dequeue++;
> - while (last_trb(xhci, ep_ring, ep_ring->deq_seg,
> + if (last_trb(xhci, ep_ring, ep_ring->deq_seg,
> ep_ring->dequeue)) {
> + if (ep_ring->dequeue ==
> + dev->eps[ep_index].queued_deq_ptr)
> + break;
> ep_ring->deq_seg =
> ep_ring->deq_seg->next;
> ep_ring->dequeue =
--
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
