From: David Laight
> From: Alan Stern
> > On Wed, 15 Jan 2014, David Laight wrote:
> >
> > > I have a theory, I'll try to write a non-invasive patch.
...
> >
> > Doesn't this mean you shouldn't change the ownership of a LINK TRB
> > until after you change the ownership of the TRB it points to?
>
> That is what I assume.
> In practise this means that the 'first_trb' (whose ownership is set
> last) has to be the one that is valid when prepare_ring() is called.
>
> The plan for the patch is:
> - Save the enq pointer in prepare_ring (in the ep_ring structure).
> - When writing a trb set the ownership unless it is the saved one
> (ignoring the value set by the caller).
> - At the end invert the ownership on the saved entry.
Below is a possible patch, I've only compile tested it.
I've minimalised the patch by not removing all the code that saves
'start_trb' and modifies the TRB_CYCLE bit.
If the patch works those parts can also be tidied up.
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 53c2e29..589d336 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2928,6 +2928,11 @@ static void queue_trb(struct xhci_hcd *xhci, struct
xhci_ring *ring,
struct xhci_generic_trb *trb;
trb = &ring->enqueue->generic;
+
+ field4 = (field4 & ~TRB_CYCLE) | ring->cycle_state;
+ if (trb == &ring->enqueue_first->generic)
+ field4 ^= TRB_CYCLE;
+
trb->field[0] = cpu_to_le32(field1);
trb->field[1] = cpu_to_le32(field2);
trb->field[2] = cpu_to_le32(field3);
@@ -2972,6 +2977,8 @@ static int prepare_ring(struct xhci_hcd *xhci, struct
xhci_ring *ep_ring,
return -EINVAL;
}
+ /* Save entry whose owner needs flipping at the end */
+ ep_ring->enqueue_first = ep_ring->enqueue;
while (1) {
if (room_on_ring(xhci, ep_ring, num_trbs)) {
union xhci_trb *trb = ep_ring->enqueue;
@@ -3014,13 +3021,16 @@ static int prepare_ring(struct xhci_hcd *xhci, struct
xhci_ring *ep_ring,
nop_cmd = cpu_to_le32(TRB_TYPE(TRB_TR_NOOP) |
ep_ring->cycle_state);
ep_ring->num_trbs_free -= usable;
- do {
+ trb->generic.field[3] = nop_cmd ^
cpu_to_le32(TRB_CYCLE);
+ for (;;) {
trb->generic.field[0] = 0;
trb->generic.field[1] = 0;
trb->generic.field[2] = 0;
- trb->generic.field[3] = nop_cmd;
trb++;
- } while (--usable);
+ if (!--usable)
+ break;
+ trb->generic.field[3] = nop_cmd;
+ }
ep_ring->enqueue = trb;
if (room_on_ring(xhci, ep_ring, num_trbs))
break;
@@ -3059,7 +3069,8 @@ static int prepare_ring(struct xhci_hcd *xhci, struct
xhci_ring *ep_ring,
next->link.control |= cpu_to_le32(TRB_CHAIN);
wmb();
- next->link.control ^= cpu_to_le32(TRB_CYCLE);
+ if (next != ep_ring->enqueue_first)
+ next->link.control ^= cpu_to_le32(TRB_CYCLE);
/* Toggle the cycle bit after the last ring segment. */
if (last_trb_on_last_seg(xhci, ring, ring->enq_seg,
next)) {
@@ -3096,11 +3107,13 @@ static int prepare_transfer(struct xhci_hcd *xhci,
return -EINVAL;
}
- ret = prepare_ring(xhci, ep_ring,
- le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK,
- num_trbs, mem_flags);
- if (ret)
- return ret;
+ if (td_index == 0) {
+ ret = prepare_ring(xhci, ep_ring,
+ le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK,
+ num_trbs, mem_flags);
+ if (ret)
+ return ret;
+ }
urb_priv = urb->hcpriv;
td = urb_priv->td[td_index];
@@ -3175,19 +3188,24 @@ static void check_trb_math(struct urb *urb, int
num_trbs, int running_total)
}
static void giveback_first_trb(struct xhci_hcd *xhci, int slot_id,
- unsigned int ep_index, unsigned int stream_id, int start_cycle,
- struct xhci_generic_trb *start_trb)
+ unsigned int ep_index, struct xhci_ring *ring)
{
/*
* Pass all the TRBs to the hardware at once and make sure this write
* isn't reordered.
*/
wmb();
- if (start_cycle)
- start_trb->field[3] |= cpu_to_le32(start_cycle);
- else
- start_trb->field[3] &= cpu_to_le32(~TRB_CYCLE);
- xhci_ring_ep_doorbell(xhci, slot_id, ep_index, stream_id);
+ ring->enqueue_first->generic.field[3] ^= cpu_to_le32(TRB_CYCLE);
+
+ if (ring->type == TYPE_COMMAND)
+ return;
+
+ /*
+ * Make sure the hardware doesn't read the ring before the write
+ * above completes.
+ */
+ wmb();
+ xhci_ring_ep_doorbell(xhci, slot_id, ep_index, ring->stream_id);
}
/*
@@ -3428,8 +3446,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
} while (running_total < urb->transfer_buffer_length);
check_trb_math(urb, num_trbs, running_total);
- giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
- start_cycle, start_trb);
+ giveback_first_trb(xhci, slot_id, ep_index, ep_ring);
return 0;
}
@@ -3567,8 +3584,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
} while (running_total < urb->transfer_buffer_length);
check_trb_math(urb, num_trbs, running_total);
- giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
- start_cycle, start_trb);
+ giveback_first_trb(xhci, slot_id, ep_index, ep_ring);
return 0;
}
@@ -3684,8 +3700,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
mem_flags,
/* Event on completion */
field | TRB_IOC | TRB_TYPE(TRB_STATUS) |
ep_ring->cycle_state);
- giveback_first_trb(xhci, slot_id, ep_index, 0,
- start_cycle, start_trb);
+ giveback_first_trb(xhci, slot_id, ep_index, ep_ring);
return 0;
}
@@ -3778,7 +3793,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci,
gfp_t mem_flags,
int running_total, trb_buff_len, td_len, td_remain_len, ret;
u64 start_addr, addr;
int i, j;
- bool more_trbs_coming;
+ bool more_trbs_coming = true;
ep_ring = xhci->devs[slot_id]->eps[ep_index].ring;
@@ -3859,7 +3874,6 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci,
gfp_t mem_flags,
*/
if (j < trbs_per_td - 1) {
field |= TRB_CHAIN;
- more_trbs_coming = true;
} else {
td->last_trb = ep_ring->enqueue;
field |= TRB_IOC;
@@ -3870,7 +3884,8 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci,
gfp_t mem_flags,
if (i < num_tds - 1)
field |= TRB_BEI;
}
- more_trbs_coming = false;
+ if (i == num_tds - 1)
+ more_trbs_coming = false;
}
/* Calculate TRB length */
@@ -3918,8 +3933,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci,
gfp_t mem_flags,
}
xhci_to_hcd(xhci)->self.bandwidth_isoc_reqs++;
- giveback_first_trb(xhci, slot_id, ep_index, urb->stream_id,
- start_cycle, start_trb);
+ giveback_first_trb(xhci, slot_id, ep_index, ep_ring);
return 0;
cleanup:
/* Clean up a partially enqueued isoc transfer. */
@@ -4044,6 +4058,7 @@ static int queue_command(struct xhci_hcd *xhci, u32
field1, u32 field2,
}
queue_trb(xhci, xhci->cmd_ring, false, field1, field2, field3,
field4 | xhci->cmd_ring->cycle_state);
+ giveback_first_trb(xhci, 0, 0, xhci->cmd_ring);
return 0;
}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 03c74b7..60da2e6 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1315,6 +1315,7 @@ struct xhci_ring {
struct xhci_segment *first_seg;
struct xhci_segment *last_seg;
union xhci_trb *enqueue;
+ union xhci_trb *enqueue_first;
struct xhci_segment *enq_seg;
unsigned int enq_updates;
union xhci_trb *dequeue;
--
David
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
