[PATCH v2 0/4] Add missing bug fixes

[PATCH v2 0/4] Add missing bug fixes

[Jianping Li]

Add missing bug fixes in memory areas. This patch series carries
following fixes:
- Add proper checks to fastrpc_buf_free to avoid potential issues.
- Add multiple fixes for remote heap which is used by Audio PD.
Patch [v1]: https://lore.kernel.org/all/20250513042825.2147985-1-ekansh.gupta@oss.qualcomm.com/

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: Add NULL check to fastrpc_buf_free to prevent crash
  misc: fastrpc: Fix initial memory allocation for Audio PD memory pool
  misc: fastrpc: Remove buffer from list prior to unmap operation

Jianping Li (1):
  misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

 drivers/misc/fastrpc.c | 135 ++++++++++++++++++++++-------------------
 1 file changed, 71 insertions(+), 64 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Jianping Li]

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

The fastrpc_buf_free function currently does not handle the case where
the input buffer pointer (buf) is NULL. This can lead to a null pointer
dereference, causing a crash or undefined behavior when the function
attempts to access members of the buf structure. Add a NULL check to
ensure safe handling of NULL pointers and prevent potential crashes.

Fixes: c68cfb718c8f9 ("misc: fastrpc: Add support for context Invoke method")
Cc: stable@kernel.org
Co-developed-by: Ekansh Gupta <ekansh.gupta@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 | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 4f5a79c50f58..515a43c9d95d 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -414,6 +414,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_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);
@@ -1591,8 +1593,7 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 	list_del(&fl->user);
 	spin_unlock_irqrestore(&cctx->lock, flags);
 
-	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);
@@ -2492,8 +2493,7 @@ 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);
+	fastrpc_buf_free(cctx->remote_heap);
 
 	of_platform_depopulate(&rpdev->dev);
 
-- 
2.43.0

Re: [PATCH v2 1/4] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Dmitry Baryshkov]

On Thu, Jan 15, 2026 at 04:28:48PM +0800, Jianping Li wrote:
> From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> 
> The fastrpc_buf_free function currently does not handle the case where
> the input buffer pointer (buf) is NULL. This can lead to a null pointer
> dereference, causing a crash or undefined behavior when the function
> attempts to access members of the buf structure. Add a NULL check to
> ensure safe handling of NULL pointers and prevent potential crashes.

When does it happen? Do you have a backtrace or is it a safety coding?
Do you pass NULL buffer pointers to the function?

> 
> Fixes: c68cfb718c8f9 ("misc: fastrpc: Add support for context Invoke method")
> Cc: stable@kernel.org
> Co-developed-by: Ekansh Gupta <ekansh.gupta@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 | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/4] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Jianping]

On 1/16/2026 4:43 AM, Dmitry Baryshkov wrote:
> On Thu, Jan 15, 2026 at 04:28:48PM +0800, Jianping Li wrote:
>> From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>>
>> The fastrpc_buf_free function currently does not handle the case where
>> the input buffer pointer (buf) is NULL. This can lead to a null pointer
>> dereference, causing a crash or undefined behavior when the function
>> attempts to access members of the buf structure. Add a NULL check to
>> ensure safe handling of NULL pointers and prevent potential crashes.
> 
> When does it happen? Do you have a backtrace or is it a safety coding?
> Do you pass NULL buffer pointers to the function?
Thanks, Dmitry.
Yes, this change is mainly for safety‑coding purposes.

This is reachable on during remove/deinit sequences when a buffer was 
never allocated or allocation failed part‑way and cleanup proceeds.

It's a saftety coding: to eliminate NULL checks on the caller side, as 
we do in a lot of other kernel API.

At the same time, there is a possibility that this buffer passes NULL, 
and during verification, this can cause the kernel to crash.

The patch makes fastrpc_buf_free() NULL‑tolerant and simplifies callers 
by removing duplicated if (ptr) checks, reducing the chance of future 
omissions.
> 
>>
>> Fixes: c68cfb718c8f9 ("misc: fastrpc: Add support for context Invoke method")
>> Cc: stable@kernel.org
>> Co-developed-by: Ekansh Gupta <ekansh.gupta@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 | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
> 

Re: [PATCH v2 1/4] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Greg KH]

On Thu, Jan 15, 2026 at 04:28:48PM +0800, Jianping Li wrote:
> From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> 
> The fastrpc_buf_free function currently does not handle the case where
> the input buffer pointer (buf) is NULL. This can lead to a null pointer
> dereference, causing a crash or undefined behavior when the function
> attempts to access members of the buf structure. Add a NULL check to
> ensure safe handling of NULL pointers and prevent potential crashes.

What caller passes in NULL here?  I did a quick look, and see where the
callers check this properly if it could be NULL, otherwise it all looks
sane to me.  What in-kernel user is causing a crash here?  Why not fix
the caller up instead?

thanks,

greg k-h

Re: [PATCH v2 1/4] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Jianping]

On 1/16/2026 10:49 PM, Greg KH wrote:
> On Thu, Jan 15, 2026 at 04:28:48PM +0800, Jianping Li wrote:
>> From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>>
>> The fastrpc_buf_free function currently does not handle the case where
>> the input buffer pointer (buf) is NULL. This can lead to a null pointer
>> dereference, causing a crash or undefined behavior when the function
>> attempts to access members of the buf structure. Add a NULL check to
>> ensure safe handling of NULL pointers and prevent potential crashes.
> 
> What caller passes in NULL here?  I did a quick look, and see where the
> callers check this properly if it could be NULL, otherwise it all looks
> sane to me.  What in-kernel user is causing a crash here?  Why not fix
> the caller up instead?
> 
> thanks,
> 
> greg k-h

It's a saftety coding: to eliminate NULL checks on the caller side, as 
we do in a lot of other kernel API.

And it was pointed out in the v1 patch discussion that this change was 
needed:
https://lore.kernel.org/all/c80c48a1-f1b6-4520-9d7c-3a83915c7717@oss.qualcomm.com/

Thanks,
Jianping.

Re: [PATCH v2 1/4] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Greg KH]

On Mon, Feb 02, 2026 at 03:13:10PM +0800, Jianping wrote:
> 
> 
> On 1/16/2026 10:49 PM, Greg KH wrote:
> > On Thu, Jan 15, 2026 at 04:28:48PM +0800, Jianping Li wrote:
> > > From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> > > 
> > > The fastrpc_buf_free function currently does not handle the case where
> > > the input buffer pointer (buf) is NULL. This can lead to a null pointer
> > > dereference, causing a crash or undefined behavior when the function
> > > attempts to access members of the buf structure. Add a NULL check to
> > > ensure safe handling of NULL pointers and prevent potential crashes.
> > 
> > What caller passes in NULL here?  I did a quick look, and see where the
> > callers check this properly if it could be NULL, otherwise it all looks
> > sane to me.  What in-kernel user is causing a crash here?  Why not fix
> > the caller up instead?
> > 
> > thanks,
> > 
> > greg k-h
> 
> It's a saftety coding: to eliminate NULL checks on the caller side, as we do
> in a lot of other kernel API.

But you do not do that for all functions in the kernel, otherwise the
kernel would be full of checks that are never hit at all.

> And it was pointed out in the v1 patch discussion that this change was
> needed:
> https://lore.kernel.org/all/c80c48a1-f1b6-4520-9d7c-3a83915c7717@oss.qualcomm.com/

Were the checks removed from the caller side like was asked for?

Also, your changelog makes it sound like this is a real bugfix for
something, when it is not at all, which is what I object to the most.
Don't make scary changelogs for things that are not actually happening.

thanks,

greg k-h

Re: [PATCH v2 1/4] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Jianping]

On 2/2/2026 4:41 PM, Greg KH wrote:
> On Mon, Feb 02, 2026 at 03:13:10PM +0800, Jianping wrote:
>>
>>
>> On 1/16/2026 10:49 PM, Greg KH wrote:
>>> On Thu, Jan 15, 2026 at 04:28:48PM +0800, Jianping Li wrote:
>>>> From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>>>>
>>>> The fastrpc_buf_free function currently does not handle the case where
>>>> the input buffer pointer (buf) is NULL. This can lead to a null pointer
>>>> dereference, causing a crash or undefined behavior when the function
>>>> attempts to access members of the buf structure. Add a NULL check to
>>>> ensure safe handling of NULL pointers and prevent potential crashes.
>>>
>>> What caller passes in NULL here?  I did a quick look, and see where the
>>> callers check this properly if it could be NULL, otherwise it all looks
>>> sane to me.  What in-kernel user is causing a crash here?  Why not fix
>>> the caller up instead?
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> It's a saftety coding: to eliminate NULL checks on the caller side, as we do
>> in a lot of other kernel API.
> 
> But you do not do that for all functions in the kernel, otherwise the
> kernel would be full of checks that are never hit at all.
To clarify the intention: this change was not triggered by any real 
crash in current callers. The motivation came from the v1 review 
discussion [1], where it was suggested that a NULL check in 
fastrpc_buf_free() would allow simplifying some of the caller paths.

[1]https://lore.kernel.org/all/c80c48a1-f1b6-4520-9d7c-3a83915c7717@oss.qualcomm.com/
> 
>> And it was pointed out in the v1 patch discussion that this change was
>> needed:
>> https://lore.kernel.org/all/c80c48a1-f1b6-4520-9d7c-3a83915c7717@oss.qualcomm.com/
> 
> Were the checks removed from the caller side like was asked for?
Currently, I have placed the check inside the API and removed all the 
checks outside the API.
> 
> Also, your changelog makes it sound like this is a real bugfix for
> something, when it is not at all, which is what I object to the most.
> Don't make scary changelogs for things that are not actually happening.
You are correct, I will modify the commit text that caused the 
misunderstanding.
> 
> thanks,
> 
> greg k-h

Re: [PATCH v2 1/4] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Jianping]

On 2/2/2026 4:41 PM, Greg KH wrote:
> On Mon, Feb 02, 2026 at 03:13:10PM +0800, Jianping wrote:
>>
>>
>> On 1/16/2026 10:49 PM, Greg KH wrote:
>>> On Thu, Jan 15, 2026 at 04:28:48PM +0800, Jianping Li wrote:
>>>> From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>>>>
>>>> The fastrpc_buf_free function currently does not handle the case where
>>>> the input buffer pointer (buf) is NULL. This can lead to a null pointer
>>>> dereference, causing a crash or undefined behavior when the function
>>>> attempts to access members of the buf structure. Add a NULL check to
>>>> ensure safe handling of NULL pointers and prevent potential crashes.
>>>
>>> What caller passes in NULL here?  I did a quick look, and see where the
>>> callers check this properly if it could be NULL, otherwise it all looks
>>> sane to me.  What in-kernel user is causing a crash here?  Why not fix
>>> the caller up instead?
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> It's a saftety coding: to eliminate NULL checks on the caller side, as we do
>> in a lot of other kernel API.
> 
> But you do not do that for all functions in the kernel, otherwise the
> kernel would be full of checks that are never hit at all.
To clarify the intention: this change was not triggered by any real 
crash in current callers. The motivation came from the v1 review 
discussion [1], where it was suggested that a NULL check in 
fastrpc_buf_free() would allow simplifying some of the caller paths.

[1] 
https://lore.kernel.org/all/c80c48a1-f1b6-4520-9d7c-3a83915c7717@oss.qualcomm.com/
> 
>> And it was pointed out in the v1 patch discussion that this change was
>> needed:
>> https://lore.kernel.org/all/c80c48a1-f1b6-4520-9d7c-3a83915c7717@oss.qualcomm.com/
> 
> Were the checks removed from the caller side like was asked for?

Currently, I have placed the check inside the API and removed all the 
checks outside the API.

> 
> Also, your changelog makes it sound like this is a real bugfix for
> something, when it is not at all, which is what I object to the most.
> Don't make scary changelogs for things that are not actually happening.

You are correct. I will modify the commit text that caused the 
misunderstanding.

> 
> thanks,
> 
> greg k-h

Re: [PATCH v2 1/4] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Bjorn Andersson]

On Tue, Feb 03, 2026 at 08:08:16PM +0800, Jianping wrote:
> 
> 
> On 2/2/2026 4:41 PM, Greg KH wrote:
> > On Mon, Feb 02, 2026 at 03:13:10PM +0800, Jianping wrote:
> > > 
> > > 
> > > On 1/16/2026 10:49 PM, Greg KH wrote:
> > > > On Thu, Jan 15, 2026 at 04:28:48PM +0800, Jianping Li wrote:
> > > > > From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> > > > > 
> > > > > The fastrpc_buf_free function currently does not handle the case where
> > > > > the input buffer pointer (buf) is NULL. This can lead to a null pointer
> > > > > dereference, causing a crash or undefined behavior when the function
> > > > > attempts to access members of the buf structure. Add a NULL check to
> > > > > ensure safe handling of NULL pointers and prevent potential crashes.
> > > > 
> > > > What caller passes in NULL here?  I did a quick look, and see where the
> > > > callers check this properly if it could be NULL, otherwise it all looks
> > > > sane to me.  What in-kernel user is causing a crash here?  Why not fix
> > > > the caller up instead?
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > It's a saftety coding: to eliminate NULL checks on the caller side, as we do
> > > in a lot of other kernel API.
> > 
> > But you do not do that for all functions in the kernel, otherwise the
> > kernel would be full of checks that are never hit at all.
> To clarify the intention: this change was not triggered by any real crash in
> current callers. The motivation came from the v1 review discussion [1],
> where it was suggested that a NULL check in fastrpc_buf_free() would allow
> simplifying some of the caller paths.
> 
> [1] https://lore.kernel.org/all/c80c48a1-f1b6-4520-9d7c-3a83915c7717@oss.qualcomm.com/
> > 
> > > And it was pointed out in the v1 patch discussion that this change was
> > > needed:
> > > https://lore.kernel.org/all/c80c48a1-f1b6-4520-9d7c-3a83915c7717@oss.qualcomm.com/
> > 
> > Were the checks removed from the caller side like was asked for?
> 
> Currently, I have placed the check inside the API and removed all the checks
> outside the API.
> 
> > 
> > Also, your changelog makes it sound like this is a real bugfix for
> > something, when it is not at all, which is what I object to the most.
> > Don't make scary changelogs for things that are not actually happening.
> 
> You are correct. I will modify the commit text that caused the
> misunderstanding.
> 

You should then also drop Cc: stable and Fixes:, as this is no longer a
bug fix. And make sure you don't put actual bug fixes after this one in
the series (i.e. it probably shouldn't be patch 1/4).

Regards,
Bjorn

> > 
> > thanks,
> > 
> > greg k-h
> 
> 

Re: [PATCH v2 1/4] misc: fastrpc: Add NULL check to fastrpc_buf_free to prevent crash

[Jianping]

On 2/4/2026 5:32 AM, Bjorn Andersson wrote:
> On Tue, Feb 03, 2026 at 08:08:16PM +0800, Jianping wrote:
>>
>>
>> On 2/2/2026 4:41 PM, Greg KH wrote:
>>> On Mon, Feb 02, 2026 at 03:13:10PM +0800, Jianping wrote:
>>>>
>>>>
>>>> On 1/16/2026 10:49 PM, Greg KH wrote:
>>>>> On Thu, Jan 15, 2026 at 04:28:48PM +0800, Jianping Li wrote:
>>>>>> From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>>>>>>
>>>>>> The fastrpc_buf_free function currently does not handle the case where
>>>>>> the input buffer pointer (buf) is NULL. This can lead to a null pointer
>>>>>> dereference, causing a crash or undefined behavior when the function
>>>>>> attempts to access members of the buf structure. Add a NULL check to
>>>>>> ensure safe handling of NULL pointers and prevent potential crashes.
>>>>>
>>>>> What caller passes in NULL here?  I did a quick look, and see where the
>>>>> callers check this properly if it could be NULL, otherwise it all looks
>>>>> sane to me.  What in-kernel user is causing a crash here?  Why not fix
>>>>> the caller up instead?
>>>>>
>>>>> thanks,
>>>>>
>>>>> greg k-h
>>>>
>>>> It's a saftety coding: to eliminate NULL checks on the caller side, as we do
>>>> in a lot of other kernel API.
>>>
>>> But you do not do that for all functions in the kernel, otherwise the
>>> kernel would be full of checks that are never hit at all.
>> To clarify the intention: this change was not triggered by any real crash in
>> current callers. The motivation came from the v1 review discussion [1],
>> where it was suggested that a NULL check in fastrpc_buf_free() would allow
>> simplifying some of the caller paths.
>>
>> [1] https://lore.kernel.org/all/c80c48a1-f1b6-4520-9d7c-3a83915c7717@oss.qualcomm.com/
>>>
>>>> And it was pointed out in the v1 patch discussion that this change was
>>>> needed:
>>>> https://lore.kernel.org/all/c80c48a1-f1b6-4520-9d7c-3a83915c7717@oss.qualcomm.com/
>>>
>>> Were the checks removed from the caller side like was asked for?
>>
>> Currently, I have placed the check inside the API and removed all the checks
>> outside the API.
>>
>>>
>>> Also, your changelog makes it sound like this is a real bugfix for
>>> something, when it is not at all, which is what I object to the most.
>>> Don't make scary changelogs for things that are not actually happening.
>>
>> You are correct. I will modify the commit text that caused the
>> misunderstanding.
>>
> 
> You should then also drop Cc: stable and Fixes:, as this is no longer a
> bug fix. And make sure you don't put actual bug fixes after this one in
> the series (i.e. it probably shouldn't be patch 1/4).
> 
> Regards,
> Bjorn

Thank Bjorn for the reminder, I will adjust the order of my patch.

> 
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>>

[PATCH v2 2/4] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Jianping Li]

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

The initially allocated memory is not properly included in the pool,
leading to potential issues with memory management. The issue is
actually a memory leak because the initial memory is never used by
Audio PD. It will immediately make a remote heap request as no memory is
added to the pool initially. Set the number of pages to one to ensure
that the initially allocated memory is correctly added to the Audio PD
memory pool.

Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
Cc: stable@kernel.org
Co-developed-by: Ekansh Gupta <ekansh.gupta@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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 515a43c9d95d..4f12fa5a05aa 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1328,7 +1328,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);
@@ -1351,12 +1353,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

Re: [PATCH v2 2/4] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Dmitry Baryshkov]

On Thu, Jan 15, 2026 at 04:28:49PM +0800, Jianping Li wrote:
> From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> 
> The initially allocated memory is not properly included in the pool,
> leading to potential issues with memory management. The issue is

Define "properly" and be more explicit about "potential issues". Please
be more precise in commit messages.

> actually a memory leak because the initial memory is never used by

Why is it not used?

> Audio PD. It will immediately make a remote heap request as no memory is

Ok, you've described one issue. Beforehand it was "issues". Are there
any others? if not, please drop the "potential issues" part.

> added to the pool initially. Set the number of pages to one to ensure
> that the initially allocated memory is correctly added to the Audio PD
> memory pool.
> 
> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
> Cc: stable@kernel.org
> Co-developed-by: Ekansh Gupta <ekansh.gupta@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 | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 

The patch  LGTM.

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/4] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Jianping]

On 1/16/2026 4:45 AM, Dmitry Baryshkov wrote:
> On Thu, Jan 15, 2026 at 04:28:49PM +0800, Jianping Li wrote:
>> From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>>
>> The initially allocated memory is not properly included in the pool,
>> leading to potential issues with memory management. The issue is
> 
> Define "properly" and be more explicit about "potential issues". Please
> be more precise in commit messages.
By “properly” I mean that the initially allocated buffer is supposed to 
be added into the Audio PD memory pool by setting pageslen accordingly.

With pageslen = 0, this buffer is never registered and therefore never 
becomes part of the pool.
I will drop the vague wording and describe the exact problem.
> 
>> actually a memory leak because the initial memory is never used by
> 
> Why is it not used?
Because pageslen = 0 indicates that no pages are provided.

As a result, Audio PD immediately issues a remote heap request, ignoring 
the initially allocated memory entirely.

That initial buffer becomes unreachable and is effectively leaked.
> 
>> Audio PD. It will immediately make a remote heap request as no memory is
> 
> Ok, you've described one issue. Beforehand it was "issues". Are there
> any others? if not, please drop the "potential issues" part.
There are no additional issues beyond the memory leak caused by the 
unused initial buffer.

I will remove the “potential issues” phrasing and state explicitly that 
the problem is a memory leak due to the initial buffer never being added 
to the pool.
> 
>> added to the pool initially. Set the number of pages to one to ensure
>> that the initially allocated memory is correctly added to the Audio PD
>> memory pool.
>>
>> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
>> Cc: stable@kernel.org
>> Co-developed-by: Ekansh Gupta <ekansh.gupta@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 | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
> 
> The patch  LGTM.
> 

[PATCH v2 3/4] 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 any other thread removes the entry
from list while unmap operation is ongoing. Remove the entry before
calling unmap operation.

Fixes: 2419e55e532de ("misc: fastrpc: add mmap/unmap support")
Cc: stable@kernel.org
Co-developed-by: Ekansh Gupta <ekansh.gupta@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 | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 4f12fa5a05aa..833c265add5e 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -202,6 +202,8 @@ struct fastrpc_buf {
 	/* mmap support */
 	struct list_head node; /* list of user requested mmaps */
 	uintptr_t raddr;
+	/* Lock for buf->node */
+	spinlock_t *list_lock;
 };
 
 struct fastrpc_dma_buf_attachment {
@@ -441,6 +443,7 @@ static int __fastrpc_buf_alloc(struct fastrpc_user *fl, struct device *dev,
 	buf->size = size;
 	buf->dev = dev;
 	buf->raddr = 0;
+	buf->list_lock = &fl->lock;
 
 	buf->virt = dma_alloc_coherent(dev, buf->size, &buf->dma_addr,
 				       GFP_KERNEL);
@@ -1865,9 +1868,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);
@@ -1881,6 +1881,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;
@@ -1888,6 +1889,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;
 		}
@@ -1900,7 +1902,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(buf->list_lock);
+		list_add_tail(&buf->node, &fl->mmaps);
+		spin_unlock(buf->list_lock);
+	}
+
+	return err;
 }
 
 static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
@@ -1985,20 +1994,23 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 		}
 	}
 
-	spin_lock(&fl->lock);
+	spin_lock(buf->list_lock);
 	list_add_tail(&buf->node, &fl->mmaps);
-	spin_unlock(&fl->lock);
+	spin_unlock(buf->list_lock);
 
 	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
 		err = -EFAULT;
-		goto err_assign;
+		goto err_copy;
 	}
 
 	dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
 		buf->raddr, buf->size);
 
 	return 0;
-
+err_copy:
+	spin_lock(buf->list_lock);
+	list_del(&buf->node);
+	spin_unlock(buf->list_lock);
 err_assign:
 	fastrpc_req_munmap_impl(fl, buf);
 
-- 
2.43.0

Re: [PATCH v2 3/4] misc: fastrpc: Remove buffer from list prior to unmap operation

[Dmitry Baryshkov]

On Thu, Jan 15, 2026 at 04:28:50PM +0800, Jianping Li wrote:
> 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 any other thread removes the entry
> from list while unmap operation is ongoing. Remove the entry before
> calling unmap operation.
> 
> Fixes: 2419e55e532de ("misc: fastrpc: add mmap/unmap support")
> Cc: stable@kernel.org
> Co-developed-by: Ekansh Gupta <ekansh.gupta@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 | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 4f12fa5a05aa..833c265add5e 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -202,6 +202,8 @@ struct fastrpc_buf {
>  	/* mmap support */
>  	struct list_head node; /* list of user requested mmaps */
>  	uintptr_t raddr;
> +	/* Lock for buf->node */
> +	spinlock_t *list_lock;

Why do you need to lock this? Isn't fl->lock enough?

>  };
>  
>  struct fastrpc_dma_buf_attachment {
> @@ -441,6 +443,7 @@ static int __fastrpc_buf_alloc(struct fastrpc_user *fl, struct device *dev,
>  	buf->size = size;
>  	buf->dev = dev;
>  	buf->raddr = 0;
> +	buf->list_lock = &fl->lock;
>  
>  	buf->virt = dma_alloc_coherent(dev, buf->size, &buf->dma_addr,
>  				       GFP_KERNEL);
> @@ -1865,9 +1868,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);
> @@ -1881,6 +1881,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;
> @@ -1888,6 +1889,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;
>  		}
> @@ -1900,7 +1902,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(buf->list_lock);
> +		list_add_tail(&buf->node, &fl->mmaps);
> +		spin_unlock(buf->list_lock);
> +	}
> +
> +	return err;
>  }
>  
>  static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> @@ -1985,20 +1994,23 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>  		}
>  	}
>  
> -	spin_lock(&fl->lock);
> +	spin_lock(buf->list_lock);
>  	list_add_tail(&buf->node, &fl->mmaps);
> -	spin_unlock(&fl->lock);
> +	spin_unlock(buf->list_lock);
>  
>  	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
>  		err = -EFAULT;
> -		goto err_assign;
> +		goto err_copy;
>  	}
>  
>  	dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
>  		buf->raddr, buf->size);
>  
>  	return 0;
> -
> +err_copy:
> +	spin_lock(buf->list_lock);
> +	list_del(&buf->node);
> +	spin_unlock(buf->list_lock);
>  err_assign:
>  	fastrpc_req_munmap_impl(fl, buf);
>  
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/4] misc: fastrpc: Remove buffer from list prior to unmap operation

[Jianping]

On 1/16/2026 4:47 AM, Dmitry Baryshkov wrote:
> On Thu, Jan 15, 2026 at 04:28:50PM +0800, Jianping Li wrote:
>> 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 any other thread removes the entry
>> from list while unmap operation is ongoing. Remove the entry before
>> calling unmap operation.
>>
>> Fixes: 2419e55e532de ("misc: fastrpc: add mmap/unmap support")
>> Cc: stable@kernel.org
>> Co-developed-by: Ekansh Gupta <ekansh.gupta@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 | 28 ++++++++++++++++++++--------
>>   1 file changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 4f12fa5a05aa..833c265add5e 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -202,6 +202,8 @@ struct fastrpc_buf {
>>   	/* mmap support */
>>   	struct list_head node; /* list of user requested mmaps */
>>   	uintptr_t raddr;
>> +	/* Lock for buf->node */
>> +	spinlock_t *list_lock;
> 
> Why do you need to lock this? Isn't fl->lock enough?

According to the discussion in v1 patch:
https://lore.kernel.org/all/p6cc5lxufmefeulx5bhlh6q6ivwluqf2muj3hu5e5526fsppuu@brcy6arm7epg/

The lock is stored in fastrpc_buf here.
> 
>>   };
>>   
>>   struct fastrpc_dma_buf_attachment {
>> @@ -441,6 +443,7 @@ static int __fastrpc_buf_alloc(struct fastrpc_user *fl, struct device *dev,
>>   	buf->size = size;
>>   	buf->dev = dev;
>>   	buf->raddr = 0;
>> +	buf->list_lock = &fl->lock;
>>   
>>   	buf->virt = dma_alloc_coherent(dev, buf->size, &buf->dma_addr,
>>   				       GFP_KERNEL);
>> @@ -1865,9 +1868,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);
>> @@ -1881,6 +1881,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;
>> @@ -1888,6 +1889,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;
>>   		}
>> @@ -1900,7 +1902,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(buf->list_lock);
>> +		list_add_tail(&buf->node, &fl->mmaps);
>> +		spin_unlock(buf->list_lock);
>> +	}
>> +
>> +	return err;
>>   }
>>   
>>   static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>> @@ -1985,20 +1994,23 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>>   		}
>>   	}
>>   
>> -	spin_lock(&fl->lock);
>> +	spin_lock(buf->list_lock);
>>   	list_add_tail(&buf->node, &fl->mmaps);
>> -	spin_unlock(&fl->lock);
>> +	spin_unlock(buf->list_lock);
>>   
>>   	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
>>   		err = -EFAULT;
>> -		goto err_assign;
>> +		goto err_copy;
>>   	}
>>   
>>   	dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
>>   		buf->raddr, buf->size);
>>   
>>   	return 0;
>> -
>> +err_copy:
>> +	spin_lock(buf->list_lock);
>> +	list_del(&buf->node);
>> +	spin_unlock(buf->list_lock);
>>   err_assign:
>>   	fastrpc_req_munmap_impl(fl, buf);
>>   
>> -- 
>> 2.43.0
>>
> 

Re: [PATCH v2 3/4] misc: fastrpc: Remove buffer from list prior to unmap operation

[Dmitry Baryshkov]

On Mon, Feb 02, 2026 at 02:51:33PM +0800, Jianping wrote:
> 
> 
> On 1/16/2026 4:47 AM, Dmitry Baryshkov wrote:
> > On Thu, Jan 15, 2026 at 04:28:50PM +0800, Jianping Li wrote:
> > > 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 any other thread removes the entry
> > > from list while unmap operation is ongoing. Remove the entry before
> > > calling unmap operation.
> > > 
> > > Fixes: 2419e55e532de ("misc: fastrpc: add mmap/unmap support")
> > > Cc: stable@kernel.org
> > > Co-developed-by: Ekansh Gupta <ekansh.gupta@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 | 28 ++++++++++++++++++++--------
> > >   1 file changed, 20 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > > index 4f12fa5a05aa..833c265add5e 100644
> > > --- a/drivers/misc/fastrpc.c
> > > +++ b/drivers/misc/fastrpc.c
> > > @@ -202,6 +202,8 @@ struct fastrpc_buf {
> > >   	/* mmap support */
> > >   	struct list_head node; /* list of user requested mmaps */
> > >   	uintptr_t raddr;
> > > +	/* Lock for buf->node */
> > > +	spinlock_t *list_lock;
> > 
> > Why do you need to lock this? Isn't fl->lock enough?
> 
> According to the discussion in v1 patch:
> https://lore.kernel.org/all/p6cc5lxufmefeulx5bhlh6q6ivwluqf2muj3hu5e5526fsppuu@brcy6arm7epg/
> 
> The lock is stored in fastrpc_buf here.

That was a separate topic. So, why fl->lock isn't enough? What is the
race that isn't prevented by it?

> > 
> > >   };
> > >   struct fastrpc_dma_buf_attachment {
> > > @@ -441,6 +443,7 @@ static int __fastrpc_buf_alloc(struct fastrpc_user *fl, struct device *dev,
> > >   	buf->size = size;
> > >   	buf->dev = dev;
> > >   	buf->raddr = 0;
> > > +	buf->list_lock = &fl->lock;
> > >   	buf->virt = dma_alloc_coherent(dev, buf->size, &buf->dma_addr,
> > >   				       GFP_KERNEL);
> > > @@ -1865,9 +1868,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);
> > > @@ -1881,6 +1881,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;
> > > @@ -1888,6 +1889,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;
> > >   		}
> > > @@ -1900,7 +1902,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(buf->list_lock);
> > > +		list_add_tail(&buf->node, &fl->mmaps);
> > > +		spin_unlock(buf->list_lock);
> > > +	}
> > > +
> > > +	return err;
> > >   }
> > >   static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> > > @@ -1985,20 +1994,23 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
> > >   		}
> > >   	}
> > > -	spin_lock(&fl->lock);
> > > +	spin_lock(buf->list_lock);
> > >   	list_add_tail(&buf->node, &fl->mmaps);
> > > -	spin_unlock(&fl->lock);
> > > +	spin_unlock(buf->list_lock);
> > >   	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
> > >   		err = -EFAULT;
> > > -		goto err_assign;
> > > +		goto err_copy;
> > >   	}
> > >   	dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
> > >   		buf->raddr, buf->size);
> > >   	return 0;
> > > -
> > > +err_copy:
> > > +	spin_lock(buf->list_lock);
> > > +	list_del(&buf->node);
> > > +	spin_unlock(buf->list_lock);
> > >   err_assign:
> > >   	fastrpc_req_munmap_impl(fl, buf);
> > > -- 
> > > 2.43.0
> > > 
> > 
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/4] misc: fastrpc: Remove buffer from list prior to unmap operation

[Jianping]

On 2/4/2026 5:15 AM, Dmitry Baryshkov wrote:
> On Mon, Feb 02, 2026 at 02:51:33PM +0800, Jianping wrote:
>>
>>
>> On 1/16/2026 4:47 AM, Dmitry Baryshkov wrote:
>>> On Thu, Jan 15, 2026 at 04:28:50PM +0800, Jianping Li wrote:
>>>> 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 any other thread removes the entry
>>>> from list while unmap operation is ongoing. Remove the entry before
>>>> calling unmap operation.
>>>>
>>>> Fixes: 2419e55e532de ("misc: fastrpc: add mmap/unmap support")
>>>> Cc: stable@kernel.org
>>>> Co-developed-by: Ekansh Gupta <ekansh.gupta@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 | 28 ++++++++++++++++++++--------
>>>>    1 file changed, 20 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index 4f12fa5a05aa..833c265add5e 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -202,6 +202,8 @@ struct fastrpc_buf {
>>>>    	/* mmap support */
>>>>    	struct list_head node; /* list of user requested mmaps */
>>>>    	uintptr_t raddr;
>>>> +	/* Lock for buf->node */
>>>> +	spinlock_t *list_lock;
>>>
>>> Why do you need to lock this? Isn't fl->lock enough?
>>
>> According to the discussion in v1 patch:
>> https://lore.kernel.org/all/p6cc5lxufmefeulx5bhlh6q6ivwluqf2muj3hu5e5526fsppuu@brcy6arm7epg/
>>
>> The lock is stored in fastrpc_buf here.
> 
> That was a separate topic. So, why fl->lock isn't enough? What is the
> race that isn't prevented by it?
"Is fl->lock not enough?" — At the granularity of the lock, fl->lock is 
sufficient. And you can see buf->list_lock reuses fl->lock.

The purpose of doing this is to pass along lock together with the buf 
via the buf->list_lock pointer, so that all operations on buf->node no 
longer need to look for fl, reducing coupling and lowering the chance of 
errors.

If you think buf->list_lock is unnecessary, I can remove it and use 
fl->lock that makes more sense.

> 
>>>
>>>>    };
>>>>    struct fastrpc_dma_buf_attachment {
>>>> @@ -441,6 +443,7 @@ static int __fastrpc_buf_alloc(struct fastrpc_user *fl, struct device *dev,
>>>>    	buf->size = size;
>>>>    	buf->dev = dev;
>>>>    	buf->raddr = 0;
>>>> +	buf->list_lock = &fl->lock;
>>>>    	buf->virt = dma_alloc_coherent(dev, buf->size, &buf->dma_addr,
>>>>    				       GFP_KERNEL);
>>>> @@ -1865,9 +1868,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);
>>>> @@ -1881,6 +1881,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;
>>>> @@ -1888,6 +1889,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;
>>>>    		}
>>>> @@ -1900,7 +1902,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(buf->list_lock);
>>>> +		list_add_tail(&buf->node, &fl->mmaps);
>>>> +		spin_unlock(buf->list_lock);
>>>> +	}
>>>> +
>>>> +	return err;
>>>>    }
>>>>    static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>>>> @@ -1985,20 +1994,23 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>>>>    		}
>>>>    	}
>>>> -	spin_lock(&fl->lock);
>>>> +	spin_lock(buf->list_lock);
>>>>    	list_add_tail(&buf->node, &fl->mmaps);
>>>> -	spin_unlock(&fl->lock);
>>>> +	spin_unlock(buf->list_lock);
>>>>    	if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
>>>>    		err = -EFAULT;
>>>> -		goto err_assign;
>>>> +		goto err_copy;
>>>>    	}
>>>>    	dev_dbg(dev, "mmap\t\tpt 0x%09lx OK [len 0x%08llx]\n",
>>>>    		buf->raddr, buf->size);
>>>>    	return 0;
>>>> -
>>>> +err_copy:
>>>> +	spin_lock(buf->list_lock);
>>>> +	list_del(&buf->node);
>>>> +	spin_unlock(buf->list_lock);
>>>>    err_assign:
>>>>    	fastrpc_req_munmap_impl(fl, buf);
>>>> -- 
>>>> 2.43.0
>>>>
>>>
>>
> 

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

[Jianping Li]

The entire reserved-memory region is now assigned to DSP VMIDs during
channel setup and stored in cctx->remote_heap. Memory is reclaimed in
rpmsg_remove by revoking DSP permissions and freeing the buffer, tying
heap lifecycle to the rpmsg channel.

Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 95 ++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 50 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 833c265add5e..f9edca7a8de1 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -278,6 +278,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;
@@ -1304,7 +1306,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	struct fastrpc_phy_page pages[1];
 	char *name;
 	int err;
-	bool scm_done = false;
 	struct {
 		int client_id;
 		u32 namelen;
@@ -1334,31 +1335,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;
 
@@ -1370,8 +1346,15 @@ 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;
+	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;
+	}
 
 	args[2].ptr = (u64)(uintptr_t) pages;
 	args[2].length = sizeof(*pages);
@@ -1389,26 +1372,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);
-err_name:
+	fl->cctx->audio_init_mem = false;
 	kfree(name);
 err:
 	kfree(args);
@@ -2396,7 +2360,7 @@ 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;
 
@@ -2408,6 +2372,15 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 				    data->vmperms, data->vmcount);
 		}
 
+		if (domain_id == ADSP_DOMAIN_ID) {
+			data->remote_heap =
+				kzalloc(sizeof(*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);
+		}
 	}
 
 	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
@@ -2488,10 +2461,13 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 	struct fastrpc_buf *buf, *b;
 	struct fastrpc_user *user;
 	unsigned long flags;
+	bool skip_free = false;
+	int err;
 
 	/* No invocations past this point */
 	spin_lock_irqsave(&cctx->lock, flags);
 	cctx->rpdev = NULL;
+	cctx->audio_init_mem = false;
 	list_for_each_entry(user, &cctx->users, user)
 		fastrpc_notify_users(user);
 	spin_unlock_irqrestore(&cctx->lock, flags);
@@ -2505,7 +2481,26 @@ 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);
 
-	fastrpc_buf_free(cctx->remote_heap);
+	if (cctx->remote_heap) {
+		if (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)
+				skip_free = true;
+		}
+		if (!skip_free)
+			fastrpc_buf_free(cctx->remote_heap);
+	}
 
 	of_platform_depopulate(&rpdev->dev);
 
-- 
2.43.0

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

[Dmitry Baryshkov]

On Thu, Jan 15, 2026 at 04:28:51PM +0800, Jianping Li wrote:
> The entire reserved-memory region is now assigned to DSP VMIDs during
> channel setup and stored in cctx->remote_heap. Memory is reclaimed in
> rpmsg_remove by revoking DSP permissions and freeing the buffer, tying
> heap lifecycle to the rpmsg channel.

Why?

> 
> Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
> ---
>  drivers/misc/fastrpc.c | 95 ++++++++++++++++++++----------------------
>  1 file changed, 45 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 833c265add5e..f9edca7a8de1 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -278,6 +278,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;
> @@ -1304,7 +1306,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  	struct fastrpc_phy_page pages[1];
>  	char *name;
>  	int err;
> -	bool scm_done = false;
>  	struct {
>  		int client_id;
>  		u32 namelen;
> @@ -1334,31 +1335,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;
>  
> @@ -1370,8 +1346,15 @@ 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;
> +	if (!fl->cctx->audio_init_mem) {
> +		pages[0].addr = fl->cctx->remote_heap->dma_addr;
> +		pages[0].size = fl->cctx->remote_heap->size;

Do we need a flag? Can't we assume that remote_heap is always to be
allocated to the PD?

> +		fl->cctx->audio_init_mem = true;
> +		inbuf.pageslen = 1;
> +	} else {
> +		pages[0].addr = 0;
> +		pages[0].size = 0;
> +	}
>  
>  	args[2].ptr = (u64)(uintptr_t) pages;
>  	args[2].length = sizeof(*pages);
> @@ -1389,26 +1372,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);
> -err_name:
> +	fl->cctx->audio_init_mem = false;
>  	kfree(name);
>  err:
>  	kfree(args);
> @@ -2396,7 +2360,7 @@ 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;
>  
> @@ -2408,6 +2372,15 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  				    data->vmperms, data->vmcount);
>  		}
>  
> +		if (domain_id == ADSP_DOMAIN_ID) {
> +			data->remote_heap =
> +				kzalloc(sizeof(*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);
> +		}
>  	}
>  
>  	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
> @@ -2488,10 +2461,13 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>  	struct fastrpc_buf *buf, *b;
>  	struct fastrpc_user *user;
>  	unsigned long flags;
> +	bool skip_free = false;
> +	int err;
>  
>  	/* No invocations past this point */
>  	spin_lock_irqsave(&cctx->lock, flags);
>  	cctx->rpdev = NULL;
> +	cctx->audio_init_mem = false;
>  	list_for_each_entry(user, &cctx->users, user)
>  		fastrpc_notify_users(user);
>  	spin_unlock_irqrestore(&cctx->lock, flags);
> @@ -2505,7 +2481,26 @@ 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);
>  
> -	fastrpc_buf_free(cctx->remote_heap);
> +	if (cctx->remote_heap) {
> +		if (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)
> +				skip_free = true;
> +		}
> +		if (!skip_free)
> +			fastrpc_buf_free(cctx->remote_heap);
> +	}
>  
>  	of_platform_depopulate(&rpdev->dev);
>  
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry

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

[Jianping]

On 1/16/2026 4:49 AM, Dmitry Baryshkov wrote:
> On Thu, Jan 15, 2026 at 04:28:51PM +0800, Jianping Li wrote:
>> The entire reserved-memory region is now assigned to DSP VMIDs during
>> channel setup and stored in cctx->remote_heap. Memory is reclaimed in
>> rpmsg_remove by revoking DSP permissions and freeing the buffer, tying
>> heap lifecycle to the rpmsg channel.
> 
> Why?
The reason for allocating the entire reserved‑memory region is to avoid 
unsafe alloc/free
operations from user.

This design(alloc/free from user) was fragile because:
Userspace could request free while the DSP was still using the memory.
There was no reliable, kernel‑controlled way to know when the DSP had 
finished using pages.
Remote‑side “reverse fastrpc” free requests could not be safely trusted 
or validated.

Allocating the full reserved region upfront removes the need for alloc 
from user completely.
This way the free can be moved to fastrpc_rpmsg_remove(When DSP is 
shutting down).

And I will be adding this detail in commit text also.
> 
>>
>> Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
>> ---
>>   drivers/misc/fastrpc.c | 95 ++++++++++++++++++++----------------------
>>   1 file changed, 45 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 833c265add5e..f9edca7a8de1 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -278,6 +278,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;
>> @@ -1304,7 +1306,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>   	struct fastrpc_phy_page pages[1];
>>   	char *name;
>>   	int err;
>> -	bool scm_done = false;
>>   	struct {
>>   		int client_id;
>>   		u32 namelen;
>> @@ -1334,31 +1335,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;
>>   
>> @@ -1370,8 +1346,15 @@ 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;
>> +	if (!fl->cctx->audio_init_mem) {
>> +		pages[0].addr = fl->cctx->remote_heap->dma_addr;
>> +		pages[0].size = fl->cctx->remote_heap->size;
> 
> Do we need a flag? Can't we assume that remote_heap is always to be
> allocated to the PD?
We do need the audio_init_mem flag.
Once the PD starts and daemon takes the memory for the first time, PD 
will start using the memory,
meanwhile, the daemon can be killed and restarted. In this case, the 
memory is still with the PD and the next
daemon connection should not take any memory for the next request. This 
flag is maintained to ensure that.
The memory needs to be resent only if Audio PD on DSP restarts(due to 
PD-restart or Subsystem-restart)

> 
>> +		fl->cctx->audio_init_mem = true;
>> +		inbuf.pageslen = 1;
>> +	} else {
>> +		pages[0].addr = 0;
>> +		pages[0].size = 0;
>> +	}
>>   
>>   	args[2].ptr = (u64)(uintptr_t) pages;
>>   	args[2].length = sizeof(*pages);
>> @@ -1389,26 +1372,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);
>> -err_name:
>> +	fl->cctx->audio_init_mem = false;
>>   	kfree(name);
>>   err:
>>   	kfree(args);
>> @@ -2396,7 +2360,7 @@ 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;
>>   
>> @@ -2408,6 +2372,15 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>   				    data->vmperms, data->vmcount);
>>   		}
>>   
>> +		if (domain_id == ADSP_DOMAIN_ID) {
>> +			data->remote_heap =
>> +				kzalloc(sizeof(*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);
>> +		}
>>   	}
>>   
>>   	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
>> @@ -2488,10 +2461,13 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>>   	struct fastrpc_buf *buf, *b;
>>   	struct fastrpc_user *user;
>>   	unsigned long flags;
>> +	bool skip_free = false;
>> +	int err;
>>   
>>   	/* No invocations past this point */
>>   	spin_lock_irqsave(&cctx->lock, flags);
>>   	cctx->rpdev = NULL;
>> +	cctx->audio_init_mem = false;
>>   	list_for_each_entry(user, &cctx->users, user)
>>   		fastrpc_notify_users(user);
>>   	spin_unlock_irqrestore(&cctx->lock, flags);
>> @@ -2505,7 +2481,26 @@ 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);
>>   
>> -	fastrpc_buf_free(cctx->remote_heap);
>> +	if (cctx->remote_heap) {
>> +		if (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)
>> +				skip_free = true;
>> +		}
>> +		if (!skip_free)
>> +			fastrpc_buf_free(cctx->remote_heap);
>> +	}
>>   
>>   	of_platform_depopulate(&rpdev->dev);
>>   
>> -- 
>> 2.43.0
>>
> 

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

[Dmitry Baryshkov]

On Mon, Feb 02, 2026 at 03:06:59PM +0800, Jianping wrote:
> 
> 
> On 1/16/2026 4:49 AM, Dmitry Baryshkov wrote:
> > On Thu, Jan 15, 2026 at 04:28:51PM +0800, Jianping Li wrote:
> > > The entire reserved-memory region is now assigned to DSP VMIDs during
> > > channel setup and stored in cctx->remote_heap. Memory is reclaimed in
> > > rpmsg_remove by revoking DSP permissions and freeing the buffer, tying
> > > heap lifecycle to the rpmsg channel.
> > 
> > > @@ -1370,8 +1346,15 @@ 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;
> > > +	if (!fl->cctx->audio_init_mem) {
> > > +		pages[0].addr = fl->cctx->remote_heap->dma_addr;
> > > +		pages[0].size = fl->cctx->remote_heap->size;
> > 
> > Do we need a flag? Can't we assume that remote_heap is always to be
> > allocated to the PD?
> We do need the audio_init_mem flag.
> Once the PD starts and daemon takes the memory for the first time, PD will
> start using the memory,
> meanwhile, the daemon can be killed and restarted. In this case, the memory
> is still with the PD and the next
> daemon connection should not take any memory for the next request. This flag
> is maintained to ensure that.
> The memory needs to be resent only if Audio PD on DSP restarts(due to
> PD-restart or Subsystem-restart)

This needs to be explained in the changelog.

> 
> > 
> > > +		fl->cctx->audio_init_mem = true;

What if there are two racing IOCTLs, trying to init AudioPD process?

> > > +		inbuf.pageslen = 1;
> > > +	} else {
> > > +		pages[0].addr = 0;
> > > +		pages[0].size = 0;
> > > +	}
> > >   	args[2].ptr = (u64)(uintptr_t) pages;
> > >   	args[2].length = sizeof(*pages);

-- 
With best wishes
Dmitry

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

[Bjorn Andersson]

On Tue, Feb 03, 2026 at 11:19:39PM +0200, Dmitry Baryshkov wrote:
> On Mon, Feb 02, 2026 at 03:06:59PM +0800, Jianping wrote:
> > 
> > 
> > On 1/16/2026 4:49 AM, Dmitry Baryshkov wrote:
> > > On Thu, Jan 15, 2026 at 04:28:51PM +0800, Jianping Li wrote:
> > > > The entire reserved-memory region is now assigned to DSP VMIDs during
> > > > channel setup and stored in cctx->remote_heap. Memory is reclaimed in
> > > > rpmsg_remove by revoking DSP permissions and freeing the buffer, tying
> > > > heap lifecycle to the rpmsg channel.
> > > 
> > > > @@ -1370,8 +1346,15 @@ 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;
> > > > +	if (!fl->cctx->audio_init_mem) {
> > > > +		pages[0].addr = fl->cctx->remote_heap->dma_addr;
> > > > +		pages[0].size = fl->cctx->remote_heap->size;
> > > 
> > > Do we need a flag? Can't we assume that remote_heap is always to be
> > > allocated to the PD?
> > We do need the audio_init_mem flag.
> > Once the PD starts and daemon takes the memory for the first time, PD will
> > start using the memory,
> > meanwhile, the daemon can be killed and restarted. In this case, the memory
> > is still with the PD and the next
> > daemon connection should not take any memory for the next request. This flag
> > is maintained to ensure that.
> > The memory needs to be resent only if Audio PD on DSP restarts(due to
> > PD-restart or Subsystem-restart)
> 
> This needs to be explained in the changelog.
> 

Not in the "changelog", in the commit message.


@Jianping please read https://docs.kernel.org/process/submitting-patches.html#describe-your-changes 

Your commit messages should "Describe your problem". From this
description it should be clear why the change is needed and why the
change is done in the specific way.

Regards,
Bjorn

> > 
> > > 
> > > > +		fl->cctx->audio_init_mem = true;
> 
> What if there are two racing IOCTLs, trying to init AudioPD process?
> 
> > > > +		inbuf.pageslen = 1;
> > > > +	} else {
> > > > +		pages[0].addr = 0;
> > > > +		pages[0].size = 0;
> > > > +	}
> > > >   	args[2].ptr = (u64)(uintptr_t) pages;
> > > >   	args[2].length = sizeof(*pages);
> 
> -- 
> With best wishes
> Dmitry
> 

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

[Jianping]

On 2/4/2026 5:42 AM, Bjorn Andersson wrote:
> On Tue, Feb 03, 2026 at 11:19:39PM +0200, Dmitry Baryshkov wrote:
>> On Mon, Feb 02, 2026 at 03:06:59PM +0800, Jianping wrote:
>>>
>>>
>>> On 1/16/2026 4:49 AM, Dmitry Baryshkov wrote:
>>>> On Thu, Jan 15, 2026 at 04:28:51PM +0800, Jianping Li wrote:
>>>>> The entire reserved-memory region is now assigned to DSP VMIDs during
>>>>> channel setup and stored in cctx->remote_heap. Memory is reclaimed in
>>>>> rpmsg_remove by revoking DSP permissions and freeing the buffer, tying
>>>>> heap lifecycle to the rpmsg channel.
>>>>
>>>>> @@ -1370,8 +1346,15 @@ 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;
>>>>> +	if (!fl->cctx->audio_init_mem) {
>>>>> +		pages[0].addr = fl->cctx->remote_heap->dma_addr;
>>>>> +		pages[0].size = fl->cctx->remote_heap->size;
>>>>
>>>> Do we need a flag? Can't we assume that remote_heap is always to be
>>>> allocated to the PD?
>>> We do need the audio_init_mem flag.
>>> Once the PD starts and daemon takes the memory for the first time, PD will
>>> start using the memory,
>>> meanwhile, the daemon can be killed and restarted. In this case, the memory
>>> is still with the PD and the next
>>> daemon connection should not take any memory for the next request. This flag
>>> is maintained to ensure that.
>>> The memory needs to be resent only if Audio PD on DSP restarts(due to
>>> PD-restart or Subsystem-restart)
>>
>> This needs to be explained in the changelog.
>>
> 
> Not in the "changelog", in the commit message.
> 
> 
> @Jianping please read https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> 
> Your commit messages should "Describe your problem". From this
> description it should be clear why the change is needed and why the
> change is done in the specific way.
> 
> Regards,
> Bjorn

Thanks Bjorn for the reminder, I will revise my commit message.

Thanks,
Jianping

> 
>>>
>>>>
>>>>> +		fl->cctx->audio_init_mem = true;
>>
>> What if there are two racing IOCTLs, trying to init AudioPD process?
>>
>>>>> +		inbuf.pageslen = 1;
>>>>> +	} else {
>>>>> +		pages[0].addr = 0;
>>>>> +		pages[0].size = 0;
>>>>> +	}
>>>>>    	args[2].ptr = (u64)(uintptr_t) pages;
>>>>>    	args[2].length = sizeof(*pages);
>>
>> -- 
>> With best wishes
>> Dmitry
>>

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

[Jianping]

On 2/4/2026 5:19 AM, Dmitry Baryshkov wrote:
> On Mon, Feb 02, 2026 at 03:06:59PM +0800, Jianping wrote:
>>
>>
>> On 1/16/2026 4:49 AM, Dmitry Baryshkov wrote:
>>> On Thu, Jan 15, 2026 at 04:28:51PM +0800, Jianping Li wrote:
>>>> The entire reserved-memory region is now assigned to DSP VMIDs during
>>>> channel setup and stored in cctx->remote_heap. Memory is reclaimed in
>>>> rpmsg_remove by revoking DSP permissions and freeing the buffer, tying
>>>> heap lifecycle to the rpmsg channel.
>>>
>>>> @@ -1370,8 +1346,15 @@ 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;
>>>> +	if (!fl->cctx->audio_init_mem) {
>>>> +		pages[0].addr = fl->cctx->remote_heap->dma_addr;
>>>> +		pages[0].size = fl->cctx->remote_heap->size;
>>>
>>> Do we need a flag? Can't we assume that remote_heap is always to be
>>> allocated to the PD?
>> We do need the audio_init_mem flag.
>> Once the PD starts and daemon takes the memory for the first time, PD will
>> start using the memory,
>> meanwhile, the daemon can be killed and restarted. In this case, the memory
>> is still with the PD and the next
>> daemon connection should not take any memory for the next request. This flag
>> is maintained to ensure that.
>> The memory needs to be resent only if Audio PD on DSP restarts(due to
>> PD-restart or Subsystem-restart)
> 
> This needs to be explained in the changelog.
I will add it.
> 
>>
>>>
>>>> +		fl->cctx->audio_init_mem = true;
> 
> What if there are two racing IOCTLs, trying to init AudioPD process?

There may be a chance that two threads enter ioctl at the same time, and 
seeing that audio_init_mem is false causes it to send twice, so a 
cctx->lock needs to be added here to ensure that two threads do not 
enter simultaneously.

> 
>>>> +		inbuf.pageslen = 1;
>>>> +	} else {
>>>> +		pages[0].addr = 0;
>>>> +		pages[0].size = 0;
>>>> +	}
>>>>    	args[2].ptr = (u64)(uintptr_t) pages;
>>>>    	args[2].length = sizeof(*pages);
>