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 <[email protected]> > >> --- [..] > >> @@ -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 > >> > >> >
