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

[Pavan Kondeti <pkondeti@codeaurora.org>]

On Mon, Oct 25, 2021 at 06:21:00PM +0300, Mathias Nyman wrote:
> On 25.10.2021 18.01, youling 257 wrote:
> > test this patch suspend resume usb can work.
> 
> Great, thanks.
> 
> Pavankumar Kondeti, does your case still work after this modification? 
> 
> 
Hi Mathias,


+	crcr = xhci_trb_virt_to_dma(xhci->cmd_ring->deq_seg,
+				    xhci->cmd_ring->dequeue);
+	xhci_write_64(xhci, crcr | CMD_RING_ABORT, &xhci->op_regs->cmd_ring);

This patch passes my test. Can you please clarify the below question that is
bothering me.

Here crcr points to the DMA address of the command which is getting
executed (soon will be aborted) by the xHC. After the ring is stopped,
we want xHC to execute the *next* command.

Is it guaranteed that the upper 32 bits of previous and next commands will
be same? Because when the issue happens, xHC takes the 32 bits of crcr
variable and update it's internal pointer from which it will fetch the commands
next time the ring is started. I think it is guaranteed because the upper 32
bits only may change when we cross the segments but in which case there will
be a link TRB in the middle. Since xHC command ring is stopped, it won't be
fetching the link TRB until door bell is rung again. Is my understanding correct?

Also, what if there is a race with xHC just finishing this command (which we
are currently aborting) and next link TRB is traversed and processing the next
command in a different segment. For some reason, we could not update our
deq pointer and in middle of aborting the command (which is already completed)
and updating the higher 32 bit with the previous deq segment. This is a
hypothetical question as we are only using 1 segment for the command ring.

Thanks,
Pavan


-- 
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.