Re: [PATCH 5/4 RFC] An alternative dma_in_range() implementation

Neronin, Niklas
2025-02-20T13:18:13Z

On 20/02/2025 14.14, Michał Pecio wrote:
> Here's my attempt at it, the patch goes on top of the whole series. It
> compiles and passes basic testing (some URBs completed, some unlinked,
> no errors or other malfunction apparent).
>
> The idea is to translate dma to trb* ASAP and work with that, because
> all data structures use trb* so the code gets shorter and less verbose.
> This approach is also easy to adapt to changes in handle_tx_event(),
> for example it could trivially return trb* instead of seg* or take trb*
> (with or without accompanying seg*) and return bool or even seg*.
>
> I tried to make the common case (start_seg == end_seg) fast, both for
> hit and miss. 5 comparisons if the range wraps around, 4 if it doesn't.
>

Thanks, I have a similar version. If it's alright with you, I'll try to
combine your version and mine into one and add it to trb_in_td-v2.

Thanks,
Niklas

> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 600842425f6d..b18e7fd7d01e 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -300,42 +300,36 @@ static struct xhci_segment *dma_in_range(struct xhci_segment *start_seg, union x
> dma_addr_t dma)
> {
> struct xhci_segment *seg = start_seg;
> + union xhci_trb *trb;
>
> - if (start_seg == end_seg) {
> - if (start_trb <= end_trb) {
> - if (xhci_trb_virt_to_dma(start_seg, start_trb) <= dma &&
> - dma <= xhci_trb_virt_to_dma(end_seg, end_trb))
> - return seg;
> - return NULL;
> + /* check if dma is a TRB in start_seg */
> + if (in_range(dma, seg->dma, TRB_SEGMENT_SIZE)) {
> + trb = seg->trbs + (dma - seg->dma) / sizeof(*trb);
> +
> + if (trb >= start_trb)
> + /* check if the range covers it and we are done */
> + return (end_seg != seg || trb <= end_trb) ? seg : NULL;
> +
> + /* check if the range circles back to the beginning of start_seg */
> + return (end_seg == seg && end_trb < start_trb && trb <= end_trb) ? seg : NULL;
> + }
> +
> + /* stop if the range doesn't pass through any other segment */
> + if (end_seg == seg && (end_trb >= start_trb || seg->next == seg))
> + return NULL;
> +
> + /* search remaining segments knowing that start_trb isn't there */
> + do {
> + seg = seg->next;
> +
> + if (in_range(dma, seg->dma, TRB_SEGMENT_SIZE)) {
> + trb = seg->trbs + (dma - seg->dma) / sizeof(*trb);
> +
> + return (seg != end_seg || trb <= end_trb) ? seg : NULL;
> }
> + } while (seg != end_seg && seg != start_seg);
>
> - /* Edge case, the TD wrapped around to the start segment. */
> - if (xhci_trb_virt_to_dma(end_seg, end_trb) < dma &&
> - dma < xhci_trb_virt_to_dma(start_seg, start_trb))
> - return NULL;
> - if (seg->dma <= dma && dma <= (seg->dma + TRB_SEGMENT_SIZE))
> - return seg;
> - seg = seg->next;
> - }
> -
> - /* Loop through segment which don't contain the DMA address. */
> - while (dma < seg->dma || (seg->dma + TRB_SEGMENT_SIZE) <= dma) {
> - if (seg == end_seg)
> - return NULL;
> -
> - seg = seg->next;
> - if (seg == start_seg)
> - return NULL;
> - }
> -
> - if (seg == start_seg) {
> - if (dma < xhci_trb_virt_to_dma(start_seg, start_trb))
> - return NULL;
> - } else if (seg == end_seg) {
> - if (xhci_trb_virt_to_dma(end_seg, end_trb) < dma)
> - return NULL;
> - }
> - return seg;
> + return NULL;
> }
>
> static struct xhci_segment *trb_in_td(struct xhci_td *td, dma_addr_t dma)