| Message ID | 20250513042825.2147985-4-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: > The initially allocated memory is not properly included in the pool, > leading to potential issues with memory management. 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 > Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> > --- > drivers/misc/fastrpc.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index d4e38b5e5e6c..b629e24f00bc 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, > goto err; > } > > + inbuf.client_id = fl->client_id; > + inbuf.namelen = init.namelen; inbuf is not used till the invoke call, why are we moving these two lines here? > + inbuf.pageslen = 0; > if (!fl->cctx->audio_init_mem) { > err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, > &buf); > @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, > list_add_tail(&buf->node, &fl->cctx->rhmaps); > spin_unlock_irqrestore(&fl->cctx->lock, flags); > fl->cctx->audio_init_mem = 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;
On Mon, May 19, 2025 at 04:23:28PM +0530, Ekansh Gupta wrote: > > > On 5/19/2025 3:47 PM, Dmitry Baryshkov wrote: > > On Tue, May 13, 2025 at 09:58:23AM +0530, Ekansh Gupta wrote: > >> The initially allocated memory is not properly included in the pool, > >> leading to potential issues with memory management. Set the number > > What is 'properly'? Which issues? > > inbuf.pageslen is getting updated to 1 in case buffer is allocated, Is it a flag or some page count? > which only > happens if (!fl->cctx->audio_init_mem). You are describing patch behaviour. > > Till now pageslen is always 0 irrespective of whether the memory is allocated > or not due to which audio PD is never able to use this memory. and the is current behaviour. So this answers the first question. 'properly'. Now, the second quesiton. 'Which issues?' > > I'll update this to the commit in the next spin. > > > > >> 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 > >> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> > >> --- > >> drivers/misc/fastrpc.c | 7 ++++--- > >> 1 file changed, 4 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > >> index d4e38b5e5e6c..b629e24f00bc 100644 > >> --- a/drivers/misc/fastrpc.c > >> +++ b/drivers/misc/fastrpc.c > >> @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, > >> goto err; > >> } > >> > >> + inbuf.client_id = fl->client_id; > >> + inbuf.namelen = init.namelen; > >> + inbuf.pageslen = 0; > >> if (!fl->cctx->audio_init_mem) { > >> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, > >> &buf); > >> @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, > >> list_add_tail(&buf->node, &fl->cctx->rhmaps); > >> spin_unlock_irqrestore(&fl->cctx->lock, flags); > >> fl->cctx->audio_init_mem = 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.34.1 > >> >
On 5/19/2025 7:01 PM, Dmitry Baryshkov wrote: > On Mon, May 19, 2025 at 04:23:28PM +0530, Ekansh Gupta wrote: >> >> On 5/19/2025 3:47 PM, Dmitry Baryshkov wrote: >>> On Tue, May 13, 2025 at 09:58:23AM +0530, Ekansh Gupta wrote: >>>> The initially allocated memory is not properly included in the pool, >>>> leading to potential issues with memory management. Set the number >>> What is 'properly'? Which issues? >> inbuf.pageslen is getting updated to 1 in case buffer is allocated, > Is it a flag or some page count? This is page count, based on this count, DSP with add memory to audioPD pool. If it is 0, the memory is not added. > >> which only >> happens if (!fl->cctx->audio_init_mem). > You are describing patch behaviour. > >> Till now pageslen is always 0 irrespective of whether the memory is allocated >> or not due to which audio PD is never able to use this memory. > and the is current behaviour. So this answers the first question. > 'properly'. Now, the second quesiton. 'Which issues?' The issues is actually memory leak as the initial memory is never used by audio PD and it will immediately make a remote heap request as no memory is added to the pool initially. I'll capture this also in the commit text. > >> I'll update this to the commit in the next spin. >> >>>> 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 >>>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> >>>> --- >>>> drivers/misc/fastrpc.c | 7 ++++--- >>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >>>> index d4e38b5e5e6c..b629e24f00bc 100644 >>>> --- a/drivers/misc/fastrpc.c >>>> +++ b/drivers/misc/fastrpc.c >>>> @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, >>>> goto err; >>>> } >>>> >>>> + inbuf.client_id = fl->client_id; >>>> + inbuf.namelen = init.namelen; >>>> + inbuf.pageslen = 0; >>>> if (!fl->cctx->audio_init_mem) { >>>> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, >>>> &buf); >>>> @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, >>>> list_add_tail(&buf->node, &fl->cctx->rhmaps); >>>> spin_unlock_irqrestore(&fl->cctx->lock, flags); >>>> fl->cctx->audio_init_mem = 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.34.1 >>>>
On 5/19/2025 5:11 PM, Srinivas Kandagatla wrote: > On 5/13/25 05:28, Ekansh Gupta wrote: >> The initially allocated memory is not properly included in the pool, >> leading to potential issues with memory management. 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 >> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> >> --- >> drivers/misc/fastrpc.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >> index d4e38b5e5e6c..b629e24f00bc 100644 >> --- a/drivers/misc/fastrpc.c >> +++ b/drivers/misc/fastrpc.c >> @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, >> goto err; >> } >> >> + inbuf.client_id = fl->client_id; >> + inbuf.namelen = init.namelen; > inbuf is not used till the invoke call, why are we moving these two > lines here? I just moved it above so that the pagelen is updated properly. Would you prefer storing pagelen to some local variable and then eventually assigning it to inbuf.pagelen before making the invoke call? > >> + inbuf.pageslen = 0; > > >> if (!fl->cctx->audio_init_mem) { >> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, >> &buf); >> @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, >> list_add_tail(&buf->node, &fl->cctx->rhmaps); >> spin_unlock_irqrestore(&fl->cctx->lock, flags); >> fl->cctx->audio_init_mem = 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;
On Thu, 22 May 2025 at 07:58, Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> wrote: > > > > On 5/19/2025 7:01 PM, Dmitry Baryshkov wrote: > > On Mon, May 19, 2025 at 04:23:28PM +0530, Ekansh Gupta wrote: > >> > >> On 5/19/2025 3:47 PM, Dmitry Baryshkov wrote: > >>> On Tue, May 13, 2025 at 09:58:23AM +0530, Ekansh Gupta wrote: > >>>> The initially allocated memory is not properly included in the pool, > >>>> leading to potential issues with memory management. Set the number > >>> What is 'properly'? Which issues? > >> inbuf.pageslen is getting updated to 1 in case buffer is allocated, > > Is it a flag or some page count? > > This is page count, If it is page count, then why is it '1' instead of being calculated based on the init.memlen? > based on this count, DSP with add memory to audioPD > pool. If it is 0, the memory is not added. > > > > >> which only > >> happens if (!fl->cctx->audio_init_mem). > > You are describing patch behaviour. > > > >> Till now pageslen is always 0 irrespective of whether the memory is allocated > >> or not due to which audio PD is never able to use this memory. > > and the is current behaviour. So this answers the first question. > > 'properly'. Now, the second quesiton. 'Which issues?' > > The issues is actually memory leak as the initial memory is never > used by audio PD and it will immediately make a remote heap request > as no memory is added to the pool initially. > > I'll capture this also in the commit text. > > > > >> I'll update this to the commit in the next spin. > >> > >>>> 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 > >>>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> > >>>> --- > >>>> drivers/misc/fastrpc.c | 7 ++++--- > >>>> 1 file changed, 4 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > >>>> index d4e38b5e5e6c..b629e24f00bc 100644 > >>>> --- a/drivers/misc/fastrpc.c > >>>> +++ b/drivers/misc/fastrpc.c > >>>> @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, > >>>> goto err; > >>>> } > >>>> > >>>> + inbuf.client_id = fl->client_id; > >>>> + inbuf.namelen = init.namelen; > >>>> + inbuf.pageslen = 0; > >>>> if (!fl->cctx->audio_init_mem) { > >>>> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, > >>>> &buf); > >>>> @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, > >>>> list_add_tail(&buf->node, &fl->cctx->rhmaps); > >>>> spin_unlock_irqrestore(&fl->cctx->lock, flags); > >>>> fl->cctx->audio_init_mem = 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.34.1 > >>>> >
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index d4e38b5e5e6c..b629e24f00bc 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -1311,6 +1311,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, goto err; } + inbuf.client_id = fl->client_id; + inbuf.namelen = init.namelen; + inbuf.pageslen = 0; if (!fl->cctx->audio_init_mem) { err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, &buf); @@ -1335,12 +1338,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, list_add_tail(&buf->node, &fl->cctx->rhmaps); spin_unlock_irqrestore(&fl->cctx->lock, flags); fl->cctx->audio_init_mem = 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;
The initially allocated memory is not properly included in the pool, leading to potential issues with memory management. 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 Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com> --- drivers/misc/fastrpc.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)