Re: [PATCH 1/2] misc: fastrpc: Add fdlist implementation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for the patch,


On 29/11/2021 05:28, Jeya R wrote:
Add fdlist implementation to support dma handles. fdlist is populated
by DSP if any map is no longer used and it is freed during put_args.

Does the dsp add all the fds (from in/out handles) to this list or only ones that are no-longer used by the dsp?



Signed-off-by: Jeya R <jeyr@xxxxxxxxxxxxxx>
---
  drivers/misc/fastrpc.c | 22 ++++++++++++++++++++--
  1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 39aca77..3c937ff 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -353,7 +353,7 @@ static void fastrpc_context_free(struct kref *ref)
  	ctx = container_of(ref, struct fastrpc_invoke_ctx, refcount);
  	cctx = ctx->cctx;
- for (i = 0; i < ctx->nscalars; i++)
+	for (i = 0; i < ctx->nbufs; i++)
  		fastrpc_map_put(ctx->maps[i]);

If above question is true, then who is going to free the rest of the scalars.

if (ctx->buf)
@@ -785,6 +785,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
  	err = fastrpc_buf_alloc(ctx->fl, dev, pkt_size, &ctx->buf);
  	if (err)
  		return err;
+	memset(ctx->buf->virt, 0, pkt_size);

Why do we need to make this zero, dma_alloc_coherent should have returned zeroed memory here anyway?


rpra = ctx->buf->virt;
  	list = ctx->buf->virt + ctx->nscalars * sizeof(*rpra);
@@ -887,9 +888,19 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
  			    u32 kernel)
  {
  	struct fastrpc_remote_arg *rpra = ctx->rpra;
-	int i, inbufs;
+	struct fastrpc_map *mmap = NULL;
+	struct fastrpc_invoke_buf *list;
+	struct fastrpc_phy_page *pages;
+	u64 *fdlist;
+	int i, inbufs, outbufs, handles;
inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
+	outbufs = REMOTE_SCALARS_OUTBUFS(ctx->sc);
+	handles = REMOTE_SCALARS_INHANDLES(ctx->sc) + REMOTE_SCALARS_OUTHANDLES(ctx->sc);
+	list = ctx->buf->virt + ctx->nscalars * sizeof(*rpra);
+	pages = ctx->buf->virt + ctx->nscalars * (sizeof(*list) +
+		sizeof(*rpra));
+	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
for (i = inbufs; i < ctx->nbufs; ++i) {
  		if (!ctx->maps[i]) {
@@ -906,6 +917,13 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
  		}
  	}
+ for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
+		if (!fdlist[i])
+			break;
+		if (!fastrpc_map_find(fl, (int)fdlist[i], &mmap))

fastrpc_map_find() is will invoke a kref_get on the map so calling single fastrpc_map_put() here is not going to work. driver will be leaking memory.

Have you tested this patch with kmemleak enabled?

--srini


+			fastrpc_map_put(mmap);


+	}
+
  	return 0;
  }



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux