[PATCH v5 0/4] Add larger page size support for USB audio offload path

[PATCH v5 0/4] Add larger page size support for USB audio offload path

[Wesley Cheng]

On some environments, 16kB pages can be enabled from the Linux subsystem,
which manages the IOMMU mappings for the audio DSP within the system.  In
the current design, the following assumptions break when 16k pages are
utilized:
  1. xHCI ring size is equal to PAGE_SIZE
  2. Ring addresses start at the beginning of a page

When the USB offload driver maps the rings (w/ the audio DSP SID), it is
set with a 16k granular, which is a problem, as several xHCI rings could
exist on the same page.  This is because the rings are currently allocated
from the segment_pool.  Hence, potentially mapping non USB audio related
rings into the region accessible by the audio DSP.

To mitigate this, sideband rings are no longer allocated from the
segment_pool.  When PAGE_SIZE is larger than TRB_SEGMENT_SIZE, xHCI
allocates the segments of sideband-tagged endpoint rings, and of the
sideband secondary event ring, using dma_alloc_coherent(), which gives
each segment a page of its own.  Before the USB audio offload path is
enabled, the USB audio data streams/endpoint are not active.  Only when
the class driver issues a usb_set_interface() call (done from
snd_usb_endpoint_prepare()), will the xHCI allocate the transfer ring
resources.  The offload client driver therefore tags the endpoints with
xhci_sideband_add_endpoint() before that point.  The new ring allocation
flow for sideband/offload clients will be as follows:

handle_uaudio_stream_req()
  ▼
enable_audio_stream(subs, ..., pcm_card_num)
  ├─ data_ep = uaudio_find_host_endpoint(subs, subs->data_endpoint)
  ├─ xhci_sideband_add_endpoint(sb, data_ep) ← ep->sideband = sb; ep->no_pool_alloc = PAGE_SIZE > TRB_SEGMENT_SIZE
  ├─ snd_usb_endpoint_prepare(chip, sync_endpoint) ─┐
  ├─ snd_usb_endpoint_prepare(chip, data_endpoint)  ├─→ xhci_add_endpoint()
  │                                                 ▼
  │            xhci_endpoint_init(xhci, virt_dev, ep, ...)
  │            new_ring = __xhci_ring_alloc(..., ep->no_pool_alloc, ...)
  │            ▼
  │            xhci_alloc_segments_for_ring(xhci, ring, flags)
  │            ▼
  │            xhci_segment_alloc(xhci, max_packet, num, ring->no_pool_alloc, flags)
  │            seg->trbs = dma_alloc_coherent(dev, TRB_SEGMENT_SIZE, &dma, flags)
  ▼
xhci_sideband_get_endpoint_buffer(sb, data_ep) → xhci_ring_to_sgtable()

Similar logic is added for the secondary interrupter path as well.  The USB
offload class driver calls xhci_sideband_create_interrupter(), which will
be responsible for allocating the secondary event ring.

xHCI core remains responsible for freeing these rings, once the endpoint
has been dropped or the slot disabled.  This also covers disconnecting the
USB audio device while audio offload is active: usb_set_interface()
returns early as the interface is unregistering, and the rings are freed
later, when USB core drops the device's endpoints.  As xhci_free_dev()
frees the virt device with interrupts disabled, where dma_free_coherent()
can't be called, those rings are freed after xhci->lock is released.

This was confirmed to work on the SM8350 MTP platform, with the
CONFIG_ARM64_16K_PAGES config enabled, alongside tinyaudio binaries:

tinymix -D 0 set 513 1  (Enables USB_RX multimedia#1 path)
tinyplay -D 0 -d 0....  (Routes PCM data to ASoC platform sound card)

Signed-off-by: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
---
Changes in v5:
- Dropped the offload client owned segment pool, as suggested by Mathias.
  xHCI now allocates sideband ring segments with dma_alloc_coherent() when
  PAGE_SIZE is larger than TRB_SEGMENT_SIZE.
- Free rings that are not allocated from the segment pool outside of
  xhci->lock in xhci_free_dev().
- Modified the xhci_ring_to_sgtable sequence to address an error from Sashiko
  where it questioned the safeness of virt_to_page.  Leverage
  dma_get_sgtable, which will call the appropriate DMA mapping
  implementation.
- Link to v4: https://patch.msgid.link/20260909-16k_offload_v1_b4-v4-0-f24a86617597@oss.qualcomm.com

Changes in v4:
- Split xHCI patches from xHCI core and xHCI sideband.
- Make the dedicated offload segment pool optional for secondary
  interrupter creation. (xfer rings support this already in previous
  revs)
- Fixed some inter-patch build/functionality failures in
  qc_audio_offload. Incremental builds between patches now
  work on 4k pages. (16k support at end)
- Link to v3: https://patch.msgid.link/20260903-16k_offload_v1_b4-v3-0-135928dc2408@oss.qualcomm.com

Changes in v3:
- Moved sideband segment pool assignment to when a sideband endpoint
  is added.
- Address an issue where during USB device disconnect (while offload
  active), where the offload client driver is freeing the DMA pool
  before xHCI core is able to free the dma allocations.
- Update xhci_ring_to_sgtable to correctly allocate and fill the sg
  table which capture rings within page offsets.
- Link to v2: https://patch.msgid.link/20260828-16k_offload_v1_b4-v2-0-8a46369ebbb6@oss.qualcomm.com

Changes in v2:
- Moved from using alignment_req to having the offload driver maintain
  its own segment pool.
- Fixed OOB condition seen in xhci_ring_to_sgtable() and will capture
  page offsets properly.
- Addressed inter-patch build failures.
- Link to v1: https://patch.msgid.link/20260824-16k_offload_v1_b4-v1-0-49a6be60ca30@oss.qualcomm.com

To: Mathias Nyman <mathias.nyman@intel.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Jaroslav Kysela <perex@perex.cz>
To: Takashi Iwai <tiwai@suse.com>
Cc: linux-usb@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-sound@vger.kernel.org

---
Wesley Cheng (4):
      usb: xhci: sideband: fix ring sg table for sub-page TRB segments
      usb: xhci: Allocate sideband ring segments with dma_alloc_coherent()
      ALSA: usb-audio: qcom: tag sideband endpoints before ring allocation
      ALSA: usb-audio: qcom: fix xfer ring IOMMU unmap on 16K+ page kernels

 drivers/usb/host/xhci-mem.c       |  63 +++++++++++++-----
 drivers/usb/host/xhci-sideband.c  |  97 +++++++++++++++++-----------
 drivers/usb/host/xhci.c           |  13 ++++
 drivers/usb/host/xhci.h           |   5 +-
 sound/usb/qcom/qc_audio_offload.c | 131 ++++++++++++++++++++++++++++++--------
 5 files changed, 228 insertions(+), 81 deletions(-)
---
base-commit: 69f0645f043feed4dc07690c74597574f43c41b4
change-id: 20260824-16k_offload_v1_b4-3d1460405774

Best regards,
-- 
Wesley Cheng <wesley.cheng@oss.qualcomm.com>

[PATCH v5 1/4] usb: xhci: sideband: fix ring sg table for sub-page TRB segments

[Wesley Cheng]

xhci_ring_to_sgtable() populated its sg_table via dma_get_sgtable()
per segment and sg_alloc_table_from_pages(), both of which only
operate at whole PAGE_SIZE granularity. Since TRB_SEGMENT_SIZE (4096)
can be smaller than PAGE_SIZE, these helpers describe a full page for
each ring segment on larger-PAGE_SIZE kernels (16K/64K), and cannot
correctly represent segments that share the same physical page.

Build the sg_table directly instead: allocate one sg entry per ring
segment with sg_alloc_table(), and fill each entry explicitly with
sg_set_page() using the segment's own page, TRB_SEGMENT_SIZE as the
length, and offset_in_page() for the exact intra-page offset. Walk the
table with for_each_sg() so this also works for chained sg tables.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
---
 drivers/usb/host/xhci-sideband.c | 80 ++++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index a5deeee4d5dc..d0c0f75d6085 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -8,68 +8,78 @@
  * Author: Mathias Nyman
  */
 
+#include <linux/dma-mapping.h>
 #include <linux/usb/xhci-sideband.h>
-#include <linux/dma-direct.h>
 
 #include "xhci.h"
 
 /* sideband internal helpers */
+static struct page *
+xhci_segment_to_page(struct device *dev, struct xhci_segment *seg)
+{
+	struct sg_table sgt;
+	struct page *page;
+
+	/*
+	 * Remapped coherent memory is in the vmalloc area, where
+	 * vmalloc_to_page() finds the page for any offset into the allocation.
+	 * dma_get_sgtable() could describe the buffer from the start of its
+	 * allocation instead.
+	 */
+	if (is_vmalloc_addr(seg->trbs))
+		return vmalloc_to_page(seg->trbs);
+
+	if (dma_get_sgtable(dev, &sgt, seg->trbs, seg->dma, TRB_SEGMENT_SIZE))
+		return NULL;
+
+	page = sg_page(sgt.sgl);
+	sg_free_table(&sgt);
+
+	return page;
+}
+
 static struct sg_table *
 xhci_ring_to_sgtable(struct xhci_sideband *sb, struct xhci_ring *ring)
 {
+	struct device *dev = xhci_to_hcd(sb->xhci)->self.sysdev;
 	struct xhci_segment *seg;
-	struct sg_table	*sgt;
-	unsigned int n_pages;
-	struct page **pages;
-	struct device *dev;
-	size_t sz;
+	struct sg_table *sgt;
+	struct scatterlist *sg;
+	struct page *page;
 	int i;
 
-	dev = xhci_to_hcd(sb->xhci)->self.sysdev;
-	sz = ring->num_segs * TRB_SEGMENT_SIZE;
-	n_pages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
-	pages = kvmalloc_objs(struct page *, n_pages);
-	if (!pages)
+	seg = ring->first_seg;
+	if (!seg)
 		return NULL;
 
 	sgt = kzalloc_obj(*sgt);
-	if (!sgt) {
-		kvfree(pages);
+	if (!sgt)
 		return NULL;
-	}
 
-	seg = ring->first_seg;
-	if (!seg)
-		goto err;
-	/*
-	 * Rings can potentially have multiple segments, create an array that
-	 * carries page references to allocated segments.  Utilize the
-	 * sg_alloc_table_from_pages() to create the sg table, and to ensure
-	 * that page links are created.
-	 */
-	for (i = 0; i < ring->num_segs; i++) {
-		dma_get_sgtable(dev, sgt, seg->trbs, seg->dma,
-				TRB_SEGMENT_SIZE);
-		pages[i] = sg_page(sgt->sgl);
-		sg_free_table(sgt);
-		seg = seg->next;
+	if (sg_alloc_table(sgt, ring->num_segs, GFP_KERNEL)) {
+		kfree(sgt);
+		return NULL;
 	}
 
-	if (sg_alloc_table_from_pages(sgt, pages, n_pages, 0, sz, GFP_KERNEL))
-		goto err;
+	for_each_sg(sgt->sgl, sg, sgt->orig_nents, i) {
+		page = xhci_segment_to_page(dev, seg);
+		if (!page)
+			goto err;
 
-	kvfree(pages);
+		sg_set_page(sg, page, TRB_SEGMENT_SIZE, offset_in_page(seg->trbs));
+		seg = seg->next;
+	}
 
 	/*
-	 * Save first segment dma address to sg dma_address field for the sideband
-	 * client to have access to the IOVA of the ring.
+	 * Save the first segment DMA address for the sideband client. The returned
+	 * table is metadata and must not be passed to dma_map_sg().
 	 */
 	sg_dma_address(sgt->sgl) = ring->first_seg->dma;
 
 	return sgt;
 
 err:
-	kvfree(pages);
+	sg_free_table(sgt);
 	kfree(sgt);
 
 	return NULL;

-- 
2.43.0

[PATCH v5 2/4] usb: xhci: Allocate sideband ring segments with dma_alloc_coherent()

[Wesley Cheng]

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), 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) would then also expose any other
ring segment sharing that page, as well as the dma_pool bookkeeping
kept in free blocks, to the entity handling the offloaded endpoints.

Add a no_pool_alloc flag for endpoints and rings, which has the ring
segments allocated with dma_alloc_coherent() instead of from
xhci->segment_pool. Coherent allocations are page aligned, so each of
these segments gets a page of its own. When PAGE_SIZE is larger than
TRB_SEGMENT_SIZE, xhci_sideband_add_endpoint() sets the flag for the
endpoint, and xhci_sideband_create_interrupter() requests it for the
secondary event ring. Normal (non-offloaded) endpoints, and kernels
using 4K pages, are unaffected, as they keep allocating from
xhci->segment_pool.

xHCI still owns and frees these rings once the endpoint has been
dropped or the slot disabled. All of the ring free paths run in
sleepable context, except for xhci_free_dev(), which frees the virt
device while holding xhci->lock with interrupts disabled, where
dma_free_coherent() can't be used. Detach these rings while holding the
lock, after the slot is disabled, and free them once it is released.

Since xHCI never queues transfers to a sideband ring, refuse ring
expansion for these rings. The flag has to be set before the endpoint
ring is allocated, so have xhci_sideband_add_endpoint() return -EBUSY if
the endpoint already has a ring.

Suggested-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
---
 drivers/usb/host/xhci-mem.c      | 63 ++++++++++++++++++++++++++++++----------
 drivers/usb/host/xhci-sideband.c | 17 +++++++++--
 drivers/usb/host/xhci.c          | 13 +++++++++
 drivers/usb/host/xhci.h          |  5 +++-
 4 files changed, 80 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 75577f441cdd..d280023ecbce 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -26,10 +26,15 @@
  *
  * Section 4.11.1.1:
  * "All components of all Command and Transfer TRBs shall be initialized to '0'"
+ *
+ * If no_pool_alloc is set, the segment is allocated with dma_alloc_coherent()
+ * instead of from the segment pool. Coherent allocations are page aligned, so
+ * the segment then has a page of its own.
  */
 static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
 					       unsigned int max_packet,
 					       unsigned int num,
+					       bool no_pool_alloc,
 					       gfp_t flags)
 {
 	struct xhci_segment *seg;
@@ -40,7 +45,11 @@ 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);
+	if (no_pool_alloc)
+		seg->trbs = dma_alloc_coherent(dev, TRB_SEGMENT_SIZE, &dma, flags);
+	else
+		seg->trbs = dma_pool_zalloc(xhci->segment_pool, flags, &dma);
+
 	if (!seg->trbs) {
 		kfree(seg);
 		return NULL;
@@ -50,7 +59,10 @@ 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);
+			if (no_pool_alloc)
+				dma_free_coherent(dev, TRB_SEGMENT_SIZE, seg->trbs, dma);
+			else
+				dma_pool_free(xhci->segment_pool, seg->trbs, dma);
 			kfree(seg);
 			return NULL;
 		}
@@ -62,10 +74,15 @@ 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 xhci_segment *seg,
+			      bool no_pool_alloc)
 {
 	if (seg->trbs) {
-		dma_pool_free(xhci->segment_pool, seg->trbs, seg->dma);
+		if (no_pool_alloc)
+			dma_free_coherent(xhci_to_hcd(xhci)->self.sysdev,
+					  TRB_SEGMENT_SIZE, seg->trbs, seg->dma);
+		else
+			dma_pool_free(xhci->segment_pool, seg->trbs, seg->dma);
 		seg->trbs = NULL;
 	}
 	kfree(seg->bounce_buf);
@@ -81,7 +98,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, seg, ring->no_pool_alloc);
 		seg = next;
 	}
 }
@@ -334,7 +351,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->bounce_buf_len, num, ring->no_pool_alloc, flags);
 	if (!prev)
 		return -ENOMEM;
 	num++;
@@ -343,7 +360,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->bounce_buf_len, num,
+					  ring->no_pool_alloc, flags);
 		if (!next)
 			goto free_segments;
 
@@ -369,8 +387,9 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd *xhci, struct xhci_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(struct xhci_hcd *xhci, unsigned int num_segs,
+					   enum xhci_ring_type type, unsigned int max_packet,
+					   bool no_pool_alloc, gfp_t flags)
 {
 	struct xhci_ring	*ring;
 	int ret;
@@ -382,6 +401,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->no_pool_alloc = no_pool_alloc;
 	INIT_LIST_HEAD(&ring->td_list);
 	ring->type = type;
 	if (num_segs == 0)
@@ -398,6 +418,12 @@ struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci, unsigned int num_segs,
 	return NULL;
 }
 
+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(xhci, num_segs, type, max_packet, false, flags);
+}
+
 void xhci_free_endpoint_ring(struct xhci_hcd *xhci,
 		struct xhci_virt_device *virt_dev,
 		unsigned int ep_index)
@@ -419,8 +445,12 @@ int xhci_ring_expansion(struct xhci_hcd *xhci, struct xhci_ring *ring,
 	if (num_new_segs == 0)
 		return 0;
 
+	if (ring->no_pool_alloc)
+		return -EINVAL;
+
 	new_ring.num_segs = num_new_segs;
 	new_ring.bounce_buf_len = ring->bounce_buf_len;
+	new_ring.no_pool_alloc = false;
 	new_ring.type = ring->type;
 	ret = xhci_alloc_segments_for_ring(xhci, &new_ring, flags);
 	if (ret)
@@ -1488,7 +1518,8 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
 
 	/* Set up the endpoint ring */
 	virt_dev->eps[ep_index].new_ring =
-		xhci_ring_alloc(xhci, 2, ring_type, max_packet, mem_flags);
+		__xhci_ring_alloc(xhci, 2, ring_type, max_packet,
+				  virt_dev->eps[ep_index].no_pool_alloc, mem_flags);
 	if (!virt_dev->eps[ep_index].new_ring)
 		return -ENOMEM;
 
@@ -2305,7 +2336,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, bool no_pool_alloc,
+		       gfp_t flags)
 {
 	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
 	struct xhci_interrupter *ir;
@@ -2322,7 +2354,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(xhci, segs, TYPE_EVENT, 0, no_pool_alloc, flags);
 	if (!ir->event_ring) {
 		xhci_warn(xhci, "Failed to allocate interrupter event ring\n");
 		kfree(ir);
@@ -2370,7 +2402,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)
+				  u32 imod_interval, unsigned int intr_num,
+				  bool no_pool_alloc)
 {
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 	struct xhci_interrupter *ir;
@@ -2381,7 +2414,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, no_pool_alloc, GFP_KERNEL);
 	if (!ir)
 		return NULL;
 
@@ -2512,7 +2545,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, false, flags);
 	if (!xhci->interrupters[0])
 		goto fail;
 
diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index d0c0f75d6085..04eb39e540c8 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -98,6 +98,7 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
 	xhci_stop_endpoint_sync(sb->xhci, ep, 0, GFP_KERNEL);
 
 	ep->sideband = NULL;
+	ep->no_pool_alloc = false;
 	sb->eps[ep->ep_index] = NULL;
 }
 
@@ -150,6 +151,9 @@ EXPORT_SYMBOL_GPL(xhci_sideband_notify_ep_ring_free);
  * After an endpoint is added the sideband client can get the endpoint transfer
  * ring buffer by calling xhci_sideband_endpoint_buffer()
  *
+ * The endpoint must be added before its transfer ring is allocated (before
+ * the alternate setting that enables it is selected).
+ *
  * Return: 0 on success, negative error otherwise.
  */
 int
@@ -177,10 +181,18 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
 	 * to add the endpoint the normal way, but instead offload it immediately,
 	 * and let this function add the endpoint and allocate the ring buffer
 	 * with the smallest common DMA mask
+	 *
+	 * Ensure that the sideband endpoint is added before xHCI has initialized
+	 * the endpoints.
 	 */
-	if (sb->eps[ep_index] || ep->sideband)
+	if (sb->eps[ep_index] || ep->sideband || ep->ring || ep->new_ring)
 		return -EBUSY;
 
+	/*
+	 * The rings are mapped for the offload entity, so on kernels with pages
+	 * larger than a ring segment, give each segment a page of its own.
+	 */
+	ep->no_pool_alloc = PAGE_SIZE > TRB_SEGMENT_SIZE;
 	ep->sideband = sb;
 	sb->eps[ep_index] = ep;
 
@@ -347,7 +359,8 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
 
 	sb->ir = xhci_create_secondary_interrupter(xhci_to_hcd(sb->xhci),
 						   num_seg, imod_interval,
-						   intr_num);
+						   intr_num,
+						   PAGE_SIZE > TRB_SEGMENT_SIZE);
 	if (!sb->ir)
 		return -ENOMEM;
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index fc1b0a6b0429..64710df97697 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4106,6 +4106,7 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd,
  */
 static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
 {
+	struct xhci_ring *no_pool_rings[EP_CTX_PER_DEV] = {};
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 	struct xhci_virt_device *virt_dev;
 	struct xhci_slot_ctx *slot_ctx;
@@ -4138,9 +4139,21 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
 	xhci_disable_slot(xhci, udev->slot_id);
 
 	spin_lock_irqsave(&xhci->lock, flags);
+	/*
+	 * Rings not allocated from the segment pool are freed with
+	 * dma_free_coherent(), which can't be called with interrupts disabled.
+	 */
+	for (i = 0; i < EP_CTX_PER_DEV; i++) {
+		if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->no_pool_alloc) {
+			no_pool_rings[i] = virt_dev->eps[i].ring;
+			virt_dev->eps[i].ring = NULL;
+		}
+	}
 	xhci_free_virt_device(xhci, virt_dev, udev->slot_id);
 	spin_unlock_irqrestore(&xhci->lock, flags);
 
+	for (i = 0; i < EP_CTX_PER_DEV; i++)
+		xhci_ring_free(xhci, no_pool_rings[i]);
 }
 
 int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index ec4bfeb4887c..7e5152862562 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -711,6 +711,7 @@ struct xhci_virt_ep {
 	bool			use_extended_tbc;
 	/* set if this endpoint is controlled via sideband access*/
 	struct xhci_sideband	*sideband;
+	bool			no_pool_alloc;
 };
 
 enum xhci_overhead_type {
@@ -1382,6 +1383,7 @@ struct xhci_ring {
 	unsigned int		stream_id;
 	unsigned int		num_segs;
 	unsigned int		bounce_buf_len;
+	bool			no_pool_alloc;
 	enum xhci_ring_type	type;
 	u32			old_trb_comp_code;
 	struct radix_tree_root	*trb_address_map;
@@ -1870,7 +1872,8 @@ void xhci_free_port_bw_ctx(struct xhci_hcd *xhci,
 		struct xhci_container_ctx *ctx);
 struct xhci_interrupter *
 xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs,
-				  u32 imod_interval, unsigned int intr_num);
+				  u32 imod_interval, unsigned int intr_num,
+				  bool no_pool_alloc);
 void xhci_remove_secondary_interrupter(struct usb_hcd
 				       *hcd, struct xhci_interrupter *ir);
 void xhci_skip_sec_intr_events(struct xhci_hcd *xhci,

-- 
2.43.0

[PATCH v5 3/4] ALSA: usb-audio: qcom: tag sideband endpoints before ring allocation

[Wesley Cheng]

On kernels with a PAGE_SIZE larger than TRB_SEGMENT_SIZE,
xhci_endpoint_init() allocates the ring segments of endpoints that have
already been tagged via xhci_sideband_add_endpoint() with
dma_alloc_coherent() instead of from the shared xhci->segment_pool, so
the buffer reported to the ADSP over QMI is guaranteed to come from a
page of its own.

xhci_sideband_add_endpoint() must therefore run before the endpoint's
transfer ring is first allocated (i.e. before snd_usb_endpoint_prepare()
triggers xhci_endpoint_init()) for that allocation to apply. Move the
xhci_sideband_add_endpoint() calls out of uaudio_endpoint_setup() and
into enable_audio_stream(), before snd_usb_endpoint_prepare() is called
for the data and sync endpoints, and unwind them on the new error paths.

At that point in the setup sequence dev->ep_in[]/ep_out[] are not yet
populated, since the endpoint's altsetting has not been activated, so
usb_pipe_endpoint() cannot be used to find the usb_host_endpoint. Add
uaudio_find_host_endpoint(), which resolves it directly from the
interface's altsetting descriptor table instead.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
---
 sound/usb/qcom/qc_audio_offload.c | 86 +++++++++++++++++++++++++++++++++------
 1 file changed, 74 insertions(+), 12 deletions(-)

diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
index e4bfd43a2488..48ee55116004 100644
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -941,6 +941,40 @@ static void uaudio_dev_release(struct kref *kref)
 	wake_up(&dev->disconnect_wq);
 }
 
+/**
+ * uaudio_find_host_endpoint() - look up usb_host_endpoint for a snd_usb_endpoint
+ * @subs: usb substream owning the target snd_usb_endpoint
+ * @endpoint: sync or data snd_usb_endpoint to resolve
+ *
+ * usb_pipe_endpoint() resolves via dev->ep_in[]/ep_out[], which are only
+ * populated after usb_set_interface() activates the endpoint's altsetting.
+ * Look the endpoint up directly in that altsetting before preparation instead.
+ *
+ * Return: matching usb_host_endpoint, or NULL if not found.
+ */
+static struct usb_host_endpoint *
+uaudio_find_host_endpoint(struct snd_usb_substream *subs,
+			  struct snd_usb_endpoint *endpoint)
+{
+	struct usb_host_interface *alt;
+	struct usb_interface *iface;
+	int i;
+
+	iface = usb_ifnum_to_if(subs->dev, endpoint->iface);
+	if (!iface)
+		return NULL;
+
+	alt = usb_altnum_to_altsetting(iface, endpoint->altsetting);
+	if (!alt)
+		return NULL;
+
+	for (i = 0; i < alt->desc.bNumEndpoints; i++)
+		if (alt->endpoint[i].desc.bEndpointAddress == endpoint->ep_num)
+			return &alt->endpoint[i];
+
+	return NULL;
+}
+
 /**
  * enable_audio_stream() - enable usb snd endpoints
  * @subs: usb substream
@@ -958,8 +992,9 @@ static void uaudio_dev_release(struct kref *kref)
 static int enable_audio_stream(struct snd_usb_substream *subs,
 			       snd_pcm_format_t pcm_format,
 			       unsigned int channels, unsigned int cur_rate,
-			       int datainterval)
+			       int datainterval, unsigned int card_num)
 {
+	struct usb_host_endpoint *data_ep = NULL, *sync_ep = NULL;
 	struct snd_pcm_hw_params params;
 	struct snd_usb_audio *chip;
 	struct snd_interval *i;
@@ -997,15 +1032,45 @@ static int enable_audio_stream(struct snd_usb_substream *subs,
 			goto detach_ep;
 		}
 
+		data_ep = uaudio_find_host_endpoint(subs, subs->data_endpoint);
+		if (!data_ep) {
+			dev_err(&subs->dev->dev, "data ep # %d not found\n",
+				subs->data_endpoint->ep_num);
+			ret = -ENODEV;
+			goto detach_ep;
+		}
+
+		ret = xhci_sideband_add_endpoint(uadev[card_num].sb, data_ep);
+		if (ret < 0) {
+			dev_err(&subs->dev->dev,
+				"failed to add data ep to sideband: %d\n", ret);
+			goto detach_ep;
+		}
+
 		if (subs->sync_endpoint) {
+			sync_ep = uaudio_find_host_endpoint(subs, subs->sync_endpoint);
+			if (!sync_ep) {
+				dev_err(&subs->dev->dev, "sync ep # %d not found\n",
+					subs->sync_endpoint->ep_num);
+				ret = -ENODEV;
+				goto remove_data_ep;
+			}
+
+			ret = xhci_sideband_add_endpoint(uadev[card_num].sb, sync_ep);
+			if (ret < 0) {
+				dev_err(&subs->dev->dev,
+					"failed to add sync ep to sideband: %d\n", ret);
+				goto remove_data_ep;
+			}
+
 			ret = snd_usb_endpoint_prepare(chip, subs->sync_endpoint);
 			if (ret < 0)
-				goto detach_ep;
+				goto remove_sync_ep;
 		}
 
 		ret = snd_usb_endpoint_prepare(chip, subs->data_endpoint);
 		if (ret < 0)
-			goto detach_ep;
+			goto remove_sync_ep;
 
 		dev_dbg(uaudio_qdev->data->dev,
 			"selected %s iface:%d altsetting:%d datainterval:%dus\n",
@@ -1019,6 +1084,11 @@ static int enable_audio_stream(struct snd_usb_substream *subs,
 
 	return 0;
 
+remove_sync_ep:
+	if (sync_ep)
+		xhci_sideband_remove_endpoint(uadev[card_num].sb, sync_ep);
+remove_data_ep:
+	xhci_sideband_remove_endpoint(uadev[card_num].sb, data_ep);
 detach_ep:
 	snd_usb_hw_free(subs);
 
@@ -1140,14 +1210,6 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
 
 	memcpy(ep_desc, &ep->desc, sizeof(ep->desc));
 
-	ret = xhci_sideband_add_endpoint(uadev[card_num].sb, ep);
-	if (ret < 0) {
-		dev_err(&subs->dev->dev,
-			"failed to add data ep to sec intr: %d\n", ret);
-		ret = -ENODEV;
-		goto exit;
-	}
-
 	sgt = xhci_sideband_get_endpoint_buffer(uadev[card_num].sb, ep);
 	if (!sgt) {
 		dev_err(&subs->dev->dev,
@@ -1637,7 +1699,7 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle,
 		ret = enable_audio_stream(subs,
 					  map_pcm_format(req_msg->audio_format),
 					  req_msg->number_of_ch, req_msg->bit_rate,
-					  datainterval);
+					  datainterval, pcm_card_num);
 
 		if (!ret)
 			ret = prepare_qmi_response(subs, req_msg, &resp,

-- 
2.43.0

Re: [PATCH v5 3/4] ALSA: usb-audio: qcom: tag sideband endpoints before ring allocation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: usb-audio: qcom: sideband endpoint leak on prepare_qmi_response() error
- [Low] ALSA: usb-audio: qcom: missing @card_num kernel-doc in enable_audio_stream()
--

commit fa22ee4117b1447d736029e2816192b5570645e6
Author: Wesley Cheng <wesley.cheng@oss.qualcomm.com>

ALSA: usb-audio: qcom: tag sideband endpoints before ring allocation

This commit moves the xhci_sideband_add_endpoint() calls from
uaudio_endpoint_setup() into enable_audio_stream() so that endpoints are
tagged as sideband before snd_usb_endpoint_prepare() triggers transfer ring
allocation. It also introduces uaudio_find_host_endpoint() to look up the
usb_host_endpoint from the altsetting descriptor table before the interface
altsetting is activated.

> diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
> index e4bfd43a2488b..48ee551160044 100644
> --- a/sound/usb/qcom/qc_audio_offload.c
> +++ b/sound/usb/qcom/qc_audio_offload.c

[ ... ]

> @@ -958,8 +992,9 @@ static void uaudio_dev_release(struct kref *kref)
>  static int enable_audio_stream(struct snd_usb_substream *subs,
>  			       snd_pcm_format_t pcm_format,
>  			       unsigned int channels, unsigned int cur_rate,
> -			       int datainterval)
> +			       int datainterval, unsigned int card_num)
>  {

[Severity: Low]
This isn't a bug, but should @card_num be added to the kernel-doc comment
block above enable_audio_stream() in sound/usb/qcom/qc_audio_offload.c to
avoid a W=1 kernel-doc warning?

> +	struct usb_host_endpoint *data_ep = NULL, *sync_ep = NULL;
>  	struct snd_pcm_hw_params params;
>  	struct snd_usb_audio *chip;
>  	struct snd_interval *i;
> @@ -997,15 +1032,45 @@ static int enable_audio_stream(struct snd_usb_substream *subs,
>  			goto detach_ep;
>  		}
>  
> +		data_ep = uaudio_find_host_endpoint(subs, subs->data_endpoint);
> +		if (!data_ep) {
> +			dev_err(&subs->dev->dev, "data ep # %d not found\n",
> +				subs->data_endpoint->ep_num);
> +			ret = -ENODEV;
> +			goto detach_ep;
> +		}
> +
> +		ret = xhci_sideband_add_endpoint(uadev[card_num].sb, data_ep);
> +		if (ret < 0) {
> +			dev_err(&subs->dev->dev,
> +				"failed to add data ep to sideband: %d\n", ret);
> +			goto detach_ep;
> +		}
> +
>  		if (subs->sync_endpoint) {
> +			sync_ep = uaudio_find_host_endpoint(subs, subs->sync_endpoint);
> +			if (!sync_ep) {
> +				dev_err(&subs->dev->dev, "sync ep # %d not found\n",
> +					subs->sync_endpoint->ep_num);
> +				ret = -ENODEV;
> +				goto remove_data_ep;
> +			}
> +
> +			ret = xhci_sideband_add_endpoint(uadev[card_num].sb, sync_ep);
> +			if (ret < 0) {
> +				dev_err(&subs->dev->dev,
> +					"failed to add sync ep to sideband: %d\n", ret);
> +				goto remove_data_ep;
> +			}
> +
>  			ret = snd_usb_endpoint_prepare(chip, subs->sync_endpoint);
>  			if (ret < 0)
> -				goto detach_ep;
> +				goto remove_sync_ep;
>  		}
>  
>  		ret = snd_usb_endpoint_prepare(chip, subs->data_endpoint);
>  		if (ret < 0)
> -			goto detach_ep;
> +			goto remove_sync_ep;

[ ... ]

> @@ -1140,14 +1210,6 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
>  
>  	memcpy(ep_desc, &ep->desc, sizeof(ep->desc));
>  
> -	ret = xhci_sideband_add_endpoint(uadev[card_num].sb, ep);
> -	if (ret < 0) {
> -		dev_err(&subs->dev->dev,
> -			"failed to add data ep to sec intr: %d\n", ret);
> -		ret = -ENODEV;
> -		goto exit;
> -	}
> -
>  	sgt = xhci_sideband_get_endpoint_buffer(uadev[card_num].sb, ep);
>  	if (!sgt) {
>  		dev_err(&subs->dev->dev,
> @@ -1637,7 +1699,7 @@ static void handle_uaudio_stream_req(struct qmi_handle *handle,
>  		ret = enable_audio_stream(subs,
>  					  map_pcm_format(req_msg->audio_format),
>  					  req_msg->number_of_ch, req_msg->bit_rate,
> -					  datainterval);
> +					  datainterval, pcm_card_num);
>  
>  		if (!ret)
>  			ret = prepare_qmi_response(subs, req_msg, &resp,

[Severity: High]
As reported by Sashiko on v4 [1], now that enable_audio_stream() calls
xhci_sideband_add_endpoint() for both data_ep and sync_ep before
handle_uaudio_stream_req() calls prepare_qmi_response(), can failure paths
in prepare_qmi_response() and uaudio_endpoint_setup() leak the sideband
endpoint registrations in uadev[card_num].sb?

In sound/usb/qcom/qc_audio_offload.c, prepare_qmi_response() has early error
returns before uaudio_endpoint_setup() that do not call
xhci_sideband_remove_endpoint() for either endpoint, and if the first
uaudio_endpoint_setup() call for subs->data_endpoint fails, it returns
without removing subs->sync_endpoint:

sound/usb/qcom/qc_audio_offload.c:prepare_qmi_response() {
	if (!uadev[card_num].ctrl_intf) {
		dev_err(&subs->dev->dev, "audio ctrl intf info not cached\n");
		return -ENODEV;
	}

	ret = uaudio_populate_uac_desc(subs, resp);
	if (ret < 0)
		return ret;
	...
	data = snd_soc_usb_find_priv_data(uaudio_qdev->auxdev->dev.parent);
	if (!data) {
		dev_err(&subs->dev->dev, "No private data found\n");
		return -ENODEV;
	}
	...
	ret = uaudio_endpoint_setup(subs, subs->data_endpoint, card_num,
				    &resp->xhci_mem_info.tr_data,
				    &resp->std_as_data_ep_desc);
	if (ret < 0)
		return ret;
}

Additionally, in uaudio_endpoint_setup(), if usb_pipe_endpoint() returns
NULL, it jumps to exit instead of remove_ep, leaving that endpoint registered
in uadev[card_num].sb:

sound/usb/qcom/qc_audio_offload.c:uaudio_endpoint_setup() {
	ep = usb_pipe_endpoint(subs->dev, endpoint->pipe);
	if (!ep) {
		dev_err(uaudio_qdev->data->dev, "data ep # %d context is null\n",
			subs->data_endpoint->ep_num);
		goto exit;
	}
}

Since xhci_sideband_add_endpoint() rejects endpoints when sb->eps[ep_index]
or ep->sideband is already set, wouldn't any of these failures leave the
endpoints registered and cause subsequent stream enable requests to fail with
-EBUSY?

[1] https://lore.kernel.org/r/20260909-16k_offload_v1_b4-v4-4-f24a86617597@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-16k_offload_v1_b4-v5-0-67184126e91c@oss.qualcomm.com?part=3

[PATCH v5 4/4] ALSA: usb-audio: qcom: fix xfer ring IOMMU unmap on 16K+ page kernels

[Wesley Cheng]

TRB_SEGMENT_SIZE is hardcoded to 4096 bytes, but on kernels built with
a larger PAGE_SIZE (e.g. 16K or 64K page arches) the IOMMU still maps
and unmaps in units of PAGE_SIZE. A ring segment can therefore start at
a non-page-aligned offset within the page that gets mapped, which the
sideband sg table reports as the offset of its entry.

Add that offset onto the mapped iova before sending it to the ADSP over
QMI, so the reported address resolves to the start of the segment
rather than the start of its containing page, and report the true
TRB_SEGMENT_SIZE (the sg entry length) instead of PAGE_SIZE as the ring
size. The offset is taken from the sg entry rather than from the ring's
DMA address, which is an IOVA when xHCI is behind an IOMMU, and whose
low bits then need not match the offset within the page.

This broke the reverse direction: recovering the raw, page-aligned
iova for iommu_unmap() by masking off the low PAGE_SIZE bits of the
QMI-reported iova only works if that iova happens to already be
page-aligned before the offset was added, which is not guaranteed.
Have uaudio_endpoint_setup() return the iova it mapped instead, and use
it for both the cached data/sync_xfer_ring_va and the
drop_sync_ep/drop_data_ep unmap error paths.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
---
 sound/usb/qcom/qc_audio_offload.c | 45 +++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
index 48ee55116004..dd904d72badd 100644
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -1178,6 +1178,7 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
  * @card_num: uadev index
  * @mem_info: QMI response info
  * @ep_desc: QMI ep desc response field
+ * @ring_iova: returns the iova the transfer ring page is mapped at
  *
  * Initialize the USB endpoint being used for a particular USB
  * stream.  Will request XHCI sec intr to reserve the EP for
@@ -1189,7 +1190,8 @@ static phys_addr_t
 uaudio_endpoint_setup(struct snd_usb_substream *subs,
 		      struct snd_usb_endpoint *endpoint, int card_num,
 		      struct mem_info_v01 *mem_info,
-		      struct usb_endpoint_descriptor_v01 *ep_desc)
+		      struct usb_endpoint_descriptor_v01 *ep_desc,
+		      unsigned long *ring_iova)
 {
 	struct usb_host_endpoint *ep;
 	phys_addr_t tr_pa = 0;
@@ -1197,6 +1199,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
 	bool dma_coherent;
 	unsigned long iova;
 	struct page *pg;
+	unsigned int ring_size;
+	unsigned int ring_offset;
 	int ret = -ENODEV;
 
 	dma_coherent = dev_is_dma_coherent(subs->dev->bus->sysdev);
@@ -1221,6 +1225,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
 	pg = sg_page(sgt->sgl);
 	tr_pa = page_to_phys(pg);
 	mem_info->dma = sg_dma_address(sgt->sgl);
+	ring_size = sgt->sgl->length;
+	ring_offset = sgt->sgl->offset;
 	sg_free_table(sgt);
 	kfree(sgt);
 
@@ -1232,8 +1238,11 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
 		goto clear_pa;
 	}
 
-	mem_info->iova = PREPEND_SID_TO_IOVA(iova, uaudio_qdev->data->sid);
-	mem_info->size = PAGE_SIZE;
+	/* add intra-page offset so DSP IOVA resolves to the correct 4K slot */
+	mem_info->iova = PREPEND_SID_TO_IOVA(iova + ring_offset,
+					     uaudio_qdev->data->sid);
+	mem_info->size = ring_size;
+	*ring_iova = iova;
 
 	return 0;
 
@@ -1263,6 +1272,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
 	bool dma_coherent;
 	unsigned long iova;
 	struct page *pg;
+	unsigned int ring_size;
+	unsigned int ring_offset;
 	int ret;
 
 	dma_coherent = dev_is_dma_coherent(subs->dev->bus->sysdev);
@@ -1291,6 +1302,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
 	pg = sg_page(sgt->sgl);
 	er_pa = page_to_phys(pg);
 	mem_info->dma = sg_dma_address(sgt->sgl);
+	ring_size = sgt->sgl->length;
+	ring_offset = sgt->sgl->offset;
 	sg_free_table(sgt);
 	kfree(sgt);
 
@@ -1301,8 +1314,10 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
 		goto clear_pa;
 	}
 
-	mem_info->iova = PREPEND_SID_TO_IOVA(iova, uaudio_qdev->data->sid);
-	mem_info->size = PAGE_SIZE;
+	/* add intra-page offset so DSP IOVA resolves to the correct 4K slot */
+	mem_info->iova = PREPEND_SID_TO_IOVA(iova + ring_offset,
+					     uaudio_qdev->data->sid);
+	mem_info->size = ring_size;
 
 	return 0;
 
@@ -1438,6 +1453,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 				struct qmi_uaudio_stream_resp_msg_v01 *resp,
 				int info_idx)
 {
+	unsigned long data_ring_iova = 0, sync_ring_iova = 0;
 	struct q6usb_offload *data;
 	int pcm_dev_num;
 	int card_num;
@@ -1470,7 +1486,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 	resp->std_as_opr_intf_desc_valid = 1;
 	ret = uaudio_endpoint_setup(subs, subs->data_endpoint, card_num,
 				    &resp->xhci_mem_info.tr_data,
-				    &resp->std_as_data_ep_desc);
+				    &resp->std_as_data_ep_desc, &data_ring_iova);
 	if (ret < 0)
 		return ret;
 
@@ -1479,7 +1495,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 	if (subs->sync_endpoint) {
 		ret = uaudio_endpoint_setup(subs, subs->sync_endpoint, card_num,
 					    &resp->xhci_mem_info.tr_sync,
-					    &resp->std_as_sync_ep_desc);
+					    &resp->std_as_sync_ep_desc, &sync_ring_iova);
 		if (ret < 0)
 			goto drop_data_ep;
 
@@ -1540,11 +1556,9 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 	uadev[card_num].usb_core_id = resp->controller_num;
 
 	/* cache intf specific info to use it for unmap and free xfer buf */
-	uadev[card_num].info[info_idx].data_xfer_ring_va =
-					IOVA_MASK(resp->xhci_mem_info.tr_data.iova);
+	uadev[card_num].info[info_idx].data_xfer_ring_va = data_ring_iova;
 	uadev[card_num].info[info_idx].data_xfer_ring_size = PAGE_SIZE;
-	uadev[card_num].info[info_idx].sync_xfer_ring_va =
-					IOVA_MASK(resp->xhci_mem_info.tr_sync.iova);
+	uadev[card_num].info[info_idx].sync_xfer_ring_va = sync_ring_iova;
 	uadev[card_num].info[info_idx].sync_xfer_ring_size = PAGE_SIZE;
 	uadev[card_num].info[info_idx].xfer_buf_iova =
 					IOVA_MASK(resp->xhci_mem_info.xfer_buff.iova);
@@ -1578,15 +1592,14 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 	usb_offload_put(subs->dev);
 drop_sync_ep:
 	if (subs->sync_endpoint) {
-		uaudio_iommu_unmap(MEM_XFER_RING,
-				   IOVA_MASK(resp->xhci_mem_info.tr_sync.iova),
-				   PAGE_SIZE, PAGE_SIZE);
+		uaudio_iommu_unmap(MEM_XFER_RING, sync_ring_iova, PAGE_SIZE,
+				   PAGE_SIZE);
 		xhci_sideband_remove_endpoint(uadev[card_num].sb,
 			usb_pipe_endpoint(subs->dev, subs->sync_endpoint->pipe));
 	}
 drop_data_ep:
-	uaudio_iommu_unmap(MEM_XFER_RING, IOVA_MASK(resp->xhci_mem_info.tr_data.iova),
-			   PAGE_SIZE, PAGE_SIZE);
+	uaudio_iommu_unmap(MEM_XFER_RING, data_ring_iova, PAGE_SIZE,
+			   PAGE_SIZE);
 	xhci_sideband_remove_endpoint(uadev[card_num].sb,
 			usb_pipe_endpoint(subs->dev, subs->data_endpoint->pipe));
 

-- 
2.43.0

Re: [PATCH v5 4/4] ALSA: usb-audio: qcom: fix xfer ring IOMMU unmap on 16K+ page kernels

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: usb-audio: qcom: endpoint and stream leak on prepare_qmi_response() err
- [Low] ALSA: usb-audio: qcom: missing card_num kernel-doc in enable_audio_stream()
--

commit 7b2fd8f864f08004dfcddc27ca3dd72dfea2f7d5
Author: Wesley Cheng <wesley.cheng@oss.qualcomm.com>

ALSA: usb-audio: qcom: fix xfer ring IOMMU unmap on 16K+ page kernels

This commit updates uaudio_endpoint_setup() and uaudio_event_ring_setup() to
add the scatterlist intra-page offset to the QMI-reported IOVA and use the
scatterlist entry length as the ring size on kernels with page sizes larger
than 4K. It also returns the raw page-aligned ring IOVA from
uaudio_endpoint_setup() so prepare_qmi_response() can cache and unmap the
exact IOVA that was mapped.

> diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
> index 48ee551160044..dd904d72baddd 100644
> --- a/sound/usb/qcom/qc_audio_offload.c
> +++ b/sound/usb/qcom/qc_audio_offload.c
> @@ -1178,6 +1178,7 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
>   * @card_num: uadev index
>   * @mem_info: QMI response info
>   * @ep_desc: QMI ep desc response field
> + * @ring_iova: returns the iova the transfer ring page is mapped at

[Severity: Low]
This isn't a bug, but while updating the kernel-doc for
uaudio_endpoint_setup() here, earlier in the series commit fa22ee4117b1
("ALSA: usb-audio: qcom: tag sideband endpoints before ring allocation")
added the card_num parameter to enable_audio_stream() in
sound/usb/qcom/qc_audio_offload.c without documenting @card_num in its
kernel-doc comment:

sound/usb/qcom/qc_audio_offload.c:enable_audio_stream() {
/**
 * enable_audio_stream() - enable usb snd endpoints
 * @subs: usb substream
 * @pcm_format: pcm format requested
 * @channels: number of channels
 * @cur_rate: sample rate
 * @datainterval: interval
...
static int enable_audio_stream(struct snd_usb_substream *subs,
			       snd_pcm_format_t pcm_format,
			       unsigned int channels, unsigned int cur_rate,
			       int datainterval, unsigned int card_num)

Should @card_num be added to the enable_audio_stream() kernel-doc block to
avoid a W=1 kernel-doc warning?

[ ... ]

> @@ -1470,7 +1486,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
>  	resp->std_as_opr_intf_desc_valid = 1;
>  	ret = uaudio_endpoint_setup(subs, subs->data_endpoint, card_num,
>  				    &resp->xhci_mem_info.tr_data,
> -				    &resp->std_as_data_ep_desc);
> +				    &resp->std_as_data_ep_desc, &data_ring_iova);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1479,7 +1495,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
>  	if (subs->sync_endpoint) {
>  		ret = uaudio_endpoint_setup(subs, subs->sync_endpoint, card_num,
>  					    &resp->xhci_mem_info.tr_sync,
> -					    &resp->std_as_sync_ep_desc);
> +					    &resp->std_as_sync_ep_desc, &sync_ring_iova);
>  		if (ret < 0)
>  			goto drop_data_ep;
>  

[ ... ]

> @@ -1578,15 +1592,14 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
>  	usb_offload_put(subs->dev);
>  drop_sync_ep:
>  	if (subs->sync_endpoint) {
> -		uaudio_iommu_unmap(MEM_XFER_RING,
> -				   IOVA_MASK(resp->xhci_mem_info.tr_sync.iova),
> -				   PAGE_SIZE, PAGE_SIZE);
> +		uaudio_iommu_unmap(MEM_XFER_RING, sync_ring_iova, PAGE_SIZE,
> +				   PAGE_SIZE);
>  		xhci_sideband_remove_endpoint(uadev[card_num].sb,
>  			usb_pipe_endpoint(subs->dev, subs->sync_endpoint->pipe));
>  	}
>  drop_data_ep:
> -	uaudio_iommu_unmap(MEM_XFER_RING, IOVA_MASK(resp->xhci_mem_info.tr_data.iova),
> -			   PAGE_SIZE, PAGE_SIZE);
> +	uaudio_iommu_unmap(MEM_XFER_RING, data_ring_iova, PAGE_SIZE,
> +			   PAGE_SIZE);
>  	xhci_sideband_remove_endpoint(uadev[card_num].sb,
>  			usb_pipe_endpoint(subs->dev, subs->data_endpoint->pipe));

[Severity: High]
As reported by Sashiko on v4 [1], does a failure in prepare_qmi_response()
after enable_audio_stream() succeeds leak the xHCI sideband endpoints, the
prepared USB audio endpoints (and their ep->ring allocations), and the USB
device runtime PM reference?

In sound/usb/qcom/qc_audio_offload.c, handle_uaudio_stream_req() calls
enable_audio_stream() before prepare_qmi_response(). Following commit
fa22ee4117b1 ("ALSA: usb-audio: qcom: tag sideband endpoints before ring
allocation"), enable_audio_stream() calls snd_usb_autoresume(),
snd_usb_hw_params(), xhci_sideband_add_endpoint() for both data_ep and
sync_ep (when subs->sync_endpoint is present), and snd_usb_endpoint_prepare()
for both endpoints.

Inside prepare_qmi_response(), if an early check fails before
uaudio_endpoint_setup() (when !uadev[card_num].ctrl_intf,
uaudio_populate_uac_desc() fails, or !data), prepare_qmi_response() returns
directly without calling xhci_sideband_remove_endpoint() for either
subs->data_endpoint or subs->sync_endpoint:

sound/usb/qcom/qc_audio_offload.c:prepare_qmi_response() {
	if (!uadev[card_num].ctrl_intf) {
		dev_err(&subs->dev->dev, "audio ctrl intf info not cached\n");
		return -ENODEV;
	}

	ret = uaudio_populate_uac_desc(subs, resp);
	if (ret < 0)
		return ret;
	...
	data = snd_soc_usb_find_priv_data(uaudio_qdev->auxdev->dev.parent);
	if (!data) {
		dev_err(&subs->dev->dev, "No private data found\n");
		return -ENODEV;
	}
}

Likewise, if uaudio_endpoint_setup() for subs->data_endpoint fails,
prepare_qmi_response() returns ret directly without removing
subs->sync_endpoint from uadev[card_num].sb, and if usb_pipe_endpoint()
returns NULL in uaudio_endpoint_setup(), jumping to exit skips
xhci_sideband_remove_endpoint() for that endpoint as well.

In addition, in handle_uaudio_stream_req(), when prepare_qmi_response()
returns a negative error after enable_audio_stream() succeeded, the error
path only resets subs->opened = 0 without calling disable_audio_stream(subs)
to run snd_usb_hw_free(subs) and snd_usb_autosuspend(chip):

sound/usb/qcom/qc_audio_offload.c:handle_uaudio_stream_req() {
	if (req_msg->enable) {
		ret = enable_audio_stream(subs,
					  map_pcm_format(req_msg->audio_format),
					  req_msg->number_of_ch, req_msg->bit_rate,
					  datainterval, pcm_card_num);

		if (!ret)
			ret = prepare_qmi_response(subs, req_msg, &resp,
						   info_idx);
		if (ret < 0) {
			guard(mutex)(&chip->mutex);
			subs->opened = 0;
		}
	}
}

Because xhci_sideband_add_endpoint() rejects endpoints that already have
ep->sideband or ep->ring set, won't any subsequent offload attempt on the
device fail with -EBUSY until the USB device is disconnected?

[1] https://lore.kernel.org/r/20260909-16k_offload_v1_b4-v4-5-f24a86617597@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-16k_offload_v1_b4-v5-0-67184126e91c@oss.qualcomm.com?part=4