Re: [PATCH v3 2/4] usb: xhci: sideband: allocate sideband ring segments from a dedicated pool

[Wesley Cheng <wesley.cheng@oss.qualcomm.com>]

On 9/9/2026 3:08 AM, Mathias Nyman wrote:
> On 9/4/26 09:57, Wesley Cheng wrote:
>> Ring segments are normally allocated from a shared DMA pool sized and
>> aligned to TRB_SEGMENT_SIZE (4096 bytes). On kernels built with a
>> larger PAGE_SIZE (e.g. 16K or 64K page arches), a segment can end up
>> at a non-page-aligned offset within its enclosing CPU page, and
>> multiple segments can share the same physical page.
>>
>> A sideband client that maps a ring buffer directly via the IOMMU
>> (which operates at page granularity) needs to know exactly which
>> page(s) back the ring, and only pages that are actually intended to
>> be exposed to that client should ever be mapped this way.
>>
>> Allow each xhci_sideband endpoint to pass its own segment_pool, allocated
>> separately from the core xhci->segment_pool, so every segment backing
>> a sideband-tagged endpoint always comes from a page that is meant to
>> be visible by the entity handling the offloaded endpoints. Normal
>> (non-offloaded) endpoints are unaffected, as they keep allocating from
>> xhci->segment_pool.
>>
>> The offload client owns the pool's full lifetime, and since
>> that lifetime is no longer tied to the sideband instance itself,
>> xhci_sideband_unregister() must free any ring still backed by a
>> client-supplied pool before returning, rather than leaving it for xhci
>> to free later when the client and its pool may already be gone.
>>
>> Assisted-by: Claude:claude-sonnet-5
>> Signed-off-by: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
>> ---
>>   drivers/usb/host/xhci-mem.c       | 63 ++++++++++++++++++++++++ 
>> +--------------
>>   drivers/usb/host/xhci-sideband.c  | 40 ++++++++++++++++++++++---
>>   drivers/usb/host/xhci.h           | 16 +++++-----
>>   include/linux/usb/xhci-sideband.h | 20 +++++++++++--
>>   sound/usb/qcom/qc_audio_offload.c | 21 +++++++++++--
>>   5 files changed, 121 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 7a21ac81f9c8..a041a35fcd4f 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -28,6 +28,7 @@
>>    * "All components of all Command and Transfer TRBs shall be 
>> initialized to '0'"
>>    */
>>   static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
>> +                           struct dma_pool *pool,
>>                              unsigned int max_packet,
>>                              unsigned int num,
>>                              gfp_t flags)
>> @@ -40,7 +41,7 @@ static struct xhci_segment *xhci_segment_alloc(struct 
>> xhci_hcd *xhci,
>>       if (!seg)
>>           return NULL;
>> -    seg->trbs = dma_pool_zalloc(xhci->segment_pool, flags, &dma);
>> +    seg->trbs = dma_pool_zalloc(pool, flags, &dma);
>>       if (!seg->trbs) {
>>           kfree(seg);
>>           return NULL;
>> @@ -50,7 +51,7 @@ static struct xhci_segment *xhci_segment_alloc(struct 
>> xhci_hcd *xhci,
>>           seg->bounce_buf = kzalloc_node(max_packet, flags,
>>                       dev_to_node(dev));
>>           if (!seg->bounce_buf) {
>> -            dma_pool_free(xhci->segment_pool, seg->trbs, dma);
>> +            dma_pool_free(pool, seg->trbs, dma);
>>               kfree(seg);
>>               return NULL;
>>           }
>> @@ -62,10 +63,11 @@ static struct xhci_segment *xhci_segment_alloc(struct 
>> xhci_hcd *xhci,
>>       return seg;
>>   }
>> -static void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment 
>> *seg)
>> +static void xhci_segment_free(struct xhci_hcd *xhci, struct dma_pool *pool,
>> +                  struct xhci_segment *seg)
>>   {
>>       if (seg->trbs) {
>> -        dma_pool_free(xhci->segment_pool, seg->trbs, seg->dma);
>> +        dma_pool_free(pool, seg->trbs, seg->dma);
>>           seg->trbs = NULL;
>>       }
>>       kfree(seg->bounce_buf);
>> @@ -81,7 +83,7 @@ static void xhci_ring_segments_free(struct xhci_hcd 
>> *xhci, struct xhci_ring *rin
>>       while (seg) {
>>           next = seg->next;
>> -        xhci_segment_free(xhci, seg);
>> +        xhci_segment_free(xhci, ring->segment_pool, seg);
>>           seg = next;
>>       }
>>   }
>> @@ -334,7 +336,7 @@ static int xhci_alloc_segments_for_ring(struct 
>> xhci_hcd *xhci, struct xhci_ring
>>       struct xhci_segment *prev;
>>       unsigned int num = 0;
>> -    prev = xhci_segment_alloc(xhci, ring->bounce_buf_len, num, flags);
>> +    prev = xhci_segment_alloc(xhci, ring->segment_pool, ring- 
>> >bounce_buf_len, num, flags);
>>       if (!prev)
>>           return -ENOMEM;
>>       num++;
>> @@ -343,7 +345,8 @@ static int xhci_alloc_segments_for_ring(struct 
>> xhci_hcd *xhci, struct xhci_ring
>>       while (num < ring->num_segs) {
>>           struct xhci_segment    *next;
>> -        next = xhci_segment_alloc(xhci, ring->bounce_buf_len, num, flags);
>> +        next = xhci_segment_alloc(xhci, ring->segment_pool, ring- 
>> >bounce_buf_len,
>> +                      num, flags);
>>           if (!next)
>>               goto free_segments;
>> @@ -362,15 +365,10 @@ static int xhci_alloc_segments_for_ring(struct 
>> xhci_hcd *xhci, struct xhci_ring
>>       return -ENOMEM;
>>   }
>> -/*
>> - * Create a new ring with zero or more segments.
>> - *
>> - * Link each segment together into a ring.
>> - * Set the end flag and the cycle toggle bit on the last segment.
>> - * See section 4.9.1 and figures 15 and 16.
>> - */
>> -struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci, unsigned int 
>> num_segs,
>> -                  enum xhci_ring_type type, unsigned int max_packet, 
>> gfp_t flags)
>> +static struct xhci_ring *
>> +xhci_ring_alloc_from_pool(struct xhci_hcd *xhci, unsigned int num_segs,
>> +              enum xhci_ring_type type, unsigned int max_packet,
>> +              struct dma_pool *pool, gfp_t flags)
>>   {
>>       struct xhci_ring    *ring;
>>       int ret;
>> @@ -382,6 +380,7 @@ struct xhci_ring *xhci_ring_alloc(struct xhci_hcd 
>> *xhci, unsigned int num_segs,
>>       ring->num_segs = num_segs;
>>       ring->bounce_buf_len = max_packet;
>> +    ring->segment_pool = pool;
>>       INIT_LIST_HEAD(&ring->td_list);
>>       ring->type = type;
>>       if (num_segs == 0)
>> @@ -398,6 +397,20 @@ struct xhci_ring *xhci_ring_alloc(struct xhci_hcd 
>> *xhci, unsigned int num_segs,
>>       return NULL;
>>   }
>> +/*
>> + * Create a new ring with zero or more segments.
>> + *
>> + * Link each segment together into a ring.
>> + * Set the end flag and the cycle toggle bit on the last segment.
>> + * See section 4.9.1 and figures 15 and 16.
>> + */
>> +struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci, unsigned int 
>> num_segs,
>> +                  enum xhci_ring_type type, unsigned int max_packet, 
>> gfp_t flags)
>> +{
>> +    return xhci_ring_alloc_from_pool(xhci, num_segs, type, max_packet,
>> +                     xhci->segment_pool, flags);
>> +}
>> +
>>   void xhci_free_endpoint_ring(struct xhci_hcd *xhci,
>>           struct xhci_virt_device *virt_dev,
>>           unsigned int ep_index)
>> @@ -422,6 +435,7 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct 
>> xhci_ring *ring,
>>       new_ring.num_segs = num_new_segs;
>>       new_ring.bounce_buf_len = ring->bounce_buf_len;
>>       new_ring.type = ring->type;
>> +    new_ring.segment_pool = ring->segment_pool;
>>       ret = xhci_alloc_segments_for_ring(xhci, &new_ring, flags);
>>       if (ret)
>>           return -ENOMEM;
>> @@ -1424,6 +1438,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
>>       unsigned int mult;
>>       unsigned int avg_trb_len;
>>       unsigned int err_count = 0;
>> +    struct dma_pool *pool;
>>       ep_index = xhci_get_endpoint_index(&ep->desc);
>>       ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->in_ctx, ep_index);
>> @@ -1487,8 +1502,10 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
>>           avg_trb_len = 8;
>>       /* Set up the endpoint ring */
>> +    pool = virt_dev->eps[ep_index].priv_seg_pool ?
>> +           virt_dev->eps[ep_index].priv_seg_pool : xhci->segment_pool;
>>       virt_dev->eps[ep_index].new_ring =
>> -        xhci_ring_alloc(xhci, 2, ring_type, max_packet, mem_flags);
>> +        xhci_ring_alloc_from_pool(xhci, 2, ring_type, max_packet, pool, 
>> mem_flags);
>>       if (!virt_dev->eps[ep_index].new_ring)
>>           return -ENOMEM;
>> @@ -2291,7 +2308,8 @@ static int xhci_setup_port_arrays(struct xhci_hcd 
>> *xhci, gfp_t flags)
>>   }
>>   static struct xhci_interrupter *
>> -xhci_alloc_interrupter(struct xhci_hcd *xhci, unsigned int segs, gfp_t 
>> flags)
>> +xhci_alloc_interrupter(struct xhci_hcd *xhci, unsigned int segs,
>> +               struct dma_pool *pool, gfp_t flags)
>>   {
>>       struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
>>       struct xhci_interrupter *ir;
>> @@ -2308,7 +2326,7 @@ xhci_alloc_interrupter(struct xhci_hcd *xhci, 
>> unsigned int segs, gfp_t flags)
>>       if (!ir)
>>           return NULL;
>> -    ir->event_ring = xhci_ring_alloc(xhci, segs, TYPE_EVENT, 0, flags);
>> +    ir->event_ring = xhci_ring_alloc_from_pool(xhci, segs, TYPE_EVENT, 
>> 0, pool, flags);
>>       if (!ir->event_ring) {
>>           xhci_warn(xhci, "Failed to allocate interrupter event ring\n");
>>           kfree(ir);
>> @@ -2356,7 +2374,8 @@ void xhci_add_interrupter(struct xhci_hcd *xhci, 
>> unsigned int intr_num)
>>   struct xhci_interrupter *
>>   xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs,
>> -                  u32 imod_interval, unsigned int intr_num)
>> +                  struct dma_pool *pool, u32 imod_interval,
>> +                  unsigned int intr_num)
>>   {
>>       struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>       struct xhci_interrupter *ir;
>> @@ -2367,7 +2386,7 @@ xhci_create_secondary_interrupter(struct usb_hcd 
>> *hcd, unsigned int segs,
>>           intr_num >= xhci->max_interrupters)
>>           return NULL;
>> -    ir = xhci_alloc_interrupter(xhci, segs, GFP_KERNEL);
>> +    ir = xhci_alloc_interrupter(xhci, segs, pool, GFP_KERNEL);
> 
> 
> Passing a custom dma pool to xhci_create_secondary_interrupters() shuld be 
> optional.
> Callers shoudn't need to be aware of the xhci->segment_pool, and pass it in 
> the default case.
> 

Makes sense, will fix this.

> Instead use the default xhci->segment_pool if caller passes NULL for pool.
> 
> 
>>       if (!ir)
>>           return NULL;
>> @@ -2498,7 +2517,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>>       if (!xhci->interrupters)
>>           goto fail;
>> -    xhci->interrupters[0] = xhci_alloc_interrupter(xhci, 0, flags);
>> +    xhci->interrupters[0] = xhci_alloc_interrupter(xhci, 0, xhci- 
>> >segment_pool, flags);
>>       if (!xhci->interrupters[0])
>>           goto fail;
> 
> I would split the patch here.
> 
> Do all xhci "core" code above in one patch, and the sideband changes in a 
> separate patch.
> Only sideband change needed in first patch would be passing NULL for pool when
> creating the secondary interrupter:
> 
>       sb->ir = xhci_create_secondary_interrupter(xhci_to_hcd(sb->xhci),
> -                           num_seg, imod_interval,
> -                           intr_num);
> +                           num_seg, NULL,
> +                           imod_interval, intr_num);
> 
Sounds good. Will split the patch accordingly.

Thanks
Wesley Cheng