[PATCH] usb: xhci: Unlock for command abort polling

[Michal Pecio <michal.pecio@gmail.com>]

xhci_abort_cmd_ring() requests abort, waits for the CRR bit to clear,
drops xhci->lock and waits for the Command Ring Stopped event.

The CRR wait timeout is 5 seconds as suggested by xHCI 4.6.1.2, which
means that if the xHC fails to complete the operation at all, we poll
with the lock held and IRQs disabled for several seconds. If any other
CPU tries to acquire the lock, it will spin likewise. IRQs get delays,
drivers log errors, tasks freeze, it's a mess.

So drop the lock earlier, before waiting for the CRR bit. It should be
safe - the sole caller sets cmd_ring_state to CMD_RING_STATE_ABORTED
before calling us, which will prevent others from ringing the command
doorbell and interfering with the abort. Queuing new commands during
this time poses no danger, and if the command we try to abort actually
completes concurrently, existing code already needs to deal with this.
And in my testing it does - it's trivial to trigger this on ASM1042,
where Address Device can't be aborted, but it completes as soon as the
offending device is unplugged, including during abort attempt.

Note that the lock still covers reinit_completion(), so it won't race
with complete() being called by the event handler. And works are not
reentrant, so another timeout can't expire while the lock is dropped.
We will configure timeout anew when restarting the ring.

One other difference is that now we also drop the lock if abort fails.
This too should be harmless. Commands queued during this time will be
released like any other pending commands. If the aborted command does
complete before we regain the lock, it's a waste, but not regression.

Reported-by: Pedro Fonseca <pedro@fonseca.com.pt>
Link: https://lore.kernel.org/linux-usb/16f65081-5a3c-4c30-9811-9017796a3373@fonseca.com.pt/
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---

This has been annoying me and various others for years, only the latest
incident is listed above.

I expected a nightmare of race conditions, but after finally taking a
serious look I think it really is quite simple. It helps that the lock
was already being dropped and existing code seems to handle it fine.

 drivers/usb/host/xhci-ring.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 8b0c27d6f12d..44a0f89110a1 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -494,7 +494,7 @@ 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;
-	int ret;
+	int ret, completed;
 
 	xhci_dbg(xhci, "Abort command ring\n");
 
@@ -521,25 +521,27 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
 	 * In the future we should distinguish between -ENODEV and -ETIMEDOUT
 	 * and try to recover a -ETIMEDOUT with a host controller reset.
 	 */
+	spin_unlock_irqrestore(&xhci->lock, flags);
 	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);
-		xhci_hc_died(xhci);
-		return ret;
-	}
 	/*
 	 * Writing the CMD_RING_ABORT bit should cause a cmd completion event,
 	 * however on some host hw the CMD_RING_RUNNING bit is correctly cleared
 	 * but the completion event in never sent. Wait 2 secs (arbitrary
 	 * number) to handle those cases after negation of CMD_RING_RUNNING.
 	 */
-	spin_unlock_irqrestore(&xhci->lock, flags);
-	ret = wait_for_completion_timeout(&xhci->cmd_ring_stop_completion,
+	if (ret >= 0)
+		completed = wait_for_completion_timeout(&xhci->cmd_ring_stop_completion,
 					  msecs_to_jiffies(2000));
 	spin_lock_irqsave(&xhci->lock, flags);
-	if (!ret) {
+
+	if (ret < 0) {
+		xhci_err(xhci, "Abort failed to stop command ring: %d\n", ret);
+		xhci_halt(xhci);
+		xhci_hc_died(xhci);
+		return ret;
+	}
+	if (!completed) {
 		xhci_dbg(xhci, "No stop event for abort, ring start fail?\n");
 		xhci_cleanup_command_queue(xhci);
 	} else {
-- 
2.48.1