* Re: [RFT PATCH] xhci: Fix commad ring abort, write all 64 bits to CRCR register.
2021-10-29 12:51 ` [RFT PATCH] xhci: Fix commad ring abort, write all 64 bits to CRCR register Mathias Nyman
@ 2021-10-29 15:35 ` youling 257
2021-11-01 8:37 ` Mathias Nyman
2021-11-01 3:31 ` Pavan Kondeti
1 sibling, 1 reply; 5+ messages in thread
From: youling 257 @ 2021-10-29 15:35 UTC (permalink / raw)
To: Mathias Nyman; +Cc: quic_pkondeti, pkondeti, gregkh, linux-usb, stable
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFT PATCH] xhci: Fix commad ring abort, write all 64 bits to CRCR register.
2021-10-29 12:51 ` [RFT PATCH] xhci: Fix commad ring abort, write all 64 bits to CRCR register Mathias Nyman
2021-10-29 15:35 ` youling 257
@ 2021-11-01 3:31 ` Pavan Kondeti
2021-11-01 8:36 ` Mathias Nyman
1 sibling, 1 reply; 5+ messages in thread
From: Pavan Kondeti @ 2021-11-01 3:31 UTC (permalink / raw)
To: Mathias Nyman
Cc: quic_pkondeti, youling257, pkondeti, gregkh, linux-usb, stable
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
^ permalink raw reply [flat|nested] 5+ messages in thread