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
