Re: [PATCH 3/4] usb: xhci: rework and simplify trb_in_td()

Michał Pecio
2025-02-19T08:56:43Z

Hi,

> + /* Edge case, the TD wrapped around to the start segment. */
> + if (xhci_trb_virt_to_dma(td->end_seg, td->end_trb) < dma &&
> + dma < xhci_trb_virt_to_dma(td->start_seg, td->start_trb))
> + return NULL;
> + if (seg->dma <= dma && dma <= (seg->dma + TRB_SEGMENT_SIZE))

It should be strict inequality for the upper bound here.

Note that this wraparound case souldn't be happening (the driver avoids
moving enqueue into deq_seg to simplify ring expansion) so no amount of
testing will catch problems here, until maybe something changes one day.

> + return seg;
> + seg = seg->next;
> + }

The situation is tricky now, because we are either in start_seg and
end_seg is elsewhere or in start_seg->next and wraparound. But it looks
like the loop below will work OK for either case.

> + /* Loop through segment which don't contain the DMA address. */
> + while (dma < seg->dma || (seg->dma + TRB_SEGMENT_SIZE) <= dma) {

This condition looks like it could use the in_range() macro.

> + if (seg == td->end_seg)
> + return NULL;
> +
> + seg = seg->next;
> + if (seg == td->start_seg)
> + return NULL;

I suppose this only happens if end_seg is not on the ring, fair enough.

> + }

Maybe a comment here? Something like:

* At this point seg contains the dma and either:
* a. start_seg != end_seg and seg can be anywhere
* b. start_seg == end_seg in wraparound case and seg != start_seg

> + if (seg == td->start_seg) {
> + if (dma < xhci_trb_virt_to_dma(td->start_seg, td->start_trb))
> + return NULL;
> + } else if (seg == td->end_seg) {
> + if (xhci_trb_virt_to_dma(td->end_seg, td->end_trb) < dma)
> + return NULL;
> + }
> + return seg;

This should be corrent, but it's not something immediately obvious.

Not sure if this new implementation is really simpler than the old one.
I wonder if it wouldn't make sense to reorder this after the API change
(patch 4/4) to allow emergency revert if something unexpected shows up.

As for efficiency, those virt_to_dma translations aren't exactly free
and there are two. Maybe it could be faster to translate dma to virt
once and then compare. Sometimes also sizeof(*) < sizeof(dma_addr_t).

Regards,
Michal