Hi Andiry,
The bug I found in John's link TRB activation patch would also effect
isochronous TDs that end just before the link TRB, when the URB needs
more TDs enqueued after that. Have you run into this problem?
Also, how are you doing on the isoch patches? Did you end up fixing the
problem when multiple URBs are skipped? I hope you're not waiting on me
to refactor the handle_tx_event() function, but if you are, please let
me know. I'd like to get all your patches (including the PM patches)
reviewed, tested, and queued before I leave for a conference/vacation on
June 25th. Otherwise they will have to wait until after July 12th.
Sarah Sharp
On Wed, Jun 09, 2010 at 05:22:33PM -0700, Sarah Sharp wrote:
> John, can you test this patch with your UAS device? It fixes the
> control transfer issue with your link TRB activation patch, but I'm not
> sure if it works for streams.
>
> --->8------------------------------------------------------------8<---
> Subject: [PATCH] xHCI: Fix bug in link TRB activation change.
>
> Commit 6c12db90f19727c76990e7f4801c67a148b30111 introduced a bug for
> control transfers. The patch was supposed to change when the link TRBs at
> the end of each ring segment were given to the hardware. If a transfer
> descriptor (TD) ended just before the link TRB, the code wouldn't give
> back the link TRB to the hardware; instead it would be given back in
> prepare_ring() just before the next TD was enqueued at the top of the
> ring.
>
> Unfortunately, the code relied on checking the chain bit of the TRB to
> determine whether the TD ended just before the link TRB. It assumed that
> the ring enqueuing code would call prepare_ring() before enqueuing the
> next TD. However, control transfers are made of multiple TDs, and
> prepare_ring() is only called once before enqueuing two or three TDs.
>
> If the first or second TD of the control transfer ended just before the
> link TRB, then the code in inc_enq() would not move the enqueue pointer
> past the link TRB, and the link TRB would get overwritten. This would
> cause the xHCI driver to start writing to memory past the ring segment,
> and eventually the system would crash or hang.
>
> The fix is to add a flag to inc_enq() that says whether the caller will
> enqueue more TDs before calling prepare_ring(). If the chain bit is
> cleared (meaning this is the last TRB in a TD), and the caller will not
> enqueue more TDs, then we defer giving back the link TRB.
>
> Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/host/xhci-ring.c | 62 +++++++++++++++++++++++++++++++-----------
> 1 files changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 0529a15..f566182 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -182,8 +182,12 @@ static void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring, bool consumer
> * set, but other sections talk about dealing with the chain bit set. This was
> * fixed in the 0.96 specification errata, but we have to assume that all 0.95
> * xHCI hardware can't handle the chain bit being cleared on a link TRB.
> + *
> + * @more_trbs_coming: Will you enqueue more TRBs before calling
> + * prepare_transfer()?
> */
> -static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring, bool consumer)
> +static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring,
> + bool consumer, bool more_trbs_coming)
> {
> u32 chain;
> union xhci_trb *next;
> @@ -199,15 +203,28 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring, bool consumer
> while (last_trb(xhci, ring, ring->enq_seg, next)) {
> if (!consumer) {
> if (ring != xhci->event_ring) {
> - if (chain) {
> - next->link.control |= TRB_CHAIN;
> -
> - /* Give this link TRB to the hardware */
> - wmb();
> - next->link.control ^= TRB_CYCLE;
> - } else {
> + /*
> + * If the caller doesn't plan on enqueueing more
> + * TDs before ringing the doorbell, then we
> + * don't want to give the link TRB to the
> + * hardware just yet. We'll give the link TRB
> + * back in prepare_ring() just before we enqueue
> + * the TD at the top of the ring.
> + */
> + if (!chain && !more_trbs_coming)
> break;
> +
> + /* If we're not dealing with 0.95 hardware,
> + * carry over the chain bit of the previous TRB
> + * (which may mean the chain bit is cleared).
> + */
> + if (!xhci_link_trb_quirk(xhci)) {
> + next->link.control &= ~TRB_CHAIN;
> + next->link.control |= chain;
> }
> + /* Give this link TRB to the hardware */
> + wmb();
> + next->link.control ^= TRB_CYCLE;
> }
> /* Toggle the cycle bit after the last ring segment. */
> if (last_trb_on_last_seg(xhci, ring, ring->enq_seg, next)) {
> @@ -1684,9 +1701,12 @@ void xhci_handle_event(struct xhci_hcd *xhci)
> /*
> * Generic function for queueing a TRB on a ring.
> * The caller must have checked to make sure there's room on the ring.
> + *
> + * @more_trbs_coming: Will you enqueue more TRBs before calling
> + * prepare_transfer()?
> */
> static void queue_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
> - bool consumer,
> + bool consumer, bool more_trbs_coming,
> u32 field1, u32 field2, u32 field3, u32 field4)
> {
> struct xhci_generic_trb *trb;
> @@ -1696,7 +1716,7 @@ static void queue_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
> trb->field[1] = field2;
> trb->field[2] = field3;
> trb->field[3] = field4;
> - inc_enq(xhci, ring, consumer);
> + inc_enq(xhci, ring, consumer, more_trbs_coming);
> }
>
> /*
> @@ -1965,6 +1985,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> int trb_buff_len, this_sg_len, running_total;
> bool first_trb;
> u64 addr;
> + bool more_trbs_coming;
>
> struct xhci_generic_trb *start_trb;
> int start_cycle;
> @@ -2050,7 +2071,11 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> length_field = TRB_LEN(trb_buff_len) |
> remainder |
> TRB_INTR_TARGET(0);
> - queue_trb(xhci, ep_ring, false,
> + if (num_trbs > 1)
> + more_trbs_coming = true;
> + else
> + more_trbs_coming = false;
> + queue_trb(xhci, ep_ring, false, more_trbs_coming,
> lower_32_bits(addr),
> upper_32_bits(addr),
> length_field,
> @@ -2101,6 +2126,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> int num_trbs;
> struct xhci_generic_trb *start_trb;
> bool first_trb;
> + bool more_trbs_coming;
> int start_cycle;
> u32 field, length_field;
>
> @@ -2189,7 +2215,11 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> length_field = TRB_LEN(trb_buff_len) |
> remainder |
> TRB_INTR_TARGET(0);
> - queue_trb(xhci, ep_ring, false,
> + if (num_trbs > 1)
> + more_trbs_coming = true;
> + else
> + more_trbs_coming = false;
> + queue_trb(xhci, ep_ring, false, more_trbs_coming,
> lower_32_bits(addr),
> upper_32_bits(addr),
> length_field,
> @@ -2268,7 +2298,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> /* Queue setup TRB - see section 6.4.1.2.1 */
> /* FIXME better way to translate setup_packet into two u32 fields? */
> setup = (struct usb_ctrlrequest *) urb->setup_packet;
> - queue_trb(xhci, ep_ring, false,
> + queue_trb(xhci, ep_ring, false, true,
> /* FIXME endianness is probably going to bite my ass here. */
> setup->bRequestType | setup->bRequest << 8 | setup->wValue << 16,
> setup->wIndex | setup->wLength << 16,
> @@ -2284,7 +2314,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> if (urb->transfer_buffer_length > 0) {
> if (setup->bRequestType & USB_DIR_IN)
> field |= TRB_DIR_IN;
> - queue_trb(xhci, ep_ring, false,
> + queue_trb(xhci, ep_ring, false, true,
> lower_32_bits(urb->transfer_dma),
> upper_32_bits(urb->transfer_dma),
> length_field,
> @@ -2301,7 +2331,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> field = 0;
> else
> field = TRB_DIR_IN;
> - queue_trb(xhci, ep_ring, false,
> + queue_trb(xhci, ep_ring, false, false,
> 0,
> 0,
> TRB_INTR_TARGET(0),
> @@ -2338,7 +2368,7 @@ static int queue_command(struct xhci_hcd *xhci, u32 field1, u32 field2,
> "unfailable commands failed.\n");
> return -ENOMEM;
> }
> - queue_trb(xhci, xhci->cmd_ring, false, field1, field2, field3,
> + queue_trb(xhci, xhci->cmd_ring, false, false, field1, field2, field3,
> field4 | xhci->cmd_ring->cycle_state);
> return 0;
> }
> --
> 1.6.3.3
>
> --
> 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
--
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
