ALSA: qc_audio_offload: address space cleanups

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

Posted by Arnd Bergmann 9 months ago

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

Posted by Luca Weiss 6 months, 1 week ago

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

Posted by Takashi Iwai 6 months, 1 week ago

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

Posted by Luca Weiss 6 months, 1 week ago

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

Posted by Takashi Iwai 6 months, 1 week ago

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

Posted by Luca Weiss 5 months ago

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

Posted by Takashi Iwai 5 months ago

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

Posted by Arnd Bergmann 5 months ago

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

Posted by Luca Weiss 4 months, 3 weeks ago

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

Posted by Takashi Iwai 4 months, 3 weeks ago

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

Posted by Luca Weiss 4 months, 3 weeks ago

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

Posted by Takashi Iwai 4 months, 3 weeks ago

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

Posted by Arnd Bergmann 4 months, 3 weeks ago

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

Posted by Takashi Iwai 4 months, 3 weeks ago

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

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

Posted by Luca Weiss 4 months, 3 weeks ago

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

Posted by Takashi Iwai 4 months, 3 weeks ago

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

Posted by Takashi Iwai 5 months ago

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

Posted by Arnd Bergmann 5 months ago

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

Posted by Luca Weiss 5 months ago

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

Posted by Arnd Bergmann 5 months ago

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

[PATCH 1/3] ALSA: qc_audio_offload: rename dma/iova/va/cpu/phys variables
[PATCH 2/3] ALSA: qc_audio_offload: avoid leaking xfer_buf allocation
[PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion