Re: [PATCH v6 1/4] misc: fastrpc: Move fdlist to invoke context structure

[Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>]

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
>>
>>