public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: "Luca Weiss" <luca.weiss@fairphone.com>
Cc: "Takashi Iwai" <tiwai@suse.de>, "Arnd Bergmann" <arnd@arndb.de>,
	"Arnd Bergmann" <arnd@kernel.org>,
	"Mark Brown" <broonie@kernel.org>,
	"Wesley Cheng" <quic_wcheng@quicinc.com>,
	"Jaroslav Kysela" <perex@perex.cz>,
	"Takashi Iwai" <tiwai@suse.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Dan Carpenter" <dan.carpenter@linaro.org>,
	<linux-sound@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion
Date: Wed, 17 Sep 2025 10:30:25 +0200	[thread overview]
Message-ID: <87v7lhwkf2.wl-tiwai@suse.de> (raw)
In-Reply-To: <DCUXFIZ5KRCU.3JANM98BSE8SE@fairphone.com>

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;

  reply	other threads:[~2025-09-17  8:30 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-05-13 12:34 [PATCH 0/3] ALSA: qc_audio_offload: address space cleanups Arnd Bergmann
2025-05-13 12:34 ` [PATCH 1/3] ALSA: qc_audio_offload: rename dma/iova/va/cpu/phys variables Arnd Bergmann
2025-05-13 12:34 ` [PATCH 2/3] ALSA: qc_audio_offload: avoid leaking xfer_buf allocation Arnd Bergmann
2025-05-13 12:34 ` [PATCH 3/3] ALSA: qc_audio_offload: try to reduce address space confusion Arnd Bergmann
2025-08-01 11:31   ` Luca Weiss
2025-08-01 12:31     ` Takashi Iwai
2025-08-01 12:35       ` Luca Weiss
2025-08-01 12:49         ` Takashi Iwai
2025-09-05 11:47           ` Luca Weiss
2025-09-05 12:08             ` Arnd Bergmann
2025-09-05 13:17               ` Luca Weiss
2025-09-05 14:50                 ` Arnd Bergmann
2025-09-05 12:26             ` Takashi Iwai
2025-09-05 14:54               ` Arnd Bergmann
2025-09-05 14:57                 ` Takashi Iwai
2025-09-16  8:41                 ` Luca Weiss
2025-09-16 16:09                   ` Takashi Iwai
2025-09-17  8:19                     ` Luca Weiss
2025-09-17  8:30                       ` Takashi Iwai [this message]
2025-09-17 12:27                         ` Luca Weiss
2025-09-17 12:35                           ` Takashi Iwai
2025-09-17 12:52                         ` Arnd Bergmann
2025-09-17 13:12                           ` Takashi Iwai
2025-05-14  9:01 ` [PATCH 0/3] ALSA: qc_audio_offload: address space cleanups Takashi Iwai
2025-05-21 12:33   ` Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87v7lhwkf2.wl-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=arnd@arndb.de \
    --cc=arnd@kernel.org \
    --cc=broonie@kernel.org \
    --cc=dan.carpenter@linaro.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sound@vger.kernel.org \
    --cc=luca.weiss@fairphone.com \
    --cc=perex@perex.cz \
    --cc=quic_wcheng@quicinc.com \
    --cc=tiwai@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox