Hi Michal,
On 10/19/2024 12:50 PM, Michał Pecio wrote:
Hi,
During the aborting of a command, the software receives a command completion event for the command ring stopped, with the TRB pointing to the next TRB after the aborted command.
If the command we abort is located just before the Link TRB in the command ring, then during the 'command ring stopped' completion event, the xHC gives the Link TRB in the event's cmd DMA, which causes a mismatch in handling command completion event.
To handle this situation, an additional check has been added to ignore the mismatch error and continue the operation.
Thanks, I remember having some issues with command aborts, but I blamed them on my own bugs, although I never found what the problem was. I may take a look at it later, but I'm currently busy with other things.
No comment about validity of this patch for now, but a few remarks:
+static bool is_dma_link_trb(struct xhci_ring *ring, dma_addr_t dma) +{
- struct xhci_segment *seg;
- union xhci_trb *trb;
- dma_addr_t trb_dma;
- int i;
- seg = ring->first_seg;
- do {
for (i = 0; i < TRBS_PER_SEGMENT; i++) {trb = &seg->trbs[i];trb_dma = seg->dma + (i * sizeof(union xhci_trb));if (trb_is_link(trb) && trb_dma == dma)return true;}You don't need to iterate the array. Something like this should work: do { if (in_range(dma, seg->dma, TRB_SEGMENT_SIZE)) { /* found the TRB, check if it's link */ trb = &seg->trbs[(dma - seg->dma) / sizeof(*trb)]; return trb_is_link(trb); } // try next seg, while (blah blah), return false
Sure, this looks good. Let me revise the patch.
We should probably have a helper for (ring, dma)->trb lookups, but for stable it may be sensible to do it without excess complication.
- if ((!cmd_dequeue_dma || cmd_dma != (u64)cmd_dequeue_dma) &&
!(cmd_comp_code == COMP_COMMAND_RING_STOPPED &&is_dma_link_trb(xhci->cmd_ring, cmd_dma))) {This if statement is quite complex now. I would be tempted to write it this way instead:
/* original comment */ if (cmd_dma != dequeue_dma) { /* your new comment */ if (! (RING_STOPPED && is_link)) { warn(); return; } }
Regards, Michal
Thank you for the suggestions. I will submit a v2 version.
Regards, Faisal