Subject: Re: [PATCH 1/2] misc: fastrpc: Add fdlist implementation
To:     Jeya R <jeyr@codeaurora.org>, linux-arm-msm@vger.kernel.org
Cc:     gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
        fastrpc.upstream@qti.qualcomm.com
References: <1638163720-23123-1-git-send-email-jeyr@codeaurora.org>
 <1638163720-23123-2-git-send-email-jeyr@codeaurora.org>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <3ee1388d-2528-56ed-ce5f-4c667beb67cb@linaro.org>
Date:   Mon, 29 Nov 2021 15:12:58 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <1638163720-23123-2-git-send-email-jeyr@codeaurora.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@codeaurora.org>
> ---
>   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;
>   }
>   
>