diff mbox series

[RESEND,v1,1/2] dt-bindings: misc: fastrpc: add fastrpc group IDs property

Message ID 1686155407-20054-2-git-send-email-quic_ekangupt@quicinc.com
State New
Headers
Series Privileged process support on remote subsystem |

Commit Message

Ekansh Gupta June 7, 2023, 4:30 p.m. UTC 
  Add "qcom,fastrpc-gids" property to the list of optional properties.
This property contains the list of privileged group IDs which is
used to offload process to remote subsystem with increased privileges.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Rob Herring June 7, 2023, 5:21 p.m. UTC | #1 
On Wed, 07 Jun 2023 22:00:06 +0530, Ekansh Gupta wrote:
> Add "qcom,fastrpc-gids" property to the list of optional properties.
> This property contains the list of privileged group IDs which is
> used to offload process to remote subsystem with increased privileges.
> 
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/misc/qcom,fastrpc.example.dts:36.17-18 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/misc/qcom,fastrpc.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1512: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1686155407-20054-2-git-send-email-quic_ekangupt@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Krzysztof Kozlowski June 7, 2023, 6:44 p.m. UTC | #2 
On 07/06/2023 18:30, Ekansh Gupta wrote:
> Add "qcom,fastrpc-gids" property to the list of optional properties.
> This property contains the list of privileged group IDs which is
> used to offload process to remote subsystem with increased privileges.

Why or when anyone would use this property?

> 
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> index 1ab9588..2a2124f 100644
> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
> @@ -57,6 +57,11 @@ properties:
>        Virtual machine IDs for remote processor.
>      $ref: "/schemas/types.yaml#/definitions/uint32-array"
>  
> +  qcom,fastrpc-gids:
> +    description:
> +      Group IDs for fastrpc.

You just pasted here property name. It does not help me to understand
what's this. Explain in description.

> +    $ref: "/schemas/types.yaml#/definitions/uint32-array"

Drop quotes.

missing min/maxItems.

> +
>    "#address-cells":
>      const: 1
>  
> @@ -120,6 +125,7 @@ examples:
>              qcom,glink-channels = "fastrpcglink-apps-dsp";
>              label = "sdsp";
>              qcom,non-secure-domain;
> +            qcom,fastrpc-gids = <2908>

Eh, what does 2908 stands for? Why not 3012 or 1842? How anyone can
figure this out?

Best regards,
Krzysztof
Ekansh Gupta June 8, 2023, 9:43 a.m. UTC | #3 
On 6/7/2023 10:51 PM, Rob Herring wrote:
> 
> On Wed, 07 Jun 2023 22:00:06 +0530, Ekansh Gupta wrote:
>> Add "qcom,fastrpc-gids" property to the list of optional properties.
>> This property contains the list of privileged group IDs which is
>> used to offload process to remote subsystem with increased privileges.
>>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/misc/qcom,fastrpc.example.dts:36.17-18 syntax error
> FATAL ERROR: Unable to parse input tree
> make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/misc/qcom,fastrpc.example.dtb] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1512: dt_binding_check] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1686155407-20054-2-git-send-email-quic_ekangupt@quicinc.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 
Thanks for helping with this information. I'll check and re-submit the 
patch.
Ekansh Gupta June 8, 2023, 10:36 a.m. UTC | #4 
On 6/8/2023 12:14 AM, Krzysztof Kozlowski wrote:
> On 07/06/2023 18:30, Ekansh Gupta wrote:
>> Add "qcom,fastrpc-gids" property to the list of optional properties.
>> This property contains the list of privileged group IDs which is
>> used to offload process to remote subsystem with increased privileges.
> 
> Why or when anyone would use this property?
> 
This property can be used for the use cases which requires prioritized 
access to remote subsystem resources.
>>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>> index 1ab9588..2a2124f 100644
>> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>> @@ -57,6 +57,11 @@ properties:
>>         Virtual machine IDs for remote processor.
>>       $ref: "/schemas/types.yaml#/definitions/uint32-array"
>>   
>> +  qcom,fastrpc-gids:
>> +    description:
>> +      Group IDs for fastrpc.
> 
> You just pasted here property name. It does not help me to understand
> what's this. Explain in description.
> 
I'll add more details here when I re-submit the patch.
>> +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
> 
> Drop quotes.
> 
> missing min/maxItems.
> 
I'll update this in v2. Thanks for reviewing.
>> +
>>     "#address-cells":
>>       const: 1
>>   
>> @@ -120,6 +125,7 @@ examples:
>>               qcom,glink-channels = "fastrpcglink-apps-dsp";
>>               label = "sdsp";
>>               qcom,non-secure-domain;
>> +            qcom,fastrpc-gids = <2908>
> 
> Eh, what does 2908 stands for? Why not 3012 or 1842? How anyone can
> figure this out?
> 
There is no hard restriction for this value to be 2908, it can be 
anything. Just that the process which needs a privileged offload to any 
remote subsystem should have the same gid which is defined here, whether 
it is 2908 or anything else.

The intention of having 2908 in example is because this is used in 
Android world where any process running with 2908 gid can offload as 
privileged process to remote subsystem.
Ref: 
https://review.lineageos.org/c/LineageOS/android_device_motorola_sm7250-common/+/305612/4/config.fs

But, it can be set to anything just that the process also needs to adapt 
to this gid.

Thanks,
Ekansh
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski June 8, 2023, 11:33 a.m. UTC | #5 
On 08/06/2023 12:36, Ekansh Gupta wrote:
> 
> 
> On 6/8/2023 12:14 AM, Krzysztof Kozlowski wrote:
>> On 07/06/2023 18:30, Ekansh Gupta wrote:
>>> Add "qcom,fastrpc-gids" property to the list of optional properties.
>>> This property contains the list of privileged group IDs which is
>>> used to offload process to remote subsystem with increased privileges.
>>
>> Why or when anyone would use this property?
>>
> This property can be used for the use cases which requires prioritized 
> access to remote subsystem resources.

This does not help me much yet.... Which systems or boards need
prioritized access?

>>>
>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>> ---
>>>   Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> index 1ab9588..2a2124f 100644
>>> --- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> +++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
>>> @@ -57,6 +57,11 @@ properties:
>>>         Virtual machine IDs for remote processor.
>>>       $ref: "/schemas/types.yaml#/definitions/uint32-array"
>>>   
>>> +  qcom,fastrpc-gids:
>>> +    description:
>>> +      Group IDs for fastrpc.
>>
>> You just pasted here property name. It does not help me to understand
>> what's this. Explain in description.
>>
> I'll add more details here when I re-submit the patch.
>>> +    $ref: "/schemas/types.yaml#/definitions/uint32-array"
>>
>> Drop quotes.
>>
>> missing min/maxItems.
>>
> I'll update this in v2. Thanks for reviewing.
>>> +
>>>     "#address-cells":
>>>       const: 1
>>>   
>>> @@ -120,6 +125,7 @@ examples:
>>>               qcom,glink-channels = "fastrpcglink-apps-dsp";
>>>               label = "sdsp";
>>>               qcom,non-secure-domain;
>>> +            qcom,fastrpc-gids = <2908>
>>
>> Eh, what does 2908 stands for? Why not 3012 or 1842? How anyone can
>> figure this out?
>>
> There is no hard restriction for this value to be 2908, it can be 
> anything. Just that the process which needs a privileged offload to any 
> remote subsystem should have the same gid which is defined here, whether 
> it is 2908 or anything else.
> 
> The intention of having 2908 in example is because this is used in 
> Android world where any process running with 2908 gid can offload as 
> privileged process to remote subsystem.
> Ref: 
> https://review.lineageos.org/c/LineageOS/android_device_motorola_sm7250-common/+/305612/4/config.fs
> 
> But, it can be set to anything just that the process also needs to adapt 
> to this gid.

So this is process ID? Group ID? GID? Why would you ever encode group
IDs, which change in every installation, or process IDs, which change
every boot, in DTS common for all devices?

This is not a DT property.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
index 1ab9588..2a2124f 100644
--- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
+++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
@@ -57,6 +57,11 @@  properties:
       Virtual machine IDs for remote processor.
     $ref: "/schemas/types.yaml#/definitions/uint32-array"
 
+  qcom,fastrpc-gids:
+    description:
+      Group IDs for fastrpc.
+    $ref: "/schemas/types.yaml#/definitions/uint32-array"
+
   "#address-cells":
     const: 1
 
@@ -120,6 +125,7 @@  examples:
             qcom,glink-channels = "fastrpcglink-apps-dsp";
             label = "sdsp";
             qcom,non-secure-domain;
+            qcom,fastrpc-gids = <2908>
             #address-cells = <1>;
             #size-cells = <0>;