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

Mathias Nyman
2025-02-27T12:09:05Z

On 27.2.2025 0.05, Michał Pecio wrote:
> 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.

Good point

> 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.
> */

True, but as you said this shouldn't happen as we allocate TRB_SEGMENT_SIZE bytes
starting at seg->dma, so seg->dma should be at least TRB_SEGMENT_SIZE bytes from
max u64 (or u32)

We can also use "if (dma >= seg->dma && (dma - seg->dma) < TRB_SEGMENT_SIZE)"
instead of in_range(). It's a bit uglier, but we can skip additional notes.

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

True, it could be just a bool

>
>> + /* 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.

Idea of this patch is to slowly migrate handle_tx_event() towards the
vision in my feature_transfer_event_rework branch

https://web.git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_transfer_event_rework

Niklas is working towards a similar goal, and he just informed me that this
patch conflicts a bit with his plan to get there, so I might drop this.

Thanks for looking at it.

-Mathias