Re: [PATCH v2] xhci: Fix Link TRB DMA in command ring stopped completion event

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 22.10.2024 15.34, Faisal Hassan wrote:
> Hi Mathias,
> 
>> Do we in this COMP_COMMAND_RING_STOPPED case even need to check if
>> cmd_dma != (u64)cmd_dequeue_dma, or if command ring stopped on a link TRB?
>>
>> Could we just move the COMP_COMMAND_RING_STOPPED handling a bit earlier?
>>
>> if (cmd_comp_code == COMP_COMMAND_RING_STOPPED) {
>>      complete_all(&xhci->cmd_ring_stop_completion);
>>          return;
>> }
>>
>> If I remember correctly it should just turn aborted command TRBs into
>> no-ops,
>> and restart the command ring
>>
> 
> Thanks for reviewing the changes!
> 
> Yes, you’re right. As part of restarting the command ring, we just ring
> the doorbell.
> 
> If we move the event handling without validating the dequeue pointer,
> wouldn’t it be a risk if we don’t check what the xHC is holding in its
> dequeue pointer? If we are not setting it, it starts from wherever it
> stopped. What if the dequeue pointer got corrupted or is not pointing to
> any of the TRBs in the command ring?

For that to happen the xHC host would have to corrupt its internal command
ring dequeue pointer. Not impossible, but an unlikely HW flaw, and a separate
issue. A case like that could be solved by writing the address of the next valid
(non-aborted) command to the CRCR register in xhci_handle_stopped_cmd_ring() before
ringing the doorbell.

The case you found where a command abort is not handled properly due to stopping
on a link TRB is a real xhci driver issue that would be nice to get solved.

For the COMP_COMMAND_RING_STOPPED case we don't really care that much
on which command it stopped, for other commands we do.

Thanks
Mathias