[PATCH 0/1] usb: xhci: hold lock over xhci_abort_cmd_ring()

[PATCH 0/1] usb: xhci: hold lock over xhci_abort_cmd_ring()

[Lu Baolu]

Hi Mathias,

This is a follow-up patch for below comment

"fix the lock to cover abort+CRR check, and send it to usb-linus +stable"

in below discussion thread.

https://lkml.org/lkml/2016/12/21/186

It's based on v4.9.

Best regards,
Lu Baolu

Lu Baolu (1):
  usb: xhci: hold lock over xhci_abort_cmd_ring()

 drivers/usb/host/xhci-ring.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

-- 
2.1.4

[PATCH 1/1] usb: xhci: hold lock over xhci_abort_cmd_ring()

[Lu Baolu]

In command timer function, xhci_handle_command_timeout(), xhci->lock
is unlocked before call into xhci_abort_cmd_ring(). This might cause
race between the timer function and the event handler.

The xhci_abort_cmd_ring() function sets the CMD_RING_ABORT bit in the
command register and polling it until the setting takes effect. A stop
command ring event might be handled between writing the abort bit and
polling for it. The event handler will restart the command ring, which
causes the failure of polling, and we ever believed that we failed to
stop it.

As a bonus, this also fixes some issues of calling functions without
locking in xhci_handle_command_timeout().

Cc: <stable@vger.kernel.org> # 3.7+
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 797137e..0f99078 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1283,29 +1283,34 @@ void xhci_handle_command_timeout(unsigned long data)
 	hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
 	if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
 	    (hw_ring_state & CMD_RING_RUNNING))  {
-		spin_unlock_irqrestore(&xhci->lock, flags);
 		xhci_dbg(xhci, "Command timeout\n");
 		ret = xhci_abort_cmd_ring(xhci);
 		if (unlikely(ret == -ESHUTDOWN)) {
 			xhci_err(xhci, "Abort command ring failed\n");
 			xhci_cleanup_command_queue(xhci);
+			spin_unlock_irqrestore(&xhci->lock, flags);
 			usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
 			xhci_dbg(xhci, "xHCI host controller is dead.\n");
+
+			return;
 		}
-		return;
+
+		goto time_out_completed;
 	}
 
 	/* command ring failed to restart, or host removed. Bail out */
 	if (second_timeout || xhci->xhc_state & XHCI_STATE_REMOVING) {
-		spin_unlock_irqrestore(&xhci->lock, flags);
 		xhci_dbg(xhci, "command timed out twice, ring start fail?\n");
 		xhci_cleanup_command_queue(xhci);
-		return;
+
+		goto time_out_completed;
 	}
 
 	/* command timeout on stopped ring, ring can't be aborted */
 	xhci_dbg(xhci, "Command timeout on stopped ring\n");
 	xhci_handle_stopped_cmd_ring(xhci, xhci->current_cmd);
+
+time_out_completed:
 	spin_unlock_irqrestore(&xhci->lock, flags);
 	return;
 }
-- 
2.1.4

Re: [PATCH 1/1] usb: xhci: hold lock over xhci_abort_cmd_ring()

[Mathias Nyman]

On 23.12.2016 08:46, Lu Baolu wrote:
> In command timer function, xhci_handle_command_timeout(), xhci->lock
> is unlocked before call into xhci_abort_cmd_ring(). This might cause
> race between the timer function and the event handler.
>
> The xhci_abort_cmd_ring() function sets the CMD_RING_ABORT bit in the
> command register and polling it until the setting takes effect. A stop
> command ring event might be handled between writing the abort bit and
> polling for it. The event handler will restart the command ring, which
> causes the failure of polling, and we ever believed that we failed to
> stop it.
>
> As a bonus, this also fixes some issues of calling functions without
> locking in xhci_handle_command_timeout().
>

Did the same thing, moved the unlock to cover also abort_cmd_ring(),

but this one takes care of locking the command ring cleanup as well
so I'll pick up this instead

-Mathias