| Message ID | 20250513042825.2147985-3-ekansh.gupta@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc: fastrpc: Add missing bug fixes | expand |
On 5/13/25 05:28, Ekansh Gupta wrote: > Remote heap allocations are not organized in a maintainable manner, > leading to potential issues with memory management. As the remote > heap allocations are maintained in fl mmaps list, the allocations > will go away if the audio daemon process is killed but there are > chances that audio PD might still be using the memory. Move all > remote heap allocations to a dedicated list where the entries are > cleaned only for user requests and subsystem shutdown. > > Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd") > Cc: stable@kernel.org > Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> > --- > drivers/misc/fastrpc.c | 93 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 72 insertions(+), 21 deletions(-) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index ca3721365ddc..d4e38b5e5e6c 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -273,10 +273,12 @@ 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; > - struct fastrpc_buf *remote_heap; > + struct list_head rhmaps; Other than Audiopd, do you see any other remote heaps getting added to this list? > struct list_head invoke_interrupted_mmaps; > bool secure; > bool unsigned_support; > @@ -1237,12 +1239,47 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques > return false; > } > > +static void fastrpc_cleanup_rhmaps(struct fastrpc_channel_ctx *cctx) > +{ > + struct fastrpc_buf *buf, *b; > + struct list_head temp_list; > + int err; > + unsigned long flags; > + > + INIT_LIST_HEAD(&temp_list); > + > + spin_lock_irqsave(&cctx->lock, flags); > + list_splice_init(&cctx->rhmaps, &temp_list); > + spin_unlock_irqrestore(&cctx->lock, flags); Can you explain why do we need to do this? > + > + list_for_each_entry_safe(buf, b, &temp_list, node) {> + if (cctx->vmcount) { > + u64 src_perms = 0; > + struct qcom_scm_vmperm dst_perms; > + u32 i; > + > + 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(buf->phys, (u64)buf->size, > + &src_perms, &dst_perms, 1); > + if (err) > + continue; Memory leak here. > + } > + fastrpc_buf_free(buf); > + } > +} > + --srini
On Mon, May 19, 2025 at 04:36:13PM +0530, Ekansh Gupta wrote: > > > On 5/19/2025 3:46 PM, Dmitry Baryshkov wrote: > > On Tue, May 13, 2025 at 09:58:22AM +0530, Ekansh Gupta wrote: > >> Remote heap allocations are not organized in a maintainable manner, > >> leading to potential issues with memory management. As the remote > > Which issues? I think I have been asking this question previously. > > Please expand the commit message here. > This is mostly related to the memory clean-up and the other patch where > unmap request was added, I'll try to pull more details about the issue > scenario. Thanks. > > > >> heap allocations are maintained in fl mmaps list, the allocations > >> will go away if the audio daemon process is killed but there are > > What is audio daemon process? > As audio PD on DSP is static, there is HLOS process(audio daemon) required to > attach to audio PD to fulfill it's memory and file operation requirements. > > This daemon can be thought of to be somewhat similar to rootPD(adsprpcd) or > sensorsPD(sscrpcd) daemons. Although, there is a slight difference in case of audio > daemon as it is required to take additional information and resources to audio PD > while attaching. I find it a little bit strange to see 'required' here, while we have working audio setup on all up platforms up to and including SM8750 without any additional daemons. This is the primary reason for my question: what is it, why is it necessary, when is it necessary, etc.
On 5/19/2025 6:59 PM, Dmitry Baryshkov wrote: > On Mon, May 19, 2025 at 04:36:13PM +0530, Ekansh Gupta wrote: >> >> On 5/19/2025 3:46 PM, Dmitry Baryshkov wrote: >>> On Tue, May 13, 2025 at 09:58:22AM +0530, Ekansh Gupta wrote: >>>> Remote heap allocations are not organized in a maintainable manner, >>>> leading to potential issues with memory management. As the remote >>> Which issues? I think I have been asking this question previously. >>> Please expand the commit message here. >> This is mostly related to the memory clean-up and the other patch where >> unmap request was added, I'll try to pull more details about the issue >> scenario. > Thanks. > >>>> heap allocations are maintained in fl mmaps list, the allocations >>>> will go away if the audio daemon process is killed but there are >>> What is audio daemon process? >> As audio PD on DSP is static, there is HLOS process(audio daemon) required to >> attach to audio PD to fulfill it's memory and file operation requirements. >> >> This daemon can be thought of to be somewhat similar to rootPD(adsprpcd) or >> sensorsPD(sscrpcd) daemons. Although, there is a slight difference in case of audio >> daemon as it is required to take additional information and resources to audio PD >> while attaching. > I find it a little bit strange to see 'required' here, while we have > working audio setup on all up platforms up to and including SM8750 > without any additional daemons. This is the primary reason for my > question: what is it, why is it necessary, when is it necessary, etc. This daemon is critical to facilitate dynamic loading and memory requirement for audio PD(running on DSP for audio processing). Even for audio testing on SM8750, I believe Alexey was enabling this daemon. What is it? - HLOS process to attached to audio PD to fulfill the requirements that cannot be met by DSP alone(like file operations, memory etc.) Why is it necessary? - There are limitation on DSP for which the PD requirements needs to be taken to HLOS. For example, DSP does not have it's own file system, so any file operation request it PD(say for dynamic loading) needs to be taken to HLOS(using listener/reverse calls) and is fulfilled there. Similarly memory requirement is another example. When is it necessary? - When audio PD needs to perform any task that requires HLOS relying operations like dynamic loading etc. >
On 5/19/2025 5:05 PM, Srinivas Kandagatla wrote: > On 5/13/25 05:28, Ekansh Gupta wrote: >> Remote heap allocations are not organized in a maintainable manner, >> leading to potential issues with memory management. As the remote >> heap allocations are maintained in fl mmaps list, the allocations >> will go away if the audio daemon process is killed but there are >> chances that audio PD might still be using the memory. Move all >> remote heap allocations to a dedicated list where the entries are >> cleaned only for user requests and subsystem shutdown. >> >> Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd") >> Cc: stable@kernel.org >> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> >> --- >> drivers/misc/fastrpc.c | 93 ++++++++++++++++++++++++++++++++---------- >> 1 file changed, 72 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >> index ca3721365ddc..d4e38b5e5e6c 100644 >> --- a/drivers/misc/fastrpc.c >> +++ b/drivers/misc/fastrpc.c >> @@ -273,10 +273,12 @@ 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; >> - struct fastrpc_buf *remote_heap; >> + struct list_head rhmaps; > Other than Audiopd, do you see any other remote heaps getting added to > this list? With current implementation it is possible but that is not correct, I will include a patch to restrict remote heap map and unmap requests to audio daemon only. > >> struct list_head invoke_interrupted_mmaps; >> bool secure; >> bool unsigned_support; >> @@ -1237,12 +1239,47 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques >> return false; >> } >> >> +static void fastrpc_cleanup_rhmaps(struct fastrpc_channel_ctx *cctx) >> +{ >> + struct fastrpc_buf *buf, *b; >> + struct list_head temp_list; >> + int err; >> + unsigned long flags; >> + >> + INIT_LIST_HEAD(&temp_list); >> + >> + spin_lock_irqsave(&cctx->lock, flags); >> + list_splice_init(&cctx->rhmaps, &temp_list); >> + spin_unlock_irqrestore(&cctx->lock, flags); > Can you explain why do we need to do this? To avoid any locking issue. While clean-up, I'm replacing the rhmaps list with an empty one under a lock and cleaning up the list without any more locking. > >> + >> + list_for_each_entry_safe(buf, b, &temp_list, node) {> + if (cctx->vmcount) { >> + u64 src_perms = 0; >> + struct qcom_scm_vmperm dst_perms; >> + u32 i; >> + >> + 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(buf->phys, (u64)buf->size, >> + &src_perms, &dst_perms, 1); >> + if (err) >> + continue; > Memory leak here. I don't see any better way to handle the failure here as the clean-up is called when the channel is going down and there won't be any way to free this memory if qcom_scm call fails? Do you see any better way to address this? Or should I add a comment highlighting this limitation? > >> + } >> + fastrpc_buf_free(buf); >> + } >> +} >> + > --srini
On Thu, 22 May 2025 at 07:54, Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> wrote: > > > > On 5/19/2025 6:59 PM, Dmitry Baryshkov wrote: > > On Mon, May 19, 2025 at 04:36:13PM +0530, Ekansh Gupta wrote: > >> > >> On 5/19/2025 3:46 PM, Dmitry Baryshkov wrote: > >>> On Tue, May 13, 2025 at 09:58:22AM +0530, Ekansh Gupta wrote: > >>>> Remote heap allocations are not organized in a maintainable manner, > >>>> leading to potential issues with memory management. As the remote > >>> Which issues? I think I have been asking this question previously. > >>> Please expand the commit message here. > >> This is mostly related to the memory clean-up and the other patch where > >> unmap request was added, I'll try to pull more details about the issue > >> scenario. > > Thanks. > > > >>>> heap allocations are maintained in fl mmaps list, the allocations > >>>> will go away if the audio daemon process is killed but there are > >>> What is audio daemon process? > >> As audio PD on DSP is static, there is HLOS process(audio daemon) required to > >> attach to audio PD to fulfill it's memory and file operation requirements. > >> > >> This daemon can be thought of to be somewhat similar to rootPD(adsprpcd) or > >> sensorsPD(sscrpcd) daemons. Although, there is a slight difference in case of audio > >> daemon as it is required to take additional information and resources to audio PD > >> while attaching. > > I find it a little bit strange to see 'required' here, while we have > > working audio setup on all up platforms up to and including SM8750 > > without any additional daemons. This is the primary reason for my > > question: what is it, why is it necessary, when is it necessary, etc. > > This daemon is critical to facilitate dynamic loading and memory > requirement for audio PD(running on DSP for audio processing). Even > for audio testing on SM8750, I believe Alexey was enabling this daemon. Could you please point out the daemon sources? As far as I remember, we didn't need it on any of the platforms up to and including SM8650, that's why I'm asking. > > What is it? > - HLOS process to attached to audio PD to fulfill the requirements that > cannot be met by DSP alone(like file operations, memory etc.) > > Why is it necessary? > - There are limitation on DSP for which the PD requirements needs to be > taken to HLOS. For example, DSP does not have it's own file system, so > any file operation request it PD(say for dynamic loading) needs to be > taken to HLOS(using listener/reverse calls) and is fulfilled there. > Similarly memory requirement is another example. > > When is it necessary? > - When audio PD needs to perform any task that requires HLOS relying > operations like dynamic loading etc. This doesn't exactly answer the question. I can play and capture audio on most of the platforms that I tested without using extra daemon. So, when is it necessary?
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index ca3721365ddc..d4e38b5e5e6c 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -273,10 +273,12 @@ 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; - struct fastrpc_buf *remote_heap; + struct list_head rhmaps; struct list_head invoke_interrupted_mmaps; bool secure; bool unsigned_support; @@ -1237,12 +1239,47 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques return false; } +static void fastrpc_cleanup_rhmaps(struct fastrpc_channel_ctx *cctx) +{ + struct fastrpc_buf *buf, *b; + struct list_head temp_list; + int err; + unsigned long flags; + + INIT_LIST_HEAD(&temp_list); + + spin_lock_irqsave(&cctx->lock, flags); + list_splice_init(&cctx->rhmaps, &temp_list); + spin_unlock_irqrestore(&cctx->lock, flags); + + list_for_each_entry_safe(buf, b, &temp_list, node) { + if (cctx->vmcount) { + u64 src_perms = 0; + struct qcom_scm_vmperm dst_perms; + u32 i; + + 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(buf->phys, (u64)buf->size, + &src_perms, &dst_perms, 1); + if (err) + continue; + } + fastrpc_buf_free(buf); + } +} + static int fastrpc_init_create_static_process(struct fastrpc_user *fl, char __user *argp) { struct fastrpc_init_create_static init; struct fastrpc_invoke_args *args; struct fastrpc_phy_page pages[1]; + struct fastrpc_buf *buf = NULL; + u64 phys = 0, size = 0; char *name; int err; bool scm_done = false; @@ -1252,6 +1289,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, u32 pageslen; } inbuf; u32 sc; + unsigned long flags; args = kcalloc(FASTRPC_CREATE_STATIC_PROCESS_NARGS, sizeof(*args), GFP_KERNEL); if (!args) @@ -1273,26 +1311,30 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, goto err; } - if (!fl->cctx->remote_heap) { + if (!fl->cctx->audio_init_mem) { err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, - &fl->cctx->remote_heap); + &buf); if (err) goto err_name; + phys = buf->phys; + size = buf->size; /* 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->phys, - (u64)fl->cctx->remote_heap->size, - &src_perms, - fl->cctx->vmperms, fl->cctx->vmcount); + err = qcom_scm_assign_mem(phys, size, &src_perms, + fl->cctx->vmperms, fl->cctx->vmcount); if (err) { dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d\n", - fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err); + phys, size, err); goto err_map; } scm_done = true; + spin_lock_irqsave(&fl->cctx->lock, flags); + list_add_tail(&buf->node, &fl->cctx->rhmaps); + spin_unlock_irqrestore(&fl->cctx->lock, flags); + fl->cctx->audio_init_mem = true; } } @@ -1309,8 +1351,8 @@ 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->phys; - pages[0].size = fl->cctx->remote_heap->size; + pages[0].addr = phys; + pages[0].size = size; args[2].ptr = (u64)(uintptr_t) pages; args[2].length = sizeof(*pages); @@ -1328,6 +1370,11 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, return 0; err_invoke: + if (buf) { + spin_lock_irqsave(&fl->cctx->lock, flags); + list_del(&buf->node); + spin_unlock_irqrestore(&fl->cctx->lock, flags); + } if (fl->cctx->vmcount && scm_done) { u64 src_perms = 0; struct qcom_scm_vmperm dst_perms; @@ -1338,15 +1385,15 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, dst_perms.vmid = QCOM_SCM_VMID_HLOS; dst_perms.perm = QCOM_SCM_PERM_RWX; - err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys, - (u64)fl->cctx->remote_heap->size, - &src_perms, &dst_perms, 1); + err = qcom_scm_assign_mem(phys, size, &src_perms, + &dst_perms, 1); if (err) dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d\n", - fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err); + phys, size, err); } err_map: - fastrpc_buf_free(fl->cctx->remote_heap); + fl->cctx->audio_init_mem = false; + fastrpc_buf_free(buf); err_name: kfree(name); err: @@ -1869,6 +1916,7 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) struct device *dev = fl->sctx->dev; int err; u32 sc; + unsigned long flags; if (copy_from_user(&req, argp, sizeof(req))) return -EFAULT; @@ -1937,12 +1985,15 @@ static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp) buf->phys, buf->size, err); goto err_assign; } + spin_lock_irqsave(&fl->cctx->lock, flags); + list_add_tail(&buf->node, &fl->cctx->rhmaps); + spin_unlock_irqrestore(&fl->cctx->lock, flags); + } else { + spin_lock(&fl->lock); + list_add_tail(&buf->node, &fl->mmaps); + spin_unlock(&fl->lock); } - spin_lock(&fl->lock); - list_add_tail(&buf->node, &fl->mmaps); - spin_unlock(&fl->lock); - if (copy_to_user((void __user *)argp, &req, sizeof(req))) { err = -EFAULT; goto err_assign; @@ -2362,6 +2413,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) rdev->dma_mask = &data->dma_mask; dma_set_mask_and_coherent(rdev, DMA_BIT_MASK(32)); INIT_LIST_HEAD(&data->users); + INIT_LIST_HEAD(&data->rhmaps); INIT_LIST_HEAD(&data->invoke_interrupted_mmaps); spin_lock_init(&data->lock); idr_init(&data->ctx_idr); @@ -2420,8 +2472,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_cleanup_rhmaps(cctx); of_platform_depopulate(&rpdev->dev);
Remote heap allocations are not organized in a maintainable manner, leading to potential issues with memory management. As the remote heap allocations are maintained in fl mmaps list, the allocations will go away if the audio daemon process is killed but there are chances that audio PD might still be using the memory. Move all remote heap allocations to a dedicated list where the entries are cleaned only for user requests and subsystem shutdown. Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd") Cc: stable@kernel.org Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> --- drivers/misc/fastrpc.c | 93 ++++++++++++++++++++++++++++++++---------- 1 file changed, 72 insertions(+), 21 deletions(-)