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: sta...@kernel.org
> Signed-off-by: Ekansh Gupta <ekansh.gu...@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
