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