Hi,
Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> writes:
> On 03.05.2016 13:30, Felipe Balbi wrote:
>> When trying to access our last TRB, XHCI was
>> actually reading memory outside of the TRB
>> array/ring due to an off-by-one error.
>>
>> This patch fixes that error and has the side effect
>> of also fixing some rare situations where long mass
>> storage transfers would timeout and XHCI would reset
>> the mass storage device before continuing.
>>
>> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>> ---
>> drivers/usb/host/xhci-ring.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 6c41dbbf9f2f..ba610a72c396 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -95,7 +95,7 @@ static bool last_trb_on_last_seg(struct xhci_hcd *xhci, struct xhci_ring *ring,
>> struct xhci_segment *seg, union xhci_trb *trb)
>> {
>> if (ring == xhci->event_ring)
>> - return (trb == &seg->trbs[TRBS_PER_SEGMENT]) &&
>> + return (trb == &seg->trbs[TRBS_PER_SEGMENT - 1]) &&
>> (seg->next == xhci->event_ring->first_seg);
>> else
>> return le32_to_cpu(trb->link.control) & LINK_TOGGLE;
>> @@ -109,7 +109,7 @@ static int last_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
>> struct xhci_segment *seg, union xhci_trb *trb)
>> {
>> if (ring == xhci->event_ring)
>> - return trb == &seg->trbs[TRBS_PER_SEGMENT];
>> + return trb == &seg->trbs[TRBS_PER_SEGMENT - 1];
>> else
>> return TRB_TYPE_LINK_LE32(trb->link.control);
>> }
>>
>
> Thanks, this needs to be fixed, but there are some changes needed to inc_enq()
> as well together with this fix.
> Otherwise the last TRB of a event ring won't be used
Any idea how a problem due to this could be triggered ? So far I can't
get any failures. BTW, the last TRB *will* be used by the HW anyway, no?
A truer argument would be that Linux XHCI driver might miss handling
that event. I still can't get this to fail. It seems to me that it's
pretty much impossible for us to miss an event.
If we consider what happens from the time an IRQ fires, we will have the
following sequence of events:
MSI/MSI-X/normal IRQ asserted
xhci_irq()
while(xhci_handle_event(xhci) > 0);
event = xhci->event_ring->dequeue;
/* handle the event given its type */
if (update_ptrs) inc_deq();
ring->deq_updates++;
do {
if (last_trb(xhci, ring, ring->deq_seg, ring->dequeue))
if (ring->type == EVENT && last_trb_on_last_seg())
ring-cycle_state ^= 1;
ring->deq_seg = ring->deq_seg->next;
ring->dequeue = ring->deq_seg->trbs;
else
ring->dequeue++
} while (last_trb())
With the patch above, here's (again, to make it easier to comment) what
last_trb() and last_trb_on_last_seg() look like:
> static bool last_trb_on_last_seg(struct xhci_hcd *xhci, struct xhci_ring *ring,
> struct xhci_segment *seg, union xhci_trb *trb)
> {
> if (ring == xhci->event_ring)
> return (trb == &seg->trbs[TRBS_PER_SEGMENT - 1]) &&
> (seg->next == xhci->event_ring->first_seg);
> else
> return le32_to_cpu(trb->link.control) & LINK_TOGGLE;
> }
for a ring segment this evaluates to:
return trb == last_trb && seg->next == xhci->event_ring->first_seg;
IOW:
if this is the last_trb on current segment and is also the last
trb on the last segment, return true.
in fact, trb == &seg->trbs[TRBS_PER_SEGMENT - 1] is a redundant check.
> static int last_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
> struct xhci_segment *seg, union xhci_trb *trb)
> {
> if (ring == xhci->event_ring)
> return trb == &seg->trbs[TRBS_PER_SEGMENT - 1];
> else
> return TRB_TYPE_LINK_LE32(trb->link.control);
> }
likewise, for an event_ring, this turns out to be:
return trb == last_trb;
IOW, before this patch, we were potentially checking memory outside of
the TRB ring which tells me that, unless we were lucky, last_trb() and
last_trb_on_last_seg() would always evaluate to false.
Oh; and, BTW, a further cleanup would be:
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index ba610a72c396..e3a647c73323 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -88,19 +88,6 @@ dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg,
return seg->dma + (segment_offset * sizeof(*trb));
}
-/* Does this link TRB point to the first segment in a ring,
- * or was the previous TRB the last TRB on the last segment in the ERST?
- */
-static bool last_trb_on_last_seg(struct xhci_hcd *xhci, struct xhci_ring *ring,
- struct xhci_segment *seg, union xhci_trb *trb)
-{
- if (ring == xhci->event_ring)
- return (trb == &seg->trbs[TRBS_PER_SEGMENT - 1]) &&
- (seg->next == xhci->event_ring->first_seg);
- else
- return le32_to_cpu(trb->link.control) & LINK_TOGGLE;
-}
-
/* Is this TRB a link TRB or was the last TRB the last TRB in this event ring
* segment? I.e. would the updated event TRB pointer step off the end of the
* event seg?
@@ -114,6 +101,22 @@ static int last_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
return TRB_TYPE_LINK_LE32(trb->link.control);
}
+/* Does this link TRB point to the first segment in a ring,
+ * or was the previous TRB the last TRB on the last segment in the ERST?
+ */
+static bool last_trb_on_last_seg(struct xhci_hcd *xhci, struct xhci_ring *ring,
+ struct xhci_segment *seg, union xhci_trb *trb)
+{
+ if (ring == xhci->event_ring) {
+ if (!last_trb(xhci, ring, seg, trb))
+ return false;
+
+ return seg->next == xhci->event_ring->first_seg;
+ } else {
+ return le32_to_cpu(trb->link.control) & LINK_TOGGLE;
+ }
+}
+
static int enqueue_is_link_trb(struct xhci_ring *ring)
{
struct xhci_link_trb *link = &ring->enqueue->link;
--
balbi
Attachment:
signature.asc
Description: PGP signature
