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

["Luca Weiss" <luca.weiss@fairphone.com>]

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;