Re: [PATCH v12] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe
Jianping Li <[email protected]>
| Newsgroups | org.freedesktop.lists.dri-devel |
|---|---|
| Message-ID | <[email protected]> |
On 8/20/2026 1:10 PM, Ekansh Gupta wrote:
> On 14-08-2026 15:49, 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.
>>
>> The reserved-memory region is now mandatory for the Audio PD domain.
>> Rather than failing rpmsg probe when it is missing, validate it in
>> fastrpc_init_create_static_process() and reject only the static-process
>> creation. This keeps the fastrpc device probing for all other domains
>> even on a misconfigured device tree.
>>
>> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
>> Cc:[email protected]
>> Signed-off-by: Jianping Li<[email protected]>
>> ---
>> Patch [v11]:https://lore.kernel.org/all/[email protected]/
>>
>> Changes in v12:
>> - Do not fail rpmsg probe when the reserved-memory region is missing,
>> validate the region in fastrpc_init_create_static_process() instead,
>> so probe keeps working for all domains.
>> - Add fastrpc_domain_has_reserved_heap() / fastrpc_domain_uses_static_heap()
>> helpers to replace the open-coded ADSP/SDSP domain checks.
>>
>> Changes in v11:
>> - Replace the remote_heap fastrpc_buf pointer with dedicated
>> remote_heap_addr and remote_heap_size fields in
>> fastrpc_channel_ctx to avoid leaving a partially
>> initialized fastrpc_buf.
>>
>> - Drop ADSP_MMAP_REMOTE_HEAP_ADDR support from
>> fastrpc_req_mmap() since the user process should no longer
>> grow or shrink the Audio PD remote heap.
>>
>> Changes in v10:
>> - Move Audio PD remote heap validation into
>> fastrpc_rpmsg_probe().
>>
>> - Treat Audio PD remote heap as a mandatory
>> resource and fail probe if the reserved
>> memory region is missing.
>>
>> Changes in v9:
>> - Make sure fastrpc_init_create_static_process()
>> only sets audio_init_mem to false when the sent
>> address is actually invalid.
>> ---
>> drivers/misc/fastrpc.c | 150 +++++++++++++++++++++--------------------
>> 1 file changed, 76 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 90fd669636ec..3f14a4673698 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -70,8 +70,6 @@
>> #define ADSP_MMAP_HEAP_ADDR 4
>> /* MAP static DMA buffer on DSP User PD */
>> #define ADSP_MMAP_DMA_BUFFER 6
>> -/* Add memory to static PD pool protection thru hypervisor */
>> -#define ADSP_MMAP_REMOTE_HEAP_ADDR 8
>> /* Add memory to userPD pool, for user heap */
>> #define ADSP_MMAP_ADD_PAGES 0x1000
>> /* Add memory to userPD pool, for LLC heap */
>> @@ -314,10 +312,14 @@ 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;
>> + /* Audio PD reserved remote heap region */
>> + phys_addr_t remote_heap_addr;
>> + u64 remote_heap_size;
>> u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
>> struct fastrpc_device *secure_fdevice;
>> struct fastrpc_device *fdevice;
>> - struct fastrpc_buf *remote_heap;
>> struct list_head invoke_interrupted_mmaps;
>> bool secure;
>> bool unsigned_support;
>> @@ -1454,15 +1456,24 @@ 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;
>> + bool sent_heap = false;
>> +
>> + if (!cctx->remote_heap_addr || !cctx->remote_heap_size) {
>> + err = -ENOMEM;
>> + dev_err(fl->sctx->dev,
>> + "remote heap memory region is not added\n");
>> + return err;
>> + }
>>
>> args = kzalloc_objs(*args, FASTRPC_CREATE_STATIC_PROCESS_NARGS);
>> if (!args)
>> @@ -1486,31 +1497,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;
>>
>> @@ -1522,8 +1508,25 @@ 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;
>> + /*
>> + * Audio PD is a static PD and retains the remote heap
>> + * information across daemon restarts. Therefore only
>> + * the first attach should provide heap information to
>> + * DSP. Subsequent attaches reuse the previously
>> + * initialized memory pool.
>> + */
>> + spin_lock_irqsave(&cctx->lock, flags);
>> + if (!cctx->audio_init_mem) {
>> + pages[0].addr = cctx->remote_heap_addr;
>> + pages[0].size = cctx->remote_heap_size;
>> + cctx->audio_init_mem = true;
>> + inbuf.pageslen = 1;
>> + sent_heap = true;
>> + } 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);
>> @@ -1541,27 +1544,11 @@ 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);
>> + if (sent_heap) {
>> + spin_lock_irqsave(&cctx->lock, flags);
>> + cctx->audio_init_mem = false;
>> + spin_unlock_irqrestore(&cctx->lock, flags);
>> }
>> -err_map:
>> - fastrpc_buf_free(fl->cctx->remote_heap);
>> - fl->cctx->remote_heap = NULL;
>> -err_name:
>> kfree(name);
>> err:
>> kfree(args);
>> @@ -2090,7 +2077,7 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>> if (copy_from_user(&req, argp, sizeof(req)))
>> return -EFAULT;
>>
>> - if (req.flags != ADSP_MMAP_ADD_PAGES && req.flags != ADSP_MMAP_REMOTE_HEAP_ADDR) {
>> + if (req.flags != ADSP_MMAP_ADD_PAGES) {
>> dev_err(dev, "flag not supported 0x%x\n", req.flags); return -EINVAL; @@ -2101,10 +2088,7 @@ static int
>> fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) return
>> -EINVAL; } - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR) - err =
>> fastrpc_remote_heap_alloc(fl, dev, req.size, &buf); - else - err =
>> fastrpc_buf_alloc(fl, dev, req.size, &buf); + err =
>> fastrpc_buf_alloc(fl, dev, req.size, &buf); if (err) { dev_err(dev, "failed to allocate buffer\n");
>> @@ -2143,20 +2127,6 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
>> /* let the client know the address to use */
>> req.vaddrout = rsp_msg.vaddr;
>>
>> - /* Add memory to static PD pool, protection thru hypervisor */
>> - if (req.flags == ADSP_MMAP_REMOTE_HEAP_ADDR && fl->cctx->vmcount) {
>> - u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
>> -
>> - err = qcom_scm_assign_mem(buf->dma_addr, (u64)buf->size,
>> - &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);
>> - if (err) {
>> - dev_err(fl->sctx->dev,
>> - "Failed to assign memory dma_addr %pad size 0x%llx err %d",
>> - &buf->dma_addr, buf->size, err);
>> - goto err_assign;
>> - }
>> - }
>> -
>> spin_lock(&fl->lock);
>> list_add_tail(&buf->node, &fl->mmaps);
>> spin_unlock(&fl->lock);
>> @@ -2537,6 +2507,16 @@ static const struct of_device_id fastrpc_poll_supported_machines[] __maybe_unuse
>> {},
>> };
>>
>> +static bool fastrpc_domain_has_reserved_heap(u32 domain_id)
>> +{
>> + return domain_id == SDSP_DOMAIN_ID || domain_id == ADSP_DOMAIN_ID;
>> +}
>> +
>> +static bool fastrpc_domain_uses_static_heap(u32 domain_id)
>> +{
>> + return domain_id == ADSP_DOMAIN_ID;
>> +}
> any reason to have functions for one time used checks?> +
Just to give some context: I added these helpers based on the v11
review, which requested replacing the open-coded ADSP/SDSP checks
with functions describing the per-domain capabilities.
In this patch, they currently have a single caller each.
Therefore, if you feel it's unnecessary, I can revert
to the version without helpers.
>> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> {
>> struct device *rdev = &rpdev->dev;
>> @@ -2584,20 +2564,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>> }
>> }
>>
>> - if (domain_id == SDSP_DOMAIN_ID) {
>> + if (fastrpc_domain_has_reserved_heap(domain_id)) {
>> struct resource res;
>> u64 src_perms;
>>
>> err = of_reserved_mem_region_to_resource(rdev->of_node, 0, &res);
>> if (!err) {
>> + if (fastrpc_domain_uses_static_heap(domain_id)) {
>> + data->remote_heap_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,
>> data->vmperms, data->vmcount);
> better to check vmcount before calling this> if (err)
Agreed. I'll wrap the qcom_scm_assign_mem() call in if (data->vmcount) in v13.
>> goto err_free_data;
>> + } else {
>> + err = 0;
>> }
>> -
>> }
>>
>> secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
>> @@ -2681,6 +2666,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>> struct fastrpc_buf *buf, *b;
>> struct fastrpc_user *user;
>> unsigned long flags;
>> + int err, i;
>>
>> /* No invocations past this point */
>> spin_lock_irqsave(&cctx->lock, flags);
>> @@ -2698,8 +2684,24 @@ 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_size && cctx->vmcount) {
>> + u64 src_perms = 0;
>> + struct qcom_scm_vmperm dst_perms;
>> +
>> + for (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_addr,
>> + cctx->remote_heap_size, &src_perms,
>> + &dst_perms, 1);
>> + if (err)
>> + dev_err(&rpdev->dev,
>> + "Failed to assign memory back to HLOS: addr %pa size %#llx err %d\n",
>> + &cctx->remote_heap_addr, cctx->remote_heap_size, err);
>> + }
>>
>> of_platform_depopulate(&rpdev->dev);
>>