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

[Mathias Nyman <mathias.nyman@linux.intel.com>]

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.

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);

Thanks
Mathias