[PATCH 1/2] usb: xhci: fix Command Aborting

[Niklas Neronin <niklas.neronin@linux.intel.com>]

Aborting the command ring requires setting the Command Abort (CA) bit in
the 64-bit Command Ring Control Register (CRCR). Historically, several
attempts have been made to implement this correctly, but each introduced
its own problems. This patch fixes the final remaining issue: accidental
modification of the Command Ring Pointer (i'll abbreviate it to CRP).

Originally [1], the full 64-bit CRCR value was read and written back after
setting CA. This is a bit unnecessary, only RsvdP bits (5:4) should be
read and written back (for future-proofing). All other writable fields
read as zero.

Later patches attempted to solve an issues, caused by 64-bit writes being
split into two 32-bit writes. Writing the lower 31:0 bits first immediately
stopped the ring (CRR=0), and the following upper-half write then overwrote
part of CRP with zeroes, thus corrupting the CRP.
Patch [2] avoided this by writing only the lower 31:0 bits with CA set,
but that broke controllers that latch 64-bit registers only when the upper
bits are written, as reported in [3].

The current solution [4] attempted to fix this by writing the full 64-bit
CRCR with both CA and an updated CRP. This does not work. The patch tries
to modify CRP while setting CA, but with CRR=1 all writes to CRP are
ignored. Updating CRP requires writing only the CA bit, waiting for the
controller to process the abort and clear CRR, and only then writing the
full CRP value.
Writing a new CRP after CA clears CRR is also unsafe:
 * TRBs are 16-byte aligned (bits 3:0 clear)
 * CRP requires 64-byte alignment (bits 5:0 clear)
Writing a TRB pointer into CRP truncates bits 5:4 and may produce an
incorrect address. Restoring the original CRP is also not possible,
CRCR reads return zero, and writing the original CRP back forces the
controller to restart execution from the beginning of the ring.

For a Command Abort to succeed, the CA bit must be set without modifying
the CRP. The following sequence ensures this:

  1. Write the lower 31:0 bits with only the CA bit set. Since CRR=1,
     CRP write is ignored.

  2. Poll CRR. If CRR becomes 0, the abort succeeded with CRP preserved.

  3. If CRR does not clear (timeout), test if controller requires an upper
     bits write to latch the register. Write the upper 63:32 bits (which
     does not update the CRP because CRR=1).
     Then poll CRR again. If CRR becomes 0, it was a latching issue and the
     abort succeeded with CRP preserved.

Because there are now two polling stages, each timeout has been reduced
from five seconds to three seconds. The comment explaining the 5 seconds
poll has been removed.

Link: https://git.kernel.org/torvalds/c/b92cc66c047f [1]
Link: https://lore.kernel.org/all/20211008092547.3996295-5-mathias.nyman@linux.intel.com/ [2]
Link: https://lore.kernel.org/all/20211022105913.7671-1-youling257@gmail.com/ [3]
Link: https://lore.kernel.org/20211126122340.1193239-2-mathias.nyman@linux.intel.com [4]
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 42 ++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1cbefee3c4ca..10160e76df68 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -489,38 +489,32 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
 /* Must be called with xhci->lock held, releases and acquires lock back */
 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
 {
-	struct xhci_segment *new_seg	= xhci->cmd_ring->deq_seg;
-	union xhci_trb *new_deq		= xhci->cmd_ring->dequeue;
-	u64 crcr;
+	u32 crcr_lo;
 	int ret;
 
 	xhci_dbg(xhci, "Abort command ring\n");
 
 	reinit_completion(&xhci->cmd_ring_stop_completion);
 
-	/*
-	 * The control bits like command stop, abort are located in lower
-	 * dword of the command ring control register.
-	 * Some controllers require all 64 bits to be written to abort the ring.
-	 * Make sure the upper dword is valid, pointing to the next command,
-	 * avoiding corrupting the command ring pointer in case the command ring
-	 * is stopped by the time the upper dword is written.
-	 */
-	next_trb(&new_seg, &new_deq);
-	if (trb_is_link(new_deq))
-		next_trb(&new_seg, &new_deq);
+	/* Preserve RsvdP (5:4), other writable bits read 0. */
+	crcr_lo = readl(&xhci->op_regs->cmd_ring);
+	crcr_lo |= CMD_RING_ABORT;
+	writel(crcr_lo, &xhci->op_regs->cmd_ring);
 
-	crcr = xhci_trb_virt_to_dma(new_seg, new_deq);
-	xhci_write_64(xhci, crcr | CMD_RING_ABORT, &xhci->op_regs->cmd_ring);
+	/* In the future we should try to recover a -ETIMEDOUT with a host controller reset */
+	ret = xhci_handshake(&xhci->op_regs->cmd_ring, CMD_RING_RUNNING, 0, 3 * USEC_PER_SEC);
+	if (ret == -ETIMEDOUT) {
+		/*
+		 * Some controllers only latch 64-bit registers when the upper (63:32) bits
+		 * are written.
+		 * While the ring is running, writes to bits 63:6 and bit 0 are ignored.
+		 */
+		xhci_dbg(xhci, "Ring still running, checking if HC needs full 64-bit CRCR write\n");
+		writel(0, (void __iomem *)(&xhci->op_regs->cmd_ring) + 4);
+		ret = xhci_handshake(&xhci->op_regs->cmd_ring, CMD_RING_RUNNING, 0,
+				     3 * USEC_PER_SEC);
+	}
 
-	/* Section 4.6.1.2 of xHCI 1.0 spec says software should also time the
-	 * completion of the Command Abort operation. If CRR is not negated in 5
-	 * seconds then driver handles it as if host died (-ENODEV).
-	 * In the future we should distinguish between -ENODEV and -ETIMEDOUT
-	 * and try to recover a -ETIMEDOUT with a host controller reset.
-	 */
-	ret = xhci_handshake(&xhci->op_regs->cmd_ring,
-			CMD_RING_RUNNING, 0, 5 * 1000 * 1000);
 	if (ret < 0) {
 		xhci_err(xhci, "Abort failed to stop command ring: %d\n", ret);
 		xhci_halt(xhci);
-- 
2.50.1