Re: [PATCH v3 0/5] xHCI: Isochronous error handling fixes and improvements

["Michał Pecio" <michal.pecio@gmail.com>]

On Wed, 26 Feb 2025 14:41:44 +0200, Mathias Nyman wrote:
> Updated my for-usb-next branch to this v3 version


A few remarks regarding "Add helper to find trb from its dma address":

xhci_dma_to_trb():
This function could use xhci_for_each_ring_seg.
The use of in_range() perhaps deserves a comment, because its
correctness is not as obvious as it might seem.

Long story short, my own version:

/*
 * Look up a TRB on the ring by its DMA address, return NULL if not found.
 * Start from deq_seg to optimize for event handling use.
 *
 * Note: false positive is possible if dma < TRB_SEGMENT_SIZE *and*
 * seg->dma > (dma_addr_t) 0 - TRB_SEGMENT_SIZE, but that shouldn't
 * happen if seg->dma is an allocation of size TRB_SEGMENT_SIZE.
 */
static union xhci_trb *xhci_dma_to_trb(struct xhci_ring *ring, dma_addr_t dma)
{
       struct xhci_segment *seg;

       xhci_for_each_ring_seg(ring->deq_seg, seg)
               if (in_range(dma, seg->dma, TRB_SEGMENT_SIZE))
                       return seg->trbs + (dma - seg->dma) / sizeof(seg->trbs[0]);

       return NULL;
}

>+       struct xhci_td *matched_td;

This variable is only used as bool so it could be declared as such.
Other places still use 'td' and assume that it equals 'matched_td'.
And that's OK because there is no need for iterating further after
the matching TD is found.

>+       /* find the transfer trb this events points to */
>+       if (ep_trb_dma)
>+               ep_trb = xhci_dma_to_trb(ep_ring, ep_trb_dma);

This may deserve a dedicated warning. It's a pathology. Either the
event is bogus due to internal corruption in the HC, or it's executing
TRBs from a wrong ring due to damaged/ignored Link TRB or bad Set Deq.
Or we completely screwed up and are looking at a wrong ep_ring here.

>-       if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !ep_trb_dma)
>+       if (trb_comp_code == COMP_MISSED_SERVICE_ERROR && !ep_trb)
>                return 0;

Good idea. I think I would go further and refuse to handle anything
when (ep_trb_dma && !ep_trb). Nothing is going to match, nothing good
will come from trying as far as I see.

But that's a behavior change, so maybe material for a separate patch.

>+               matched_td = NULL;
> 
>                /* Is this a TRB in the currently executing TD? */
>-               ep_seg = trb_in_td(xhci, td, ep_trb_dma, false);
>+               if (trb_in_td(xhci, td, ep_trb_dma, false))
>+                       matched_td = td;

If 'matched_td' is declared as bool, this will become simply:

matched_td = trb_in_td(xhci, td, ep_trb_dma, false);

Regards,
Michal