Re: [PATCH v3 2/4] usb: xhci: sideband: allocate sideband ring segments from a dedicated pool
From: Wesley Cheng
Date: Wed Sep 09 2026 - 20:38:08 EST
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@xxxxxxxxxxxxxxxx>
---
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.Sounds good. Will split the patch accordingly.
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
Wesley Cheng