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

[Jianping <jianping.li@oss.qualcomm.com>]

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
>>
>