| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87wm6doskd.wl-tiwai@suse.de>
Date: Fri, 05 Sep 2025 16:57:38 +0200
From: Takashi Iwai <tiwai@...e.de>
To: "Arnd Bergmann" <arnd@...db.de>
Cc: "Takashi Iwai" <tiwai@...e.de>,
"Luca Weiss" <luca.weiss@...rphone.com>,
"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
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;
Powered by blists - more mailing lists