[PATCH 0/3] ALSA: qc_audio_offload: address space cleanups

[PATCH 0/3] ALSA: qc_audio_offload: address space cleanups

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

I ran into a build time warning and spent some time trying to
gently rewrite the new driver to avoid the warning and make it
behave more like other drivers.

I'm still a bit confused about what the driver actually does
and why a buffer has to be mapped into a two devices, but I hope
that either my patches clear this up enough, or if they are wrong
are helpful to have someone else sort it out properly.

The patches are currently queued for v6.16 in Greg's usb-next
tree, so my fixes look correct, I hope he can apply them there
before the merge window.

Arnd Bergmann (3):
  ALSA: qc_audio_offload: rename dma/iova/va/cpu/phys variables
  ALSA: qc_audio_offload: avoid leaking xfer_buf allocation
  ALSA: qc_audio_offload: try to reduce address space confusion

 sound/usb/qcom/qc_audio_offload.c  | 158 ++++++++++++++++-------------
 sound/usb/qcom/usb_audio_qmi_v01.c |   4 +-
 sound/usb/qcom/usb_audio_qmi_v01.h |   4 +-
 3 files changed, 89 insertions(+), 77 deletions(-)


Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Wesley Cheng <quic_wcheng@quicinc.com>
Cc: Dan Carpenter <dan.carpenter@linaro.org>
Cc: linux-sound@vger.kernel.org
Cc: linux-kernel@vger.kernel.org


-- 
2.39.5

[PATCH 1/3] ALSA: qc_audio_offload: rename dma/iova/va/cpu/phys variables

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

While trying to understand a bug in the audio offload code, I had
to spend extra time due to unfortunate nameing of local variables
and struct members.

Change these to more conventional names that reflect the actual
usage:

 - pointers to the CPU virtual addresses of a dma buffer get a
   _cpu suffix to disambiguate them for MMIO virtual addresses

 - MMIO virtual addresses that are mapped explicitly through
   the IOMMU get a _iova suffix consistently, rather than a
   mix of iova and va.

 - DMA addresses (dma_addr_t) that are in a device address
   space (linear or IOMMU) get a _dma suffix in place of the
   _pa suffix.

 - CPU physical (phys_addr_t) addresses get a _pa suffix.
   There is still a mixup with dma addresses here that I address
   in another patch.

No functional changes are intended here.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/usb/qcom/qc_audio_offload.c  | 136 ++++++++++++++---------------
 sound/usb/qcom/usb_audio_qmi_v01.c |   4 +-
 sound/usb/qcom/usb_audio_qmi_v01.h |   4 +-
 3 files changed, 72 insertions(+), 72 deletions(-)

diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
index 8b096f37ad4c..d2256a26eaaa 100644
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -78,10 +78,10 @@ struct intf_info {
 	size_t data_xfer_ring_size;
 	unsigned long sync_xfer_ring_va;
 	size_t sync_xfer_ring_size;
-	unsigned long xfer_buf_va;
+	unsigned long xfer_buf_iova;
 	size_t xfer_buf_size;
-	phys_addr_t xfer_buf_pa;
-	u8 *xfer_buf;
+	phys_addr_t xfer_buf_dma;
+	u8 *xfer_buf_cpu;
 
 	/* USB endpoint information */
 	unsigned int data_ep_pipe;
@@ -396,7 +396,7 @@ static unsigned long uaudio_get_iova(unsigned long *curr_iova,
 	struct iova_info *info, *new_info = NULL;
 	struct list_head *curr_head;
 	size_t tmp_size = size;
-	unsigned long va = 0;
+	unsigned long iova = 0;
 
 	if (size % PAGE_SIZE)
 		goto done;
@@ -411,7 +411,7 @@ static unsigned long uaudio_get_iova(unsigned long *curr_iova,
 		/* exact size iova_info */
 		if (!info->in_use && info->size == size) {
 			info->in_use = true;
-			va = info->start_iova;
+			iova = info->start_iova;
 			*curr_iova_size -= size;
 			goto done;
 		} else if (!info->in_use && tmp_size >= info->size) {
@@ -421,7 +421,7 @@ static unsigned long uaudio_get_iova(unsigned long *curr_iova,
 			if (tmp_size)
 				continue;
 
-			va = new_info->start_iova;
+			iova = new_info->start_iova;
 			for (curr_head = &new_info->list; curr_head !=
 			&info->list; curr_head = curr_head->next) {
 				new_info = list_entry(curr_head, struct
@@ -440,11 +440,11 @@ static unsigned long uaudio_get_iova(unsigned long *curr_iova,
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info) {
-		va = 0;
+		iova = 0;
 		goto done;
 	}
 
-	va = *curr_iova;
+	iova = *curr_iova;
 	info->start_iova = *curr_iova;
 	info->size = size;
 	info->in_use = true;
@@ -453,10 +453,10 @@ static unsigned long uaudio_get_iova(unsigned long *curr_iova,
 	list_add_tail(&info->list, head);
 
 done:
-	return va;
+	return iova;
 }
 
-static void uaudio_put_iova(unsigned long va, size_t size, struct list_head
+static void uaudio_put_iova(unsigned long iova, size_t size, struct list_head
 	*head, size_t *curr_iova_size)
 {
 	struct iova_info *info;
@@ -464,7 +464,7 @@ static void uaudio_put_iova(unsigned long va, size_t size, struct list_head
 	bool found = false;
 
 	list_for_each_entry(info, head, list) {
-		if (info->start_iova == va) {
+		if (info->start_iova == iova) {
 			if (!info->in_use)
 				return;
 
@@ -492,20 +492,20 @@ static void uaudio_put_iova(unsigned long va, size_t size, struct list_head
 /**
  * uaudio_iommu_unmap() - unmaps iommu memory for adsp
  * @mtype: ring type
- * @va: virtual address to unmap
+ * @iova: virtual address to unmap
  * @iova_size: region size
  * @mapped_iova_size: mapped region size
  *
  * Unmaps the memory region that was previously assigned to the adsp.
  *
  */
-static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long va,
+static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long iova,
 			       size_t iova_size, size_t mapped_iova_size)
 {
 	size_t umap_size;
 	bool unmap = true;
 
-	if (!va || !iova_size)
+	if (!iova || !iova_size)
 		return;
 
 	switch (mtype) {
@@ -517,11 +517,11 @@ static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long va,
 		break;
 
 	case MEM_XFER_RING:
-		uaudio_put_iova(va, iova_size, &uaudio_qdev->xfer_ring_list,
+		uaudio_put_iova(iova, iova_size, &uaudio_qdev->xfer_ring_list,
 				&uaudio_qdev->xfer_ring_iova_size);
 		break;
 	case MEM_XFER_BUF:
-		uaudio_put_iova(va, iova_size, &uaudio_qdev->xfer_buf_list,
+		uaudio_put_iova(iova, iova_size, &uaudio_qdev->xfer_buf_list,
 				&uaudio_qdev->xfer_buf_iova_size);
 		break;
 	default:
@@ -531,11 +531,11 @@ static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long va,
 	if (!unmap || !mapped_iova_size)
 		return;
 
-	umap_size = iommu_unmap(uaudio_qdev->data->domain, va, mapped_iova_size);
+	umap_size = iommu_unmap(uaudio_qdev->data->domain, iova, mapped_iova_size);
 	if (umap_size != mapped_iova_size)
 		dev_err(uaudio_qdev->data->dev,
 			"unmapped size %zu for iova 0x%08lx of mapped size %zu\n",
-			umap_size, va, mapped_iova_size);
+			umap_size, iova, mapped_iova_size);
 }
 
 /**
@@ -556,9 +556,9 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
 				      struct sg_table *sgt)
 {
 	struct scatterlist *sg;
-	unsigned long va = 0;
+	unsigned long iova = 0;
 	size_t total_len = 0;
-	unsigned long va_sg;
+	unsigned long iova_sg;
 	phys_addr_t pa_sg;
 	bool map = true;
 	size_t sg_len;
@@ -573,18 +573,18 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
 
 	switch (mtype) {
 	case MEM_EVENT_RING:
-		va = IOVA_BASE;
+		iova = IOVA_BASE;
 		/* er already mapped */
 		if (uaudio_qdev->er_mapped)
 			map = false;
 		break;
 	case MEM_XFER_RING:
-		va = uaudio_get_iova(&uaudio_qdev->curr_xfer_ring_iova,
+		iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_ring_iova,
 				     &uaudio_qdev->xfer_ring_iova_size,
 				     &uaudio_qdev->xfer_ring_list, size);
 		break;
 	case MEM_XFER_BUF:
-		va = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
+		iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
 				     &uaudio_qdev->xfer_buf_iova_size,
 				     &uaudio_qdev->xfer_buf_list, size);
 		break;
@@ -592,39 +592,39 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
 		dev_err(uaudio_qdev->data->dev, "unknown mem type %d\n", mtype);
 	}
 
-	if (!va || !map)
+	if (!iova || !map)
 		goto done;
 
 	if (!sgt)
 		goto skip_sgt_map;
 
-	va_sg = va;
+	iova_sg = iova;
 	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
 		sg_len = PAGE_ALIGN(sg->offset + sg->length);
 		pa_sg = page_to_phys(sg_page(sg));
-		ret = iommu_map(uaudio_qdev->data->domain, va_sg, pa_sg, sg_len,
+		ret = iommu_map(uaudio_qdev->data->domain, iova_sg, pa_sg, sg_len,
 				prot, GFP_KERNEL);
 		if (ret) {
-			uaudio_iommu_unmap(MEM_XFER_BUF, va, size, total_len);
-			va = 0;
+			uaudio_iommu_unmap(MEM_XFER_BUF, iova, size, total_len);
+			iova = 0;
 			goto done;
 		}
 
-		va_sg += sg_len;
+		iova_sg += sg_len;
 		total_len += sg_len;
 	}
 
 	if (size != total_len) {
-		uaudio_iommu_unmap(MEM_XFER_BUF, va, size, total_len);
-		va = 0;
+		uaudio_iommu_unmap(MEM_XFER_BUF, iova, size, total_len);
+		iova = 0;
 	}
-	return va;
+	return iova;
 
 skip_sgt_map:
-	iommu_map(uaudio_qdev->data->domain, va, pa, size, prot, GFP_KERNEL);
+	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
 
 done:
-	return va;
+	return iova;
 }
 
 /* looks up alias, if any, for controller DT node and returns the index */
@@ -658,15 +658,15 @@ static void uaudio_dev_intf_cleanup(struct usb_device *udev, struct intf_info *i
 	info->sync_xfer_ring_va = 0;
 	info->sync_xfer_ring_size = 0;
 
-	uaudio_iommu_unmap(MEM_XFER_BUF, info->xfer_buf_va, info->xfer_buf_size,
+	uaudio_iommu_unmap(MEM_XFER_BUF, info->xfer_buf_iova, info->xfer_buf_size,
 			   info->xfer_buf_size);
-	info->xfer_buf_va = 0;
+	info->xfer_buf_iova = 0;
 
-	usb_free_coherent(udev, info->xfer_buf_size, info->xfer_buf,
-			  info->xfer_buf_pa);
+	usb_free_coherent(udev, info->xfer_buf_size, info->xfer_buf_cpu,
+			  info->xfer_buf_dma);
 	info->xfer_buf_size = 0;
-	info->xfer_buf = NULL;
-	info->xfer_buf_pa = 0;
+	info->xfer_buf_cpu = NULL;
+	info->xfer_buf_dma = 0;
 
 	info->in_use = false;
 }
@@ -1021,7 +1021,7 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 	phys_addr_t xfer_buf_pa;
 	u32 len = xfer_buf_len;
 	bool dma_coherent;
-	unsigned long va;
+	unsigned long iova;
 	u32 remainder;
 	u32 mult;
 	int ret;
@@ -1050,16 +1050,16 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 
 	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
 			xfer_buf_pa, len);
-	va = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent, xfer_buf_pa, len,
+	iova = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent, xfer_buf_pa, len,
 			      &xfer_buf_sgt);
-	if (!va) {
+	if (!iova) {
 		ret = -ENOMEM;
 		goto unmap_sync;
 	}
 
-	mem_info->pa = xfer_buf_pa;
+	mem_info->dma = xfer_buf_pa;
 	mem_info->size = len;
-	mem_info->va = PREPEND_SID_TO_IOVA(va, uaudio_qdev->data->sid);
+	mem_info->iova = PREPEND_SID_TO_IOVA(iova, uaudio_qdev->data->sid);
 	sg_free_table(&xfer_buf_sgt);
 
 	return 0;
@@ -1094,7 +1094,7 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
 	phys_addr_t tr_pa = 0;
 	struct sg_table *sgt;
 	bool dma_coherent;
-	unsigned long va;
+	unsigned long iova;
 	struct page *pg;
 	int ret = -ENODEV;
 
@@ -1127,24 +1127,24 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
 
 	pg = sg_page(sgt->sgl);
 	tr_pa = page_to_phys(pg);
-	mem_info->pa = sg_dma_address(sgt->sgl);
+	mem_info->dma = sg_dma_address(sgt->sgl);
 	sg_free_table(sgt);
 
 	/* data transfer ring */
-	va = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_pa,
+	iova = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_pa,
 			      PAGE_SIZE, NULL);
-	if (!va) {
+	if (!iova) {
 		ret = -ENOMEM;
 		goto clear_pa;
 	}
 
-	mem_info->va = PREPEND_SID_TO_IOVA(va, uaudio_qdev->data->sid);
+	mem_info->iova = PREPEND_SID_TO_IOVA(iova, uaudio_qdev->data->sid);
 	mem_info->size = PAGE_SIZE;
 
 	return 0;
 
 clear_pa:
-	mem_info->pa = 0;
+	mem_info->dma = 0;
 remove_ep:
 	xhci_sideband_remove_endpoint(uadev[card_num].sb, ep);
 exit:
@@ -1167,7 +1167,7 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
 	struct sg_table *sgt;
 	phys_addr_t er_pa;
 	bool dma_coherent;
-	unsigned long va;
+	unsigned long iova;
 	struct page *pg;
 	int ret;
 
@@ -1192,23 +1192,23 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
 
 	pg = sg_page(sgt->sgl);
 	er_pa = page_to_phys(pg);
-	mem_info->pa = sg_dma_address(sgt->sgl);
+	mem_info->dma = sg_dma_address(sgt->sgl);
 	sg_free_table(sgt);
 
-	va = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, er_pa,
+	iova = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, er_pa,
 			      PAGE_SIZE, NULL);
-	if (!va) {
+	if (!iova) {
 		ret = -ENOMEM;
 		goto clear_pa;
 	}
 
-	mem_info->va = PREPEND_SID_TO_IOVA(va, uaudio_qdev->data->sid);
+	mem_info->iova = PREPEND_SID_TO_IOVA(iova, uaudio_qdev->data->sid);
 	mem_info->size = PAGE_SIZE;
 
 	return 0;
 
 clear_pa:
-	mem_info->pa = 0;
+	mem_info->dma = 0;
 remove_interrupter:
 	xhci_sideband_remove_interrupter(uadev[card_num].sb);
 exit:
@@ -1340,7 +1340,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 	struct q6usb_offload *data;
 	int pcm_dev_num;
 	int card_num;
-	u8 *xfer_buf = NULL;
+	u8 *xfer_buf_cpu = NULL;
 	int ret;
 
 	pcm_dev_num = (req_msg->usb_token & QMI_STREAM_REQ_DEV_NUM_MASK) >> 8;
@@ -1409,7 +1409,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 
 	resp->speed_info_valid = 1;
 
-	ret = uaudio_transfer_buffer_setup(subs, xfer_buf, req_msg->xfer_buff_size,
+	ret = uaudio_transfer_buffer_setup(subs, xfer_buf_cpu, req_msg->xfer_buff_size,
 					   &resp->xhci_mem_info.xfer_buff);
 	if (ret < 0) {
 		ret = -ENOMEM;
@@ -1440,15 +1440,15 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 
 	/* 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.va);
+					IOVA_MASK(resp->xhci_mem_info.tr_data.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.va);
+					IOVA_MASK(resp->xhci_mem_info.tr_sync.iova);
 	uadev[card_num].info[info_idx].sync_xfer_ring_size = PAGE_SIZE;
-	uadev[card_num].info[info_idx].xfer_buf_va =
-					IOVA_MASK(resp->xhci_mem_info.xfer_buff.va);
-	uadev[card_num].info[info_idx].xfer_buf_pa =
-					resp->xhci_mem_info.xfer_buff.pa;
+	uadev[card_num].info[info_idx].xfer_buf_iova =
+					IOVA_MASK(resp->xhci_mem_info.xfer_buff.iova);
+	uadev[card_num].info[info_idx].xfer_buf_dma =
+					resp->xhci_mem_info.xfer_buff.dma;
 	uadev[card_num].info[info_idx].xfer_buf_size =
 					resp->xhci_mem_info.xfer_buff.size;
 	uadev[card_num].info[info_idx].data_ep_pipe = subs->data_endpoint ?
@@ -1459,7 +1459,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 						subs->data_endpoint->ep_num : 0;
 	uadev[card_num].info[info_idx].sync_ep_idx = subs->sync_endpoint ?
 						subs->sync_endpoint->ep_num : 0;
-	uadev[card_num].info[info_idx].xfer_buf = xfer_buf;
+	uadev[card_num].info[info_idx].xfer_buf_cpu = xfer_buf_cpu;
 	uadev[card_num].info[info_idx].pcm_card_num = card_num;
 	uadev[card_num].info[info_idx].pcm_dev_num = pcm_dev_num;
 	uadev[card_num].info[info_idx].direction = subs->direction;
@@ -1477,13 +1477,13 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 drop_sync_ep:
 	if (subs->sync_endpoint) {
 		uaudio_iommu_unmap(MEM_XFER_RING,
-				   IOVA_MASK(resp->xhci_mem_info.tr_sync.va),
+				   IOVA_MASK(resp->xhci_mem_info.tr_sync.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.va),
+	uaudio_iommu_unmap(MEM_XFER_RING, IOVA_MASK(resp->xhci_mem_info.tr_data.iova),
 			   PAGE_SIZE, PAGE_SIZE);
 	xhci_sideband_remove_endpoint(uadev[card_num].sb,
 			usb_pipe_endpoint(subs->dev, subs->data_endpoint->pipe));
diff --git a/sound/usb/qcom/usb_audio_qmi_v01.c b/sound/usb/qcom/usb_audio_qmi_v01.c
index 151ae7b591de..502857612277 100644
--- a/sound/usb/qcom/usb_audio_qmi_v01.c
+++ b/sound/usb/qcom/usb_audio_qmi_v01.c
@@ -14,7 +14,7 @@ static const struct qmi_elem_info mem_info_v01_ei[] = {
 		.elem_size	= sizeof(u64),
 		.array_type	= NO_ARRAY,
 		.tlv_type	= 0,
-		.offset		= offsetof(struct mem_info_v01, va),
+		.offset		= offsetof(struct mem_info_v01, iova),
 	},
 	{
 		.data_type	= QMI_UNSIGNED_8_BYTE,
@@ -22,7 +22,7 @@ static const struct qmi_elem_info mem_info_v01_ei[] = {
 		.elem_size	= sizeof(u64),
 		.array_type	= NO_ARRAY,
 		.tlv_type	= 0,
-		.offset		= offsetof(struct mem_info_v01, pa),
+		.offset		= offsetof(struct mem_info_v01, dma),
 	},
 	{
 		.data_type	= QMI_UNSIGNED_4_BYTE,
diff --git a/sound/usb/qcom/usb_audio_qmi_v01.h b/sound/usb/qcom/usb_audio_qmi_v01.h
index f2563537ed40..a1298d75d9f8 100644
--- a/sound/usb/qcom/usb_audio_qmi_v01.h
+++ b/sound/usb/qcom/usb_audio_qmi_v01.h
@@ -14,8 +14,8 @@
 #define QMI_UAUDIO_STREAM_IND_V01 0x0001
 
 struct mem_info_v01 {
-	u64 va;
-	u64 pa;
+	u64 iova;	/* mapped into sysdev */
+	u64 dma;	/* mapped into usb host */
 	u32 size;
 };
 
-- 
2.39.5

[PATCH 2/3] ALSA: qc_audio_offload: avoid leaking xfer_buf allocation

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The info->xfer_buf_cpu member is set to a NULL value because the
allocation happens in a different function and is only assigned
to the function argument but never passed back.

Pass it by reference instead to have a handle that can actually be
freed by the final usb_free_coherent() call.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/usb/qcom/qc_audio_offload.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
index d2256a26eaaa..c4dde2fa5a1f 100644
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -1014,10 +1014,11 @@ static int enable_audio_stream(struct snd_usb_substream *subs,
  *
  */
 static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
-					u8 *xfer_buf, u32 xfer_buf_len,
+					void **xfer_buf_cpu, u32 xfer_buf_len,
 					struct mem_info_v01 *mem_info)
 {
 	struct sg_table xfer_buf_sgt;
+	void *xfer_buf;
 	phys_addr_t xfer_buf_pa;
 	u32 len = xfer_buf_len;
 	bool dma_coherent;
@@ -1060,6 +1061,7 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 	mem_info->dma = xfer_buf_pa;
 	mem_info->size = len;
 	mem_info->iova = PREPEND_SID_TO_IOVA(iova, uaudio_qdev->data->sid);
+	*xfer_buf_cpu = xfer_buf;
 	sg_free_table(&xfer_buf_sgt);
 
 	return 0;
@@ -1340,7 +1342,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 	struct q6usb_offload *data;
 	int pcm_dev_num;
 	int card_num;
-	u8 *xfer_buf_cpu = NULL;
+	void *xfer_buf_cpu;
 	int ret;
 
 	pcm_dev_num = (req_msg->usb_token & QMI_STREAM_REQ_DEV_NUM_MASK) >> 8;
@@ -1409,7 +1411,7 @@ static int prepare_qmi_response(struct snd_usb_substream *subs,
 
 	resp->speed_info_valid = 1;
 
-	ret = uaudio_transfer_buffer_setup(subs, xfer_buf_cpu, req_msg->xfer_buff_size,
+	ret = uaudio_transfer_buffer_setup(subs, &xfer_buf_cpu, req_msg->xfer_buff_size,
 					   &resp->xhci_mem_info.xfer_buff);
 	if (ret < 0) {
 		ret = -ENOMEM;
-- 
2.39.5

[PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

uaudio_transfer_buffer_setup() allocates a buffer for the subs->dev
device, and the returned address for the buffer is a CPU local virtual
address that may or may not be in the linear mapping, as well as a DMA
address token that is accessible by the USB device, and this in turn
may or may not correspond to the physical address.

The use in the driver however assumes that these addresses are the
linear map and the CPU physical address, respectively. Both are
nonportable here, but in the end only the virtual address gets
used by converting it to a physical address that gets mapped into
a second iommu.

Make this more explicit by pulling the conversion out first
and warning if it is not part of the linear map, and using the
actual physical address to map into the iommu in place of the
dma address that may already be iommu-mapped into the usb host.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/usb/qcom/qc_audio_offload.c | 32 ++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
index c4dde2fa5a1f..46379387c9a5 100644
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -78,9 +78,9 @@ struct intf_info {
 	size_t data_xfer_ring_size;
 	unsigned long sync_xfer_ring_va;
 	size_t sync_xfer_ring_size;
-	unsigned long xfer_buf_iova;
+	dma_addr_t xfer_buf_iova;
 	size_t xfer_buf_size;
-	phys_addr_t xfer_buf_dma;
+	dma_addr_t xfer_buf_dma;
 	u8 *xfer_buf_cpu;
 
 	/* USB endpoint information */
@@ -1018,11 +1018,12 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 					struct mem_info_v01 *mem_info)
 {
 	struct sg_table xfer_buf_sgt;
+	dma_addr_t xfer_buf_dma;
 	void *xfer_buf;
 	phys_addr_t xfer_buf_pa;
 	u32 len = xfer_buf_len;
 	bool dma_coherent;
-	unsigned long iova;
+	dma_addr_t xfer_buf_dma_sysdev;
 	u32 remainder;
 	u32 mult;
 	int ret;
@@ -1045,29 +1046,38 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 		len = MAX_XFER_BUFF_LEN;
 	}
 
-	xfer_buf = usb_alloc_coherent(subs->dev, len, GFP_KERNEL, &xfer_buf_pa);
+	/* get buffer mapped into subs->dev */
+	xfer_buf = usb_alloc_coherent(subs->dev, len, GFP_KERNEL, &xfer_buf_dma);
 	if (!xfer_buf)
 		return -ENOMEM;
 
+	/* Remapping is not possible if xfer_buf is outside of linear map */
+	xfer_buf_pa = virt_to_phys(xfer_buf);
+	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
+		ret = -ENXIO;
+		goto unmap_sync;
+	}
 	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
-			xfer_buf_pa, len);
-	iova = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent, xfer_buf_pa, len,
-			      &xfer_buf_sgt);
-	if (!iova) {
+			xfer_buf_dma, len);
+
+	/* map the physical buffer into sysdev as well */
+	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
+					       xfer_buf_pa, len, &xfer_buf_sgt);
+	if (!xfer_buf_dma_sysdev) {
 		ret = -ENOMEM;
 		goto unmap_sync;
 	}
 
-	mem_info->dma = xfer_buf_pa;
+	mem_info->dma = xfer_buf_dma;
 	mem_info->size = len;
-	mem_info->iova = PREPEND_SID_TO_IOVA(iova, uaudio_qdev->data->sid);
+	mem_info->iova = PREPEND_SID_TO_IOVA(xfer_buf_dma_sysdev, uaudio_qdev->data->sid);
 	*xfer_buf_cpu = xfer_buf;
 	sg_free_table(&xfer_buf_sgt);
 
 	return 0;
 
 unmap_sync:
-	usb_free_coherent(subs->dev, len, xfer_buf, xfer_buf_pa);
+	usb_free_coherent(subs->dev, len, xfer_buf, xfer_buf_dma);
 
 	return ret;
 }
-- 
2.39.5

Re: [PATCH 0/3] ALSA: qc_audio_offload: address space cleanups

[Takashi Iwai]

On Tue, 13 May 2025 14:34:39 +0200,
Arnd Bergmann wrote:
> 
> From: Arnd Bergmann <arnd@arndb.de>
> 
> I ran into a build time warning and spent some time trying to
> gently rewrite the new driver to avoid the warning and make it
> behave more like other drivers.
> 
> I'm still a bit confused about what the driver actually does
> and why a buffer has to be mapped into a two devices, but I hope
> that either my patches clear this up enough, or if they are wrong
> are helpful to have someone else sort it out properly.
> 
> The patches are currently queued for v6.16 in Greg's usb-next
> tree, so my fixes look correct, I hope he can apply them there
> before the merge window.
> 
> Arnd Bergmann (3):
>   ALSA: qc_audio_offload: rename dma/iova/va/cpu/phys variables
>   ALSA: qc_audio_offload: avoid leaking xfer_buf allocation
>   ALSA: qc_audio_offload: try to reduce address space confusion

JFYI, the qcom offload stuff is currently only on Greg's USB tree, not
on Mark's or my sound git tree.


Takashi

> 
>  sound/usb/qcom/qc_audio_offload.c  | 158 ++++++++++++++++-------------
>  sound/usb/qcom/usb_audio_qmi_v01.c |   4 +-
>  sound/usb/qcom/usb_audio_qmi_v01.h |   4 +-
>  3 files changed, 89 insertions(+), 77 deletions(-)
> 
> 
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Wesley Cheng <quic_wcheng@quicinc.com>
> Cc: Dan Carpenter <dan.carpenter@linaro.org>
> Cc: linux-sound@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> 
> -- 
> 2.39.5
> 
> 

Re: [PATCH 0/3] ALSA: qc_audio_offload: address space cleanups

[Greg Kroah-Hartman]

On Wed, May 14, 2025 at 11:01:02AM +0200, Takashi Iwai wrote:
> On Tue, 13 May 2025 14:34:39 +0200,
> Arnd Bergmann wrote:
> > 
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > I ran into a build time warning and spent some time trying to
> > gently rewrite the new driver to avoid the warning and make it
> > behave more like other drivers.
> > 
> > I'm still a bit confused about what the driver actually does
> > and why a buffer has to be mapped into a two devices, but I hope
> > that either my patches clear this up enough, or if they are wrong
> > are helpful to have someone else sort it out properly.
> > 
> > The patches are currently queued for v6.16 in Greg's usb-next
> > tree, so my fixes look correct, I hope he can apply them there
> > before the merge window.
> > 
> > Arnd Bergmann (3):
> >   ALSA: qc_audio_offload: rename dma/iova/va/cpu/phys variables
> >   ALSA: qc_audio_offload: avoid leaking xfer_buf allocation
> >   ALSA: qc_audio_offload: try to reduce address space confusion
> 
> JFYI, the qcom offload stuff is currently only on Greg's USB tree, not
> on Mark's or my sound git tree.

Ok, I'll take this through the usb tree now, thanks.

greg k-h

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Luca Weiss]

Hi Arnd,

On Tue May 13, 2025 at 2:34 PM CEST, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> uaudio_transfer_buffer_setup() allocates a buffer for the subs->dev
> device, and the returned address for the buffer is a CPU local virtual
> address that may or may not be in the linear mapping, as well as a DMA
> address token that is accessible by the USB device, and this in turn
> may or may not correspond to the physical address.
>
> The use in the driver however assumes that these addresses are the
> linear map and the CPU physical address, respectively. Both are
> nonportable here, but in the end only the virtual address gets
> used by converting it to a physical address that gets mapped into
> a second iommu.
>
> Make this more explicit by pulling the conversion out first
> and warning if it is not part of the linear map, and using the
> actual physical address to map into the iommu in place of the
> dma address that may already be iommu-mapped into the usb host.

This patch is breaking USB audio offloading on Qualcomm devices on 6.16,
as tested on sm6350 and sc7280-based smartphones.

[  420.463176] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: AFE Port already open
[  420.472676] ------------[ cut here ]------------
[  420.472691] WARNING: CPU: 2 PID: 175 at sound/usb/qcom/qc_audio_offload.c:1056 handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
[  420.472726] Modules linked in: rfcomm zram zsmalloc zstd_compress algif_hash algif_skcipher bnep nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nf_tables nfnetlink ipv6 fuse uhid uinput snd_usb_audio_qmi q6asm_dai q6routing q6afe_dai q6usb q6afe_clocks q6adm q6asm snd_q6dsp_common q6afe q6core apr pdr_interface snd_soc_sm8250 snd_soc_qcom
_common snd_soc_qcom_offload_utils snd_soc_qcom_sdw soundwire_bus soc_usb snd_soc_core snd_compress snd_usb_audio ath10k_snoc ath10k_core snd_hwdep snd_usbmidi_lib ath fastrpc snd_pcm mac80211 hci_uart qrtr_smd snd_timer btqca qcom_pd_mapper snd_rawmidi bluetooth libarc4 qcom_pdr_msg cfg80211 snd soundcore ecdh_generic ecc rfkill qrtr qcom_stats qcom_q6v5_pas ipa qcom_pil_info qcom_q6v5 qcom_common
[  420.473018] CPU: 2 UID: 0 PID: 175 Comm: kworker/u32:9 Tainted: G        W           6.16.0 #1-postmarketos-qcom-sm6350 NONE
[  420.473033] Tainted: [W]=WARN
[  420.473038] Hardware name: Fairphone 4 (DT)
[  420.473045] Workqueue: qmi_msg_handler qmi_data_ready_work
[  420.473065] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  420.473075] pc : handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
[  420.473091] lr : handle_uaudio_stream_req+0xc84/0x13f8 [snd_usb_audio_qmi]
[  420.473104] sp : ffff800082f939a0
[  420.473110] x29: ffff800082f93b10 x28: ffff0000cfb796b8 x27: 0000000000008000
[  420.473128] x26: ffff0000842afc80 x25: ffffa8e75a23b0e0 x24: 0000000000008000
[  420.473145] x23: ffffa8e75a23bcf0 x22: ffff800082f93bd0 x21: 0000000000000000
[  420.473161] x20: ffff800082f93c98 x19: ffff0000939bb740 x18: ffffa8e77925a4d0
[  420.473178] x17: ffffffffffffffff x16: ffffa8e777ef9728 x15: ffffa8e77925a000
[  420.473194] x14: 0000000000000000 x13: 0000000000000dc0 x12: ffff800080000000
[  420.473211] x11: 0000000000000cc0 x10: 0000000000000001 x9 : ffffa8e77944b880
[  420.473227] x8 : ffffd719b5f4d000 x7 : ffff00009033da18 x6 : 0000000000000000
[  420.473244] x5 : 0000000000000000 x4 : ffff800082f93938 x3 : 0000000000000000
[  420.473260] x2 : 0000000000000000 x1 : ffff0000857790c0 x0 : 0000000000000000
[  420.473277] Call trace:
[  420.473283]  handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi] (P)
[  420.473300]  qmi_invoke_handler+0xbc/0x108
[  420.473314]  qmi_handle_message+0x90/0x1a8
[  420.473326]  qmi_data_ready_work+0x210/0x390
[  420.473339]  process_one_work+0x150/0x3a0
[  420.473351]  worker_thread+0x288/0x480
[  420.473362]  kthread+0x118/0x1e0
[  420.473375]  ret_from_fork+0x10/0x20
[  420.473390] ---[ end trace 0000000000000000 ]---
[  420.479244] qcom-q6afe aprsvc:service:4:4: cmd = 0x100e5 returned error = 0x1
[  420.479540] qcom-q6afe aprsvc:service:4:4: DSP returned error[1]
[  420.479558] qcom-q6afe aprsvc:service:4:4: AFE enable for port 0x7000 failed -22
[  420.479572] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: fail to start AFE port 88
[  420.479583] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: ASoC error (-22): at snd_soc_dai_prepare() on USB_RX

Reverting this patch makes it work as expected on 6.16.0.

Let me know if I can be of any help to resolve this.

Regards
Luca

>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  sound/usb/qcom/qc_audio_offload.c | 32 ++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
> index c4dde2fa5a1f..46379387c9a5 100644
> --- a/sound/usb/qcom/qc_audio_offload.c
> +++ b/sound/usb/qcom/qc_audio_offload.c
> @@ -78,9 +78,9 @@ struct intf_info {
>  	size_t data_xfer_ring_size;
>  	unsigned long sync_xfer_ring_va;
>  	size_t sync_xfer_ring_size;
> -	unsigned long xfer_buf_iova;
> +	dma_addr_t xfer_buf_iova;
>  	size_t xfer_buf_size;
> -	phys_addr_t xfer_buf_dma;
> +	dma_addr_t xfer_buf_dma;
>  	u8 *xfer_buf_cpu;
>  
>  	/* USB endpoint information */
> @@ -1018,11 +1018,12 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
>  					struct mem_info_v01 *mem_info)
>  {
>  	struct sg_table xfer_buf_sgt;
> +	dma_addr_t xfer_buf_dma;
>  	void *xfer_buf;
>  	phys_addr_t xfer_buf_pa;
>  	u32 len = xfer_buf_len;
>  	bool dma_coherent;
> -	unsigned long iova;
> +	dma_addr_t xfer_buf_dma_sysdev;
>  	u32 remainder;
>  	u32 mult;
>  	int ret;
> @@ -1045,29 +1046,38 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
>  		len = MAX_XFER_BUFF_LEN;
>  	}
>  
> -	xfer_buf = usb_alloc_coherent(subs->dev, len, GFP_KERNEL, &xfer_buf_pa);
> +	/* get buffer mapped into subs->dev */
> +	xfer_buf = usb_alloc_coherent(subs->dev, len, GFP_KERNEL, &xfer_buf_dma);
>  	if (!xfer_buf)
>  		return -ENOMEM;
>  
> +	/* Remapping is not possible if xfer_buf is outside of linear map */
> +	xfer_buf_pa = virt_to_phys(xfer_buf);
> +	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> +		ret = -ENXIO;
> +		goto unmap_sync;
> +	}
>  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
> -			xfer_buf_pa, len);
> -	iova = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent, xfer_buf_pa, len,
> -			      &xfer_buf_sgt);
> -	if (!iova) {
> +			xfer_buf_dma, len);
> +
> +	/* map the physical buffer into sysdev as well */
> +	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> +					       xfer_buf_pa, len, &xfer_buf_sgt);
> +	if (!xfer_buf_dma_sysdev) {
>  		ret = -ENOMEM;
>  		goto unmap_sync;
>  	}
>  
> -	mem_info->dma = xfer_buf_pa;
> +	mem_info->dma = xfer_buf_dma;
>  	mem_info->size = len;
> -	mem_info->iova = PREPEND_SID_TO_IOVA(iova, uaudio_qdev->data->sid);
> +	mem_info->iova = PREPEND_SID_TO_IOVA(xfer_buf_dma_sysdev, uaudio_qdev->data->sid);
>  	*xfer_buf_cpu = xfer_buf;
>  	sg_free_table(&xfer_buf_sgt);
>  
>  	return 0;
>  
>  unmap_sync:
> -	usb_free_coherent(subs->dev, len, xfer_buf, xfer_buf_pa);
> +	usb_free_coherent(subs->dev, len, xfer_buf, xfer_buf_dma);
>  
>  	return ret;
>  }

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Takashi Iwai]

On Fri, 01 Aug 2025 13:31:42 +0200,
Luca Weiss wrote:
> 
> Hi Arnd,
> 
> On Tue May 13, 2025 at 2:34 PM CEST, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > uaudio_transfer_buffer_setup() allocates a buffer for the subs->dev
> > device, and the returned address for the buffer is a CPU local virtual
> > address that may or may not be in the linear mapping, as well as a DMA
> > address token that is accessible by the USB device, and this in turn
> > may or may not correspond to the physical address.
> >
> > The use in the driver however assumes that these addresses are the
> > linear map and the CPU physical address, respectively. Both are
> > nonportable here, but in the end only the virtual address gets
> > used by converting it to a physical address that gets mapped into
> > a second iommu.
> >
> > Make this more explicit by pulling the conversion out first
> > and warning if it is not part of the linear map, and using the
> > actual physical address to map into the iommu in place of the
> > dma address that may already be iommu-mapped into the usb host.
> 
> This patch is breaking USB audio offloading on Qualcomm devices on 6.16,
> as tested on sm6350 and sc7280-based smartphones.
> 
> [  420.463176] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: AFE Port already open
> [  420.472676] ------------[ cut here ]------------
> [  420.472691] WARNING: CPU: 2 PID: 175 at sound/usb/qcom/qc_audio_offload.c:1056 handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
> [  420.472726] Modules linked in: rfcomm zram zsmalloc zstd_compress algif_hash algif_skcipher bnep nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nf_tables nfnetlink ipv6 fuse uhid uinput snd_usb_audio_qmi q6asm_dai q6routing q6afe_dai q6usb q6afe_clocks q6adm q6asm snd_q6dsp_common q6afe q6core apr pdr_interface snd_soc_sm8250 snd_soc_qcom
> _common snd_soc_qcom_offload_utils snd_soc_qcom_sdw soundwire_bus soc_usb snd_soc_core snd_compress snd_usb_audio ath10k_snoc ath10k_core snd_hwdep snd_usbmidi_lib ath fastrpc snd_pcm mac80211 hci_uart qrtr_smd snd_timer btqca qcom_pd_mapper snd_rawmidi bluetooth libarc4 qcom_pdr_msg cfg80211 snd soundcore ecdh_generic ecc rfkill qrtr qcom_stats qcom_q6v5_pas ipa qcom_pil_info qcom_q6v5 qcom_common
> [  420.473018] CPU: 2 UID: 0 PID: 175 Comm: kworker/u32:9 Tainted: G        W           6.16.0 #1-postmarketos-qcom-sm6350 NONE
> [  420.473033] Tainted: [W]=WARN
> [  420.473038] Hardware name: Fairphone 4 (DT)
> [  420.473045] Workqueue: qmi_msg_handler qmi_data_ready_work
> [  420.473065] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  420.473075] pc : handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
> [  420.473091] lr : handle_uaudio_stream_req+0xc84/0x13f8 [snd_usb_audio_qmi]
> [  420.473104] sp : ffff800082f939a0
> [  420.473110] x29: ffff800082f93b10 x28: ffff0000cfb796b8 x27: 0000000000008000
> [  420.473128] x26: ffff0000842afc80 x25: ffffa8e75a23b0e0 x24: 0000000000008000
> [  420.473145] x23: ffffa8e75a23bcf0 x22: ffff800082f93bd0 x21: 0000000000000000
> [  420.473161] x20: ffff800082f93c98 x19: ffff0000939bb740 x18: ffffa8e77925a4d0
> [  420.473178] x17: ffffffffffffffff x16: ffffa8e777ef9728 x15: ffffa8e77925a000
> [  420.473194] x14: 0000000000000000 x13: 0000000000000dc0 x12: ffff800080000000
> [  420.473211] x11: 0000000000000cc0 x10: 0000000000000001 x9 : ffffa8e77944b880
> [  420.473227] x8 : ffffd719b5f4d000 x7 : ffff00009033da18 x6 : 0000000000000000
> [  420.473244] x5 : 0000000000000000 x4 : ffff800082f93938 x3 : 0000000000000000
> [  420.473260] x2 : 0000000000000000 x1 : ffff0000857790c0 x0 : 0000000000000000
> [  420.473277] Call trace:
> [  420.473283]  handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi] (P)
> [  420.473300]  qmi_invoke_handler+0xbc/0x108
> [  420.473314]  qmi_handle_message+0x90/0x1a8
> [  420.473326]  qmi_data_ready_work+0x210/0x390
> [  420.473339]  process_one_work+0x150/0x3a0
> [  420.473351]  worker_thread+0x288/0x480
> [  420.473362]  kthread+0x118/0x1e0
> [  420.473375]  ret_from_fork+0x10/0x20
> [  420.473390] ---[ end trace 0000000000000000 ]---
> [  420.479244] qcom-q6afe aprsvc:service:4:4: cmd = 0x100e5 returned error = 0x1
> [  420.479540] qcom-q6afe aprsvc:service:4:4: DSP returned error[1]
> [  420.479558] qcom-q6afe aprsvc:service:4:4: AFE enable for port 0x7000 failed -22
> [  420.479572] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: fail to start AFE port 88
> [  420.479583] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: ASoC error (-22): at snd_soc_dai_prepare() on USB_RX
> 
> Reverting this patch makes it work as expected on 6.16.0.
> 
> Let me know if I can be of any help to resolve this.

I guess just dropping WARN_ON() would help?

As far as I read the code, pa argument isn't used at all in
uaudio_iommu_map() unless as sgt is NULL.  In this case, sgt is never
NULL, hence the pa argument is just a placeholder.
That said, the whole xfer_buf_pa (and its sanity check) can be dropped
there.


Takashi

--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -1020,7 +1020,6 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 	struct sg_table xfer_buf_sgt;
 	dma_addr_t xfer_buf_dma;
 	void *xfer_buf;
-	phys_addr_t xfer_buf_pa;
 	u32 len = xfer_buf_len;
 	bool dma_coherent;
 	dma_addr_t xfer_buf_dma_sysdev;
@@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 	if (!xfer_buf)
 		return -ENOMEM;
 
-	/* Remapping is not possible if xfer_buf is outside of linear map */
-	xfer_buf_pa = virt_to_phys(xfer_buf);
-	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
-		ret = -ENXIO;
-		goto unmap_sync;
-	}
 	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
 			xfer_buf_dma, len);
 
 	/* map the physical buffer into sysdev as well */
+	/* note: NULL passed to pa argument as we use sgt */
 	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
-					       xfer_buf_pa, len, &xfer_buf_sgt);
+					       NULL, len, &xfer_buf_sgt);
 	if (!xfer_buf_dma_sysdev) {
 		ret = -ENOMEM;
 		goto unmap_sync;

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Luca Weiss]

Hi Takashi,

On Fri Aug 1, 2025 at 2:31 PM CEST, Takashi Iwai wrote:
> On Fri, 01 Aug 2025 13:31:42 +0200,
> Luca Weiss wrote:
>> 
>> Hi Arnd,
>> 
>> On Tue May 13, 2025 at 2:34 PM CEST, Arnd Bergmann wrote:
>> > From: Arnd Bergmann <arnd@arndb.de>
>> >
>> > uaudio_transfer_buffer_setup() allocates a buffer for the subs->dev
>> > device, and the returned address for the buffer is a CPU local virtual
>> > address that may or may not be in the linear mapping, as well as a DMA
>> > address token that is accessible by the USB device, and this in turn
>> > may or may not correspond to the physical address.
>> >
>> > The use in the driver however assumes that these addresses are the
>> > linear map and the CPU physical address, respectively. Both are
>> > nonportable here, but in the end only the virtual address gets
>> > used by converting it to a physical address that gets mapped into
>> > a second iommu.
>> >
>> > Make this more explicit by pulling the conversion out first
>> > and warning if it is not part of the linear map, and using the
>> > actual physical address to map into the iommu in place of the
>> > dma address that may already be iommu-mapped into the usb host.
>> 
>> This patch is breaking USB audio offloading on Qualcomm devices on 6.16,
>> as tested on sm6350 and sc7280-based smartphones.
>> 
>> [  420.463176] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: AFE Port already open
>> [  420.472676] ------------[ cut here ]------------
>> [  420.472691] WARNING: CPU: 2 PID: 175 at sound/usb/qcom/qc_audio_offload.c:1056 handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
>> [  420.472726] Modules linked in: rfcomm zram zsmalloc zstd_compress algif_hash algif_skcipher bnep nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nf_tables nfnetlink ipv6 fuse uhid uinput snd_usb_audio_qmi q6asm_dai q6routing q6afe_dai q6usb q6afe_clocks q6adm q6asm snd_q6dsp_common q6afe q6core apr pdr_interface snd_soc_sm8250 snd_soc_qcom
>> _common snd_soc_qcom_offload_utils snd_soc_qcom_sdw soundwire_bus soc_usb snd_soc_core snd_compress snd_usb_audio ath10k_snoc ath10k_core snd_hwdep snd_usbmidi_lib ath fastrpc snd_pcm mac80211 hci_uart qrtr_smd snd_timer btqca qcom_pd_mapper snd_rawmidi bluetooth libarc4 qcom_pdr_msg cfg80211 snd soundcore ecdh_generic ecc rfkill qrtr qcom_stats qcom_q6v5_pas ipa qcom_pil_info qcom_q6v5 qcom_common
>> [  420.473018] CPU: 2 UID: 0 PID: 175 Comm: kworker/u32:9 Tainted: G        W           6.16.0 #1-postmarketos-qcom-sm6350 NONE
>> [  420.473033] Tainted: [W]=WARN
>> [  420.473038] Hardware name: Fairphone 4 (DT)
>> [  420.473045] Workqueue: qmi_msg_handler qmi_data_ready_work
>> [  420.473065] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [  420.473075] pc : handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
>> [  420.473091] lr : handle_uaudio_stream_req+0xc84/0x13f8 [snd_usb_audio_qmi]
>> [  420.473104] sp : ffff800082f939a0
>> [  420.473110] x29: ffff800082f93b10 x28: ffff0000cfb796b8 x27: 0000000000008000
>> [  420.473128] x26: ffff0000842afc80 x25: ffffa8e75a23b0e0 x24: 0000000000008000
>> [  420.473145] x23: ffffa8e75a23bcf0 x22: ffff800082f93bd0 x21: 0000000000000000
>> [  420.473161] x20: ffff800082f93c98 x19: ffff0000939bb740 x18: ffffa8e77925a4d0
>> [  420.473178] x17: ffffffffffffffff x16: ffffa8e777ef9728 x15: ffffa8e77925a000
>> [  420.473194] x14: 0000000000000000 x13: 0000000000000dc0 x12: ffff800080000000
>> [  420.473211] x11: 0000000000000cc0 x10: 0000000000000001 x9 : ffffa8e77944b880
>> [  420.473227] x8 : ffffd719b5f4d000 x7 : ffff00009033da18 x6 : 0000000000000000
>> [  420.473244] x5 : 0000000000000000 x4 : ffff800082f93938 x3 : 0000000000000000
>> [  420.473260] x2 : 0000000000000000 x1 : ffff0000857790c0 x0 : 0000000000000000
>> [  420.473277] Call trace:
>> [  420.473283]  handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi] (P)
>> [  420.473300]  qmi_invoke_handler+0xbc/0x108
>> [  420.473314]  qmi_handle_message+0x90/0x1a8
>> [  420.473326]  qmi_data_ready_work+0x210/0x390
>> [  420.473339]  process_one_work+0x150/0x3a0
>> [  420.473351]  worker_thread+0x288/0x480
>> [  420.473362]  kthread+0x118/0x1e0
>> [  420.473375]  ret_from_fork+0x10/0x20
>> [  420.473390] ---[ end trace 0000000000000000 ]---
>> [  420.479244] qcom-q6afe aprsvc:service:4:4: cmd = 0x100e5 returned error = 0x1
>> [  420.479540] qcom-q6afe aprsvc:service:4:4: DSP returned error[1]
>> [  420.479558] qcom-q6afe aprsvc:service:4:4: AFE enable for port 0x7000 failed -22
>> [  420.479572] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: fail to start AFE port 88
>> [  420.479583] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: ASoC error (-22): at snd_soc_dai_prepare() on USB_RX
>> 
>> Reverting this patch makes it work as expected on 6.16.0.
>> 
>> Let me know if I can be of any help to resolve this.
>
> I guess just dropping WARN_ON() would help?
>
> As far as I read the code, pa argument isn't used at all in
> uaudio_iommu_map() unless as sgt is NULL.  In this case, sgt is never
> NULL, hence the pa argument is just a placeholder.
> That said, the whole xfer_buf_pa (and its sanity check) can be dropped
> there.

Just the WARN splat is not the problem, it's actually failing
afterwards. Without the patch it works as expected.

But I'm not familiar with this code, or other deep technical details for
the USB audio offloading or DMA things so not sure what's actually going
on.

Regards
Luca

>
>
> Takashi
>
> --- a/sound/usb/qcom/qc_audio_offload.c
> +++ b/sound/usb/qcom/qc_audio_offload.c
> @@ -1020,7 +1020,6 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
>  	struct sg_table xfer_buf_sgt;
>  	dma_addr_t xfer_buf_dma;
>  	void *xfer_buf;
> -	phys_addr_t xfer_buf_pa;
>  	u32 len = xfer_buf_len;
>  	bool dma_coherent;
>  	dma_addr_t xfer_buf_dma_sysdev;
> @@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
>  	if (!xfer_buf)
>  		return -ENOMEM;
>  
> -	/* Remapping is not possible if xfer_buf is outside of linear map */
> -	xfer_buf_pa = virt_to_phys(xfer_buf);
> -	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> -		ret = -ENXIO;
> -		goto unmap_sync;
> -	}
>  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
>  			xfer_buf_dma, len);
>  
>  	/* map the physical buffer into sysdev as well */
> +	/* note: NULL passed to pa argument as we use sgt */
>  	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> -					       xfer_buf_pa, len, &xfer_buf_sgt);
> +					       NULL, len, &xfer_buf_sgt);
>  	if (!xfer_buf_dma_sysdev) {
>  		ret = -ENOMEM;
>  		goto unmap_sync;

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Takashi Iwai]

On Fri, 01 Aug 2025 14:35:27 +0200,
Luca Weiss wrote:
> 
> Hi Takashi,
> 
> On Fri Aug 1, 2025 at 2:31 PM CEST, Takashi Iwai wrote:
> > On Fri, 01 Aug 2025 13:31:42 +0200,
> > Luca Weiss wrote:
> >> 
> >> Hi Arnd,
> >> 
> >> On Tue May 13, 2025 at 2:34 PM CEST, Arnd Bergmann wrote:
> >> > From: Arnd Bergmann <arnd@arndb.de>
> >> >
> >> > uaudio_transfer_buffer_setup() allocates a buffer for the subs->dev
> >> > device, and the returned address for the buffer is a CPU local virtual
> >> > address that may or may not be in the linear mapping, as well as a DMA
> >> > address token that is accessible by the USB device, and this in turn
> >> > may or may not correspond to the physical address.
> >> >
> >> > The use in the driver however assumes that these addresses are the
> >> > linear map and the CPU physical address, respectively. Both are
> >> > nonportable here, but in the end only the virtual address gets
> >> > used by converting it to a physical address that gets mapped into
> >> > a second iommu.
> >> >
> >> > Make this more explicit by pulling the conversion out first
> >> > and warning if it is not part of the linear map, and using the
> >> > actual physical address to map into the iommu in place of the
> >> > dma address that may already be iommu-mapped into the usb host.
> >> 
> >> This patch is breaking USB audio offloading on Qualcomm devices on 6.16,
> >> as tested on sm6350 and sc7280-based smartphones.
> >> 
> >> [  420.463176] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: AFE Port already open
> >> [  420.472676] ------------[ cut here ]------------
> >> [  420.472691] WARNING: CPU: 2 PID: 175 at sound/usb/qcom/qc_audio_offload.c:1056 handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
> >> [  420.472726] Modules linked in: rfcomm zram zsmalloc zstd_compress algif_hash algif_skcipher bnep nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nf_tables nfnetlink ipv6 fuse uhid uinput snd_usb_audio_qmi q6asm_dai q6routing q6afe_dai q6usb q6afe_clocks q6adm q6asm snd_q6dsp_common q6afe q6core apr pdr_interface snd_soc_sm8250 snd_soc_qcom
> >> _common snd_soc_qcom_offload_utils snd_soc_qcom_sdw soundwire_bus soc_usb snd_soc_core snd_compress snd_usb_audio ath10k_snoc ath10k_core snd_hwdep snd_usbmidi_lib ath fastrpc snd_pcm mac80211 hci_uart qrtr_smd snd_timer btqca qcom_pd_mapper snd_rawmidi bluetooth libarc4 qcom_pdr_msg cfg80211 snd soundcore ecdh_generic ecc rfkill qrtr qcom_stats qcom_q6v5_pas ipa qcom_pil_info qcom_q6v5 qcom_common
> >> [  420.473018] CPU: 2 UID: 0 PID: 175 Comm: kworker/u32:9 Tainted: G        W           6.16.0 #1-postmarketos-qcom-sm6350 NONE
> >> [  420.473033] Tainted: [W]=WARN
> >> [  420.473038] Hardware name: Fairphone 4 (DT)
> >> [  420.473045] Workqueue: qmi_msg_handler qmi_data_ready_work
> >> [  420.473065] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >> [  420.473075] pc : handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
> >> [  420.473091] lr : handle_uaudio_stream_req+0xc84/0x13f8 [snd_usb_audio_qmi]
> >> [  420.473104] sp : ffff800082f939a0
> >> [  420.473110] x29: ffff800082f93b10 x28: ffff0000cfb796b8 x27: 0000000000008000
> >> [  420.473128] x26: ffff0000842afc80 x25: ffffa8e75a23b0e0 x24: 0000000000008000
> >> [  420.473145] x23: ffffa8e75a23bcf0 x22: ffff800082f93bd0 x21: 0000000000000000
> >> [  420.473161] x20: ffff800082f93c98 x19: ffff0000939bb740 x18: ffffa8e77925a4d0
> >> [  420.473178] x17: ffffffffffffffff x16: ffffa8e777ef9728 x15: ffffa8e77925a000
> >> [  420.473194] x14: 0000000000000000 x13: 0000000000000dc0 x12: ffff800080000000
> >> [  420.473211] x11: 0000000000000cc0 x10: 0000000000000001 x9 : ffffa8e77944b880
> >> [  420.473227] x8 : ffffd719b5f4d000 x7 : ffff00009033da18 x6 : 0000000000000000
> >> [  420.473244] x5 : 0000000000000000 x4 : ffff800082f93938 x3 : 0000000000000000
> >> [  420.473260] x2 : 0000000000000000 x1 : ffff0000857790c0 x0 : 0000000000000000
> >> [  420.473277] Call trace:
> >> [  420.473283]  handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi] (P)
> >> [  420.473300]  qmi_invoke_handler+0xbc/0x108
> >> [  420.473314]  qmi_handle_message+0x90/0x1a8
> >> [  420.473326]  qmi_data_ready_work+0x210/0x390
> >> [  420.473339]  process_one_work+0x150/0x3a0
> >> [  420.473351]  worker_thread+0x288/0x480
> >> [  420.473362]  kthread+0x118/0x1e0
> >> [  420.473375]  ret_from_fork+0x10/0x20
> >> [  420.473390] ---[ end trace 0000000000000000 ]---
> >> [  420.479244] qcom-q6afe aprsvc:service:4:4: cmd = 0x100e5 returned error = 0x1
> >> [  420.479540] qcom-q6afe aprsvc:service:4:4: DSP returned error[1]
> >> [  420.479558] qcom-q6afe aprsvc:service:4:4: AFE enable for port 0x7000 failed -22
> >> [  420.479572] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: fail to start AFE port 88
> >> [  420.479583] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: ASoC error (-22): at snd_soc_dai_prepare() on USB_RX
> >> 
> >> Reverting this patch makes it work as expected on 6.16.0.
> >> 
> >> Let me know if I can be of any help to resolve this.
> >
> > I guess just dropping WARN_ON() would help?
> >
> > As far as I read the code, pa argument isn't used at all in
> > uaudio_iommu_map() unless as sgt is NULL.  In this case, sgt is never
> > NULL, hence the pa argument is just a placeholder.
> > That said, the whole xfer_buf_pa (and its sanity check) can be dropped
> > there.
> 
> Just the WARN splat is not the problem, it's actually failing
> afterwards. Without the patch it works as expected.

OK, I wasn't clear enough; I meant to drop WARN_ON() *and* its error
handling:

	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
		ret = -ENXIO;
		goto unmap_sync;
	}

That is, replace WARN_ON() with 0.

	if (0 /*WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))*/) {
		ret = -ENXIO;
		goto unmap_sync;
	}

But you can try the patch I put in my previous reply instead (which
will remove all unneeded ).


thanks,

Takashi

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Luca Weiss]

Hi Takashi,

Sorry for the late reply, things got in the way.

On Fri Aug 1, 2025 at 2:49 PM CEST, Takashi Iwai wrote:
> On Fri, 01 Aug 2025 14:35:27 +0200,
> Luca Weiss wrote:
>> 
>> Hi Takashi,
>> 
>> On Fri Aug 1, 2025 at 2:31 PM CEST, Takashi Iwai wrote:
>> > On Fri, 01 Aug 2025 13:31:42 +0200,
>> > Luca Weiss wrote:
>> >> 
>> >> Hi Arnd,
>> >> 
>> >> On Tue May 13, 2025 at 2:34 PM CEST, Arnd Bergmann wrote:
>> >> > From: Arnd Bergmann <arnd@arndb.de>
>> >> >
>> >> > uaudio_transfer_buffer_setup() allocates a buffer for the subs->dev
>> >> > device, and the returned address for the buffer is a CPU local virtual
>> >> > address that may or may not be in the linear mapping, as well as a DMA
>> >> > address token that is accessible by the USB device, and this in turn
>> >> > may or may not correspond to the physical address.
>> >> >
>> >> > The use in the driver however assumes that these addresses are the
>> >> > linear map and the CPU physical address, respectively. Both are
>> >> > nonportable here, but in the end only the virtual address gets
>> >> > used by converting it to a physical address that gets mapped into
>> >> > a second iommu.
>> >> >
>> >> > Make this more explicit by pulling the conversion out first
>> >> > and warning if it is not part of the linear map, and using the
>> >> > actual physical address to map into the iommu in place of the
>> >> > dma address that may already be iommu-mapped into the usb host.
>> >> 
>> >> This patch is breaking USB audio offloading on Qualcomm devices on 6.16,
>> >> as tested on sm6350 and sc7280-based smartphones.
>> >> 
>> >> [  420.463176] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: AFE Port already open
>> >> [  420.472676] ------------[ cut here ]------------
>> >> [  420.472691] WARNING: CPU: 2 PID: 175 at sound/usb/qcom/qc_audio_offload.c:1056 handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
>> >> [  420.472726] Modules linked in: rfcomm zram zsmalloc zstd_compress algif_hash algif_skcipher bnep nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nf_tables nfnetlink ipv6 fuse uhid uinput snd_usb_audio_qmi q6asm_dai q6routing q6afe_dai q6usb q6afe_clocks q6adm q6asm snd_q6dsp_common q6afe q6core apr pdr_interface snd_soc_sm8250 snd_soc_qcom
>> >> _common snd_soc_qcom_offload_utils snd_soc_qcom_sdw soundwire_bus soc_usb snd_soc_core snd_compress snd_usb_audio ath10k_snoc ath10k_core snd_hwdep snd_usbmidi_lib ath fastrpc snd_pcm mac80211 hci_uart qrtr_smd snd_timer btqca qcom_pd_mapper snd_rawmidi bluetooth libarc4 qcom_pdr_msg cfg80211 snd soundcore ecdh_generic ecc rfkill qrtr qcom_stats qcom_q6v5_pas ipa qcom_pil_info qcom_q6v5 qcom_common
>> >> [  420.473018] CPU: 2 UID: 0 PID: 175 Comm: kworker/u32:9 Tainted: G        W           6.16.0 #1-postmarketos-qcom-sm6350 NONE
>> >> [  420.473033] Tainted: [W]=WARN
>> >> [  420.473038] Hardware name: Fairphone 4 (DT)
>> >> [  420.473045] Workqueue: qmi_msg_handler qmi_data_ready_work
>> >> [  420.473065] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> >> [  420.473075] pc : handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
>> >> [  420.473091] lr : handle_uaudio_stream_req+0xc84/0x13f8 [snd_usb_audio_qmi]
>> >> [  420.473104] sp : ffff800082f939a0
>> >> [  420.473110] x29: ffff800082f93b10 x28: ffff0000cfb796b8 x27: 0000000000008000
>> >> [  420.473128] x26: ffff0000842afc80 x25: ffffa8e75a23b0e0 x24: 0000000000008000
>> >> [  420.473145] x23: ffffa8e75a23bcf0 x22: ffff800082f93bd0 x21: 0000000000000000
>> >> [  420.473161] x20: ffff800082f93c98 x19: ffff0000939bb740 x18: ffffa8e77925a4d0
>> >> [  420.473178] x17: ffffffffffffffff x16: ffffa8e777ef9728 x15: ffffa8e77925a000
>> >> [  420.473194] x14: 0000000000000000 x13: 0000000000000dc0 x12: ffff800080000000
>> >> [  420.473211] x11: 0000000000000cc0 x10: 0000000000000001 x9 : ffffa8e77944b880
>> >> [  420.473227] x8 : ffffd719b5f4d000 x7 : ffff00009033da18 x6 : 0000000000000000
>> >> [  420.473244] x5 : 0000000000000000 x4 : ffff800082f93938 x3 : 0000000000000000
>> >> [  420.473260] x2 : 0000000000000000 x1 : ffff0000857790c0 x0 : 0000000000000000
>> >> [  420.473277] Call trace:
>> >> [  420.473283]  handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi] (P)
>> >> [  420.473300]  qmi_invoke_handler+0xbc/0x108
>> >> [  420.473314]  qmi_handle_message+0x90/0x1a8
>> >> [  420.473326]  qmi_data_ready_work+0x210/0x390
>> >> [  420.473339]  process_one_work+0x150/0x3a0
>> >> [  420.473351]  worker_thread+0x288/0x480
>> >> [  420.473362]  kthread+0x118/0x1e0
>> >> [  420.473375]  ret_from_fork+0x10/0x20
>> >> [  420.473390] ---[ end trace 0000000000000000 ]---
>> >> [  420.479244] qcom-q6afe aprsvc:service:4:4: cmd = 0x100e5 returned error = 0x1
>> >> [  420.479540] qcom-q6afe aprsvc:service:4:4: DSP returned error[1]
>> >> [  420.479558] qcom-q6afe aprsvc:service:4:4: AFE enable for port 0x7000 failed -22
>> >> [  420.479572] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: fail to start AFE port 88
>> >> [  420.479583] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: ASoC error (-22): at snd_soc_dai_prepare() on USB_RX
>> >> 
>> >> Reverting this patch makes it work as expected on 6.16.0.
>> >> 
>> >> Let me know if I can be of any help to resolve this.
>> >
>> > I guess just dropping WARN_ON() would help?
>> >
>> > As far as I read the code, pa argument isn't used at all in
>> > uaudio_iommu_map() unless as sgt is NULL.  In this case, sgt is never
>> > NULL, hence the pa argument is just a placeholder.
>> > That said, the whole xfer_buf_pa (and its sanity check) can be dropped
>> > there.
>> 
>> Just the WARN splat is not the problem, it's actually failing
>> afterwards. Without the patch it works as expected.
>
> OK, I wasn't clear enough; I meant to drop WARN_ON() *and* its error
> handling:
>
> 	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> 		ret = -ENXIO;
> 		goto unmap_sync;
> 	}
>
> That is, replace WARN_ON() with 0.
>
> 	if (0 /*WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))*/) {
> 		ret = -ENXIO;
> 		goto unmap_sync;
> 	}

Yes, that appears to work fine as well. Playback works again.

>
> But you can try the patch I put in my previous reply instead (which
> will remove all unneeded ).

That patch doesn't compile for me with this error:

In file included from ./include/uapi/linux/posix_types.h:5,
                 from ./include/uapi/linux/types.h:14,
                 from ./include/linux/types.h:6,
                 from ./include/linux/kasan-checks.h:5,
                 from ./include/asm-generic/rwonce.h:26,
                 from ./arch/arm64/include/asm/rwonce.h:67,
                 from ./include/linux/compiler.h:390,
                 from ./include/linux/dev_printk.h:14,
                 from ./include/linux/device.h:15,
                 from ./include/linux/auxiliary_bus.h:11,
                 from sound/usb/qcom/qc_audio_offload.c:6:
sound/usb/qcom/qc_audio_offload.c: In function 'uaudio_transfer_buffer_setup':
./include/linux/stddef.h:8:14: error: passing argument 3 of 'uaudio_iommu_map' makes integer from pointer without a cast [-Wint-conversion]
    8 | #define NULL ((void *)0)
      |              ^~~~~~~~~~~
      |              |
      |              void *
sound/usb/qcom/qc_audio_offload.c:1059:48: note: in expansion of macro 'NULL'
 1059 |                                                NULL, len, &xfer_buf_sgt);
      |                                                ^~~~
sound/usb/qcom/qc_audio_offload.c:555:51: note: expected 'phys_addr_t' {aka 'long long unsigned int'} but argument is of type 'void *'
  555 |                                       phys_addr_t pa, size_t size,
      |                                       ~~~~~~~~~~~~^~


Regards
Luca

>
>
> thanks,
>
> Takashi

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Arnd Bergmann]

On Fri, Sep 5, 2025, at 13:47, Luca Weiss wrote:
> On Fri Aug 1, 2025 at 2:49 PM CEST, Takashi Iwai wrote:
>> On Fri, 01 Aug 2025 14:35:27 +0200,
>>> On Fri Aug 1, 2025 at 2:31 PM CEST, Takashi Iwai wrote:
>>> > On Fri, 01 Aug 2025 13:31:42 +0200,
>>> >> On Tue May 13, 2025 at 2:34 PM CEST, Arnd Bergmann wrote:
>>> >> >
>>> >> > Make this more explicit by pulling the conversion out first
>>> >> > and warning if it is not part of the linear map, and using the
>>> >> > actual physical address to map into the iommu in place of the
>>> >> > dma address that may already be iommu-mapped into the usb host.
>>> >> 
>>> >> This patch is breaking USB audio offloading on Qualcomm devices on 6.16,
>>> >> as tested on sm6350 and sc7280-based smartphones.
>>> >> 
>>> >> Let me know if I can be of any help to resolve this.
>>> >
>>> > I guess just dropping WARN_ON() would help?
>>> >
>>> > As far as I read the code, pa argument isn't used at all in
>>> > uaudio_iommu_map() unless as sgt is NULL.  In this case, sgt is never
>>> > NULL, hence the pa argument is just a placeholder.
>>> > That said, the whole xfer_buf_pa (and its sanity check) can be dropped
>>> > there.
>>> 
>>> Just the WARN splat is not the problem, it's actually failing
>>> afterwards. Without the patch it works as expected.
>>
>> That is, replace WARN_ON() with 0.
>>
>> 	if (0 /*WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))*/) {
>> 		ret = -ENXIO;
>> 		goto unmap_sync;
>> 	}
>
> Yes, that appears to work fine as well. Playback works again.
>

This does mean that the address returned from xfer_buf is not
a kernel address in the virtual map though, and converting it
through virt_to_phys() makes the pa undefined for
uaudio_iommu_map(). Can you print what that pa value
is that you get here, and where that sits in the address space?

    Arnd

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Takashi Iwai]

On Fri, 05 Sep 2025 13:47:28 +0200,
Luca Weiss wrote:
> 
> Hi Takashi,
> 
> Sorry for the late reply, things got in the way.
> 
> On Fri Aug 1, 2025 at 2:49 PM CEST, Takashi Iwai wrote:
> > On Fri, 01 Aug 2025 14:35:27 +0200,
> > Luca Weiss wrote:
> >> 
> >> Hi Takashi,
> >> 
> >> On Fri Aug 1, 2025 at 2:31 PM CEST, Takashi Iwai wrote:
> >> > On Fri, 01 Aug 2025 13:31:42 +0200,
> >> > Luca Weiss wrote:
> >> >> 
> >> >> Hi Arnd,
> >> >> 
> >> >> On Tue May 13, 2025 at 2:34 PM CEST, Arnd Bergmann wrote:
> >> >> > From: Arnd Bergmann <arnd@arndb.de>
> >> >> >
> >> >> > uaudio_transfer_buffer_setup() allocates a buffer for the subs->dev
> >> >> > device, and the returned address for the buffer is a CPU local virtual
> >> >> > address that may or may not be in the linear mapping, as well as a DMA
> >> >> > address token that is accessible by the USB device, and this in turn
> >> >> > may or may not correspond to the physical address.
> >> >> >
> >> >> > The use in the driver however assumes that these addresses are the
> >> >> > linear map and the CPU physical address, respectively. Both are
> >> >> > nonportable here, but in the end only the virtual address gets
> >> >> > used by converting it to a physical address that gets mapped into
> >> >> > a second iommu.
> >> >> >
> >> >> > Make this more explicit by pulling the conversion out first
> >> >> > and warning if it is not part of the linear map, and using the
> >> >> > actual physical address to map into the iommu in place of the
> >> >> > dma address that may already be iommu-mapped into the usb host.
> >> >> 
> >> >> This patch is breaking USB audio offloading on Qualcomm devices on 6.16,
> >> >> as tested on sm6350 and sc7280-based smartphones.
> >> >> 
> >> >> [  420.463176] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: AFE Port already open
> >> >> [  420.472676] ------------[ cut here ]------------
> >> >> [  420.472691] WARNING: CPU: 2 PID: 175 at sound/usb/qcom/qc_audio_offload.c:1056 handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
> >> >> [  420.472726] Modules linked in: rfcomm zram zsmalloc zstd_compress algif_hash algif_skcipher bnep nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nf_tables nfnetlink ipv6 fuse uhid uinput snd_usb_audio_qmi q6asm_dai q6routing q6afe_dai q6usb q6afe_clocks q6adm q6asm snd_q6dsp_common q6afe q6core apr pdr_interface snd_soc_sm8250 snd_soc_qcom
> >> >> _common snd_soc_qcom_offload_utils snd_soc_qcom_sdw soundwire_bus soc_usb snd_soc_core snd_compress snd_usb_audio ath10k_snoc ath10k_core snd_hwdep snd_usbmidi_lib ath fastrpc snd_pcm mac80211 hci_uart qrtr_smd snd_timer btqca qcom_pd_mapper snd_rawmidi bluetooth libarc4 qcom_pdr_msg cfg80211 snd soundcore ecdh_generic ecc rfkill qrtr qcom_stats qcom_q6v5_pas ipa qcom_pil_info qcom_q6v5 qcom_common
> >> >> [  420.473018] CPU: 2 UID: 0 PID: 175 Comm: kworker/u32:9 Tainted: G        W           6.16.0 #1-postmarketos-qcom-sm6350 NONE
> >> >> [  420.473033] Tainted: [W]=WARN
> >> >> [  420.473038] Hardware name: Fairphone 4 (DT)
> >> >> [  420.473045] Workqueue: qmi_msg_handler qmi_data_ready_work
> >> >> [  420.473065] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >> >> [  420.473075] pc : handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi]
> >> >> [  420.473091] lr : handle_uaudio_stream_req+0xc84/0x13f8 [snd_usb_audio_qmi]
> >> >> [  420.473104] sp : ffff800082f939a0
> >> >> [  420.473110] x29: ffff800082f93b10 x28: ffff0000cfb796b8 x27: 0000000000008000
> >> >> [  420.473128] x26: ffff0000842afc80 x25: ffffa8e75a23b0e0 x24: 0000000000008000
> >> >> [  420.473145] x23: ffffa8e75a23bcf0 x22: ffff800082f93bd0 x21: 0000000000000000
> >> >> [  420.473161] x20: ffff800082f93c98 x19: ffff0000939bb740 x18: ffffa8e77925a4d0
> >> >> [  420.473178] x17: ffffffffffffffff x16: ffffa8e777ef9728 x15: ffffa8e77925a000
> >> >> [  420.473194] x14: 0000000000000000 x13: 0000000000000dc0 x12: ffff800080000000
> >> >> [  420.473211] x11: 0000000000000cc0 x10: 0000000000000001 x9 : ffffa8e77944b880
> >> >> [  420.473227] x8 : ffffd719b5f4d000 x7 : ffff00009033da18 x6 : 0000000000000000
> >> >> [  420.473244] x5 : 0000000000000000 x4 : ffff800082f93938 x3 : 0000000000000000
> >> >> [  420.473260] x2 : 0000000000000000 x1 : ffff0000857790c0 x0 : 0000000000000000
> >> >> [  420.473277] Call trace:
> >> >> [  420.473283]  handle_uaudio_stream_req+0xea8/0x13f8 [snd_usb_audio_qmi] (P)
> >> >> [  420.473300]  qmi_invoke_handler+0xbc/0x108
> >> >> [  420.473314]  qmi_handle_message+0x90/0x1a8
> >> >> [  420.473326]  qmi_data_ready_work+0x210/0x390
> >> >> [  420.473339]  process_one_work+0x150/0x3a0
> >> >> [  420.473351]  worker_thread+0x288/0x480
> >> >> [  420.473362]  kthread+0x118/0x1e0
> >> >> [  420.473375]  ret_from_fork+0x10/0x20
> >> >> [  420.473390] ---[ end trace 0000000000000000 ]---
> >> >> [  420.479244] qcom-q6afe aprsvc:service:4:4: cmd = 0x100e5 returned error = 0x1
> >> >> [  420.479540] qcom-q6afe aprsvc:service:4:4: DSP returned error[1]
> >> >> [  420.479558] qcom-q6afe aprsvc:service:4:4: AFE enable for port 0x7000 failed -22
> >> >> [  420.479572] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: fail to start AFE port 88
> >> >> [  420.479583] q6afe-dai 3000000.remoteproc:glink-edge:apr:service@4:dais: ASoC error (-22): at snd_soc_dai_prepare() on USB_RX
> >> >> 
> >> >> Reverting this patch makes it work as expected on 6.16.0.
> >> >> 
> >> >> Let me know if I can be of any help to resolve this.
> >> >
> >> > I guess just dropping WARN_ON() would help?
> >> >
> >> > As far as I read the code, pa argument isn't used at all in
> >> > uaudio_iommu_map() unless as sgt is NULL.  In this case, sgt is never
> >> > NULL, hence the pa argument is just a placeholder.
> >> > That said, the whole xfer_buf_pa (and its sanity check) can be dropped
> >> > there.
> >> 
> >> Just the WARN splat is not the problem, it's actually failing
> >> afterwards. Without the patch it works as expected.
> >
> > OK, I wasn't clear enough; I meant to drop WARN_ON() *and* its error
> > handling:
> >
> > 	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> > 		ret = -ENXIO;
> > 		goto unmap_sync;
> > 	}
> >
> > That is, replace WARN_ON() with 0.
> >
> > 	if (0 /*WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))*/) {
> > 		ret = -ENXIO;
> > 		goto unmap_sync;
> > 	}
> 
> Yes, that appears to work fine as well. Playback works again.
> 
> >
> > But you can try the patch I put in my previous reply instead (which
> > will remove all unneeded ).
> 
> That patch doesn't compile for me with this error:
> 
> In file included from ./include/uapi/linux/posix_types.h:5,
>                  from ./include/uapi/linux/types.h:14,
>                  from ./include/linux/types.h:6,
>                  from ./include/linux/kasan-checks.h:5,
>                  from ./include/asm-generic/rwonce.h:26,
>                  from ./arch/arm64/include/asm/rwonce.h:67,
>                  from ./include/linux/compiler.h:390,
>                  from ./include/linux/dev_printk.h:14,
>                  from ./include/linux/device.h:15,
>                  from ./include/linux/auxiliary_bus.h:11,
>                  from sound/usb/qcom/qc_audio_offload.c:6:
> sound/usb/qcom/qc_audio_offload.c: In function 'uaudio_transfer_buffer_setup':
> ./include/linux/stddef.h:8:14: error: passing argument 3 of 'uaudio_iommu_map' makes integer from pointer without a cast [-Wint-conversion]
>     8 | #define NULL ((void *)0)
>       |              ^~~~~~~~~~~
>       |              |
>       |              void *
> sound/usb/qcom/qc_audio_offload.c:1059:48: note: in expansion of macro 'NULL'
>  1059 |                                                NULL, len, &xfer_buf_sgt);
>       |                                                ^~~~
> sound/usb/qcom/qc_audio_offload.c:555:51: note: expected 'phys_addr_t' {aka 'long long unsigned int'} but argument is of type 'void *'
>   555 |                                       phys_addr_t pa, size_t size,
>       |                                       ~~~~~~~~~~~~^~

Then replace NULL with 0.  That is, like below.


Takashi

-- 8< --
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -1020,7 +1020,6 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 	struct sg_table xfer_buf_sgt;
 	dma_addr_t xfer_buf_dma;
 	void *xfer_buf;
-	phys_addr_t xfer_buf_pa;
 	u32 len = xfer_buf_len;
 	bool dma_coherent;
 	dma_addr_t xfer_buf_dma_sysdev;
@@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 	if (!xfer_buf)
 		return -ENOMEM;
 
-	/* Remapping is not possible if xfer_buf is outside of linear map */
-	xfer_buf_pa = virt_to_phys(xfer_buf);
-	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
-		ret = -ENXIO;
-		goto unmap_sync;
-	}
 	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
 			xfer_buf_dma, len);
 
 	/* map the physical buffer into sysdev as well */
+	/* note: 0 is passed to pa argument as we use sgt */
 	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
-					       xfer_buf_pa, len, &xfer_buf_sgt);
+					       0, len, &xfer_buf_sgt);
 	if (!xfer_buf_dma_sysdev) {
 		ret = -ENOMEM;
 		goto unmap_sync;

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Luca Weiss]

Hi Arnd,

On Fri Sep 5, 2025 at 2:08 PM CEST, Arnd Bergmann wrote:
> On Fri, Sep 5, 2025, at 13:47, Luca Weiss wrote:
>> On Fri Aug 1, 2025 at 2:49 PM CEST, Takashi Iwai wrote:
>>> On Fri, 01 Aug 2025 14:35:27 +0200,
>>>> On Fri Aug 1, 2025 at 2:31 PM CEST, Takashi Iwai wrote:
>>>> > On Fri, 01 Aug 2025 13:31:42 +0200,
>>>> >> On Tue May 13, 2025 at 2:34 PM CEST, Arnd Bergmann wrote:
>>>> >> >
>>>> >> > Make this more explicit by pulling the conversion out first
>>>> >> > and warning if it is not part of the linear map, and using the
>>>> >> > actual physical address to map into the iommu in place of the
>>>> >> > dma address that may already be iommu-mapped into the usb host.
>>>> >> 
>>>> >> This patch is breaking USB audio offloading on Qualcomm devices on 6.16,
>>>> >> as tested on sm6350 and sc7280-based smartphones.
>>>> >> 
>>>> >> Let me know if I can be of any help to resolve this.
>>>> >
>>>> > I guess just dropping WARN_ON() would help?
>>>> >
>>>> > As far as I read the code, pa argument isn't used at all in
>>>> > uaudio_iommu_map() unless as sgt is NULL.  In this case, sgt is never
>>>> > NULL, hence the pa argument is just a placeholder.
>>>> > That said, the whole xfer_buf_pa (and its sanity check) can be dropped
>>>> > there.
>>>> 
>>>> Just the WARN splat is not the problem, it's actually failing
>>>> afterwards. Without the patch it works as expected.
>>>
>>> That is, replace WARN_ON() with 0.
>>>
>>> 	if (0 /*WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))*/) {
>>> 		ret = -ENXIO;
>>> 		goto unmap_sync;
>>> 	}
>>
>> Yes, that appears to work fine as well. Playback works again.
>>
>
> This does mean that the address returned from xfer_buf is not
> a kernel address in the virtual map though, and converting it
> through virt_to_phys() makes the pa undefined for
> uaudio_iommu_map(). Can you print what that pa value
> is that you get here, and where that sits in the address space?

Adding a debug print gives me the following below.

  dev_err(uaudio_qdev->data->dev, "xfer_buf_pa=%llx\n", xfer_buf_pa);

Not sure what exactly you mean with "where that sits in the address
space" and how I can figure that out.

[  130.124938] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0486ea6000
[  130.141583] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0484be5000
[  130.145826] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0484bfd000
[  130.150031] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0484c0d000
[  130.155437] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0484ec5000
[  130.159573] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0484f0d000
[  130.164778] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0484f2d000
[  130.180878] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0484fe5000
[  130.185178] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0485005000
[  130.189393] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba048507d000
[  130.194323] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba04850e5000
[  130.198375] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba048561d000
[  130.202280] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0485655000
[  130.215915] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba04856a5000
[  130.219701] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0486f15000
[  130.223351] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba04870b5000
[  130.227881] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba04871bd000
[  130.231780] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba04871dd000
[  130.235432] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba04871f5000
[  130.249669] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0487525000
[  130.253451] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba048755d000
[  130.257113] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba048758d000
[  130.261473] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba04875c5000
[  130.265226] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba04875d5000
[  130.268856] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba04875de000
[  130.283635] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0486ed3000
[  130.287445] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0486eca000
[  130.291147] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba04875e7000
[  130.295957] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba04875f0000
[  130.299995] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0489fd5000
[  130.303607] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0489fde000
[  130.317503] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0489fe7000
[  130.321291] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba0489ff0000
[  130.324979] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba048a005000
[  130.329604] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba048a00e000
[  130.333397] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba048a01d000
[  130.337089] q6usb-dai 3000000.remoteproc:glink-edge:apr:service@4:usbd: xfer_buf_pa=ffffba048a026000

Regards
Luca

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Arnd Bergmann]

On Fri, Sep 5, 2025, at 15:17, Luca Weiss wrote:
> On Fri Sep 5, 2025 at 2:08 PM CEST, Arnd Bergmann wrote:
>> On Fri, Sep 5, 2025, at 13:47, Luca Weiss wrote:
>>
>> This does mean that the address returned from xfer_buf is not
>> a kernel address in the virtual map though, and converting it
>> through virt_to_phys() makes the pa undefined for
>> uaudio_iommu_map(). Can you print what that pa value
>> is that you get here, and where that sits in the address space?
>
> Adding a debug print gives me the following below.
>
>   dev_err(uaudio_qdev->data->dev, "xfer_buf_pa=%llx\n", xfer_buf_pa);
>
> Not sure what exactly you mean with "where that sits in the address
> space" and how I can figure that out.
>
> [  130.124938] q6usb-dai 
> 3000000.remoteproc:glink-edge:apr:service@4:usbd: 
> xfer_buf_pa=ffffba0486ea6000

Splitting the address in 16-bit chunks, this is

0x00ff.ffba0.486ea.6000

which is well outside of the 40-bit physical address space of
the CPU, which confirms that the virtual address was not in
the linear map at all, and probably it should not get passed
into dma_get_sgtable() either.

From what I can tell, this seems to correspond to a virtual
address in the vmalloc space instead, which is what happens
e.g. on arm64 when you ask for an allocation on a noncoherent
device.

It seems that dma_get_sgtable() does have a special case for
this and ends up walking the page table for it, which I
assume is what the driver is relying on, so Takashi's patch
seems fine but could use a few more comments.

It's still unclear to me why the driver has custom iommu
handling rather than just using dma_map_single() on buffers
allocated with dma_alloc_noncoherent or DMA_ATTR_NO_KERNEL_MAPPING
here, since it doesn't even use the virtual address.

      Arnd

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Arnd Bergmann]

On Fri, Sep 5, 2025, at 14:26, Takashi Iwai wrote:
> On Fri, 05 Sep 2025 13:47:28 +0200,

> @@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct 
> snd_usb_substream *subs,
>  	if (!xfer_buf)
>  		return -ENOMEM;
> 
> -	/* Remapping is not possible if xfer_buf is outside of linear map */
> -	xfer_buf_pa = virt_to_phys(xfer_buf);
> -	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> -		ret = -ENXIO;
> -		goto unmap_sync;
> -	}
>  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
>  			xfer_buf_dma, len);
> 
>  	/* map the physical buffer into sysdev as well */
> +	/* note: 0 is passed to pa argument as we use sgt */
>  	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> -					       xfer_buf_pa, len, &xfer_buf_sgt);
> +					       0, len, &xfer_buf_sgt);
>  	if (!xfer_buf_dma_sysdev) {
>  		ret = -ENOMEM;
>  		goto unmap_sync;


Makes sense. I had to rework the code a little more to actually
understand how it fits together, for reference the below version
(I don't expect it to build cleanly) would split up
uaudio_iommu_map() into one function that takes a physical
address and another function that takes an sg table.

       Arnd

diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
index a25c5a531690..f843c2181da5 100644
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -539,32 +539,24 @@ static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long iova,
 }
 
 /**
- * uaudio_iommu_map() - maps iommu memory for adsp
+ * uaudio_iommu_map_pa() - maps iommu memory for adsp
  * @mtype: ring type
  * @dma_coherent: dma coherent
  * @pa: physical address for ring/buffer
  * @size: size of memory region
- * @sgt: sg table for memory region
  *
  * Maps the XHCI related resources to a memory region that is assigned to be
  * used by the adsp.  This will be mapped to the domain, which is created by
  * the ASoC USB backend driver.
  *
  */
-static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
-				      phys_addr_t pa, size_t size,
-				      struct sg_table *sgt)
+static unsigned long uaudio_iommu_map_pa(enum mem_type mtype, bool dma_coherent,
+				      phys_addr_t pa, size_t size)
 {
 	struct scatterlist *sg;
 	unsigned long iova = 0;
-	size_t total_len = 0;
-	unsigned long iova_sg;
-	phys_addr_t pa_sg;
 	bool map = true;
-	size_t sg_len;
 	int prot;
-	int ret;
-	int i;
 
 	prot = IOMMU_READ | IOMMU_WRITE;
 
@@ -583,20 +575,42 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
 				     &uaudio_qdev->xfer_ring_iova_size,
 				     &uaudio_qdev->xfer_ring_list, size);
 		break;
-	case MEM_XFER_BUF:
-		iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
-				     &uaudio_qdev->xfer_buf_iova_size,
-				     &uaudio_qdev->xfer_buf_list, size);
-		break;
 	default:
 		dev_err(uaudio_qdev->data->dev, "unknown mem type %d\n", mtype);
 	}
 
 	if (!iova || !map)
-		goto done;
+		return 0;
 
-	if (!sgt)
-		goto skip_sgt_map;
+	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
+
+done:
+	return iova;
+}
+
+static unsigned long uaudio_iommu_map_xfer_buf(bool dma_coherent,
+				      size_t size, struct sg_table *sgt)
+{
+	struct scatterlist *sg;
+	unsigned long iova = 0;
+	size_t total_len = 0;
+	unsigned long iova_sg;
+	phys_addr_t pa_sg;
+	size_t sg_len;
+	int prot;
+	int ret;
+	int i;
+
+	prot = IOMMU_READ | IOMMU_WRITE;
+
+	if (dma_coherent)
+		prot |= IOMMU_CACHE;
+
+	iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
+			     &uaudio_qdev->xfer_buf_iova_size,
+			     &uaudio_qdev->xfer_buf_list, size);
+	if (!iova)
+		goto done;
 
 	iova_sg = iova;
 	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
@@ -618,11 +632,6 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
 		uaudio_iommu_unmap(MEM_XFER_BUF, iova, size, total_len);
 		iova = 0;
 	}
-	return iova;
-
-skip_sgt_map:
-	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
-
 done:
 	return iova;
 }
@@ -1061,8 +1070,8 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 			xfer_buf_dma, len);
 
 	/* map the physical buffer into sysdev as well */
-	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
-					       xfer_buf_pa, len, &xfer_buf_sgt);
+	xfer_buf_dma_sysdev = uaudio_iommu_map_xfer_buf(dma_coherent, xfer_buf_pa,
+							len, &xfer_buf_sgt);
 	if (!xfer_buf_dma_sysdev) {
 		ret = -ENOMEM;
 		goto unmap_sync;
@@ -1143,8 +1152,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
 	sg_free_table(sgt);
 
 	/* data transfer ring */
-	iova = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_pa,
-			      PAGE_SIZE, NULL);
+	iova = uaudio_iommu_map_pa(MEM_XFER_RING, dma_coherent, tr_pa,
+				   PAGE_SIZE);
 	if (!iova) {
 		ret = -ENOMEM;
 		goto clear_pa;
@@ -1207,8 +1216,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
 	mem_info->dma = sg_dma_address(sgt->sgl);
 	sg_free_table(sgt);
 
-	iova = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, er_pa,
-			      PAGE_SIZE, NULL);
+	iova = uaudio_iommu_map_pa(MEM_EVENT_RING, dma_coherent, er_pa,
+				   PAGE_SIZE);
 	if (!iova) {
 		ret = -ENOMEM;
 		goto clear_pa;

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Takashi Iwai]

On Fri, 05 Sep 2025 16:54:06 +0200,
Arnd Bergmann wrote:
> 
> On Fri, Sep 5, 2025, at 14:26, Takashi Iwai wrote:
> > On Fri, 05 Sep 2025 13:47:28 +0200,
> 
> > @@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct 
> > snd_usb_substream *subs,
> >  	if (!xfer_buf)
> >  		return -ENOMEM;
> > 
> > -	/* Remapping is not possible if xfer_buf is outside of linear map */
> > -	xfer_buf_pa = virt_to_phys(xfer_buf);
> > -	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> > -		ret = -ENXIO;
> > -		goto unmap_sync;
> > -	}
> >  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
> >  			xfer_buf_dma, len);
> > 
> >  	/* map the physical buffer into sysdev as well */
> > +	/* note: 0 is passed to pa argument as we use sgt */
> >  	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> > -					       xfer_buf_pa, len, &xfer_buf_sgt);
> > +					       0, len, &xfer_buf_sgt);
> >  	if (!xfer_buf_dma_sysdev) {
> >  		ret = -ENOMEM;
> >  		goto unmap_sync;
> 
> 
> Makes sense. I had to rework the code a little more to actually
> understand how it fits together, for reference the below version
> (I don't expect it to build cleanly) would split up
> uaudio_iommu_map() into one function that takes a physical
> address and another function that takes an sg table.

Yes, that's much clearer.


thanks,

Takashi

> 
>        Arnd
> 
> diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
> index a25c5a531690..f843c2181da5 100644
> --- a/sound/usb/qcom/qc_audio_offload.c
> +++ b/sound/usb/qcom/qc_audio_offload.c
> @@ -539,32 +539,24 @@ static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long iova,
>  }
>  
>  /**
> - * uaudio_iommu_map() - maps iommu memory for adsp
> + * uaudio_iommu_map_pa() - maps iommu memory for adsp
>   * @mtype: ring type
>   * @dma_coherent: dma coherent
>   * @pa: physical address for ring/buffer
>   * @size: size of memory region
> - * @sgt: sg table for memory region
>   *
>   * Maps the XHCI related resources to a memory region that is assigned to be
>   * used by the adsp.  This will be mapped to the domain, which is created by
>   * the ASoC USB backend driver.
>   *
>   */
> -static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
> -				      phys_addr_t pa, size_t size,
> -				      struct sg_table *sgt)
> +static unsigned long uaudio_iommu_map_pa(enum mem_type mtype, bool dma_coherent,
> +				      phys_addr_t pa, size_t size)
>  {
>  	struct scatterlist *sg;
>  	unsigned long iova = 0;
> -	size_t total_len = 0;
> -	unsigned long iova_sg;
> -	phys_addr_t pa_sg;
>  	bool map = true;
> -	size_t sg_len;
>  	int prot;
> -	int ret;
> -	int i;
>  
>  	prot = IOMMU_READ | IOMMU_WRITE;
>  
> @@ -583,20 +575,42 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
>  				     &uaudio_qdev->xfer_ring_iova_size,
>  				     &uaudio_qdev->xfer_ring_list, size);
>  		break;
> -	case MEM_XFER_BUF:
> -		iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
> -				     &uaudio_qdev->xfer_buf_iova_size,
> -				     &uaudio_qdev->xfer_buf_list, size);
> -		break;
>  	default:
>  		dev_err(uaudio_qdev->data->dev, "unknown mem type %d\n", mtype);
>  	}
>  
>  	if (!iova || !map)
> -		goto done;
> +		return 0;
>  
> -	if (!sgt)
> -		goto skip_sgt_map;
> +	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
> +
> +done:
> +	return iova;
> +}
> +
> +static unsigned long uaudio_iommu_map_xfer_buf(bool dma_coherent,
> +				      size_t size, struct sg_table *sgt)
> +{
> +	struct scatterlist *sg;
> +	unsigned long iova = 0;
> +	size_t total_len = 0;
> +	unsigned long iova_sg;
> +	phys_addr_t pa_sg;
> +	size_t sg_len;
> +	int prot;
> +	int ret;
> +	int i;
> +
> +	prot = IOMMU_READ | IOMMU_WRITE;
> +
> +	if (dma_coherent)
> +		prot |= IOMMU_CACHE;
> +
> +	iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
> +			     &uaudio_qdev->xfer_buf_iova_size,
> +			     &uaudio_qdev->xfer_buf_list, size);
> +	if (!iova)
> +		goto done;
>  
>  	iova_sg = iova;
>  	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
> @@ -618,11 +632,6 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
>  		uaudio_iommu_unmap(MEM_XFER_BUF, iova, size, total_len);
>  		iova = 0;
>  	}
> -	return iova;
> -
> -skip_sgt_map:
> -	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
> -
>  done:
>  	return iova;
>  }
> @@ -1061,8 +1070,8 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
>  			xfer_buf_dma, len);
>  
>  	/* map the physical buffer into sysdev as well */
> -	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> -					       xfer_buf_pa, len, &xfer_buf_sgt);
> +	xfer_buf_dma_sysdev = uaudio_iommu_map_xfer_buf(dma_coherent, xfer_buf_pa,
> +							len, &xfer_buf_sgt);
>  	if (!xfer_buf_dma_sysdev) {
>  		ret = -ENOMEM;
>  		goto unmap_sync;
> @@ -1143,8 +1152,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
>  	sg_free_table(sgt);
>  
>  	/* data transfer ring */
> -	iova = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_pa,
> -			      PAGE_SIZE, NULL);
> +	iova = uaudio_iommu_map_pa(MEM_XFER_RING, dma_coherent, tr_pa,
> +				   PAGE_SIZE);
>  	if (!iova) {
>  		ret = -ENOMEM;
>  		goto clear_pa;
> @@ -1207,8 +1216,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
>  	mem_info->dma = sg_dma_address(sgt->sgl);
>  	sg_free_table(sgt);
>  
> -	iova = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, er_pa,
> -			      PAGE_SIZE, NULL);
> +	iova = uaudio_iommu_map_pa(MEM_EVENT_RING, dma_coherent, er_pa,
> +				   PAGE_SIZE);
>  	if (!iova) {
>  		ret = -ENOMEM;
>  		goto clear_pa;

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Luca Weiss]

Hi Arnd,

On Fri Sep 5, 2025 at 4:54 PM CEST, Arnd Bergmann wrote:
> On Fri, Sep 5, 2025, at 14:26, Takashi Iwai wrote:
>> On Fri, 05 Sep 2025 13:47:28 +0200,
>
>> @@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct 
>> snd_usb_substream *subs,
>>  	if (!xfer_buf)
>>  		return -ENOMEM;
>> 
>> -	/* Remapping is not possible if xfer_buf is outside of linear map */
>> -	xfer_buf_pa = virt_to_phys(xfer_buf);
>> -	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
>> -		ret = -ENXIO;
>> -		goto unmap_sync;
>> -	}
>>  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
>>  			xfer_buf_dma, len);
>> 
>>  	/* map the physical buffer into sysdev as well */
>> +	/* note: 0 is passed to pa argument as we use sgt */
>>  	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
>> -					       xfer_buf_pa, len, &xfer_buf_sgt);
>> +					       0, len, &xfer_buf_sgt);
>>  	if (!xfer_buf_dma_sysdev) {
>>  		ret = -ENOMEM;
>>  		goto unmap_sync;
>
>
> Makes sense. I had to rework the code a little more to actually
> understand how it fits together, for reference the below version
> (I don't expect it to build cleanly) would split up
> uaudio_iommu_map() into one function that takes a physical
> address and another function that takes an sg table.

Are you planning to post this as a proper patch? It's a bit late in the
6.17 cycle already but good to still get this fixed for final release.

Or revert the original commit that broke it for now.

I couldn't really test your patch since there's a couple of compile
errors where I wasn't sure how to resolve them correctly.

Regards
Luca

>
>        Arnd
>
> diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
> index a25c5a531690..f843c2181da5 100644
> --- a/sound/usb/qcom/qc_audio_offload.c
> +++ b/sound/usb/qcom/qc_audio_offload.c
> @@ -539,32 +539,24 @@ static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long iova,
>  }
>  
>  /**
> - * uaudio_iommu_map() - maps iommu memory for adsp
> + * uaudio_iommu_map_pa() - maps iommu memory for adsp
>   * @mtype: ring type
>   * @dma_coherent: dma coherent
>   * @pa: physical address for ring/buffer
>   * @size: size of memory region
> - * @sgt: sg table for memory region
>   *
>   * Maps the XHCI related resources to a memory region that is assigned to be
>   * used by the adsp.  This will be mapped to the domain, which is created by
>   * the ASoC USB backend driver.
>   *
>   */
> -static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
> -				      phys_addr_t pa, size_t size,
> -				      struct sg_table *sgt)
> +static unsigned long uaudio_iommu_map_pa(enum mem_type mtype, bool dma_coherent,
> +				      phys_addr_t pa, size_t size)
>  {
>  	struct scatterlist *sg;
>  	unsigned long iova = 0;
> -	size_t total_len = 0;
> -	unsigned long iova_sg;
> -	phys_addr_t pa_sg;
>  	bool map = true;
> -	size_t sg_len;
>  	int prot;
> -	int ret;
> -	int i;
>  
>  	prot = IOMMU_READ | IOMMU_WRITE;
>  
> @@ -583,20 +575,42 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
>  				     &uaudio_qdev->xfer_ring_iova_size,
>  				     &uaudio_qdev->xfer_ring_list, size);
>  		break;
> -	case MEM_XFER_BUF:
> -		iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
> -				     &uaudio_qdev->xfer_buf_iova_size,
> -				     &uaudio_qdev->xfer_buf_list, size);
> -		break;
>  	default:
>  		dev_err(uaudio_qdev->data->dev, "unknown mem type %d\n", mtype);
>  	}
>  
>  	if (!iova || !map)
> -		goto done;
> +		return 0;
>  
> -	if (!sgt)
> -		goto skip_sgt_map;
> +	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
> +
> +done:
> +	return iova;
> +}
> +
> +static unsigned long uaudio_iommu_map_xfer_buf(bool dma_coherent,
> +				      size_t size, struct sg_table *sgt)
> +{
> +	struct scatterlist *sg;
> +	unsigned long iova = 0;
> +	size_t total_len = 0;
> +	unsigned long iova_sg;
> +	phys_addr_t pa_sg;
> +	size_t sg_len;
> +	int prot;
> +	int ret;
> +	int i;
> +
> +	prot = IOMMU_READ | IOMMU_WRITE;
> +
> +	if (dma_coherent)
> +		prot |= IOMMU_CACHE;
> +
> +	iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
> +			     &uaudio_qdev->xfer_buf_iova_size,
> +			     &uaudio_qdev->xfer_buf_list, size);
> +	if (!iova)
> +		goto done;
>  
>  	iova_sg = iova;
>  	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
> @@ -618,11 +632,6 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
>  		uaudio_iommu_unmap(MEM_XFER_BUF, iova, size, total_len);
>  		iova = 0;
>  	}
> -	return iova;
> -
> -skip_sgt_map:
> -	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
> -
>  done:
>  	return iova;
>  }
> @@ -1061,8 +1070,8 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
>  			xfer_buf_dma, len);
>  
>  	/* map the physical buffer into sysdev as well */
> -	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> -					       xfer_buf_pa, len, &xfer_buf_sgt);
> +	xfer_buf_dma_sysdev = uaudio_iommu_map_xfer_buf(dma_coherent, xfer_buf_pa,
> +							len, &xfer_buf_sgt);
>  	if (!xfer_buf_dma_sysdev) {
>  		ret = -ENOMEM;
>  		goto unmap_sync;
> @@ -1143,8 +1152,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
>  	sg_free_table(sgt);
>  
>  	/* data transfer ring */
> -	iova = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_pa,
> -			      PAGE_SIZE, NULL);
> +	iova = uaudio_iommu_map_pa(MEM_XFER_RING, dma_coherent, tr_pa,
> +				   PAGE_SIZE);
>  	if (!iova) {
>  		ret = -ENOMEM;
>  		goto clear_pa;
> @@ -1207,8 +1216,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
>  	mem_info->dma = sg_dma_address(sgt->sgl);
>  	sg_free_table(sgt);
>  
> -	iova = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, er_pa,
> -			      PAGE_SIZE, NULL);
> +	iova = uaudio_iommu_map_pa(MEM_EVENT_RING, dma_coherent, er_pa,
> +				   PAGE_SIZE);
>  	if (!iova) {
>  		ret = -ENOMEM;
>  		goto clear_pa;

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Takashi Iwai]

On Tue, 16 Sep 2025 10:41:01 +0200,
Luca Weiss wrote:
> 
> Hi Arnd,
> 
> On Fri Sep 5, 2025 at 4:54 PM CEST, Arnd Bergmann wrote:
> > On Fri, Sep 5, 2025, at 14:26, Takashi Iwai wrote:
> >> On Fri, 05 Sep 2025 13:47:28 +0200,
> >
> >> @@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct 
> >> snd_usb_substream *subs,
> >>  	if (!xfer_buf)
> >>  		return -ENOMEM;
> >> 
> >> -	/* Remapping is not possible if xfer_buf is outside of linear map */
> >> -	xfer_buf_pa = virt_to_phys(xfer_buf);
> >> -	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> >> -		ret = -ENXIO;
> >> -		goto unmap_sync;
> >> -	}
> >>  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
> >>  			xfer_buf_dma, len);
> >> 
> >>  	/* map the physical buffer into sysdev as well */
> >> +	/* note: 0 is passed to pa argument as we use sgt */
> >>  	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> >> -					       xfer_buf_pa, len, &xfer_buf_sgt);
> >> +					       0, len, &xfer_buf_sgt);
> >>  	if (!xfer_buf_dma_sysdev) {
> >>  		ret = -ENOMEM;
> >>  		goto unmap_sync;
> >
> >
> > Makes sense. I had to rework the code a little more to actually
> > understand how it fits together, for reference the below version
> > (I don't expect it to build cleanly) would split up
> > uaudio_iommu_map() into one function that takes a physical
> > address and another function that takes an sg table.
> 
> Are you planning to post this as a proper patch? It's a bit late in the
> 6.17 cycle already but good to still get this fixed for final release.
> 
> Or revert the original commit that broke it for now.
> 
> I couldn't really test your patch since there's a couple of compile
> errors where I wasn't sure how to resolve them correctly.

Could you check the patch below, then?  At least it compiles without
errors.


Takashi

--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -538,38 +538,33 @@ static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long iova,
 			umap_size, iova, mapped_iova_size);
 }
 
+static int uaudio_iommu_map_prot(bool dma_coherent)
+{
+	int prot = IOMMU_READ | IOMMU_WRITE;
+
+	if (dma_coherent)
+		prot |= IOMMU_CACHE;
+	return prot;
+}
+
 /**
- * uaudio_iommu_map() - maps iommu memory for adsp
+ * uaudio_iommu_map_pa() - maps iommu memory for adsp
  * @mtype: ring type
  * @dma_coherent: dma coherent
  * @pa: physical address for ring/buffer
  * @size: size of memory region
- * @sgt: sg table for memory region
  *
  * Maps the XHCI related resources to a memory region that is assigned to be
  * used by the adsp.  This will be mapped to the domain, which is created by
  * the ASoC USB backend driver.
  *
  */
-static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
-				      phys_addr_t pa, size_t size,
-				      struct sg_table *sgt)
+static unsigned long uaudio_iommu_map_pa(enum mem_type mtype, bool dma_coherent,
+					 phys_addr_t pa, size_t size)
 {
-	struct scatterlist *sg;
 	unsigned long iova = 0;
-	size_t total_len = 0;
-	unsigned long iova_sg;
-	phys_addr_t pa_sg;
 	bool map = true;
-	size_t sg_len;
-	int prot;
-	int ret;
-	int i;
-
-	prot = IOMMU_READ | IOMMU_WRITE;
-
-	if (dma_coherent)
-		prot |= IOMMU_CACHE;
+	int prot = uaudio_iommu_map_prot(dma_coherent);
 
 	switch (mtype) {
 	case MEM_EVENT_RING:
@@ -583,20 +578,41 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
 				     &uaudio_qdev->xfer_ring_iova_size,
 				     &uaudio_qdev->xfer_ring_list, size);
 		break;
-	case MEM_XFER_BUF:
-		iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
-				     &uaudio_qdev->xfer_buf_iova_size,
-				     &uaudio_qdev->xfer_buf_list, size);
-		break;
 	default:
 		dev_err(uaudio_qdev->data->dev, "unknown mem type %d\n", mtype);
 	}
 
 	if (!iova || !map)
-		goto done;
+		return 0;
 
-	if (!sgt)
-		goto skip_sgt_map;
+	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
+
+	return iova;
+}
+
+static unsigned long uaudio_iommu_map_xfer_buf(bool dma_coherent, size_t size,
+					       struct sg_table *sgt)
+{
+	struct scatterlist *sg;
+	unsigned long iova = 0;
+	size_t total_len = 0;
+	unsigned long iova_sg;
+	phys_addr_t pa_sg;
+	size_t sg_len;
+	int prot = uaudio_iommu_map_prot(dma_coherent);
+	int ret;
+	int i;
+
+	prot = IOMMU_READ | IOMMU_WRITE;
+
+	if (dma_coherent)
+		prot |= IOMMU_CACHE;
+
+	iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
+			       &uaudio_qdev->xfer_buf_iova_size,
+			       &uaudio_qdev->xfer_buf_list, size);
+	if (!iova)
+		goto done;
 
 	iova_sg = iova;
 	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
@@ -618,11 +634,6 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
 		uaudio_iommu_unmap(MEM_XFER_BUF, iova, size, total_len);
 		iova = 0;
 	}
-	return iova;
-
-skip_sgt_map:
-	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
-
 done:
 	return iova;
 }
@@ -1061,8 +1072,8 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 			xfer_buf_dma, len);
 
 	/* map the physical buffer into sysdev as well */
-	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
-					       xfer_buf_pa, len, &xfer_buf_sgt);
+	xfer_buf_dma_sysdev = uaudio_iommu_map_xfer_buf(dma_coherent,
+							len, &xfer_buf_sgt);
 	if (!xfer_buf_dma_sysdev) {
 		ret = -ENOMEM;
 		goto unmap_sync;
@@ -1143,8 +1154,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
 	sg_free_table(sgt);
 
 	/* data transfer ring */
-	iova = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_pa,
-			      PAGE_SIZE, NULL);
+	iova = uaudio_iommu_map_pa(MEM_XFER_RING, dma_coherent, tr_pa,
+				   PAGE_SIZE);
 	if (!iova) {
 		ret = -ENOMEM;
 		goto clear_pa;
@@ -1207,8 +1218,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
 	mem_info->dma = sg_dma_address(sgt->sgl);
 	sg_free_table(sgt);
 
-	iova = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, er_pa,
-			      PAGE_SIZE, NULL);
+	iova = uaudio_iommu_map_pa(MEM_EVENT_RING, dma_coherent, er_pa,
+				   PAGE_SIZE);
 	if (!iova) {
 		ret = -ENOMEM;
 		goto clear_pa;

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Luca Weiss]

Hi Takashi,

On Tue Sep 16, 2025 at 6:09 PM CEST, Takashi Iwai wrote:
> On Tue, 16 Sep 2025 10:41:01 +0200,
> Luca Weiss wrote:
>> 
>> Hi Arnd,
>> 
>> On Fri Sep 5, 2025 at 4:54 PM CEST, Arnd Bergmann wrote:
>> > On Fri, Sep 5, 2025, at 14:26, Takashi Iwai wrote:
>> >> On Fri, 05 Sep 2025 13:47:28 +0200,
>> >
>> >> @@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct 
>> >> snd_usb_substream *subs,
>> >>  	if (!xfer_buf)
>> >>  		return -ENOMEM;
>> >> 
>> >> -	/* Remapping is not possible if xfer_buf is outside of linear map */
>> >> -	xfer_buf_pa = virt_to_phys(xfer_buf);
>> >> -	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
>> >> -		ret = -ENXIO;
>> >> -		goto unmap_sync;
>> >> -	}
>> >>  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
>> >>  			xfer_buf_dma, len);
>> >> 
>> >>  	/* map the physical buffer into sysdev as well */
>> >> +	/* note: 0 is passed to pa argument as we use sgt */
>> >>  	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
>> >> -					       xfer_buf_pa, len, &xfer_buf_sgt);
>> >> +					       0, len, &xfer_buf_sgt);
>> >>  	if (!xfer_buf_dma_sysdev) {
>> >>  		ret = -ENOMEM;
>> >>  		goto unmap_sync;
>> >
>> >
>> > Makes sense. I had to rework the code a little more to actually
>> > understand how it fits together, for reference the below version
>> > (I don't expect it to build cleanly) would split up
>> > uaudio_iommu_map() into one function that takes a physical
>> > address and another function that takes an sg table.
>> 
>> Are you planning to post this as a proper patch? It's a bit late in the
>> 6.17 cycle already but good to still get this fixed for final release.
>> 
>> Or revert the original commit that broke it for now.
>> 
>> I couldn't really test your patch since there's a couple of compile
>> errors where I wasn't sure how to resolve them correctly.
>
> Could you check the patch below, then?  At least it compiles without
> errors.

It does compile as well for me, but looks like it's not working.

It's still triggering the WARN_ON from

  if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {

[  214.157471] ------------[ cut here ]------------
[  214.157491] WARNING: CPU: 4 PID: 12 at sound/usb/qcom/qc_audio_offload.c:1067 handle_uaudio_stream_req+0xecc/0x13c4
[  214.157510] Modules linked in:
[  214.157522] CPU: 4 UID: 0 PID: 12 Comm: kworker/u32:0 Tainted: G        W           6.16.0-00047-gfa3c1e37ba38 #1 NONE 
[  214.157531] Tainted: [W]=WARN
[  214.157535] Hardware name: Fairphone 4 (DT)
[  214.157541] Workqueue: qmi_msg_handler qmi_data_ready_work
[  214.157553] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  214.157560] pc : handle_uaudio_stream_req+0xecc/0x13c4
[  214.157568] lr : handle_uaudio_stream_req+0xcdc/0x13c4
[  214.157575] sp : ffff8000800b39d0
[  214.157579] x29: ffff8000800b3b40 x28: ffff0000895eb6b0 x27: 0000000000008000
[  214.157589] x26: ffff0000d6bae960 x25: ffffa11dbe275e28 x24: 0000000000008000
[  214.157598] x23: ffffa11dbe0a4ec0 x22: ffff8000800b3c00 x21: 0000000000000000
[  214.157608] x20: ffff8000800b3cc8 x19: ffff00008b128ac0 x18: ffffa11dbdfaa258
[  214.157617] x17: ffff0000803e9388 x16: ffff0000800162c0 x15: ffffa11dbdfaa000
[  214.157626] x14: 0000000000000500 x13: ffffa11dbe03c000 x12: 0000000000000000
[  214.157636] x11: 0000000000000001 x10: 0000000000000001 x9 : ffffa11dbe0f78a0
[  214.157645] x8 : ffffdee36909b000 x7 : ffff0000d6ac8418 x6 : 0000000000000000
[  214.157654] x5 : 0000000000000000 x4 : ffff8000800b3968 x3 : 0000000000000000
[  214.157663] x2 : 0000000000000000 x1 : ffff0000801a4100 x0 : 0000000000000000
[  214.157672] Call trace:
[  214.157677]  handle_uaudio_stream_req+0xecc/0x13c4 (P)
[  214.157687]  qmi_invoke_handler+0xb4/0x100
[  214.157694]  qmi_handle_message+0x88/0x1a0
[  214.157702]  qmi_data_ready_work+0x208/0x35c
[  214.157709]  process_one_work+0x144/0x2c4
[  214.157719]  worker_thread+0x280/0x45c
[  214.157726]  kthread+0xfc/0x1dc
[  214.157733]  ret_from_fork+0x10/0x20
[  214.157744] ---[ end trace 0000000000000000 ]---

Should that code be removed with the new code now?

Regards
Luca

>
>
> Takashi
>
> --- a/sound/usb/qcom/qc_audio_offload.c
> +++ b/sound/usb/qcom/qc_audio_offload.c
> @@ -538,38 +538,33 @@ static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long iova,
>  			umap_size, iova, mapped_iova_size);
>  }
>  
> +static int uaudio_iommu_map_prot(bool dma_coherent)
> +{
> +	int prot = IOMMU_READ | IOMMU_WRITE;
> +
> +	if (dma_coherent)
> +		prot |= IOMMU_CACHE;
> +	return prot;
> +}
> +
>  /**
> - * uaudio_iommu_map() - maps iommu memory for adsp
> + * uaudio_iommu_map_pa() - maps iommu memory for adsp
>   * @mtype: ring type
>   * @dma_coherent: dma coherent
>   * @pa: physical address for ring/buffer
>   * @size: size of memory region
> - * @sgt: sg table for memory region
>   *
>   * Maps the XHCI related resources to a memory region that is assigned to be
>   * used by the adsp.  This will be mapped to the domain, which is created by
>   * the ASoC USB backend driver.
>   *
>   */
> -static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
> -				      phys_addr_t pa, size_t size,
> -				      struct sg_table *sgt)
> +static unsigned long uaudio_iommu_map_pa(enum mem_type mtype, bool dma_coherent,
> +					 phys_addr_t pa, size_t size)
>  {
> -	struct scatterlist *sg;
>  	unsigned long iova = 0;
> -	size_t total_len = 0;
> -	unsigned long iova_sg;
> -	phys_addr_t pa_sg;
>  	bool map = true;
> -	size_t sg_len;
> -	int prot;
> -	int ret;
> -	int i;
> -
> -	prot = IOMMU_READ | IOMMU_WRITE;
> -
> -	if (dma_coherent)
> -		prot |= IOMMU_CACHE;
> +	int prot = uaudio_iommu_map_prot(dma_coherent);
>  
>  	switch (mtype) {
>  	case MEM_EVENT_RING:
> @@ -583,20 +578,41 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
>  				     &uaudio_qdev->xfer_ring_iova_size,
>  				     &uaudio_qdev->xfer_ring_list, size);
>  		break;
> -	case MEM_XFER_BUF:
> -		iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
> -				     &uaudio_qdev->xfer_buf_iova_size,
> -				     &uaudio_qdev->xfer_buf_list, size);
> -		break;
>  	default:
>  		dev_err(uaudio_qdev->data->dev, "unknown mem type %d\n", mtype);
>  	}
>  
>  	if (!iova || !map)
> -		goto done;
> +		return 0;
>  
> -	if (!sgt)
> -		goto skip_sgt_map;
> +	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
> +
> +	return iova;
> +}
> +
> +static unsigned long uaudio_iommu_map_xfer_buf(bool dma_coherent, size_t size,
> +					       struct sg_table *sgt)
> +{
> +	struct scatterlist *sg;
> +	unsigned long iova = 0;
> +	size_t total_len = 0;
> +	unsigned long iova_sg;
> +	phys_addr_t pa_sg;
> +	size_t sg_len;
> +	int prot = uaudio_iommu_map_prot(dma_coherent);
> +	int ret;
> +	int i;
> +
> +	prot = IOMMU_READ | IOMMU_WRITE;
> +
> +	if (dma_coherent)
> +		prot |= IOMMU_CACHE;
> +
> +	iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
> +			       &uaudio_qdev->xfer_buf_iova_size,
> +			       &uaudio_qdev->xfer_buf_list, size);
> +	if (!iova)
> +		goto done;
>  
>  	iova_sg = iova;
>  	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
> @@ -618,11 +634,6 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
>  		uaudio_iommu_unmap(MEM_XFER_BUF, iova, size, total_len);
>  		iova = 0;
>  	}
> -	return iova;
> -
> -skip_sgt_map:
> -	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
> -
>  done:
>  	return iova;
>  }
> @@ -1061,8 +1072,8 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
>  			xfer_buf_dma, len);
>  
>  	/* map the physical buffer into sysdev as well */
> -	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> -					       xfer_buf_pa, len, &xfer_buf_sgt);
> +	xfer_buf_dma_sysdev = uaudio_iommu_map_xfer_buf(dma_coherent,
> +							len, &xfer_buf_sgt);
>  	if (!xfer_buf_dma_sysdev) {
>  		ret = -ENOMEM;
>  		goto unmap_sync;
> @@ -1143,8 +1154,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
>  	sg_free_table(sgt);
>  
>  	/* data transfer ring */
> -	iova = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_pa,
> -			      PAGE_SIZE, NULL);
> +	iova = uaudio_iommu_map_pa(MEM_XFER_RING, dma_coherent, tr_pa,
> +				   PAGE_SIZE);
>  	if (!iova) {
>  		ret = -ENOMEM;
>  		goto clear_pa;
> @@ -1207,8 +1218,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
>  	mem_info->dma = sg_dma_address(sgt->sgl);
>  	sg_free_table(sgt);
>  
> -	iova = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, er_pa,
> -			      PAGE_SIZE, NULL);
> +	iova = uaudio_iommu_map_pa(MEM_EVENT_RING, dma_coherent, er_pa,
> +				   PAGE_SIZE);
>  	if (!iova) {
>  		ret = -ENOMEM;
>  		goto clear_pa;

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Takashi Iwai]

On Wed, 17 Sep 2025 10:19:23 +0200,
Luca Weiss wrote:
> 
> Hi Takashi,
> 
> On Tue Sep 16, 2025 at 6:09 PM CEST, Takashi Iwai wrote:
> > On Tue, 16 Sep 2025 10:41:01 +0200,
> > Luca Weiss wrote:
> >> 
> >> Hi Arnd,
> >> 
> >> On Fri Sep 5, 2025 at 4:54 PM CEST, Arnd Bergmann wrote:
> >> > On Fri, Sep 5, 2025, at 14:26, Takashi Iwai wrote:
> >> >> On Fri, 05 Sep 2025 13:47:28 +0200,
> >> >
> >> >> @@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct 
> >> >> snd_usb_substream *subs,
> >> >>  	if (!xfer_buf)
> >> >>  		return -ENOMEM;
> >> >> 
> >> >> -	/* Remapping is not possible if xfer_buf is outside of linear map */
> >> >> -	xfer_buf_pa = virt_to_phys(xfer_buf);
> >> >> -	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> >> >> -		ret = -ENXIO;
> >> >> -		goto unmap_sync;
> >> >> -	}
> >> >>  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
> >> >>  			xfer_buf_dma, len);
> >> >> 
> >> >>  	/* map the physical buffer into sysdev as well */
> >> >> +	/* note: 0 is passed to pa argument as we use sgt */
> >> >>  	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> >> >> -					       xfer_buf_pa, len, &xfer_buf_sgt);
> >> >> +					       0, len, &xfer_buf_sgt);
> >> >>  	if (!xfer_buf_dma_sysdev) {
> >> >>  		ret = -ENOMEM;
> >> >>  		goto unmap_sync;
> >> >
> >> >
> >> > Makes sense. I had to rework the code a little more to actually
> >> > understand how it fits together, for reference the below version
> >> > (I don't expect it to build cleanly) would split up
> >> > uaudio_iommu_map() into one function that takes a physical
> >> > address and another function that takes an sg table.
> >> 
> >> Are you planning to post this as a proper patch? It's a bit late in the
> >> 6.17 cycle already but good to still get this fixed for final release.
> >> 
> >> Or revert the original commit that broke it for now.
> >> 
> >> I couldn't really test your patch since there's a couple of compile
> >> errors where I wasn't sure how to resolve them correctly.
> >
> > Could you check the patch below, then?  At least it compiles without
> > errors.
> 
> It does compile as well for me, but looks like it's not working.
> 
> It's still triggering the WARN_ON from
> 
>   if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> 
> [  214.157471] ------------[ cut here ]------------
> [  214.157491] WARNING: CPU: 4 PID: 12 at sound/usb/qcom/qc_audio_offload.c:1067 handle_uaudio_stream_req+0xecc/0x13c4
> [  214.157510] Modules linked in:
> [  214.157522] CPU: 4 UID: 0 PID: 12 Comm: kworker/u32:0 Tainted: G        W           6.16.0-00047-gfa3c1e37ba38 #1 NONE 
> [  214.157531] Tainted: [W]=WARN
> [  214.157535] Hardware name: Fairphone 4 (DT)
> [  214.157541] Workqueue: qmi_msg_handler qmi_data_ready_work
> [  214.157553] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  214.157560] pc : handle_uaudio_stream_req+0xecc/0x13c4
> [  214.157568] lr : handle_uaudio_stream_req+0xcdc/0x13c4
> [  214.157575] sp : ffff8000800b39d0
> [  214.157579] x29: ffff8000800b3b40 x28: ffff0000895eb6b0 x27: 0000000000008000
> [  214.157589] x26: ffff0000d6bae960 x25: ffffa11dbe275e28 x24: 0000000000008000
> [  214.157598] x23: ffffa11dbe0a4ec0 x22: ffff8000800b3c00 x21: 0000000000000000
> [  214.157608] x20: ffff8000800b3cc8 x19: ffff00008b128ac0 x18: ffffa11dbdfaa258
> [  214.157617] x17: ffff0000803e9388 x16: ffff0000800162c0 x15: ffffa11dbdfaa000
> [  214.157626] x14: 0000000000000500 x13: ffffa11dbe03c000 x12: 0000000000000000
> [  214.157636] x11: 0000000000000001 x10: 0000000000000001 x9 : ffffa11dbe0f78a0
> [  214.157645] x8 : ffffdee36909b000 x7 : ffff0000d6ac8418 x6 : 0000000000000000
> [  214.157654] x5 : 0000000000000000 x4 : ffff8000800b3968 x3 : 0000000000000000
> [  214.157663] x2 : 0000000000000000 x1 : ffff0000801a4100 x0 : 0000000000000000
> [  214.157672] Call trace:
> [  214.157677]  handle_uaudio_stream_req+0xecc/0x13c4 (P)
> [  214.157687]  qmi_invoke_handler+0xb4/0x100
> [  214.157694]  qmi_handle_message+0x88/0x1a0
> [  214.157702]  qmi_data_ready_work+0x208/0x35c
> [  214.157709]  process_one_work+0x144/0x2c4
> [  214.157719]  worker_thread+0x280/0x45c
> [  214.157726]  kthread+0xfc/0x1dc
> [  214.157733]  ret_from_fork+0x10/0x20
> [  214.157744] ---[ end trace 0000000000000000 ]---
> 
> Should that code be removed with the new code now?

Yes, please try the revised patch below.


thanks,

Takashi

--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -538,38 +538,33 @@ static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long iova,
 			umap_size, iova, mapped_iova_size);
 }
 
+static int uaudio_iommu_map_prot(bool dma_coherent)
+{
+	int prot = IOMMU_READ | IOMMU_WRITE;
+
+	if (dma_coherent)
+		prot |= IOMMU_CACHE;
+	return prot;
+}
+
 /**
- * uaudio_iommu_map() - maps iommu memory for adsp
+ * uaudio_iommu_map_pa() - maps iommu memory for adsp
  * @mtype: ring type
  * @dma_coherent: dma coherent
  * @pa: physical address for ring/buffer
  * @size: size of memory region
- * @sgt: sg table for memory region
  *
  * Maps the XHCI related resources to a memory region that is assigned to be
  * used by the adsp.  This will be mapped to the domain, which is created by
  * the ASoC USB backend driver.
  *
  */
-static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
-				      phys_addr_t pa, size_t size,
-				      struct sg_table *sgt)
+static unsigned long uaudio_iommu_map_pa(enum mem_type mtype, bool dma_coherent,
+					 phys_addr_t pa, size_t size)
 {
-	struct scatterlist *sg;
 	unsigned long iova = 0;
-	size_t total_len = 0;
-	unsigned long iova_sg;
-	phys_addr_t pa_sg;
 	bool map = true;
-	size_t sg_len;
-	int prot;
-	int ret;
-	int i;
-
-	prot = IOMMU_READ | IOMMU_WRITE;
-
-	if (dma_coherent)
-		prot |= IOMMU_CACHE;
+	int prot = uaudio_iommu_map_prot(dma_coherent);
 
 	switch (mtype) {
 	case MEM_EVENT_RING:
@@ -583,20 +578,41 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
 				     &uaudio_qdev->xfer_ring_iova_size,
 				     &uaudio_qdev->xfer_ring_list, size);
 		break;
-	case MEM_XFER_BUF:
-		iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
-				     &uaudio_qdev->xfer_buf_iova_size,
-				     &uaudio_qdev->xfer_buf_list, size);
-		break;
 	default:
 		dev_err(uaudio_qdev->data->dev, "unknown mem type %d\n", mtype);
 	}
 
 	if (!iova || !map)
-		goto done;
+		return 0;
 
-	if (!sgt)
-		goto skip_sgt_map;
+	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
+
+	return iova;
+}
+
+static unsigned long uaudio_iommu_map_xfer_buf(bool dma_coherent, size_t size,
+					       struct sg_table *sgt)
+{
+	struct scatterlist *sg;
+	unsigned long iova = 0;
+	size_t total_len = 0;
+	unsigned long iova_sg;
+	phys_addr_t pa_sg;
+	size_t sg_len;
+	int prot = uaudio_iommu_map_prot(dma_coherent);
+	int ret;
+	int i;
+
+	prot = IOMMU_READ | IOMMU_WRITE;
+
+	if (dma_coherent)
+		prot |= IOMMU_CACHE;
+
+	iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
+			       &uaudio_qdev->xfer_buf_iova_size,
+			       &uaudio_qdev->xfer_buf_list, size);
+	if (!iova)
+		goto done;
 
 	iova_sg = iova;
 	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
@@ -618,11 +634,6 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
 		uaudio_iommu_unmap(MEM_XFER_BUF, iova, size, total_len);
 		iova = 0;
 	}
-	return iova;
-
-skip_sgt_map:
-	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
-
 done:
 	return iova;
 }
@@ -1020,7 +1031,6 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 	struct sg_table xfer_buf_sgt;
 	dma_addr_t xfer_buf_dma;
 	void *xfer_buf;
-	phys_addr_t xfer_buf_pa;
 	u32 len = xfer_buf_len;
 	bool dma_coherent;
 	dma_addr_t xfer_buf_dma_sysdev;
@@ -1051,18 +1061,12 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
 	if (!xfer_buf)
 		return -ENOMEM;
 
-	/* Remapping is not possible if xfer_buf is outside of linear map */
-	xfer_buf_pa = virt_to_phys(xfer_buf);
-	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
-		ret = -ENXIO;
-		goto unmap_sync;
-	}
 	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
 			xfer_buf_dma, len);
 
 	/* map the physical buffer into sysdev as well */
-	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
-					       xfer_buf_pa, len, &xfer_buf_sgt);
+	xfer_buf_dma_sysdev = uaudio_iommu_map_xfer_buf(dma_coherent,
+							len, &xfer_buf_sgt);
 	if (!xfer_buf_dma_sysdev) {
 		ret = -ENOMEM;
 		goto unmap_sync;
@@ -1143,8 +1147,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
 	sg_free_table(sgt);
 
 	/* data transfer ring */
-	iova = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_pa,
-			      PAGE_SIZE, NULL);
+	iova = uaudio_iommu_map_pa(MEM_XFER_RING, dma_coherent, tr_pa,
+				   PAGE_SIZE);
 	if (!iova) {
 		ret = -ENOMEM;
 		goto clear_pa;
@@ -1207,8 +1211,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
 	mem_info->dma = sg_dma_address(sgt->sgl);
 	sg_free_table(sgt);
 
-	iova = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, er_pa,
-			      PAGE_SIZE, NULL);
+	iova = uaudio_iommu_map_pa(MEM_EVENT_RING, dma_coherent, er_pa,
+				   PAGE_SIZE);
 	if (!iova) {
 		ret = -ENOMEM;
 		goto clear_pa;

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Luca Weiss]

Hi Takashi,

On Wed Sep 17, 2025 at 10:30 AM CEST, Takashi Iwai wrote:
> On Wed, 17 Sep 2025 10:19:23 +0200,
> Luca Weiss wrote:
>> 
>> Hi Takashi,
>> 
>> On Tue Sep 16, 2025 at 6:09 PM CEST, Takashi Iwai wrote:
>> > On Tue, 16 Sep 2025 10:41:01 +0200,
>> > Luca Weiss wrote:
>> >> 
>> >> Hi Arnd,
>> >> 
>> >> On Fri Sep 5, 2025 at 4:54 PM CEST, Arnd Bergmann wrote:
>> >> > On Fri, Sep 5, 2025, at 14:26, Takashi Iwai wrote:
>> >> >> On Fri, 05 Sep 2025 13:47:28 +0200,
>> >> >
>> >> >> @@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct 
>> >> >> snd_usb_substream *subs,
>> >> >>  	if (!xfer_buf)
>> >> >>  		return -ENOMEM;
>> >> >> 
>> >> >> -	/* Remapping is not possible if xfer_buf is outside of linear map */
>> >> >> -	xfer_buf_pa = virt_to_phys(xfer_buf);
>> >> >> -	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
>> >> >> -		ret = -ENXIO;
>> >> >> -		goto unmap_sync;
>> >> >> -	}
>> >> >>  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
>> >> >>  			xfer_buf_dma, len);
>> >> >> 
>> >> >>  	/* map the physical buffer into sysdev as well */
>> >> >> +	/* note: 0 is passed to pa argument as we use sgt */
>> >> >>  	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
>> >> >> -					       xfer_buf_pa, len, &xfer_buf_sgt);
>> >> >> +					       0, len, &xfer_buf_sgt);
>> >> >>  	if (!xfer_buf_dma_sysdev) {
>> >> >>  		ret = -ENOMEM;
>> >> >>  		goto unmap_sync;
>> >> >
>> >> >
>> >> > Makes sense. I had to rework the code a little more to actually
>> >> > understand how it fits together, for reference the below version
>> >> > (I don't expect it to build cleanly) would split up
>> >> > uaudio_iommu_map() into one function that takes a physical
>> >> > address and another function that takes an sg table.
>> >> 
>> >> Are you planning to post this as a proper patch? It's a bit late in the
>> >> 6.17 cycle already but good to still get this fixed for final release.
>> >> 
>> >> Or revert the original commit that broke it for now.
>> >> 
>> >> I couldn't really test your patch since there's a couple of compile
>> >> errors where I wasn't sure how to resolve them correctly.
>> >
>> > Could you check the patch below, then?  At least it compiles without
>> > errors.
>> 
>> It does compile as well for me, but looks like it's not working.
>> 
>> It's still triggering the WARN_ON from
>> 
>>   if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
>> 
>> [  214.157471] ------------[ cut here ]------------
>> [  214.157491] WARNING: CPU: 4 PID: 12 at sound/usb/qcom/qc_audio_offload.c:1067 handle_uaudio_stream_req+0xecc/0x13c4
>> [  214.157510] Modules linked in:
>> [  214.157522] CPU: 4 UID: 0 PID: 12 Comm: kworker/u32:0 Tainted: G        W           6.16.0-00047-gfa3c1e37ba38 #1 NONE 
>> [  214.157531] Tainted: [W]=WARN
>> [  214.157535] Hardware name: Fairphone 4 (DT)
>> [  214.157541] Workqueue: qmi_msg_handler qmi_data_ready_work
>> [  214.157553] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [  214.157560] pc : handle_uaudio_stream_req+0xecc/0x13c4
>> [  214.157568] lr : handle_uaudio_stream_req+0xcdc/0x13c4
>> [  214.157575] sp : ffff8000800b39d0
>> [  214.157579] x29: ffff8000800b3b40 x28: ffff0000895eb6b0 x27: 0000000000008000
>> [  214.157589] x26: ffff0000d6bae960 x25: ffffa11dbe275e28 x24: 0000000000008000
>> [  214.157598] x23: ffffa11dbe0a4ec0 x22: ffff8000800b3c00 x21: 0000000000000000
>> [  214.157608] x20: ffff8000800b3cc8 x19: ffff00008b128ac0 x18: ffffa11dbdfaa258
>> [  214.157617] x17: ffff0000803e9388 x16: ffff0000800162c0 x15: ffffa11dbdfaa000
>> [  214.157626] x14: 0000000000000500 x13: ffffa11dbe03c000 x12: 0000000000000000
>> [  214.157636] x11: 0000000000000001 x10: 0000000000000001 x9 : ffffa11dbe0f78a0
>> [  214.157645] x8 : ffffdee36909b000 x7 : ffff0000d6ac8418 x6 : 0000000000000000
>> [  214.157654] x5 : 0000000000000000 x4 : ffff8000800b3968 x3 : 0000000000000000
>> [  214.157663] x2 : 0000000000000000 x1 : ffff0000801a4100 x0 : 0000000000000000
>> [  214.157672] Call trace:
>> [  214.157677]  handle_uaudio_stream_req+0xecc/0x13c4 (P)
>> [  214.157687]  qmi_invoke_handler+0xb4/0x100
>> [  214.157694]  qmi_handle_message+0x88/0x1a0
>> [  214.157702]  qmi_data_ready_work+0x208/0x35c
>> [  214.157709]  process_one_work+0x144/0x2c4
>> [  214.157719]  worker_thread+0x280/0x45c
>> [  214.157726]  kthread+0xfc/0x1dc
>> [  214.157733]  ret_from_fork+0x10/0x20
>> [  214.157744] ---[ end trace 0000000000000000 ]---
>> 
>> Should that code be removed with the new code now?
>
> Yes, please try the revised patch below.

This is indeed now working for me, playback works again.

Let me know if I should do anything else for this.

Regards
Luca

>
>
> thanks,
>
> Takashi
>
> --- a/sound/usb/qcom/qc_audio_offload.c
> +++ b/sound/usb/qcom/qc_audio_offload.c
> @@ -538,38 +538,33 @@ static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long iova,
>  			umap_size, iova, mapped_iova_size);
>  }
>  
> +static int uaudio_iommu_map_prot(bool dma_coherent)
> +{
> +	int prot = IOMMU_READ | IOMMU_WRITE;
> +
> +	if (dma_coherent)
> +		prot |= IOMMU_CACHE;
> +	return prot;
> +}
> +
>  /**
> - * uaudio_iommu_map() - maps iommu memory for adsp
> + * uaudio_iommu_map_pa() - maps iommu memory for adsp
>   * @mtype: ring type
>   * @dma_coherent: dma coherent
>   * @pa: physical address for ring/buffer
>   * @size: size of memory region
> - * @sgt: sg table for memory region
>   *
>   * Maps the XHCI related resources to a memory region that is assigned to be
>   * used by the adsp.  This will be mapped to the domain, which is created by
>   * the ASoC USB backend driver.
>   *
>   */
> -static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
> -				      phys_addr_t pa, size_t size,
> -				      struct sg_table *sgt)
> +static unsigned long uaudio_iommu_map_pa(enum mem_type mtype, bool dma_coherent,
> +					 phys_addr_t pa, size_t size)
>  {
> -	struct scatterlist *sg;
>  	unsigned long iova = 0;
> -	size_t total_len = 0;
> -	unsigned long iova_sg;
> -	phys_addr_t pa_sg;
>  	bool map = true;
> -	size_t sg_len;
> -	int prot;
> -	int ret;
> -	int i;
> -
> -	prot = IOMMU_READ | IOMMU_WRITE;
> -
> -	if (dma_coherent)
> -		prot |= IOMMU_CACHE;
> +	int prot = uaudio_iommu_map_prot(dma_coherent);
>  
>  	switch (mtype) {
>  	case MEM_EVENT_RING:
> @@ -583,20 +578,41 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
>  				     &uaudio_qdev->xfer_ring_iova_size,
>  				     &uaudio_qdev->xfer_ring_list, size);
>  		break;
> -	case MEM_XFER_BUF:
> -		iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
> -				     &uaudio_qdev->xfer_buf_iova_size,
> -				     &uaudio_qdev->xfer_buf_list, size);
> -		break;
>  	default:
>  		dev_err(uaudio_qdev->data->dev, "unknown mem type %d\n", mtype);
>  	}
>  
>  	if (!iova || !map)
> -		goto done;
> +		return 0;
>  
> -	if (!sgt)
> -		goto skip_sgt_map;
> +	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
> +
> +	return iova;
> +}
> +
> +static unsigned long uaudio_iommu_map_xfer_buf(bool dma_coherent, size_t size,
> +					       struct sg_table *sgt)
> +{
> +	struct scatterlist *sg;
> +	unsigned long iova = 0;
> +	size_t total_len = 0;
> +	unsigned long iova_sg;
> +	phys_addr_t pa_sg;
> +	size_t sg_len;
> +	int prot = uaudio_iommu_map_prot(dma_coherent);
> +	int ret;
> +	int i;
> +
> +	prot = IOMMU_READ | IOMMU_WRITE;
> +
> +	if (dma_coherent)
> +		prot |= IOMMU_CACHE;
> +
> +	iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
> +			       &uaudio_qdev->xfer_buf_iova_size,
> +			       &uaudio_qdev->xfer_buf_list, size);
> +	if (!iova)
> +		goto done;
>  
>  	iova_sg = iova;
>  	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
> @@ -618,11 +634,6 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
>  		uaudio_iommu_unmap(MEM_XFER_BUF, iova, size, total_len);
>  		iova = 0;
>  	}
> -	return iova;
> -
> -skip_sgt_map:
> -	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
> -
>  done:
>  	return iova;
>  }
> @@ -1020,7 +1031,6 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
>  	struct sg_table xfer_buf_sgt;
>  	dma_addr_t xfer_buf_dma;
>  	void *xfer_buf;
> -	phys_addr_t xfer_buf_pa;
>  	u32 len = xfer_buf_len;
>  	bool dma_coherent;
>  	dma_addr_t xfer_buf_dma_sysdev;
> @@ -1051,18 +1061,12 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
>  	if (!xfer_buf)
>  		return -ENOMEM;
>  
> -	/* Remapping is not possible if xfer_buf is outside of linear map */
> -	xfer_buf_pa = virt_to_phys(xfer_buf);
> -	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> -		ret = -ENXIO;
> -		goto unmap_sync;
> -	}
>  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
>  			xfer_buf_dma, len);
>  
>  	/* map the physical buffer into sysdev as well */
> -	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> -					       xfer_buf_pa, len, &xfer_buf_sgt);
> +	xfer_buf_dma_sysdev = uaudio_iommu_map_xfer_buf(dma_coherent,
> +							len, &xfer_buf_sgt);
>  	if (!xfer_buf_dma_sysdev) {
>  		ret = -ENOMEM;
>  		goto unmap_sync;
> @@ -1143,8 +1147,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
>  	sg_free_table(sgt);
>  
>  	/* data transfer ring */
> -	iova = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_pa,
> -			      PAGE_SIZE, NULL);
> +	iova = uaudio_iommu_map_pa(MEM_XFER_RING, dma_coherent, tr_pa,
> +				   PAGE_SIZE);
>  	if (!iova) {
>  		ret = -ENOMEM;
>  		goto clear_pa;
> @@ -1207,8 +1211,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
>  	mem_info->dma = sg_dma_address(sgt->sgl);
>  	sg_free_table(sgt);
>  
> -	iova = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, er_pa,
> -			      PAGE_SIZE, NULL);
> +	iova = uaudio_iommu_map_pa(MEM_EVENT_RING, dma_coherent, er_pa,
> +				   PAGE_SIZE);
>  	if (!iova) {
>  		ret = -ENOMEM;
>  		goto clear_pa;

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Takashi Iwai]

On Wed, 17 Sep 2025 14:27:50 +0200,
Luca Weiss wrote:
> 
> Hi Takashi,
> 
> On Wed Sep 17, 2025 at 10:30 AM CEST, Takashi Iwai wrote:
> > On Wed, 17 Sep 2025 10:19:23 +0200,
> > Luca Weiss wrote:
> >> 
> >> Hi Takashi,
> >> 
> >> On Tue Sep 16, 2025 at 6:09 PM CEST, Takashi Iwai wrote:
> >> > On Tue, 16 Sep 2025 10:41:01 +0200,
> >> > Luca Weiss wrote:
> >> >> 
> >> >> Hi Arnd,
> >> >> 
> >> >> On Fri Sep 5, 2025 at 4:54 PM CEST, Arnd Bergmann wrote:
> >> >> > On Fri, Sep 5, 2025, at 14:26, Takashi Iwai wrote:
> >> >> >> On Fri, 05 Sep 2025 13:47:28 +0200,
> >> >> >
> >> >> >> @@ -1051,18 +1050,13 @@ static int uaudio_transfer_buffer_setup(struct 
> >> >> >> snd_usb_substream *subs,
> >> >> >>  	if (!xfer_buf)
> >> >> >>  		return -ENOMEM;
> >> >> >> 
> >> >> >> -	/* Remapping is not possible if xfer_buf is outside of linear map */
> >> >> >> -	xfer_buf_pa = virt_to_phys(xfer_buf);
> >> >> >> -	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> >> >> >> -		ret = -ENXIO;
> >> >> >> -		goto unmap_sync;
> >> >> >> -	}
> >> >> >>  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
> >> >> >>  			xfer_buf_dma, len);
> >> >> >> 
> >> >> >>  	/* map the physical buffer into sysdev as well */
> >> >> >> +	/* note: 0 is passed to pa argument as we use sgt */
> >> >> >>  	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> >> >> >> -					       xfer_buf_pa, len, &xfer_buf_sgt);
> >> >> >> +					       0, len, &xfer_buf_sgt);
> >> >> >>  	if (!xfer_buf_dma_sysdev) {
> >> >> >>  		ret = -ENOMEM;
> >> >> >>  		goto unmap_sync;
> >> >> >
> >> >> >
> >> >> > Makes sense. I had to rework the code a little more to actually
> >> >> > understand how it fits together, for reference the below version
> >> >> > (I don't expect it to build cleanly) would split up
> >> >> > uaudio_iommu_map() into one function that takes a physical
> >> >> > address and another function that takes an sg table.
> >> >> 
> >> >> Are you planning to post this as a proper patch? It's a bit late in the
> >> >> 6.17 cycle already but good to still get this fixed for final release.
> >> >> 
> >> >> Or revert the original commit that broke it for now.
> >> >> 
> >> >> I couldn't really test your patch since there's a couple of compile
> >> >> errors where I wasn't sure how to resolve them correctly.
> >> >
> >> > Could you check the patch below, then?  At least it compiles without
> >> > errors.
> >> 
> >> It does compile as well for me, but looks like it's not working.
> >> 
> >> It's still triggering the WARN_ON from
> >> 
> >>   if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> >> 
> >> [  214.157471] ------------[ cut here ]------------
> >> [  214.157491] WARNING: CPU: 4 PID: 12 at sound/usb/qcom/qc_audio_offload.c:1067 handle_uaudio_stream_req+0xecc/0x13c4
> >> [  214.157510] Modules linked in:
> >> [  214.157522] CPU: 4 UID: 0 PID: 12 Comm: kworker/u32:0 Tainted: G        W           6.16.0-00047-gfa3c1e37ba38 #1 NONE 
> >> [  214.157531] Tainted: [W]=WARN
> >> [  214.157535] Hardware name: Fairphone 4 (DT)
> >> [  214.157541] Workqueue: qmi_msg_handler qmi_data_ready_work
> >> [  214.157553] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >> [  214.157560] pc : handle_uaudio_stream_req+0xecc/0x13c4
> >> [  214.157568] lr : handle_uaudio_stream_req+0xcdc/0x13c4
> >> [  214.157575] sp : ffff8000800b39d0
> >> [  214.157579] x29: ffff8000800b3b40 x28: ffff0000895eb6b0 x27: 0000000000008000
> >> [  214.157589] x26: ffff0000d6bae960 x25: ffffa11dbe275e28 x24: 0000000000008000
> >> [  214.157598] x23: ffffa11dbe0a4ec0 x22: ffff8000800b3c00 x21: 0000000000000000
> >> [  214.157608] x20: ffff8000800b3cc8 x19: ffff00008b128ac0 x18: ffffa11dbdfaa258
> >> [  214.157617] x17: ffff0000803e9388 x16: ffff0000800162c0 x15: ffffa11dbdfaa000
> >> [  214.157626] x14: 0000000000000500 x13: ffffa11dbe03c000 x12: 0000000000000000
> >> [  214.157636] x11: 0000000000000001 x10: 0000000000000001 x9 : ffffa11dbe0f78a0
> >> [  214.157645] x8 : ffffdee36909b000 x7 : ffff0000d6ac8418 x6 : 0000000000000000
> >> [  214.157654] x5 : 0000000000000000 x4 : ffff8000800b3968 x3 : 0000000000000000
> >> [  214.157663] x2 : 0000000000000000 x1 : ffff0000801a4100 x0 : 0000000000000000
> >> [  214.157672] Call trace:
> >> [  214.157677]  handle_uaudio_stream_req+0xecc/0x13c4 (P)
> >> [  214.157687]  qmi_invoke_handler+0xb4/0x100
> >> [  214.157694]  qmi_handle_message+0x88/0x1a0
> >> [  214.157702]  qmi_data_ready_work+0x208/0x35c
> >> [  214.157709]  process_one_work+0x144/0x2c4
> >> [  214.157719]  worker_thread+0x280/0x45c
> >> [  214.157726]  kthread+0xfc/0x1dc
> >> [  214.157733]  ret_from_fork+0x10/0x20
> >> [  214.157744] ---[ end trace 0000000000000000 ]---
> >> 
> >> Should that code be removed with the new code now?
> >
> > Yes, please try the revised patch below.
> 
> This is indeed now working for me, playback works again.
> 
> Let me know if I should do anything else for this.

Thanks for quick testing.  Them I'm going to submit the proper patch
with your Reported-and-tested-by tag.


Takashi

> 
> Regards
> Luca
> 
> >
> >
> > thanks,
> >
> > Takashi
> >
> > --- a/sound/usb/qcom/qc_audio_offload.c
> > +++ b/sound/usb/qcom/qc_audio_offload.c
> > @@ -538,38 +538,33 @@ static void uaudio_iommu_unmap(enum mem_type mtype, unsigned long iova,
> >  			umap_size, iova, mapped_iova_size);
> >  }
> >  
> > +static int uaudio_iommu_map_prot(bool dma_coherent)
> > +{
> > +	int prot = IOMMU_READ | IOMMU_WRITE;
> > +
> > +	if (dma_coherent)
> > +		prot |= IOMMU_CACHE;
> > +	return prot;
> > +}
> > +
> >  /**
> > - * uaudio_iommu_map() - maps iommu memory for adsp
> > + * uaudio_iommu_map_pa() - maps iommu memory for adsp
> >   * @mtype: ring type
> >   * @dma_coherent: dma coherent
> >   * @pa: physical address for ring/buffer
> >   * @size: size of memory region
> > - * @sgt: sg table for memory region
> >   *
> >   * Maps the XHCI related resources to a memory region that is assigned to be
> >   * used by the adsp.  This will be mapped to the domain, which is created by
> >   * the ASoC USB backend driver.
> >   *
> >   */
> > -static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
> > -				      phys_addr_t pa, size_t size,
> > -				      struct sg_table *sgt)
> > +static unsigned long uaudio_iommu_map_pa(enum mem_type mtype, bool dma_coherent,
> > +					 phys_addr_t pa, size_t size)
> >  {
> > -	struct scatterlist *sg;
> >  	unsigned long iova = 0;
> > -	size_t total_len = 0;
> > -	unsigned long iova_sg;
> > -	phys_addr_t pa_sg;
> >  	bool map = true;
> > -	size_t sg_len;
> > -	int prot;
> > -	int ret;
> > -	int i;
> > -
> > -	prot = IOMMU_READ | IOMMU_WRITE;
> > -
> > -	if (dma_coherent)
> > -		prot |= IOMMU_CACHE;
> > +	int prot = uaudio_iommu_map_prot(dma_coherent);
> >  
> >  	switch (mtype) {
> >  	case MEM_EVENT_RING:
> > @@ -583,20 +578,41 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
> >  				     &uaudio_qdev->xfer_ring_iova_size,
> >  				     &uaudio_qdev->xfer_ring_list, size);
> >  		break;
> > -	case MEM_XFER_BUF:
> > -		iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
> > -				     &uaudio_qdev->xfer_buf_iova_size,
> > -				     &uaudio_qdev->xfer_buf_list, size);
> > -		break;
> >  	default:
> >  		dev_err(uaudio_qdev->data->dev, "unknown mem type %d\n", mtype);
> >  	}
> >  
> >  	if (!iova || !map)
> > -		goto done;
> > +		return 0;
> >  
> > -	if (!sgt)
> > -		goto skip_sgt_map;
> > +	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
> > +
> > +	return iova;
> > +}
> > +
> > +static unsigned long uaudio_iommu_map_xfer_buf(bool dma_coherent, size_t size,
> > +					       struct sg_table *sgt)
> > +{
> > +	struct scatterlist *sg;
> > +	unsigned long iova = 0;
> > +	size_t total_len = 0;
> > +	unsigned long iova_sg;
> > +	phys_addr_t pa_sg;
> > +	size_t sg_len;
> > +	int prot = uaudio_iommu_map_prot(dma_coherent);
> > +	int ret;
> > +	int i;
> > +
> > +	prot = IOMMU_READ | IOMMU_WRITE;
> > +
> > +	if (dma_coherent)
> > +		prot |= IOMMU_CACHE;
> > +
> > +	iova = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
> > +			       &uaudio_qdev->xfer_buf_iova_size,
> > +			       &uaudio_qdev->xfer_buf_list, size);
> > +	if (!iova)
> > +		goto done;
> >  
> >  	iova_sg = iova;
> >  	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
> > @@ -618,11 +634,6 @@ static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,
> >  		uaudio_iommu_unmap(MEM_XFER_BUF, iova, size, total_len);
> >  		iova = 0;
> >  	}
> > -	return iova;
> > -
> > -skip_sgt_map:
> > -	iommu_map(uaudio_qdev->data->domain, iova, pa, size, prot, GFP_KERNEL);
> > -
> >  done:
> >  	return iova;
> >  }
> > @@ -1020,7 +1031,6 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
> >  	struct sg_table xfer_buf_sgt;
> >  	dma_addr_t xfer_buf_dma;
> >  	void *xfer_buf;
> > -	phys_addr_t xfer_buf_pa;
> >  	u32 len = xfer_buf_len;
> >  	bool dma_coherent;
> >  	dma_addr_t xfer_buf_dma_sysdev;
> > @@ -1051,18 +1061,12 @@ static int uaudio_transfer_buffer_setup(struct snd_usb_substream *subs,
> >  	if (!xfer_buf)
> >  		return -ENOMEM;
> >  
> > -	/* Remapping is not possible if xfer_buf is outside of linear map */
> > -	xfer_buf_pa = virt_to_phys(xfer_buf);
> > -	if (WARN_ON(!page_is_ram(PFN_DOWN(xfer_buf_pa)))) {
> > -		ret = -ENXIO;
> > -		goto unmap_sync;
> > -	}
> >  	dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf,
> >  			xfer_buf_dma, len);
> >  
> >  	/* map the physical buffer into sysdev as well */
> > -	xfer_buf_dma_sysdev = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent,
> > -					       xfer_buf_pa, len, &xfer_buf_sgt);
> > +	xfer_buf_dma_sysdev = uaudio_iommu_map_xfer_buf(dma_coherent,
> > +							len, &xfer_buf_sgt);
> >  	if (!xfer_buf_dma_sysdev) {
> >  		ret = -ENOMEM;
> >  		goto unmap_sync;
> > @@ -1143,8 +1147,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
> >  	sg_free_table(sgt);
> >  
> >  	/* data transfer ring */
> > -	iova = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_pa,
> > -			      PAGE_SIZE, NULL);
> > +	iova = uaudio_iommu_map_pa(MEM_XFER_RING, dma_coherent, tr_pa,
> > +				   PAGE_SIZE);
> >  	if (!iova) {
> >  		ret = -ENOMEM;
> >  		goto clear_pa;
> > @@ -1207,8 +1211,8 @@ static int uaudio_event_ring_setup(struct snd_usb_substream *subs,
> >  	mem_info->dma = sg_dma_address(sgt->sgl);
> >  	sg_free_table(sgt);
> >  
> > -	iova = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, er_pa,
> > -			      PAGE_SIZE, NULL);
> > +	iova = uaudio_iommu_map_pa(MEM_EVENT_RING, dma_coherent, er_pa,
> > +				   PAGE_SIZE);
> >  	if (!iova) {
> >  		ret = -ENOMEM;
> >  		goto clear_pa;
> 

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Arnd Bergmann]

On Wed, Sep 17, 2025, at 10:30, Takashi Iwai wrote:
> On Wed, 17 Sep 2025 10:19:23 +0200,
>> >> 
>> >> Are you planning to post this as a proper patch? It's a bit late in the
>> >> 6.17 cycle already but good to still get this fixed for final release.

I was expecting your earlier suggestion (without my experimental
changes) to get merged for 6.17.

>> Should that code be removed with the new code now?
>
> Yes, please try the revised patch below.

This version looks good to me, thanks for following up,
and sorry if I caused you extra work.

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion

[Takashi Iwai]

On Wed, 17 Sep 2025 14:52:54 +0200,
Arnd Bergmann wrote:
> 
> On Wed, Sep 17, 2025, at 10:30, Takashi Iwai wrote:
> > On Wed, 17 Sep 2025 10:19:23 +0200,
> >> >> 
> >> >> Are you planning to post this as a proper patch? It's a bit late in the
> >> >> 6.17 cycle already but good to still get this fixed for final release.
> 
> I was expecting your earlier suggestion (without my experimental
> changes) to get merged for 6.17.

Ah that was misunderstanding, then.

> >> Should that code be removed with the new code now?
> >
> > Yes, please try the revised patch below.
> 
> This version looks good to me, thanks for following up,
> and sorry if I caused you extra work.
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Thanks, I'm going to include this for the next PR for 6.17-rc7.


Takashi