[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 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 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