[v1] misc: fastrpc: Pass proper scm arguments for static process init

[PATCH v1] misc: fastrpc: Pass proper scm arguments for static process init

Posted by Ekansh Gupta 2 years, 8 months ago

Memory is allocated for dynamic loading when audio daemon is trying
to attach to audioPD on DSP side. This memory is allocated from
reserved CMA memory region and needs ownership assignment to
new VMID in order to use it from audioPD.

In the current implementation, arguments are not correctly passed
to the scm call which might result in failure of dynamic loading
on audioPD. Added changes to pass correct arguments during daemon
attach request.

Fixes: 	0871561055e6 ("misc: fastrpc: Add support for audiopd")
Cc: stable <stable@kernel.org>
Tested-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 30d4d04..b7335dd 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1278,10 +1278,23 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 
 		/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
 		if (fl->cctx->vmcount) {
+			u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
+			struct qcom_scm_vmperm *dst_perms;
+			u32 i;
+
+			dst_perms = kcalloc(fl->cctx->vmcount,
+							sizeof(struct qcom_scm_vmperm), GFP_KERNEL);
+			if (!dst_perms)
+				return -ENOMEM;
+			for (i = 0; i < fl->cctx->vmcount; i++) {
+				dst_perms[i].vmid = fl->cctx->vmperms[i].vmid;
+				dst_perms[i].perm = fl->cctx->vmperms[i].perm;
+			}
+
 			err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
 							(u64)fl->cctx->remote_heap->size,
-							&fl->cctx->perms,
-							fl->cctx->vmperms, fl->cctx->vmcount);
+							&src_perms, dst_perms, fl->cctx->vmcount);
+			kfree(dst_perms);
 			if (err) {
 				dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
 					fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
@@ -1322,13 +1335,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	return 0;
 err_invoke:
 	if (fl->cctx->vmcount) {
-		struct qcom_scm_vmperm perm;
+		u64 src_perms = 0;
+		struct qcom_scm_vmperm dst_perms;
+		u32 i;
 
-		perm.vmid = QCOM_SCM_VMID_HLOS;
-		perm.perm = QCOM_SCM_PERM_RWX;
+		for (i = 0; i < fl->cctx->vmcount; i++) {
+			src_perms |= BIT(fl->cctx->vmperms[i].vmid);
+		}
+
+		dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+		dst_perms.perm = QCOM_SCM_PERM_RWX;
 		err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
 						(u64)fl->cctx->remote_heap->size,
-						&fl->cctx->perms, &perm, 1);
+						&src_perms, &dst_perms, 1);
 		if (err)
 			dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
 				fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
-- 
2.7.4

Re: [PATCH v1] misc: fastrpc: Pass proper scm arguments for static process init

Posted by Srinivas Kandagatla 2 years, 6 months ago

On Mon, 05 Jun 2023 19:18:38 +0530, Ekansh Gupta wrote:
> Memory is allocated for dynamic loading when audio daemon is trying
> to attach to audioPD on DSP side. This memory is allocated from
> reserved CMA memory region and needs ownership assignment to
> new VMID in order to use it from audioPD.
> 
> In the current implementation, arguments are not correctly passed
> to the scm call which might result in failure of dynamic loading
> on audioPD. Added changes to pass correct arguments during daemon
> attach request.
> 
> [...]

Applied, thanks!

[1/1] misc: fastrpc: Pass proper scm arguments for static process init
      commit: 64227235abd9a3ebfb5927dff2202771ffc92b8b

Best regards,
-- 
Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Re: [PATCH v1] misc: fastrpc: Pass proper scm arguments for static process init

Posted by Srinivas Kandagatla 2 years, 8 months ago

Thanks Ekansh for the patch.


On 05/06/2023 14:48, Ekansh Gupta wrote:
> Memory is allocated for dynamic loading when audio daemon is trying
> to attach to audioPD on DSP side. This memory is allocated from
> reserved CMA memory region and needs ownership assignment to
> new VMID in order to use it from audioPD.
> 
> In the current implementation, arguments are not correctly passed
> to the scm call which might result in failure of dynamic loading
> on audioPD. Added changes to pass correct arguments during daemon
> attach request.
> 
> Fixes: 	0871561055e6 ("misc: fastrpc: Add support for audiopd")
> Cc: stable <stable@kernel.org>
> Tested-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>   drivers/misc/fastrpc.c | 31 +++++++++++++++++++++++++------
>   1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 30d4d04..b7335dd 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1278,10 +1278,23 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>   
>   		/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
>   		if (fl->cctx->vmcount) {
> +			u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);

This is redundant too, we already have cctx->perms initialized to 
exactly same bit map.

> +			struct qcom_scm_vmperm *dst_perms;
> +			u32 i;
> +
> +			dst_perms = kcalloc(fl->cctx->vmcount,
> +							sizeof(struct qcom_scm_vmperm), GFP_KERNEL);
> +			if (!dst_perms)
> +				return -ENOMEM;
> +			for (i = 0; i < fl->cctx->vmcount; i++) {
> +				dst_perms[i].vmid = fl->cctx->vmperms[i].vmid;
> +				dst_perms[i].perm = fl->cctx->vmperms[i].perm;

why do we need to copy this to another struct when we already have this 
information in fl->cctx->vmperms ?

> +			}
> +
>   			err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>   							(u64)fl->cctx->remote_heap->size,
> -							&fl->cctx->perms,
> -							fl->cctx->vmperms, fl->cctx->vmcount);
> +							&src_perms, dst_perms, fl->cctx->vmcount);


> +			kfree(dst_perms);
>   			if (err) {
>   				dev_err(fl->sctx->dev, "Failed to assign memory with phys 0x%llx size 0x%llx err %d",
>   					fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
> @@ -1322,13 +1335,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>   	return 0;
>   err_invoke:
>   	if (fl->cctx->vmcount) {
> -		struct qcom_scm_vmperm perm;
> +		u64 src_perms = 0;
> +		struct qcom_scm_vmperm dst_perms;
> +		u32 i;
>   
> -		perm.vmid = QCOM_SCM_VMID_HLOS;
> -		perm.perm = QCOM_SCM_PERM_RWX;
> +		for (i = 0; i < fl->cctx->vmcount; i++) {
> +			src_perms |= BIT(fl->cctx->vmperms[i].vmid);
> +		}
no need for brackets here.
> +
> +		dst_perms.vmid = QCOM_SCM_VMID_HLOS;
> +		dst_perms.perm = QCOM_SCM_PERM_RWX;
>   		err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>   						(u64)fl->cctx->remote_heap->size,
> -						&fl->cctx->perms, &perm, 1);
> +						&src_perms, &dst_perms, 1);
>   		if (err)
>   			dev_err(fl->sctx->dev, "Failed to assign memory phys 0x%llx size 0x%llx err %d",
>   				fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);

Re: [PATCH v1] misc: fastrpc: Pass proper scm arguments for static process init

Posted by Ekansh Gupta 2 years, 7 months ago


On 6/12/2023 4:40 PM, Srinivas Kandagatla wrote:
> Thanks Ekansh for the patch.
> 
> 
> On 05/06/2023 14:48, Ekansh Gupta wrote:
>> Memory is allocated for dynamic loading when audio daemon is trying
>> to attach to audioPD on DSP side. This memory is allocated from
>> reserved CMA memory region and needs ownership assignment to
>> new VMID in order to use it from audioPD.
>>
>> In the current implementation, arguments are not correctly passed
>> to the scm call which might result in failure of dynamic loading
>> on audioPD. Added changes to pass correct arguments during daemon
>> attach request.
>>
>> Fixes:     0871561055e6 ("misc: fastrpc: Add support for audiopd")
>> Cc: stable <stable@kernel.org>
>> Tested-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>   drivers/misc/fastrpc.c | 31 +++++++++++++++++++++++++------
>>   1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 30d4d04..b7335dd 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1278,10 +1278,23 @@ static int 
>> fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>           /* Map if we have any heap VMIDs associated with this ADSP 
>> Static Process. */
>>           if (fl->cctx->vmcount) {
>> +            u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
> 
> This is redundant too, we already have cctx->perms initialized to 
> exactly same bit map.
> 
>> +            struct qcom_scm_vmperm *dst_perms;
>> +            u32 i;
>> +
>> +            dst_perms = kcalloc(fl->cctx->vmcount,
>> +                            sizeof(struct qcom_scm_vmperm), GFP_KERNEL);
>> +            if (!dst_perms)
>> +                return -ENOMEM;
>> +            for (i = 0; i < fl->cctx->vmcount; i++) {
>> +                dst_perms[i].vmid = fl->cctx->vmperms[i].vmid;
>> +                dst_perms[i].perm = fl->cctx->vmperms[i].perm;
> 
> why do we need to copy this to another struct when we already have this 
> information in fl->cctx->vmperms ?
> 
>> +            }
>> +
>>               err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>>                               (u64)fl->cctx->remote_heap->size,
>> -                            &fl->cctx->perms,
>> -                            fl->cctx->vmperms, fl->cctx->vmcount);
>> +                            &src_perms, dst_perms, fl->cctx->vmcount);
> 
> 
>> +            kfree(dst_perms);
>>               if (err) {
>>                   dev_err(fl->sctx->dev, "Failed to assign memory with 
>> phys 0x%llx size 0x%llx err %d",
>>                       fl->cctx->remote_heap->phys, 
>> fl->cctx->remote_heap->size, err);
>> @@ -1322,13 +1335,19 @@ static int 
>> fastrpc_init_create_static_process(struct fastrpc_user *fl,
>>       return 0;
>>   err_invoke:
>>       if (fl->cctx->vmcount) {
>> -        struct qcom_scm_vmperm perm;
>> +        u64 src_perms = 0;
>> +        struct qcom_scm_vmperm dst_perms;
>> +        u32 i;
>> -        perm.vmid = QCOM_SCM_VMID_HLOS;
>> -        perm.perm = QCOM_SCM_PERM_RWX;
>> +        for (i = 0; i < fl->cctx->vmcount; i++) {
>> +            src_perms |= BIT(fl->cctx->vmperms[i].vmid);
>> +        }
> no need for brackets here.
Thanks Srini for your comments. I'll address them in next patch.

-ekansh
>> +
>> +        dst_perms.vmid = QCOM_SCM_VMID_HLOS;
>> +        dst_perms.perm = QCOM_SCM_PERM_RWX;
>>           err = qcom_scm_assign_mem(fl->cctx->remote_heap->phys,
>>                           (u64)fl->cctx->remote_heap->size,
>> -                        &fl->cctx->perms, &perm, 1);
>> +                        &src_perms, &dst_perms, 1);
>>           if (err)
>>               dev_err(fl->sctx->dev, "Failed to assign memory phys 
>> 0x%llx size 0x%llx err %d",
>>                   fl->cctx->remote_heap->phys, 
>> fl->cctx->remote_heap->size, err);