[PATCH] xhci: Fix command ring pointer corruption while aborting a command

[PATCH] xhci: Fix command ring pointer corruption while aborting a command

[Pavankumar Kondeti]

The command ring pointer is located at [6:63] bits of the command
ring control register (CRCR). All the control bits like command stop,
abort are located at [0:3] bits. While aborting a command, we read the
CRCR and set the abort bit and write to the CRCR. The read will always
give command ring pointer as all zeros. So we essentially write only
the control bits. Since we split the 64 bit write into two 32 bit writes,
there is a possibility of xHC command ring stopped before the upper
dword (all zeros) is written. If that happens, xHC updates the upper
dword of its internal command ring pointer with all zeros. Next time,
when the command ring is restarted, we see xHC memory access failures.
Fix this issue by only writing to the lower dword of CRCR where all
control bits are located.

Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
---
 drivers/usb/host/xhci-ring.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index e676749..3c8456b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -366,16 +366,22 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
 /* Must be called with xhci->lock held, releases and aquires lock back */
 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
 {
-	u64 temp_64;
+	u64 temp_32;
 	int ret;
 
 	xhci_dbg(xhci, "Abort command ring\n");
 
 	reinit_completion(&xhci->cmd_ring_stop_completion);
 
-	temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
-	xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
-			&xhci->op_regs->cmd_ring);
+	/*
+	 * The control bits like command stop, abort are located in lower
+	 * dword of the command ring control register. Limit the write
+	 * to the lower dword to avoid corrupting the command ring pointer
+	 * in case if the command ring is stopped by the time upper dword
+	 * is written.
+	 */
+	temp_32 = readl(&xhci->op_regs->cmd_ring);
+	writel(temp_32 | CMD_RING_ABORT, &xhci->op_regs->cmd_ring);
 
 	/* 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
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] xhci: Fix command ring pointer corruption while aborting a command

[Pavankumar Kondeti]

The command ring pointer is located at [6:63] bits of the command
ring control register (CRCR). All the control bits like command stop,
abort are located at [0:3] bits. While aborting a command, we read the
CRCR and set the abort bit and write to the CRCR. The read will always
give command ring pointer as all zeros. So we essentially write only
the control bits. Since we split the 64 bit write into two 32 bit writes,
there is a possibility of xHC command ring stopped before the upper
dword (all zeros) is written. If that happens, xHC updates the upper
dword of its internal command ring pointer with all zeros. Next time,
when the command ring is restarted, we see xHC memory access failures.
Fix this issue by only writing to the lower dword of CRCR where all
control bits are located.

Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
---
v2:
%s/u64/u32 for a variable in xhci_abort_cmd_ring()

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index e676749..d7fe6d4 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -366,16 +366,22 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
 /* Must be called with xhci->lock held, releases and aquires lock back */
 static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
 {
-	u64 temp_64;
+	u32 temp_32;
 	int ret;
 
 	xhci_dbg(xhci, "Abort command ring\n");
 
 	reinit_completion(&xhci->cmd_ring_stop_completion);
 
-	temp_64 = xhci_read_64(xhci, &xhci->op_regs->cmd_ring);
-	xhci_write_64(xhci, temp_64 | CMD_RING_ABORT,
-			&xhci->op_regs->cmd_ring);
+	/*
+	 * The control bits like command stop, abort are located in lower
+	 * dword of the command ring control register. Limit the write
+	 * to the lower dword to avoid corrupting the command ring pointer
+	 * in case if the command ring is stopped by the time upper dword
+	 * is written.
+	 */
+	temp_32 = readl(&xhci->op_regs->cmd_ring);
+	writel(temp_32 | CMD_RING_ABORT, &xhci->op_regs->cmd_ring);
 
 	/* 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
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] xhci: Fix command ring pointer corruption while aborting a command

[Mathias Nyman]

On 1.10.2021 10.22, Pavankumar Kondeti wrote:
> The command ring pointer is located at [6:63] bits of the command
> ring control register (CRCR). All the control bits like command stop,
> abort are located at [0:3] bits. While aborting a command, we read the
> CRCR and set the abort bit and write to the CRCR. The read will always
> give command ring pointer as all zeros. So we essentially write only
> the control bits. Since we split the 64 bit write into two 32 bit writes,
> there is a possibility of xHC command ring stopped before the upper
> dword (all zeros) is written. If that happens, xHC updates the upper
> dword of its internal command ring pointer with all zeros. Next time,
> when the command ring is restarted, we see xHC memory access failures.
> Fix this issue by only writing to the lower dword of CRCR where all
> control bits are located.
> 
> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>


Thanks, nice catch.

Adding to queue

-Mathias

Re: [PATCH] xhci: Fix command ring pointer corruption while aborting a command

[Mathias Nyman]

On 4.10.2021 11.22, Mathias Nyman wrote:
> On 1.10.2021 10.22, Pavankumar Kondeti wrote:
>> The command ring pointer is located at [6:63] bits of the command
>> ring control register (CRCR). All the control bits like command stop,
>> abort are located at [0:3] bits. While aborting a command, we read the
>> CRCR and set the abort bit and write to the CRCR. The read will always
>> give command ring pointer as all zeros. So we essentially write only
>> the control bits. Since we split the 64 bit write into two 32 bit writes,
>> there is a possibility of xHC command ring stopped before the upper
>> dword (all zeros) is written. If that happens, xHC updates the upper
>> dword of its internal command ring pointer with all zeros. Next time,
>> when the command ring is restarted, we see xHC memory access failures.
>> Fix this issue by only writing to the lower dword of CRCR where all
>> control bits are located.
>>
>> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
> 
> 
> Thanks, nice catch.
> 
> Adding to queue

just to clarify, took v2

-Mathias