Claude review: misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

Claude review: misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Claude Code Review Bot]

Patch Review

**Verdict: Looks correct.**

The bug analysis is sound. In the original code, `inbuf.pageslen` is unconditionally set to 0 *after* the remote_heap allocation block:

```c
// Original code (after the allocation/SCM block):
inbuf.client_id = fl->client_id;
inbuf.namelen = init.namelen;
inbuf.pageslen = 0;  // always 0, even if heap was just allocated
```

This means the buffer page info is never passed to the DSP, so the memory pool starts empty and the initial allocation is leaked.

The fix moves the `inbuf` initialization before the allocation block and conditionally sets `pageslen = 1` when SCM assignment succeeds:

```c
inbuf.pageslen = 0;
if (!fl->cctx->remote_heap) {
    // allocate + SCM...
    scm_done = true;
    inbuf.pageslen = 1;  // <-- now set when memory is actually registered
}
```

This correctly distinguishes first-time allocation (pageslen=1, send page info) from subsequent calls where the heap already exists (pageslen=0). Note that Patch 3 later removes and replaces this code entirely, but each patch in the series is independently correct.

**Minor nit:** The blank line after `goto err;` followed by `}` and the removed blank line before `if (!fl->cctx->remote_heap)` changes the spacing slightly. No functional issue.

---

---
Generated by Claude Code Patch Reviewer

Claude review: misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Claude Code Review Bot]

Patch Review

**Verdict: Looks good.**

The fix is correct. In the original code, `inbuf.pageslen` is unconditionally set to 0 *after* the remote heap allocation and SCM assignment block, so the DSP never receives the allocated buffer:

```c
// original: pageslen always 0
inbuf.client_id = fl->client_id;
inbuf.namelen = init.namelen;
inbuf.pageslen = 0;
```

The patch moves these assignments before the allocation block and sets `pageslen = 1` when the SCM assignment succeeds:

```c
+			scm_done = true;
+			inbuf.pageslen = 1;
```

This ensures the initially allocated buffer is actually registered with the Audio PD. The logic is clean: default `pageslen=0`, set to 1 only when memory is both allocated and SCM-assigned.

Note: This code is removed by patch 4, so this fix is primarily for stable backports.

---

---
Generated by Claude Code Patch Reviewer

[PATCH v7 0/5] misc: fastrpc: Add missing bug fixes

[Jianping Li]

Add missing bug fixes in memory areas. This patch series fixes multiple memory
handling issues in the FastRPC driver, primarily around the Audio PD remote heap.

The Audio PD uses a reserved memory-region that is shared between HLOS
and the DSP. Allocating and freeing this memory from userspace is unsafe,
as the kernel cannot reliably determine when the DSP has finished using
the buffers.

To address this, the entire reserved memory-region for the Audio PD is
now fully assigned to the DSP during remoteproc boot-up, and its lifetime
is tied to the rpmsg channel.

Patch [v6]: https://lore.kernel.org/all/20260526111124.515-2-jianping.li@oss.qualcomm.com/

Change in v7:
  - Removed duplicate remote heap validation check; keep it only at
    the beginning of fastrpc_init_create_static_process()

Change in v6:
  - Separate the handling of err_copy
  - Place the check for remote_heap at the beginning of the function

Change in v5:
  - Split reserved-memory handling into separate patches

Change in v4:
  - Fail Audio PD static process creation when no reserved memory-region
    is present, instead of silently proceeding

Change in v3:
  - Adjusted the order of the series, placing NULL check changes that are not bug fixes at the end
  - Modified the commit message to describe the bug background in detail
  - Switch buf->list_lock back to fl->lock
  - Add locking to the operation of audio_init_mem

Changes in v2:
  - Remove the if check outside fastrpc_buf_free
  - Store the spinlock pointer in the struct fastrpc_buf instead
  - Allocate entire reserved memory to audio PD through remote heap

Ekansh Gupta (3):
  misc: fastrpc: Fix initial memory allocation for Audio PD memory pool
  misc: fastrpc: Remove buffer from list prior to unmap operation
  misc: fastrpc: Allow fastrpc_buf_free() to accept NULL

Jianping Li (2):
  misc: fastrpc: Fail Audio PD init when reserved memory is missing
  misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

 drivers/misc/fastrpc.c | 128 +++++++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 62 deletions(-)

-- 
2.43.0

[PATCH v7 1/5] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Jianping Li]

From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

The initial buffer allocated for the Audio PD memory pool is never added
to the pool because pageslen is set to 0. As a result, the buffer is not
registered with Audio PD and is never used, causing a memory leak. Audio
PD immediately falls back to allocating memory from the remote heap since
the pool starts out empty.

Fix this by setting pageslen to 1 so that the initially allocated buffer
is correctly registered and becomes part of the Audio PD memory pool.

Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
Cc: stable@kernel.org
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 5524354f3ccc..80a636962357 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1373,7 +1373,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 		err = PTR_ERR(name);
 		goto err;
 	}
-
+	inbuf.client_id = fl->client_id;
+	inbuf.namelen = init.namelen;
+	inbuf.pageslen = 0;
 	if (!fl->cctx->remote_heap) {
 		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
 						&fl->cctx->remote_heap);
@@ -1396,12 +1398,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 				goto err_map;
 			}
 			scm_done = true;
+			inbuf.pageslen = 1;
 		}
 	}
 
-	inbuf.client_id = fl->client_id;
-	inbuf.namelen = init.namelen;
-	inbuf.pageslen = 0;
 	fl->pd = USER_PD;
 
 	args[0].ptr = (u64)(uintptr_t)&inbuf;
-- 
2.43.0

[PATCH v7 2/5] misc: fastrpc: Remove buffer from list prior to unmap operation

[Jianping Li]

From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

fastrpc_req_munmap_impl() is called to unmap any buffer. The buffer is
getting removed from the list after it is unmapped from DSP. This can
create potential race conditions if multiple threads invoke unmap
concurrently, where one thread may remove the entry from the list while
another thread's unmap operation is still ongoing.

Fix this by removing the buffer entry from the list before calling the
unmap operation. If the unmap fails, the entry is re-added to the list
so that userspace can retry the unmap, or alternatively, the buffer
will be cleaned up during device release when the DSP process is torn
down and all DSP-side mappings are freed along with remaining buffers
in the list.

Fixes: 2419e55e532de ("misc: fastrpc: add mmap/unmap support")
Cc: stable@kernel.org
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 80a636962357..a8a58f889d07 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1892,9 +1892,6 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
 				      &args[0]);
 	if (!err) {
 		dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
-		spin_lock(&fl->lock);
-		list_del(&buf->node);
-		spin_unlock(&fl->lock);
 		fastrpc_buf_free(buf);
 	} else {
 		dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);
@@ -1908,6 +1905,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
 	struct fastrpc_buf *buf = NULL, *iter, *b;
 	struct fastrpc_req_munmap req;
 	struct device *dev = fl->sctx->dev;
+	int err;
 
 	if (copy_from_user(&req, argp, sizeof(req)))
 		return -EFAULT;
@@ -1915,6 +1913,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
 	spin_lock(&fl->lock);
 	list_for_each_entry_safe(iter, b, &fl->mmaps, node) {
 		if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
+			list_del(&iter->node);
 			buf = iter;
 			break;
 		}
@@ -1927,7 +1926,14 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
 		return -EINVAL;
 	}
 
-	return fastrpc_req_munmap_impl(fl, buf);
+	err = fastrpc_req_munmap_impl(fl, buf);
+	if (err) {
+		spin_lock(&fl->lock);
+		list_add_tail(&buf->node, &fl->mmaps);
+		spin_unlock(&fl->lock);
+	}
+
+	return err;
 }
 
 static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
-- 
2.43.0

[PATCH v7 3/5] misc: fastrpc: Fail Audio PD init when reserved memory is missing

[Jianping Li]

Audio PD static process creation assumes that a reserved-memory
region is defined in DT and exposed via cctx->remote_heap.

If reserved-memory is missing or incomplete, the driver may pass
invalid address/size information to the DSP, leading to undefined
behavior or crashes.

Add explicit validation for remote_heap presence and size before
sending the memory to DSP, and fail early if the configuration is
invalid.

Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
Cc: stable@kernel.org
Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index a8a58f889d07..f46a8f53970d 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1354,6 +1354,13 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	} inbuf;
 	u32 sc;
 
+	if (!fl->cctx->remote_heap ||
+	    !fl->cctx->remote_heap->dma_addr ||
+	    !fl->cctx->remote_heap->size) {
+		err = -ENOMEM;
+		dev_dbg(fl->sctx->dev, "remote heap memory region is not added\n");
+		return err;
+	}
 	args = kzalloc_objs(*args, FASTRPC_CREATE_STATIC_PROCESS_NARGS);
 	if (!args)
 		return -ENOMEM;
-- 
2.43.0

[PATCH v7 4/5] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

[Jianping Li]

Allocating and freeing Audio PD memory from userspace is unsafe because
the kernel cannot reliably determine when the DSP has finished using the
memory. Userspace may free buffers while they are still in use by the DSP,
and remote free requests cannot be safely trusted.

Additionally, the current implementation allows userspace to repeatedly
grow the Audio PD heap, but does not support shrinking it. This can lead
to unbounded memory usage over time, effectively causing a memory leak.

Fix this by allocating the entire Audio PD reserved-memory region during
rpmsg probe and tying its lifetime to the rpmsg channel. This removes
userspace-controlled alloc/free and ensures that memory is reclaimed only
when the DSP process is torn down.

Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
Cc: stable@kernel.org
Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 96 +++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 53 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index f46a8f53970d..33be8bed6a0b 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -276,6 +276,8 @@ struct fastrpc_channel_ctx {
 	struct kref refcount;
 	/* Flag if dsp attributes are cached */
 	bool valid_attributes;
+	/* Flag if audio PD init mem was allocated */
+	bool audio_init_mem;
 	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
 	struct fastrpc_device *secure_fdevice;
 	struct fastrpc_device *fdevice;
@@ -1344,15 +1346,16 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	struct fastrpc_init_create_static init;
 	struct fastrpc_invoke_args *args;
 	struct fastrpc_phy_page pages[1];
+	struct fastrpc_channel_ctx *cctx = fl->cctx;
 	char *name;
 	int err;
-	bool scm_done = false;
 	struct {
 		int client_id;
 		u32 namelen;
 		u32 pageslen;
 	} inbuf;
 	u32 sc;
+	unsigned long flags;
 
 	if (!fl->cctx->remote_heap ||
 	    !fl->cctx->remote_heap->dma_addr ||
@@ -1383,31 +1386,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	inbuf.client_id = fl->client_id;
 	inbuf.namelen = init.namelen;
 	inbuf.pageslen = 0;
-	if (!fl->cctx->remote_heap) {
-		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
-						&fl->cctx->remote_heap);
-		if (err)
-			goto err_name;
-
-		/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
-		if (fl->cctx->vmcount) {
-			u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
-
-			err = qcom_scm_assign_mem(fl->cctx->remote_heap->dma_addr,
-							(u64)fl->cctx->remote_heap->size,
-							&src_perms,
-							fl->cctx->vmperms, fl->cctx->vmcount);
-			if (err) {
-				dev_err(fl->sctx->dev,
-					"Failed to assign memory with dma_addr %pad size 0x%llx err %d\n",
-					&fl->cctx->remote_heap->dma_addr,
-					fl->cctx->remote_heap->size, err);
-				goto err_map;
-			}
-			scm_done = true;
-			inbuf.pageslen = 1;
-		}
-	}
 
 	fl->pd = USER_PD;
 
@@ -1419,8 +1397,17 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	args[1].length = inbuf.namelen;
 	args[1].fd = -1;
 
-	pages[0].addr = fl->cctx->remote_heap->dma_addr;
-	pages[0].size = fl->cctx->remote_heap->size;
+	spin_lock_irqsave(&cctx->lock, flags);
+	if (!fl->cctx->audio_init_mem) {
+		pages[0].addr = fl->cctx->remote_heap->dma_addr;
+		pages[0].size = fl->cctx->remote_heap->size;
+		fl->cctx->audio_init_mem = true;
+		inbuf.pageslen = 1;
+	} else {
+		pages[0].addr = 0;
+		pages[0].size = 0;
+	}
+	spin_unlock_irqrestore(&cctx->lock, flags);
 
 	args[2].ptr = (u64)(uintptr_t) pages;
 	args[2].length = sizeof(*pages);
@@ -1438,27 +1425,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 
 	return 0;
 err_invoke:
-	if (fl->cctx->vmcount && scm_done) {
-		u64 src_perms = 0;
-		struct qcom_scm_vmperm dst_perms;
-		u32 i;
-
-		for (i = 0; i < fl->cctx->vmcount; i++)
-			src_perms |= BIT(fl->cctx->vmperms[i].vmid);
-
-		dst_perms.vmid = QCOM_SCM_VMID_HLOS;
-		dst_perms.perm = QCOM_SCM_PERM_RWX;
-		err = qcom_scm_assign_mem(fl->cctx->remote_heap->dma_addr,
-						(u64)fl->cctx->remote_heap->size,
-						&src_perms, &dst_perms, 1);
-		if (err)
-			dev_err(fl->sctx->dev, "Failed to assign memory dma_addr %pad size 0x%llx err %d\n",
-				&fl->cctx->remote_heap->dma_addr, fl->cctx->remote_heap->size, err);
-	}
-err_map:
-	fastrpc_buf_free(fl->cctx->remote_heap);
-	fl->cctx->remote_heap = NULL;
-err_name:
+	fl->cctx->audio_init_mem = false;
 	kfree(name);
 err:
 	kfree(args);
@@ -2425,12 +2392,21 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 		}
 	}
 
-	if (domain_id == SDSP_DOMAIN_ID) {
+	if (domain_id == SDSP_DOMAIN_ID || domain_id == ADSP_DOMAIN_ID) {
 		struct resource res;
 		u64 src_perms;
 
 		err = of_reserved_mem_region_to_resource(rdev->of_node, 0, &res);
 		if (!err) {
+			if (domain_id == ADSP_DOMAIN_ID) {
+				data->remote_heap =
+					kzalloc_obj(*data->remote_heap, GFP_KERNEL);
+				if (!data->remote_heap)
+					return -ENOMEM;
+
+				data->remote_heap->dma_addr = res.start;
+				data->remote_heap->size = resource_size(&res);
+			}
 			src_perms = BIT(QCOM_SCM_VMID_HLOS);
 
 			err = qcom_scm_assign_mem(res.start, resource_size(&res), &src_perms,
@@ -2438,7 +2414,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 			if (err)
 				goto err_free_data;
 		}
-
 	}
 
 	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
@@ -2519,6 +2494,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 	struct fastrpc_buf *buf, *b;
 	struct fastrpc_user *user;
 	unsigned long flags;
+	int err;
 
 	/* No invocations past this point */
 	spin_lock_irqsave(&cctx->lock, flags);
@@ -2536,8 +2512,22 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 	list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
 		list_del(&buf->node);
 
-	if (cctx->remote_heap)
-		fastrpc_buf_free(cctx->remote_heap);
+	if (cctx->remote_heap && cctx->vmcount) {
+		u64 src_perms = 0;
+		struct qcom_scm_vmperm dst_perms;
+
+		for (u32 i = 0; i < cctx->vmcount; i++)
+			src_perms |= BIT(cctx->vmperms[i].vmid);
+
+		dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+		dst_perms.perm = QCOM_SCM_PERM_RWX;
+
+		err = qcom_scm_assign_mem(cctx->remote_heap->dma_addr,
+					  cctx->remote_heap->size, &src_perms,
+					  &dst_perms, 1);
+		if (!err)
+			kfree(cctx->remote_heap);
+	}
 
 	of_platform_depopulate(&rpdev->dev);
 
-- 
2.43.0

[PATCH v7 5/5] misc: fastrpc: Allow fastrpc_buf_free() to accept NULL

[Jianping Li]

From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

Make fastrpc_buf_free() a no-op when passed a NULL pointer, allowing
callers to avoid open-coded NULL checks.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 33be8bed6a0b..370b4f9ca135 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -418,6 +418,9 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
 
 static void fastrpc_buf_free(struct fastrpc_buf *buf)
 {
+	if (!buf)
+		return;
+
 	dma_free_coherent(buf->dev, buf->size, buf->virt,
 			  fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr));
 	kfree(buf);
@@ -510,8 +513,7 @@ static void fastrpc_user_free(struct kref *ref)
 	struct fastrpc_map *map, *m;
 	struct fastrpc_buf *buf, *b;
 
-	if (fl->init_mem)
-		fastrpc_buf_free(fl->init_mem);
+	fastrpc_buf_free(fl->init_mem);
 
 	list_for_each_entry_safe(ctx, n, &fl->pending, node) {
 		list_del(&ctx->node);
@@ -556,8 +558,7 @@ static void fastrpc_context_free(struct kref *ref)
 	for (i = 0; i < ctx->nbufs; i++)
 		fastrpc_map_put(ctx->maps[i]);
 
-	if (ctx->buf)
-		fastrpc_buf_free(ctx->buf);
+	fastrpc_buf_free(ctx->buf);
 
 	spin_lock_irqsave(&cctx->lock, flags);
 	idr_remove(&cctx->ctx_idr, ctx->ctxid >> 4);
-- 
2.43.0

Re: [PATCH v7 3/5] misc: fastrpc: Fail Audio PD init when reserved memory is missing

[Jie Gan]

On 6/2/2026 3:17 PM, Jianping Li wrote:
> Audio PD static process creation assumes that a reserved-memory
> region is defined in DT and exposed via cctx->remote_heap.
> 
> If reserved-memory is missing or incomplete, the driver may pass
> invalid address/size information to the DSP, leading to undefined
> behavior or crashes.
> 
> Add explicit validation for remote_heap presence and size before
> sending the memory to DSP, and fail early if the configuration is
> invalid.
> 
> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
> Cc: stable@kernel.org
> Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
> ---
>   drivers/misc/fastrpc.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index a8a58f889d07..f46a8f53970d 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1354,6 +1354,13 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>   	} inbuf;
>   	u32 sc;
>   
> +	if (!fl->cctx->remote_heap ||
> +	    !fl->cctx->remote_heap->dma_addr ||
> +	    !fl->cctx->remote_heap->size) {
> +		err = -ENOMEM;
> +		dev_dbg(fl->sctx->dev, "remote heap memory region is not added\n");

should be an error instead of dbg info.

Thanks,
Jie

> +		return err;
> +	}
>   	args = kzalloc_objs(*args, FASTRPC_CREATE_STATIC_PROCESS_NARGS);
>   	if (!args)
>   		return -ENOMEM;

Re: [PATCH v7 4/5] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

[Jie Gan]

On 6/2/2026 3:17 PM, Jianping Li wrote:
> Allocating and freeing Audio PD memory from userspace is unsafe because
> the kernel cannot reliably determine when the DSP has finished using the
> memory. Userspace may free buffers while they are still in use by the DSP,
> and remote free requests cannot be safely trusted.
> 
> Additionally, the current implementation allows userspace to repeatedly
> grow the Audio PD heap, but does not support shrinking it. This can lead
> to unbounded memory usage over time, effectively causing a memory leak.
> 
> Fix this by allocating the entire Audio PD reserved-memory region during
> rpmsg probe and tying its lifetime to the rpmsg channel. This removes
> userspace-controlled alloc/free and ensures that memory is reclaimed only
> when the DSP process is torn down.
> 
> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
> Cc: stable@kernel.org
> Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
> ---
>   drivers/misc/fastrpc.c | 96 +++++++++++++++++++-----------------------
>   1 file changed, 43 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f46a8f53970d..33be8bed6a0b 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -276,6 +276,8 @@ struct fastrpc_channel_ctx {
>   	struct kref refcount;
>   	/* Flag if dsp attributes are cached */
>   	bool valid_attributes;
> +	/* Flag if audio PD init mem was allocated */
> +	bool audio_init_mem;
>   	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
>   	struct fastrpc_device *secure_fdevice;
>   	struct fastrpc_device *fdevice;
> @@ -1344,15 +1346,16 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>   	struct fastrpc_init_create_static init;
>   	struct fastrpc_invoke_args *args;
>   	struct fastrpc_phy_page pages[1];
> +	struct fastrpc_channel_ctx *cctx = fl->cctx;
>   	char *name;
>   	int err;
> -	bool scm_done = false;
>   	struct {
>   		int client_id;
>   		u32 namelen;
>   		u32 pageslen;
>   	} inbuf;
>   	u32 sc;
> +	unsigned long flags;
>   
>   	if (!fl->cctx->remote_heap ||
>   	    !fl->cctx->remote_heap->dma_addr ||
> @@ -1383,31 +1386,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>   	inbuf.client_id = fl->client_id;
>   	inbuf.namelen = init.namelen;
>   	inbuf.pageslen = 0;
> -	if (!fl->cctx->remote_heap) {
> -		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
> -						&fl->cctx->remote_heap);
> -		if (err)
> -			goto err_name;
> -
> -		/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
> -		if (fl->cctx->vmcount) {
> -			u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> -
> -			err = qcom_scm_assign_mem(fl->cctx->remote_heap->dma_addr,
> -							(u64)fl->cctx->remote_heap->size,
> -							&src_perms,
> -							fl->cctx->vmperms, fl->cctx->vmcount);
> -			if (err) {
> -				dev_err(fl->sctx->dev,
> -					"Failed to assign memory with dma_addr %pad size 0x%llx err %d\n",
> -					&fl->cctx->remote_heap->dma_addr,
> -					fl->cctx->remote_heap->size, err);
> -				goto err_map;
> -			}
> -			scm_done = true;
> -			inbuf.pageslen = 1;
> -		}
> -	}
>   
>   	fl->pd = USER_PD;
>   
> @@ -1419,8 +1397,17 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>   	args[1].length = inbuf.namelen;
>   	args[1].fd = -1;
>   
> -	pages[0].addr = fl->cctx->remote_heap->dma_addr;
> -	pages[0].size = fl->cctx->remote_heap->size;
> +	spin_lock_irqsave(&cctx->lock, flags);
> +	if (!fl->cctx->audio_init_mem) {
> +		pages[0].addr = fl->cctx->remote_heap->dma_addr;
> +		pages[0].size = fl->cctx->remote_heap->size;
> +		fl->cctx->audio_init_mem = true;
> +		inbuf.pageslen = 1;
> +	} else {
> +		pages[0].addr = 0;
> +		pages[0].size = 0;
> +	}
> +	spin_unlock_irqrestore(&cctx->lock, flags);
>   
>   	args[2].ptr = (u64)(uintptr_t) pages;
>   	args[2].length = sizeof(*pages);
> @@ -1438,27 +1425,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>   
>   	return 0;
>   err_invoke:
> -	if (fl->cctx->vmcount && scm_done) {
> -		u64 src_perms = 0;
> -		struct qcom_scm_vmperm dst_perms;
> -		u32 i;
> -
> -		for (i = 0; i < fl->cctx->vmcount; i++)
> -			src_perms |= BIT(fl->cctx->vmperms[i].vmid);
> -
> -		dst_perms.vmid = QCOM_SCM_VMID_HLOS;
> -		dst_perms.perm = QCOM_SCM_PERM_RWX;
> -		err = qcom_scm_assign_mem(fl->cctx->remote_heap->dma_addr,
> -						(u64)fl->cctx->remote_heap->size,
> -						&src_perms, &dst_perms, 1);
> -		if (err)
> -			dev_err(fl->sctx->dev, "Failed to assign memory dma_addr %pad size 0x%llx err %d\n",
> -				&fl->cctx->remote_heap->dma_addr, fl->cctx->remote_heap->size, err);
> -	}
> -err_map:
> -	fastrpc_buf_free(fl->cctx->remote_heap);
> -	fl->cctx->remote_heap = NULL;
> -err_name:
> +	fl->cctx->audio_init_mem = false;
>   	kfree(name);
>   err:
>   	kfree(args);
> @@ -2425,12 +2392,21 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>   		}
>   	}
>   
> -	if (domain_id == SDSP_DOMAIN_ID) {
> +	if (domain_id == SDSP_DOMAIN_ID || domain_id == ADSP_DOMAIN_ID) {
>   		struct resource res;
>   		u64 src_perms;
>   
>   		err = of_reserved_mem_region_to_resource(rdev->of_node, 0, &res);
>   		if (!err) {
> +			if (domain_id == ADSP_DOMAIN_ID) {
> +				data->remote_heap =
> +					kzalloc_obj(*data->remote_heap, GFP_KERNEL);
> +				if (!data->remote_heap)
> +					return -ENOMEM;

allocated data never free with directly return.

goto err_free_data;

Beside, we also need free data->remote_heap in err_free_data path as you 
added new memory allocation.

> +
> +				data->remote_heap->dma_addr = res.start;
> +				data->remote_heap->size = resource_size(&res);
> +			}
>   			src_perms = BIT(QCOM_SCM_VMID_HLOS);
>   
>   			err = qcom_scm_assign_mem(res.start, resource_size(&res), &src_perms,
> @@ -2438,7 +2414,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>   			if (err)
>   				goto err_free_data;
>   		}
> -
>   	}
>   
>   	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
> @@ -2519,6 +2494,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>   	struct fastrpc_buf *buf, *b;
>   	struct fastrpc_user *user;
>   	unsigned long flags;
> +	int err;
>   
>   	/* No invocations past this point */
>   	spin_lock_irqsave(&cctx->lock, flags);
> @@ -2536,8 +2512,22 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>   	list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
>   		list_del(&buf->node);
>   
> -	if (cctx->remote_heap)
> -		fastrpc_buf_free(cctx->remote_heap);

after removed the code, the cctx->remote_heap is not freed:
1. cctx->vmcount == 0
2. if (!err) is false

we should free the cctx->remote_heap unconditionally if it exists.

Thanks,
Jie

> +	if (cctx->remote_heap && cctx->vmcount) {
> +		u64 src_perms = 0;
> +		struct qcom_scm_vmperm dst_perms;
> +
> +		for (u32 i = 0; i < cctx->vmcount; i++)
> +			src_perms |= BIT(cctx->vmperms[i].vmid);
> +
> +		dst_perms.vmid = QCOM_SCM_VMID_HLOS;
> +		dst_perms.perm = QCOM_SCM_PERM_RWX;
> +
> +		err = qcom_scm_assign_mem(cctx->remote_heap->dma_addr,
> +					  cctx->remote_heap->size, &src_perms,
> +					  &dst_perms, 1);
> +		if (!err)
> +			kfree(cctx->remote_heap);
> +	}
>   
>   	of_platform_depopulate(&rpdev->dev);
>   

Claude review: misc: fastrpc: Add missing bug fixes

[Claude Code Review Bot]

Overall Series Review

Subject: misc: fastrpc: Add missing bug fixes
Author: Jianping Li <jianping.li@oss.qualcomm.com>
Patches: 8
Reviewed: 2026-06-04T13:11:15.093832

---

This is a v7 series of 5 patches fixing memory handling bugs in the FastRPC driver, specifically around Audio PD (Processing Domain) remote heap management. The overall goal is sound: the Audio PD's reserved memory region should be fully assigned to the DSP at probe time, with its lifetime tied to the rpmsg channel, rather than allowing unsafe userspace-controlled alloc/free.

The series is generally well-structured with a logical progression: fix the initial allocation bug (patch 1), fix a race in unmap (patch 2), add validation (patch 3), move allocation to probe (patch 4), and a cleanup (patch 5). However, there are several issues, most notably in patches 3 and 4, that need attention before this can be merged.

**Key concerns:**
- Patch 3's validation check is placed before `remote_heap` has been allocated (in the pre-patch-4 code), making it dead code or an unconditional failure depending on ordering
- Patch 4 has a memory leak on the error path when `kzalloc` succeeds but `qcom_scm_assign_mem` fails in probe
- Patch 4's removal cleanup only `kfree`s `remote_heap` but the ADSP path never called `fastrpc_remote_heap_alloc`/`__fastrpc_buf_alloc`, so there's no DMA mapping to tear down - this is correct but should be documented

---

---
Generated by Claude Code Patch Reviewer

Claude review: misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Claude Code Review Bot]

Patch Review

**Purpose:** Fix a bug where `inbuf.pageslen` is always 0, so the initially allocated buffer is never registered with the Audio PD.

**Analysis:** In the current upstream code (line 1355), `inbuf.pageslen = 0` is set unconditionally *after* the remote heap allocation block. The remote heap buffer is allocated and SCM-assigned, but pageslen stays 0, meaning the DSP never learns about the allocated page. This is a real bug.

The fix moves the `inbuf` field initialization *above* the `if (!fl->cctx->remote_heap)` block, and conditionally sets `inbuf.pageslen = 1` inside the `if (fl->cctx->vmcount)` block after a successful `qcom_scm_assign_mem`.

```c
+	inbuf.client_id = fl->client_id;
+	inbuf.namelen = init.namelen;
+	inbuf.pageslen = 0;
 	if (!fl->cctx->remote_heap) {
...
+			inbuf.pageslen = 1;
 		}
 	}
-	inbuf.client_id = fl->client_id;
-	inbuf.namelen = init.namelen;
-	inbuf.pageslen = 0;
```

**Verdict:** Looks correct. The logic is: default pageslen to 0, and only set it to 1 when the memory was freshly allocated and SCM-assigned in this call. On subsequent calls where `remote_heap` already exists, pageslen stays 0, which is the intended behavior (don't re-register already-registered memory). The Fixes tag and stable CC are appropriate.

---

---
Generated by Claude Code Patch Reviewer

Claude review: misc: fastrpc: Remove buffer from list prior to unmap operation

[Claude Code Review Bot]

Patch Review

**Purpose:** Fix a race condition where two concurrent unmap calls on the same buffer could both find it in the list and proceed concurrently.

**Analysis:** The current code finds the buffer in the list under `fl->lock`, drops the lock, calls `fastrpc_req_munmap_impl` (which does the DSP unmap), and only then removes from the list inside `fastrpc_req_munmap_impl`. Two threads could both find the same buffer and both attempt to unmap it.

The fix moves `list_del` into `fastrpc_req_munmap` (under the existing `fl->lock`) *before* calling `fastrpc_req_munmap_impl`. If the unmap fails, the buffer is re-added:

```c
+	err = fastrpc_req_munmap_impl(fl, buf);
+	if (err) {
+		spin_lock(&fl->lock);
+		list_add_tail(&buf->node, &fl->mmaps);
+		spin_unlock(&fl->lock);
+	}
```

The corresponding `list_del` + `spin_unlock` in `fastrpc_req_munmap_impl` is removed, and on success it just calls `fastrpc_buf_free(buf)`.

**Minor note:** After the `list_del` in `fastrpc_req_munmap`, `buf->node` is in an undefined state. The `list_add_tail` in the error path works here because `list_del` doesn't corrupt the node, but using `list_del_init` would be slightly safer and more idiomatic for the "remove, try, re-add on failure" pattern. Not a blocker.

**Verdict:** Correct fix for a real race. The re-add on failure approach is reasonable.

---

---
Generated by Claude Code Patch Reviewer

Claude review: misc: fastrpc: Fail Audio PD init when reserved memory is missing

[Claude Code Review Bot]

Patch Review

**Purpose:** Add early validation that `remote_heap` exists before proceeding with static process creation.

**Analysis:** This patch adds a check at the very top of `fastrpc_init_create_static_process`:

```c
+	if (!fl->cctx->remote_heap ||
+	    !fl->cctx->remote_heap->dma_addr ||
+	    !fl->cctx->remote_heap->size) {
+		err = -ENOMEM;
+		dev_dbg(fl->sctx->dev, "remote heap memory region is not added\n");
+		return err;
+	}
```

**Issue 1 - Series ordering problem:** This check is added in patch 3, but at this point in the series (after patches 1-2), `remote_heap` is still allocated lazily inside `fastrpc_init_create_static_process` itself (the `if (!fl->cctx->remote_heap)` block from line 1328). So this check would cause the *first* call to `fastrpc_init_create_static_process` to always fail with `-ENOMEM`, since `remote_heap` hasn't been allocated yet. This only makes sense *after* patch 4 moves allocation to probe. The patch ordering should be swapped: patch 4 should come before patch 3, or they should be squashed.

**Issue 2 - Error code:** `-ENOMEM` is not the right error code when reserved memory is missing from the DT configuration. `-ENODEV` or `-EINVAL` would be more appropriate, as this is a configuration/platform issue, not a memory allocation failure.

**Issue 3:** The function also applies to SDSP, not just Audio PD. This check could break SDSP static process creation if `remote_heap` is not set up for SDSP (which it wouldn't be, since SDSP remote heap allocation works differently). Need to verify whether this function is called for SDSP contexts as well or only ADSP.

**Verdict:** The ordering relative to patch 4 is problematic. As written, this would break Audio PD initialization when applied without patch 4. Additionally, the check is applied unconditionally but the cover letter only discusses Audio PD.

---

---
Generated by Claude Code Patch Reviewer

Claude review: misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

[Claude Code Review Bot]

Patch Review

**Purpose:** Move Audio PD reserved memory allocation from lazy (on first static process creation) to probe time, tying its lifetime to the rpmsg channel.

**Analysis:** This is the most significant patch in the series.

**Probe changes:**
```c
-	if (domain_id == SDSP_DOMAIN_ID) {
+	if (domain_id == SDSP_DOMAIN_ID || domain_id == ADSP_DOMAIN_ID) {
 		struct resource res;
 		u64 src_perms;
 
 		err = of_reserved_mem_region_to_resource(rdev->of_node, 0, &res);
 		if (!err) {
+			if (domain_id == ADSP_DOMAIN_ID) {
+				data->remote_heap =
+					kzalloc_obj(*data->remote_heap, GFP_KERNEL);
+				if (!data->remote_heap)
+					return -ENOMEM;
+
+				data->remote_heap->dma_addr = res.start;
+				data->remote_heap->size = resource_size(&res);
+			}
 			src_perms = BIT(QCOM_SCM_VMID_HLOS);
```

**Issue 1 - Memory leak on SCM failure:** If `kzalloc_obj` succeeds but the subsequent `qcom_scm_assign_mem` fails, code jumps to `err_free_data` which does not free `data->remote_heap`. This is a leak.

**Issue 2 - ADSP without reserved memory:** If `of_reserved_mem_region_to_resource` returns an error for ADSP, the code skips the entire block — `remote_heap` remains NULL. Combined with patch 3, this means Audio PD init will always fail on platforms without reserved memory, which is the intended behavior per the cover letter. Good.

**Issue 3 - Missing return value check:** The `return -ENOMEM` in probe skips the error cleanup path. The probe function has allocated other resources before this point (potentially `data` itself, sessions, etc.) that won't be cleaned up. Should use `goto err_free_data` instead of `return -ENOMEM`.

**Static process creation changes:** The lazy allocation and SCM assignment logic is removed. Instead, a new `audio_init_mem` flag protected by `cctx->lock` (with `spin_lock_irqsave`) gates whether pages info is sent:

```c
+	spin_lock_irqsave(&cctx->lock, flags);
+	if (!fl->cctx->audio_init_mem) {
+		pages[0].addr = fl->cctx->remote_heap->dma_addr;
+		pages[0].size = fl->cctx->remote_heap->size;
+		fl->cctx->audio_init_mem = true;
+		inbuf.pageslen = 1;
+	} else {
+		pages[0].addr = 0;
+		pages[0].size = 0;
+	}
+	spin_unlock_irqrestore(&cctx->lock, flags);
```

**Issue 4 - Inconsistent use of cctx vs fl->cctx:** The code introduces `struct fastrpc_channel_ctx *cctx = fl->cctx;` but then uses `fl->cctx->` in several places within the spinlock section. Should consistently use `cctx->` for clarity.

**Error path changes:**
```c
 err_invoke:
-	if (fl->cctx->vmcount && scm_done) {
-		...scm_assign_mem reversal...
-	}
-err_map:
-	fastrpc_buf_free(fl->cctx->remote_heap);
-	fl->cctx->remote_heap = NULL;
-err_name:
+	fl->cctx->audio_init_mem = false;
```

This correctly resets the flag so a retry can re-send pages, and no longer frees the remote_heap since its lifetime is now tied to the rpmsg channel.

**Remove path changes:**
```c
+	if (cctx->remote_heap && cctx->vmcount) {
+		...reverse SCM assignment...
+		err = qcom_scm_assign_mem(...);
+		if (!err)
+			kfree(cctx->remote_heap);
+	}
```

**Issue 5 - Leak when SCM reassign fails:** If `qcom_scm_assign_mem` fails in the remove path, the `remote_heap` struct is leaked (never freed). The old code called `fastrpc_buf_free` unconditionally. This is a deliberate choice (can't free memory the DSP still has access to), but the error should at least be logged.

**Issue 6 - SDSP remote_heap handling regression:** The removal path previously called `fastrpc_buf_free(cctx->remote_heap)` unconditionally for any domain with a remote_heap. Now it only does `kfree` and only when `vmcount > 0`. For SDSP (which also sets `remote_heap` via `fastrpc_remote_heap_alloc`), the old `fastrpc_buf_free` call freed DMA memory. The new code uses `kfree` only, which would leak DMA memory for SDSP. Wait — looking more carefully, the SDSP path doesn't set `remote_heap` in the current upstream; `remote_heap` is only set in `fastrpc_init_create_static_process`. So SDSP would still go through the static process creation path and set it that way. But with patch 4, the `fastrpc_init_create_static_process` no longer allocates `remote_heap`. This means SDSP `remote_heap` allocation is now broken — there's no code path left that allocates it for SDSP.

Actually, re-reading more carefully: the SDSP probe path does SCM assign but does NOT set `remote_heap`. The SDSP `remote_heap` would still be allocated in `fastrpc_init_create_static_process`... but patch 4 removes that allocation code. This seems like a regression for SDSP.

**Verdict:** Several issues need fixing: the probe error path leak, inconsistent variable usage, and the potential SDSP regression.

---

---
Generated by Claude Code Patch Reviewer

Claude review: misc: fastrpc: Allow fastrpc_buf_free() to accept NULL

[Claude Code Review Bot]

Patch Review

**Purpose:** Add a NULL guard to `fastrpc_buf_free()` so callers don't need to check.

```c
 static void fastrpc_buf_free(struct fastrpc_buf *buf)
 {
+	if (!buf)
+		return;
+
 	dma_free_coherent(buf->dev, buf->size, buf->virt,
```

And removes two caller-side NULL checks:
```c
-	if (fl->init_mem)
-		fastrpc_buf_free(fl->init_mem);
+	fastrpc_buf_free(fl->init_mem);
```

```c
-	if (ctx->buf)
-		fastrpc_buf_free(ctx->buf);
+	fastrpc_buf_free(ctx->buf);
```

**Note:** The patch applies to a function `fastrpc_user_free` that doesn't exist in the current upstream tree. It's likely that the line numbers correspond to `fastrpc_device_release` (line 1593 upstream). Since the patches don't apply cleanly, this may be based on a slightly different tree. The change itself is correct regardless.

**Verdict:** Clean, straightforward cleanup. This is a common pattern in kernel code (cf. `kfree(NULL)`). No issues.

---

**Summary of actionable items:**
1. **Patch 3 & 4 ordering:** Patch 3's validation makes no sense without patch 4. Swap their order or squash them.
2. **Patch 3:** Use `-ENODEV` or `-EINVAL` instead of `-ENOMEM`.
3. **Patch 4 probe:** `return -ENOMEM` should be `goto err_free_data` to avoid leaking earlier allocations.
4. **Patch 4 probe:** `remote_heap` is leaked if `qcom_scm_assign_mem` fails.
5. **Patch 4:** Verify SDSP is not regressed by the removal of lazy `remote_heap` allocation in `fastrpc_init_create_static_process`.
6. **Patch 4:** Use `cctx->` consistently instead of mixing with `fl->cctx->`.

---
Generated by Claude Code Patch Reviewer