| Message ID | 1693463029-9311-1-git-send-email-quic_ekangupt@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] misc: fastrpc: Free DMA handles for RPC calls with no arguments | expand |
Thanks Ekansh for this patch. few comments below On 31/08/2023 07:23, Ekansh Gupta wrote: > The FDs for DMA handles to be freed is updated in fdlist by DSP over So the dsp is updating the fd list after invoke? > 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. > > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > --- > drivers/misc/fastrpc.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 9666d28..e6df66e 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -1153,11 +1153,9 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl, u32 kernel, > if (IS_ERR(ctx)) > return PTR_ERR(ctx); > > - if (ctx->nscalars) { Why do we need to remove this check? fastrpc_internal_invoke will have nscalars set before calling. and we are not dealing with fdlist in fastrpc_get_args(), so am not sure what this change is helping with. > - err = fastrpc_get_args(kernel, ctx); > - if (err) > - goto bail; > - } > + err = fastrpc_get_args(kernel, ctx); > + if (err) > + goto bail; > > /* make sure that all CPU memory writes are seen by DSP */ > dma_wmb(); > @@ -1181,14 +1179,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 */ A comment about fdlist here would be really useful > + err = fastrpc_put_args(ctx, kernel); > + if (err) > + goto bail; > > bail: > if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
On 9/26/2023 3:57 PM, Srinivas Kandagatla wrote: > Thanks Ekansh for this patch. > > few comments below > > On 31/08/2023 07:23, Ekansh Gupta wrote: >> The FDs for DMA handles to be freed is updated in fdlist by DSP over > > So the dsp is updating the fd list after invoke? > Yes, correct. DSP updates this fd list when the buffer is no longer needed by the user PD. > >> 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. >> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> >> --- >> drivers/misc/fastrpc.c | 22 +++++++++------------- >> 1 file changed, 9 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >> index 9666d28..e6df66e 100644 >> --- a/drivers/misc/fastrpc.c >> +++ b/drivers/misc/fastrpc.c >> @@ -1153,11 +1153,9 @@ static int fastrpc_internal_invoke(struct >> fastrpc_user *fl, u32 kernel, >> if (IS_ERR(ctx)) >> return PTR_ERR(ctx); >> - if (ctx->nscalars) { > > Why do we need to remove this check? > fastrpc_internal_invoke will have nscalars set before calling. and we > are not dealing with fdlist in fastrpc_get_args(), so am not sure what > this change is helping with. Memory for fdlist is allocated as part of fastrpc_get_args. The reason to add this change is that fdlist can be updated by DSP over a call with no remote arguments, for this call, there should be some fdlist allocated so the DSP can update the list if needed. Same apply for fastrpc_put_args also as when DSP updates fdlist for any remote call with no arguments, the maps corresponding to the fdlist should be freed. > > >> - err = fastrpc_get_args(kernel, ctx); >> - if (err) >> - goto bail; >> - } >> + err = fastrpc_get_args(kernel, ctx); >> + if (err) >> + goto bail; >> /* make sure that all CPU memory writes are seen by DSP */ >> dma_wmb(); >> @@ -1181,14 +1179,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 */ > > A comment about fdlist here would be really useful Sure, I will add a comment in the next patch. Do you suggest to add comment here or inside fastrpc_put_args function: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n1092 > >> + err = fastrpc_put_args(ctx, kernel); >> + if (err) >> + goto bail; >> bail: >> if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
On Thu, Aug 31, 2023 at 11:53:49AM +0530, 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. > > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> What commit id does this fix? Or is this new functionality? thanks, greg k-h
On 9/28/2023 6:35 PM, Greg KH wrote: > On Thu, Aug 31, 2023 at 11:53:49AM +0530, 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. >> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > > What commit id does this fix? Or is this new functionality? > I'll update the details in the next patch. -ekansh > thanks, > > greg k-h
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 9666d28..e6df66e 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -1153,11 +1153,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; /* make sure that all CPU memory writes are seen by DSP */ dma_wmb(); @@ -1181,14 +1179,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) {
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. Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> --- drivers/misc/fastrpc.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)