Re: [PATCH v1 2/3] misc: fastrpc: Free DMA handles for RPC calls with no arguments

[Srinivas Kandagatla]

On 03/10/2023 12:07, Ekansh Gupta wrote:
> On 10/2/2023 3:00 PM, Srinivas Kandagatla wrote:
> > On 29/09/2023 08:42, Ekansh Gupta wrote:
> > > The FDs for DMA handles to be freed is updated in fdlist by DSP over
> > > a remote call. This holds true even for remote calls with no
> > > arguments. To handle this, get_args and put_args are needed to
> > > be called for remote calls with no arguments also as fdlist
> > > is allocated in get_args and FDs updated in fdlist is freed
> > > in put_args.
> > >
> > > Fixes: 8f6c1d8c4f0c ("misc: fastrpc: Add fdlist implementation")
> > > Cc: stable <stable@xxxxxxxxxx>
> > > Signed-off-by: Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx>
> > > ---
> > >   drivers/misc/fastrpc.c | 23 ++++++++++-------------
> > >   1 file changed, 10 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > > index fb92197..a52701c 100644
> > > --- a/drivers/misc/fastrpc.c
> > > +++ b/drivers/misc/fastrpc.c
> > > @@ -1091,6 +1091,7 @@ static int fastrpc_put_args(struct 
> > > fastrpc_invoke_ctx *ctx,
> > >           }
> > >       }
> > > +    /* Clean up fdlist which is updated by DSP */
> > >       for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
> > >           if (!fdlist[i])
> > >               break;
> > > @@ -1157,11 +1158,9 @@ static int fastrpc_internal_invoke(struct 
> > > fastrpc_user *fl,  u32 kernel,
> > >       if (IS_ERR(ctx))
> > >           return PTR_ERR(ctx);
> > <---
> > > -    if (ctx->nscalars) {
> > > -        err = fastrpc_get_args(kernel, ctx);
> > > -        if (err)
> > > -            goto bail;
> > > -    }
> > > +    err = fastrpc_get_args(kernel, ctx);
> > > +    if (err)
> > > +        goto bail;
> > -->
> > I dont see any point of the above change as fastrpc_internal_invoke 
> > will be called from kernel with nscalars always set.
> >
> > do you see a path that does not set this?
> The context specific rpra buffer is allocated as part of 
> fastrpc_get_args and there is a possibility that the DSP intends to 
> update fdlist for a call with 0 nscalars. In that scenario, the driver 
> needs to ensure that the rpra is allocated which will carry the fdlist. 
> The same can be extended to crc and dsp perf memory(to be added, patches 
> shared for missing features) for remote calls with 0 nscalars.
Yes, we need this to setup remote args.

thanks
srini
> Thanks for taking your time to review the patches Srini, please let me 
> know if you have more queries.
>
> -ekansh
> > --srini
> > >       /* make sure that all CPU memory writes are seen by DSP */
> > >       dma_wmb();
> > > @@ -1185,14 +1184,12 @@ static int fastrpc_internal_invoke(struct 
> > > fastrpc_user *fl,  u32 kernel,
> > >       if (err)
> > >           goto bail;
> > > -    if (ctx->nscalars) {
> > > -        /* make sure that all memory writes by DSP are seen by CPU */
> > > -        dma_rmb();
> > > -        /* populate all the output buffers with results */
> > > -        err = fastrpc_put_args(ctx, kernel);
> > > -        if (err)
> > > -            goto bail;
> > > -    }
> > > +    /* make sure that all memory writes by DSP are seen by CPU */
> > > +    dma_rmb();
> > > +    /* populate all the output buffers with results */
> > > +    err = fastrpc_put_args(ctx, kernel);
> > > +    if (err)
> > > +        goto bail;
> > >   bail:
> > >       if (err != -ERESTARTSYS && err != -ETIMEDOUT) {