Message-Id: <DCUXFIZ5KRCU.3JANM98BSE8SE@fairphone.com>
Date: Wed, 17 Sep 2025 10:19:23 +0200
From: "Luca Weiss" <luca.weiss@...rphone.com>
To: "Takashi Iwai" <tiwai@...e.de>, "Luca Weiss" <luca.weiss@...rphone.com>
Cc: "Arnd Bergmann" <arnd@...db.de>, "Arnd Bergmann" <arnd@...nel.org>,
 "Mark Brown" <broonie@...nel.org>, "Wesley Cheng"
 <quic_wcheng@...cinc.com>, "Jaroslav Kysela" <perex@...ex.cz>, "Takashi
 Iwai" <tiwai@...e.com>, "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
 "Dan Carpenter" <dan.carpenter@...aro.org>, <linux-sound@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space
 confusion

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;

Powered by blists - more mailing lists