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

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

On 18/09/2023 06:20, Yi Chou wrote:
> On Sun, Sep 17, 2023 at 4:40 PM Krzysztof Kozlowski
> <krzysztof.kozlowski-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>
>> 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?
> 
> These DT fields would not be consumed by the OS.
> https://www.spinics.net/lists/devicetree-spec/msg01195.html
> The previous discussion tended to use the "options" node.
> Do we have any better place for these widevine related fields?

I'll let Rob comment on this in such case.

> 
>>
>>> @@ -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.
> 
> Got it, will be fixed in the next patch.
> 
>>
>>> +
>>> +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?
> 
> Are you talking about the /firmware/optee node?
> If I understand correctly, that node was talking about how the kernel
> communicates with the OP-TEE.
> But what we are doing here is passing some secrets from trusted
> firmware into OP-TEE, and the data would not go through the linux
> kernel.
> I'm not sure if it is a good idea to mix two different purpose fields
> in the same 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.
> 
> We already have a "chosen" node that is "not hardware" in the DT.
> Should we just remove the compatible field from this node?
> 
> BTW, Widevine is a digital rights management (DRM) system to make sure
> the video stream can only be decoded on the valid devices.

Then describe it in the description.

Best regards,
Krzysztof