[RFT PATCH] xhci: Fix commad ring abort, write all 64 bits to CRCR register.

[RFT PATCH] xhci: Fix commad ring abort, write all 64 bits to CRCR register.

[Mathias Nyman]

Turns out some xHC controllers require all 64 bits in the CRCR register
to be written to execute a command abort.

The lower 32 bits containing the command abort bit is written first.
In case the command ring stops before we write the upper 32 bits then
hardware may use these upper bits to set the commnd ring dequeue pointer.

Solve this by making sure the upper 32 bits contain a valid command
ring dequeue pointer.

The original patch that only wrote the first 32 to stop the ring went
to stable, so this fix should go there as well.

Fixes: ff0e50d3564f ("xhci: Fix command ring pointer corruption while aborting a command")
Cc: stable@vger.kernel.org
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 311597bba80e..eaa49aef2935 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -366,7 +366,9 @@ 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)
 {
-	u32 temp_32;
+	struct xhci_segment *new_seg	= xhci->cmd_ring->deq_seg;
+	union xhci_trb *new_deq		= xhci->cmd_ring->dequeue;
+	u64 crcr;
 	int ret;
 
 	xhci_dbg(xhci, "Abort command ring\n");
@@ -375,13 +377,18 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
 
 	/*
 	 * 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.
+	 * 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.
 	 */
-	temp_32 = readl(&xhci->op_regs->cmd_ring);
-	writel(temp_32 | CMD_RING_ABORT, &xhci->op_regs->cmd_ring);
+	next_trb(xhci, NULL, &new_seg, &new_deq);
+	if (trb_is_link(new_deq))
+		next_trb(xhci, NULL, &new_seg, &new_deq);
+
+	crcr = xhci_trb_virt_to_dma(new_seg, new_deq);
+	xhci_write_64(xhci, crcr | 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
-- 
2.25.1

Re: [RFT PATCH] xhci: Fix commad ring abort, write all 64 bits to CRCR register.

[youling 257]

test it can work for me.

2021-10-29 20:51 GMT+08:00, Mathias Nyman <mathias.nyman@linux.intel.com>:
> Turns out some xHC controllers require all 64 bits in the CRCR register
> to be written to execute a command abort.
>
> The lower 32 bits containing the command abort bit is written first.
> In case the command ring stops before we write the upper 32 bits then
> hardware may use these upper bits to set the commnd ring dequeue pointer.
>
> Solve this by making sure the upper 32 bits contain a valid command
> ring dequeue pointer.
>
> The original patch that only wrote the first 32 to stop the ring went
> to stable, so this fix should go there as well.
>
> Fixes: ff0e50d3564f ("xhci: Fix command ring pointer corruption while
> aborting a command")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-ring.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 311597bba80e..eaa49aef2935 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -366,7 +366,9 @@ 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)
>  {
> -	u32 temp_32;
> +	struct xhci_segment *new_seg	= xhci->cmd_ring->deq_seg;
> +	union xhci_trb *new_deq		= xhci->cmd_ring->dequeue;
> +	u64 crcr;
>  	int ret;
>
>  	xhci_dbg(xhci, "Abort command ring\n");
> @@ -375,13 +377,18 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci,
> unsigned long flags)
>
>  	/*
>  	 * 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.
> +	 * 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.
>  	 */
> -	temp_32 = readl(&xhci->op_regs->cmd_ring);
> -	writel(temp_32 | CMD_RING_ABORT, &xhci->op_regs->cmd_ring);
> +	next_trb(xhci, NULL, &new_seg, &new_deq);
> +	if (trb_is_link(new_deq))
> +		next_trb(xhci, NULL, &new_seg, &new_deq);
> +
> +	crcr = xhci_trb_virt_to_dma(new_seg, new_deq);
> +	xhci_write_64(xhci, crcr | 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
> --
> 2.25.1
>
>

Re: [RFT PATCH] xhci: Fix commad ring abort, write all 64 bits to CRCR register.

[Pavan Kondeti]

Hi Mathias,

On Fri, Oct 29, 2021 at 03:51:54PM +0300, Mathias Nyman wrote:
> Turns out some xHC controllers require all 64 bits in the CRCR register
> to be written to execute a command abort.
> 
> The lower 32 bits containing the command abort bit is written first.
> In case the command ring stops before we write the upper 32 bits then
> hardware may use these upper bits to set the commnd ring dequeue pointer.
> 
> Solve this by making sure the upper 32 bits contain a valid command
> ring dequeue pointer.
> 
> The original patch that only wrote the first 32 to stop the ring went
> to stable, so this fix should go there as well.
> 
> Fixes: ff0e50d3564f ("xhci: Fix command ring pointer corruption while aborting a command")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-ring.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 311597bba80e..eaa49aef2935 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -366,7 +366,9 @@ 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)
>  {
> -	u32 temp_32;
> +	struct xhci_segment *new_seg	= xhci->cmd_ring->deq_seg;
> +	union xhci_trb *new_deq		= xhci->cmd_ring->dequeue;
> +	u64 crcr;
>  	int ret;
>  
>  	xhci_dbg(xhci, "Abort command ring\n");
> @@ -375,13 +377,18 @@ static int xhci_abort_cmd_ring(struct xhci_hcd *xhci, unsigned long flags)
>  
>  	/*
>  	 * 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.
> +	 * 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.
>  	 */
> -	temp_32 = readl(&xhci->op_regs->cmd_ring);
> -	writel(temp_32 | CMD_RING_ABORT, &xhci->op_regs->cmd_ring);
> +	next_trb(xhci, NULL, &new_seg, &new_deq);
> +	if (trb_is_link(new_deq))
> +		next_trb(xhci, NULL, &new_seg, &new_deq);
> +
> +	crcr = xhci_trb_virt_to_dma(new_seg, new_deq);
> +	xhci_write_64(xhci, crcr | 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

Thanks for the patch. I don't see any issues with this patch.

Feel free to add

Tested-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>

Thanks,
Pavan

Re: [RFT PATCH] xhci: Fix commad ring abort, write all 64 bits to CRCR register.

[Mathias Nyman]

On 1.11.2021 5.31, Pavan Kondeti wrote:

> Thanks for the patch. I don't see any issues with this patch.
> 
> Feel free to add
> 
> Tested-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
> 

Thanks, will do

-Mathias

Re: [RFT PATCH] xhci: Fix commad ring abort, write all 64 bits to CRCR register.

[Mathias Nyman]

On 29.10.2021 18.35, youling 257 wrote:
> test it can work for me.
> 

Thanks for testing

-Mathias