The fdlist is currently part of the meta buffer, computed during
put_args. This leads to code duplication when preparing and reading
critical meta buffer contents used by the FastRPC driver.
Move fdlist to the invoke context structure to improve maintainability
and reduce redundancy. This centralizes its handling and simplifies
meta buffer preparation and reading logic.
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
---
drivers/misc/fastrpc.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 4f5a79c50f58..ce397c687161 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -233,6 +233,7 @@ struct fastrpc_invoke_ctx {
int pid;
int client_id;
u32 sc;
+ u64 *fdlist;
u32 *crc;
u64 ctxid;
u64 msg_sz;
@@ -1018,6 +1019,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
rpra = ctx->buf->virt;
list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
pages = fastrpc_phy_page_start(list, ctx->nscalars);
+ ctx->fdlist = (u64 *)(pages + ctx->nscalars);
args = (uintptr_t)ctx->buf->virt + metalen;
rlen = pkt_size - metalen;
ctx->rpra = rpra;
@@ -1120,18 +1122,10 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
union fastrpc_remote_arg *rpra = ctx->rpra;
struct fastrpc_user *fl = ctx->fl;
struct fastrpc_map *mmap = NULL;
- struct fastrpc_invoke_buf *list;
- struct fastrpc_phy_page *pages;
- u64 *fdlist;
- int i, inbufs, outbufs, handles;
+ int i, inbufs;
int ret = 0;
inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
- outbufs = REMOTE_SCALARS_OUTBUFS(ctx->sc);
- handles = REMOTE_SCALARS_INHANDLES(ctx->sc) + REMOTE_SCALARS_OUTHANDLES(ctx->sc);
- list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
- pages = fastrpc_phy_page_start(list, ctx->nscalars);
- fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
for (i = inbufs; i < ctx->nbufs; ++i) {
if (!ctx->maps[i]) {
@@ -1153,9 +1147,9 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
cleanup_fdlist:
/* Clean up fdlist which is updated by DSP */
for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
- if (!fdlist[i])
+ if (!ctx->fdlist[i])
break;
- if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap))
+ if (!fastrpc_map_lookup(fl, (int)ctx->fdlist[i], &mmap))
fastrpc_map_put(mmap);
}
--
2.34.1On Sun, Feb 15, 2026 at 11:51:32PM +0530, Ekansh Gupta wrote:
> The fdlist is currently part of the meta buffer,
Do you mean "already part of"?
> computed during
> put_args.
The only "computation" I can see wrt fdlist in fastrpc_put_args() is
where we calculate it to be at pages + inbufs + outbufs + handles.
So, why do you say that the content of the meta buffer is calculated
there? Did you mean fastrpc_get_args()?
PS. Use full function names and suffix them with (), to be clear in your
description.
> This leads to code duplication when preparing and reading
> critical meta buffer contents used by the FastRPC driver.
"used by the whole entire FastRPC driver" is rather broad...
As far as I can tell, the thing this patch is doing is caching the
"fdlist" address, to avoid having to derive "pages" (and thereby
indirectly "list"), "outbufs", "handles", and then sum these up.
>
> Move fdlist to the invoke context structure to improve maintainability
> and reduce redundancy. This centralizes its handling and simplifies
> meta buffer preparation and reading logic.
I think the patch looks good, but your commit message is too high-level
sales pitch.
Describe the specific problem that you're solving (i.e. you're
recalculating the offset which was known at the time of
fastrpc_get_args()) and leave it at that.
>
> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> ---
> drivers/misc/fastrpc.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 4f5a79c50f58..ce397c687161 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -233,6 +233,7 @@ struct fastrpc_invoke_ctx {
> int pid;
> int client_id;
> u32 sc;
> + u64 *fdlist;
> u32 *crc;
> u64 ctxid;
> u64 msg_sz;
> @@ -1018,6 +1019,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
> rpra = ctx->buf->virt;
> list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
> pages = fastrpc_phy_page_start(list, ctx->nscalars);
> + ctx->fdlist = (u64 *)(pages + ctx->nscalars);
> args = (uintptr_t)ctx->buf->virt + metalen;
> rlen = pkt_size - metalen;
> ctx->rpra = rpra;
> @@ -1120,18 +1122,10 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> union fastrpc_remote_arg *rpra = ctx->rpra;
> struct fastrpc_user *fl = ctx->fl;
> struct fastrpc_map *mmap = NULL;
> - struct fastrpc_invoke_buf *list;
> - struct fastrpc_phy_page *pages;
> - u64 *fdlist;
> - int i, inbufs, outbufs, handles;
> + int i, inbufs;
> int ret = 0;
>
> inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
> - outbufs = REMOTE_SCALARS_OUTBUFS(ctx->sc);
> - handles = REMOTE_SCALARS_INHANDLES(ctx->sc) + REMOTE_SCALARS_OUTHANDLES(ctx->sc);
> - list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
> - pages = fastrpc_phy_page_start(list, ctx->nscalars);
> - fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
>
> for (i = inbufs; i < ctx->nbufs; ++i) {
> if (!ctx->maps[i]) {
> @@ -1153,9 +1147,9 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> cleanup_fdlist:
> /* Clean up fdlist which is updated by DSP */
> for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
> - if (!fdlist[i])
> + if (!ctx->fdlist[i])
It wouldn't hurt to keep the local fdlist variable, keeps the code less
noisy - and you don't need to change these two places.
> break;
> - if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap))
> + if (!fastrpc_map_lookup(fl, (int)ctx->fdlist[i], &mmap))
Why are the fds stored as u64 in the metadata? Are the actual fd numbers
somehow consumed by the DSP side?
Regards,
Bjorn
> fastrpc_map_put(mmap);
> }
>
> --
> 2.34.1
>
>
On 2/16/2026 9:26 AM, Bjorn Andersson wrote:
> On Sun, Feb 15, 2026 at 11:51:32PM +0530, Ekansh Gupta wrote:
>> The fdlist is currently part of the meta buffer,
> Do you mean "already part of"?
yes.
>
>> computed during
>> put_args.
> The only "computation" I can see wrt fdlist in fastrpc_put_args() is
> where we calculate it to be at pages + inbufs + outbufs + handles.
>
> So, why do you say that the content of the meta buffer is calculated
> there? Did you mean fastrpc_get_args()?
The fdlist is updated by DSP. By "computation", I meant getting the location of fdlist
in metadata buffer.
fastrpc_get_args allocates metadata buffer, fdlist is at some offset in this buffer. This
list is updated by the DSP and rechecked during fastrpc_put_args for any entries.
>
> PS. Use full function names and suffix them with (), to be clear in your
> description.
Ack.
>
>> This leads to code duplication when preparing and reading
>> critical meta buffer contents used by the FastRPC driver.
> "used by the whole entire FastRPC driver" is rather broad...
>
> As far as I can tell, the thing this patch is doing is caching the
> "fdlist" address, to avoid having to derive "pages" (and thereby
> indirectly "list"), "outbufs", "handles", and then sum these up.
>
>> Move fdlist to the invoke context structure to improve maintainability
>> and reduce redundancy. This centralizes its handling and simplifies
>> meta buffer preparation and reading logic.
> I think the patch looks good, but your commit message is too high-level
> sales pitch.
>
> Describe the specific problem that you're solving (i.e. you're
> recalculating the offset which was known at the time of
> fastrpc_get_args()) and leave it at that.
I see the problem with the commit text. Let me come with a better description.
>
>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>> ---
>> drivers/misc/fastrpc.c | 16 +++++-----------
>> 1 file changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 4f5a79c50f58..ce397c687161 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -233,6 +233,7 @@ struct fastrpc_invoke_ctx {
>> int pid;
>> int client_id;
>> u32 sc;
>> + u64 *fdlist;
>> u32 *crc;
>> u64 ctxid;
>> u64 msg_sz;
>> @@ -1018,6 +1019,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
>> rpra = ctx->buf->virt;
>> list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
>> pages = fastrpc_phy_page_start(list, ctx->nscalars);
>> + ctx->fdlist = (u64 *)(pages + ctx->nscalars);
>> args = (uintptr_t)ctx->buf->virt + metalen;
>> rlen = pkt_size - metalen;
>> ctx->rpra = rpra;
>> @@ -1120,18 +1122,10 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>> union fastrpc_remote_arg *rpra = ctx->rpra;
>> struct fastrpc_user *fl = ctx->fl;
>> struct fastrpc_map *mmap = NULL;
>> - struct fastrpc_invoke_buf *list;
>> - struct fastrpc_phy_page *pages;
>> - u64 *fdlist;
>> - int i, inbufs, outbufs, handles;
>> + int i, inbufs;
>> int ret = 0;
>>
>> inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
>> - outbufs = REMOTE_SCALARS_OUTBUFS(ctx->sc);
>> - handles = REMOTE_SCALARS_INHANDLES(ctx->sc) + REMOTE_SCALARS_OUTHANDLES(ctx->sc);
>> - list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
>> - pages = fastrpc_phy_page_start(list, ctx->nscalars);
>> - fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
>>
>> for (i = inbufs; i < ctx->nbufs; ++i) {
>> if (!ctx->maps[i]) {
>> @@ -1153,9 +1147,9 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>> cleanup_fdlist:
>> /* Clean up fdlist which is updated by DSP */
>> for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
>> - if (!fdlist[i])
>> + if (!ctx->fdlist[i])
> It wouldn't hurt to keep the local fdlist variable, keeps the code less
> noisy - and you don't need to change these two places.
Ack.
>
>> break;
>> - if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap))
>> + if (!fastrpc_map_lookup(fl, (int)ctx->fdlist[i], &mmap))
> Why are the fds stored as u64 in the metadata? Are the actual fd numbers
> somehow consumed by the DSP side?
DSP uses it for book-keeping mostly for maintaining DSP side mapping based on fd.
Thanks for spending time on reviewing this change. I'll fix your concerns in the next spin.
//Ekansh
>
> Regards,
> Bjorn
>
>> fastrpc_map_put(mmap);
>> }
>>
>> --
>> 2.34.1
>>
>>
On Mon, Feb 16, 2026 at 02:49:36PM +0530, Ekansh Gupta wrote:
>
>
> On 2/16/2026 9:26 AM, Bjorn Andersson wrote:
> > On Sun, Feb 15, 2026 at 11:51:32PM +0530, Ekansh Gupta wrote:
> >> The fdlist is currently part of the meta buffer,
> > Do you mean "already part of"?
> yes.
> >
> >> computed during
> >> put_args.
> > The only "computation" I can see wrt fdlist in fastrpc_put_args() is
> > where we calculate it to be at pages + inbufs + outbufs + handles.
> >
> > So, why do you say that the content of the meta buffer is calculated
> > there? Did you mean fastrpc_get_args()?
> The fdlist is updated by DSP. By "computation", I meant getting the location of fdlist
> in metadata buffer.
>
> fastrpc_get_args allocates metadata buffer, fdlist is at some offset in this buffer. This
> list is updated by the DSP and rechecked during fastrpc_put_args for any entries.
Does that mean that fastrpc_get_args() and fastrpc_put_args() aren't
symmetrical?
What is the life cycle for the mappings then? Can a buffer be "passed"
to the DSP in one invoke only to be "returned" in a later invoke?
> >
> > PS. Use full function names and suffix them with (), to be clear in your
> > description.
> Ack.
> >
> >> This leads to code duplication when preparing and reading
> >> critical meta buffer contents used by the FastRPC driver.
> > "used by the whole entire FastRPC driver" is rather broad...
> >
> > As far as I can tell, the thing this patch is doing is caching the
> > "fdlist" address, to avoid having to derive "pages" (and thereby
> > indirectly "list"), "outbufs", "handles", and then sum these up.
> >
> >> Move fdlist to the invoke context structure to improve maintainability
> >> and reduce redundancy. This centralizes its handling and simplifies
> >> meta buffer preparation and reading logic.
> > I think the patch looks good, but your commit message is too high-level
> > sales pitch.
> >
> > Describe the specific problem that you're solving (i.e. you're
> > recalculating the offset which was known at the time of
> > fastrpc_get_args()) and leave it at that.
> I see the problem with the commit text. Let me come with a better description.
Thank you.
> >
> >> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
> >> ---
[..]
> >> @@ -1153,9 +1147,9 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
> >> cleanup_fdlist:
> >> /* Clean up fdlist which is updated by DSP */
> >> for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
> >> - if (!fdlist[i])
> >> + if (!ctx->fdlist[i])
> > It wouldn't hurt to keep the local fdlist variable, keeps the code less
> > noisy - and you don't need to change these two places.
> Ack.
> >
> >> break;
> >> - if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap))
> >> + if (!fastrpc_map_lookup(fl, (int)ctx->fdlist[i], &mmap))
> > Why are the fds stored as u64 in the metadata? Are the actual fd numbers
> > somehow consumed by the DSP side?
> DSP uses it for book-keeping mostly for maintaining DSP side mapping based on fd.
>
Okay, so then the size is part of the ABI. Thanks for clarifying.
Regards,
Bjorn
> Thanks for spending time on reviewing this change. I'll fix your concerns in the next spin.
>
> //Ekansh
> >
> > Regards,
> > Bjorn
> >
> >> fastrpc_map_put(mmap);
> >> }
> >>
> >> --
> >> 2.34.1
> >>
> >>
>
© 2016 - 2026 Red Hat, Inc.