Some xhci controllers (eg ASMedia) don't like processing a LINK TRB and
then finding that they can't process the next TRB.

Instead of flipping the cycle bit on the first data TRB, remember the
real first TRB in prepare_ring() and flip that in giveback_first_trb().

In queue_trb() ignore the cycle bit value set by xhci_queue_bulk_tx() etc,
and changes the ownership of all but the first TRB.

If prepare_ring() adds any NOP TRB, the ownership of all but the first
and the LINK are changed. The wmb() is no longer needed before changing
the ownership of a LINK trb since is is preceeded by a NOP.

Since the 'first trb' might be a LINK trb queue_command() must also now
call giveback_first_trb(). Don't ring the doorbell here though.

The isoc code calls prepare_ring() right at the start, this ensures that
there is enough space for all the TRB and advances the write-ptr past
any LINK TRB. It then calls prepare_transfer() multiple times.
However the prepare_ring() calls must now be matched with those to
giveback_first_trb().
Don't call prepare_ring() from prepare_transfer() for isoc rings.
Set 'more_trbs_coming' (it means 'advance past a LINK TRB) on all
the queue_trb() calls except the very last one.

Signed-off-by: David Laight <david.lai...@aculab.com>
---
Note that this differs to any previous similar patches I've sent
because it contains a fix to the isoc code.
However I've only tsted it with USB3 ethernet.

 drivers/usb/host/xhci-ring.c | 84 +++++++++++++++++++++++++++++---------------
 drivers/usb/host/xhci.h      |  1 +
 2 files changed, 57 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a0b248c..62049e5 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2920,6 +2920,16 @@ static void queue_trb(struct xhci_hcd *xhci, struct 
xhci_ring *ring,
        struct xhci_generic_trb *trb;
 
        trb = &ring->enqueue->generic;
+
+       /*
+        * Ignore the caller's CYCLE bit.
+        * The caller doesn't know whether the real first TRB is
+        * actually a LINK (or NOP) TRB.
+        */
+       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);
@@ -2964,6 +2974,8 @@ static int prepare_ring(struct xhci_hcd *xhci, struct 
xhci_ring *ep_ring,
                return -EINVAL;
        }
 
+       /* Save entry whose cycle bit 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;
@@ -3006,13 +3018,18 @@ 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 {
+                       /* Leave TRB_CYCLE unchanged on first NOP */
+                       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;
@@ -3050,8 +3067,9 @@ static int prepare_ring(struct xhci_hcd *xhci, struct 
xhci_ring *ep_ring,
                        else
                                next->link.control |= cpu_to_le32(TRB_CHAIN);
 
-                       wmb();
-                       next->link.control ^= cpu_to_le32(TRB_CYCLE);
+                       /* If LINK follows a NOP, invert 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)) {
@@ -3088,11 +3106,14 @@ 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;
+       /* xhci_queue_isoc_tx_prepare() called prepare ring earlier. */
+       if (ep_ring->type != TYPE_ISOC) {
+               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];
@@ -3167,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);
 }
 
 /*
@@ -3420,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;
 }
 
@@ -3559,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;
 }
 
@@ -3676,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;
 }
 
@@ -3770,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;
 
@@ -3851,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;
@@ -3862,7 +3884,12 @@ 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;
+                               /*
+                                * We want to advance past LINK TRBs until the
+                                * very last TRB.
+                                */
+                               if (i == num_tds - 1)
+                                       more_trbs_coming = false;
                        }
 
                        /* Calculate TRB length */
@@ -3910,8 +3937,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. */
@@ -4036,6 +4062,8 @@ 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);
+       /* The 'first' TRB might be a LINK TRB... */
+       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 f8416639..6973c56 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1334,6 +1334,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;
-- 
1.8.1.2



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to