Re: [PATCH] dt-bindings: Add Google Widevine initialization parameters

[Krzysztof Kozlowski <krzysztof.kozlowski-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On 08/09/2023 12:15, Yi Chou wrote:
> The necessary fields to initialize the widevine related functions in
> OP-TEE.
> 
> Signed-off-by: Yi Chou <yich-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  .../bindings/options/google,widevine.yaml     | 124 ++++++++++++++++++
>  1 file changed, 124 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/options/google,widevine.yaml
> 
> diff --git a/Documentation/devicetree/bindings/options/google,widevine.yaml b/Documentation/devicetree/bindings/options/google,widevine.yaml
> new file mode 100644
> index 0000000000000..bf2b834cb1454
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/options/google,widevine.yaml

There is no such hardware as "options". What is this supposed to be for?
firmware?

> @@ -0,0 +1,124 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/options/google,widevine.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Google Widevine initialization parameters.

This is a title, drop full stop.

> +
> +maintainers:
> +  - Jeffrey Kardatzke <jkardatzke-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> +  - Yi Chou <yich-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> +
> +description:
> +  The necessary fields to initialize the widevine related functions in
> +  OP-TEE. This node does not represent a real device, but serves as a
> +  place for passing data between firmware and OP-TEE.
> +  The public fields (e.g. tpm-auth-public-key & root-of-trust-cert) can
> +  be ignored because it's safe to pass the public information with the
> +  other methods(e.g. userland OP-TEE plugins).

Then why isn't this a property of optee node?

> +
> +properties:
> +  compatible:
> +    const: google,widevine

From the description I have no clue what is "widevine". The more
surprising is to see it as "not hardware" but having its node and
compatible, like it was a hardware node.

> +
> +  hardware-unique-key:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description: |
> +      The hardware-unique key of the Widevine OP-TEE. It will be used
> +      to derive the secure storage key. The length should be 32 bytes.
> +      For more information, please reference:
> +      https://optee.readthedocs.io/en/latest/architecture/porting_guidelines.html#hardware-unique-key

Why would you store it in DT? This is world readable... or you mean this
is some seed?

> +
> +  tpm-auth-public-key:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description: |
> +      The TPM auth public key. Used to communicate the TPM from OP-TEE.
> +      The format of data should be TPM2B_PUBLIC.
> +      For more information, please reference the 12.2.5 section:
> +      https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part2_Structures_pub.pdf
> +
> +  root-of-trust:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description: |
> +      The Widevine root of trust secret. Used to sign the widevine
> +      request in OP-TEE. The length should be 32 bytes. The value
> +      is an ECC NIST P-256 scalar.
> +      For more information, please reference the G.1.2 section:
> +      https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-186.pdf
> +
> +  root-of-trust-cert:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description: |
> +      The X.509 certificate of the Widevine root of trust on this
> +      device. Used to provision the device status with the Widevine
> +      server in OP-TEE.
> +      For more information, please reference:
> +      https://www.itu.int/rec/T-REC-X.509
> +
> +required:
> +  - compatible
> +  - hardware-unique-key
> +  - root-of-trust
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+

Why + ?

> +    options {

There is no such node as "options".


Best regards,
Krzysztof