diff mbox series

[v1,2/3] misc: fastrpc: Free DMA handles for RPC calls with no arguments

Message ID 1695973360-14369-3-git-send-email-quic_ekangupt@quicinc.com
State New
Headers
Series Add fixes for FastRPC driver |

Commit Message

Ekansh Gupta Sept. 29, 2023, 7:42 a.m. UTC 
  The FDs for DMA handles to be freed is updated in fdlist by DSP over
a remote call. This holds true even for remote calls with no
arguments. To handle this, get_args and put_args are needed to
be called for remote calls with no arguments also as fdlist
is allocated in get_args and FDs updated in fdlist is freed
in put_args.

Fixes: 8f6c1d8c4f0c ("misc: fastrpc: Add fdlist implementation")
Cc: stable <stable@kernel.org>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Comments

Srinivas Kandagatla Oct. 2, 2023, 9:30 a.m. UTC | #1 
On 29/09/2023 08:42, Ekansh Gupta wrote:
> The FDs for DMA handles to be freed is updated in fdlist by DSP over
> a remote call. This holds true even for remote calls with no
> arguments. To handle this, get_args and put_args are needed to
> be called for remote calls with no arguments also as fdlist
> is allocated in get_args and FDs updated in fdlist is freed
> in put_args.
> 
> Fixes: 8f6c1d8c4f0c ("misc: fastrpc: Add fdlist implementation")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>   drivers/misc/fastrpc.c | 23 ++++++++++-------------
>   1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index fb92197..a52701c 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1091,6 +1091,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>   		}
>   	}
>   
> +	/* Clean up fdlist which is updated by DSP */
>   	for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
>   		if (!fdlist[i])
>   			break;
> @@ -1157,11 +1158,9 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>   	if (IS_ERR(ctx))
>   		return PTR_ERR(ctx);
>   
<---
> -	if (ctx->nscalars) {
> -		err = fastrpc_get_args(kernel, ctx);
> -		if (err)
> -			goto bail;
> -	}
> +	err = fastrpc_get_args(kernel, ctx);
> +	if (err)
> +		goto bail;
-->
I dont see any point of the above change as fastrpc_internal_invoke will 
be called from kernel with nscalars always set.

do you see a path that does not set this?

--srini
>   
>   	/* make sure that all CPU memory writes are seen by DSP */
>   	dma_wmb();
> @@ -1185,14 +1184,12 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>   	if (err)
>   		goto bail;
>   
> -	if (ctx->nscalars) {
> -		/* make sure that all memory writes by DSP are seen by CPU */
> -		dma_rmb();
> -		/* populate all the output buffers with results */
> -		err = fastrpc_put_args(ctx, kernel);
> -		if (err)
> -			goto bail;
> -	}
> +	/* make sure that all memory writes by DSP are seen by CPU */
> +	dma_rmb();
> +	/* populate all the output buffers with results */
> +	err = fastrpc_put_args(ctx, kernel);
> +	if (err)
> +		goto bail;
>   
>   bail:
>   	if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
Ekansh Gupta Oct. 3, 2023, 11:07 a.m. UTC | #2 
On 10/2/2023 3:00 PM, Srinivas Kandagatla wrote:
> 
> 
> On 29/09/2023 08:42, Ekansh Gupta wrote:
>> The FDs for DMA handles to be freed is updated in fdlist by DSP over
>> a remote call. This holds true even for remote calls with no
>> arguments. To handle this, get_args and put_args are needed to
>> be called for remote calls with no arguments also as fdlist
>> is allocated in get_args and FDs updated in fdlist is freed
>> in put_args.
>>
>> Fixes: 8f6c1d8c4f0c ("misc: fastrpc: Add fdlist implementation")
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>   drivers/misc/fastrpc.c | 23 ++++++++++-------------
>>   1 file changed, 10 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index fb92197..a52701c 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1091,6 +1091,7 @@ static int fastrpc_put_args(struct 
>> fastrpc_invoke_ctx *ctx,
>>           }
>>       }
>> +    /* Clean up fdlist which is updated by DSP */
>>       for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
>>           if (!fdlist[i])
>>               break;
>> @@ -1157,11 +1158,9 @@ static int fastrpc_internal_invoke(struct 
>> fastrpc_user *fl,  u32 kernel,
>>       if (IS_ERR(ctx))
>>           return PTR_ERR(ctx);
> <---
>> -    if (ctx->nscalars) {
>> -        err = fastrpc_get_args(kernel, ctx);
>> -        if (err)
>> -            goto bail;
>> -    }
>> +    err = fastrpc_get_args(kernel, ctx);
>> +    if (err)
>> +        goto bail;
> -->
> I dont see any point of the above change as fastrpc_internal_invoke will 
> be called from kernel with nscalars always set.
> 
> do you see a path that does not set this?
> 
The context specific rpra buffer is allocated as part of 
fastrpc_get_args and there is a possibility that the DSP intends to 
update fdlist for a call with 0 nscalars. In that scenario, the driver 
needs to ensure that the rpra is allocated which will carry the fdlist. 
The same can be extended to crc and dsp perf memory(to be added, patches 
shared for missing features) for remote calls with 0 nscalars.

Thanks for taking your time to review the patches Srini, please let me 
know if you have more queries.

-ekansh
> --srini
>>       /* make sure that all CPU memory writes are seen by DSP */
>>       dma_wmb();
>> @@ -1185,14 +1184,12 @@ static int fastrpc_internal_invoke(struct 
>> fastrpc_user *fl,  u32 kernel,
>>       if (err)
>>           goto bail;
>> -    if (ctx->nscalars) {
>> -        /* make sure that all memory writes by DSP are seen by CPU */
>> -        dma_rmb();
>> -        /* populate all the output buffers with results */
>> -        err = fastrpc_put_args(ctx, kernel);
>> -        if (err)
>> -            goto bail;
>> -    }
>> +    /* make sure that all memory writes by DSP are seen by CPU */
>> +    dma_rmb();
>> +    /* populate all the output buffers with results */
>> +    err = fastrpc_put_args(ctx, kernel);
>> +    if (err)
>> +        goto bail;
>>   bail:
>>       if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
Srinivas Kandagatla Oct. 9, 2023, 6:04 a.m. UTC | #3 
On 03/10/2023 12:07, Ekansh Gupta wrote:
> 
> 
> On 10/2/2023 3:00 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 29/09/2023 08:42, Ekansh Gupta wrote:
>>> The FDs for DMA handles to be freed is updated in fdlist by DSP over
>>> a remote call. This holds true even for remote calls with no
>>> arguments. To handle this, get_args and put_args are needed to
>>> be called for remote calls with no arguments also as fdlist
>>> is allocated in get_args and FDs updated in fdlist is freed
>>> in put_args.
>>>
>>> Fixes: 8f6c1d8c4f0c ("misc: fastrpc: Add fdlist implementation")
>>> Cc: stable <stable@kernel.org>
>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>> ---
>>>   drivers/misc/fastrpc.c | 23 ++++++++++-------------
>>>   1 file changed, 10 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>> index fb92197..a52701c 100644
>>> --- a/drivers/misc/fastrpc.c
>>> +++ b/drivers/misc/fastrpc.c
>>> @@ -1091,6 +1091,7 @@ static int fastrpc_put_args(struct 
>>> fastrpc_invoke_ctx *ctx,
>>>           }
>>>       }
>>> +    /* Clean up fdlist which is updated by DSP */
>>>       for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
>>>           if (!fdlist[i])
>>>               break;
>>> @@ -1157,11 +1158,9 @@ static int fastrpc_internal_invoke(struct 
>>> fastrpc_user *fl,  u32 kernel,
>>>       if (IS_ERR(ctx))
>>>           return PTR_ERR(ctx);
>> <---
>>> -    if (ctx->nscalars) {
>>> -        err = fastrpc_get_args(kernel, ctx);
>>> -        if (err)
>>> -            goto bail;
>>> -    }
>>> +    err = fastrpc_get_args(kernel, ctx);
>>> +    if (err)
>>> +        goto bail;
>> -->
>> I dont see any point of the above change as fastrpc_internal_invoke 
>> will be called from kernel with nscalars always set.
>>
>> do you see a path that does not set this?
>>
> The context specific rpra buffer is allocated as part of 
> fastrpc_get_args and there is a possibility that the DSP intends to 
> update fdlist for a call with 0 nscalars. In that scenario, the driver 
> needs to ensure that the rpra is allocated which will carry the fdlist. 
> The same can be extended to crc and dsp perf memory(to be added, patches 
> shared for missing features) for remote calls with 0 nscalars.
> 
Yes, we need this to setup remote args.

thanks
srini
> Thanks for taking your time to review the patches Srini, please let me 
> know if you have more queries.
> 
> -ekansh
>> --srini
>>>       /* make sure that all CPU memory writes are seen by DSP */
>>>       dma_wmb();
>>> @@ -1185,14 +1184,12 @@ static int fastrpc_internal_invoke(struct 
>>> fastrpc_user *fl,  u32 kernel,
>>>       if (err)
>>>           goto bail;
>>> -    if (ctx->nscalars) {
>>> -        /* make sure that all memory writes by DSP are seen by CPU */
>>> -        dma_rmb();
>>> -        /* populate all the output buffers with results */
>>> -        err = fastrpc_put_args(ctx, kernel);
>>> -        if (err)
>>> -            goto bail;
>>> -    }
>>> +    /* make sure that all memory writes by DSP are seen by CPU */
>>> +    dma_rmb();
>>> +    /* populate all the output buffers with results */
>>> +    err = fastrpc_put_args(ctx, kernel);
>>> +    if (err)
>>> +        goto bail;
>>>   bail:
>>>       if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
diff mbox series

Patch

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index fb92197..a52701c 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1091,6 +1091,7 @@  static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
 		}
 	}
 
+	/* Clean up fdlist which is updated by DSP */
 	for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
 		if (!fdlist[i])
 			break;
@@ -1157,11 +1158,9 @@  static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 
-	if (ctx->nscalars) {
-		err = fastrpc_get_args(kernel, ctx);
-		if (err)
-			goto bail;
-	}
+	err = fastrpc_get_args(kernel, ctx);
+	if (err)
+		goto bail;
 
 	/* make sure that all CPU memory writes are seen by DSP */
 	dma_wmb();
@@ -1185,14 +1184,12 @@  static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
 	if (err)
 		goto bail;
 
-	if (ctx->nscalars) {
-		/* make sure that all memory writes by DSP are seen by CPU */
-		dma_rmb();
-		/* populate all the output buffers with results */
-		err = fastrpc_put_args(ctx, kernel);
-		if (err)
-			goto bail;
-	}
+	/* make sure that all memory writes by DSP are seen by CPU */
+	dma_rmb();
+	/* populate all the output buffers with results */
+	err = fastrpc_put_args(ctx, kernel);
+	if (err)
+		goto bail;
 
 bail:
 	if (err != -ERESTARTSYS && err != -ETIMEDOUT) {