Message-ID: <53a602e2-0590-6c6a-597b-fd55faa3a4ab@linaro.org>
Date:   Wed, 27 Jul 2022 13:24:40 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Maximilian Luz <luzmaximilian@...il.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Ard Biesheuvel <ardb@...nel.org>
Cc:     Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Steev Klimaszewski <steev@...i.org>,
        Shawn Guo <shawn.guo@...aro.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Cristian Marussi <cristian.marussi@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-arm-msm@...r.kernel.org, linux-efi@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Vinod Koul <vkoul@...nel.org>
Subject: Re: [PATCH 4/4] dt-bindings: firmware: Add Qualcomm UEFI Secure
 Application client

On 26/07/2022 17:00, Maximilian Luz wrote:
> On 7/26/22 15:25, Krzysztof Kozlowski wrote:
>> On 26/07/2022 13:15, Maximilian Luz wrote:
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: qcom,tee-uefisecapp
>>>>
>>>> Isn't this SoC-specific device? Generic compatibles are usually not
>>>> expected.
>>>
>>> This is essentially software (kernel driver) talking to software (in the
>>> TrustZone), so I don't expect there to be anything SoC specific about it.
>>
>> You are documenting here firmware in TZ (not kernel driver). Isn't this
>> a specific piece which might vary from device to device?
>>
>> IOW, do you expect the same compatible to work for all possible Qualcomm
>> boards (past and future like in 10 years from now)?
> 
> I'm not sure if Qualcomm will still use the "uefisecapp" approach in 10
> years, but I don't expect the interface of uefisecapp to change. The
> interface is modeled after the respective UEFI functions, which are spec
> and thus I don't expect those to change. Also, it seems to have been
> around for a couple of generations and it hasn't changed. The oldest
> tested is sdm850 (Lenovo Yoga C630), and the latest is sc8280xp
> (Thinkpad X13s).

Expectation is not the same as having a specification saying it will not
change.
> 
> Why not make this behave like a "normal" third-party device? If the
> interface ever changes use qcom,tee-uefisecapp-v2 or something like
> that? Again, this does not seem to be directly tied to the SoC.

Such approach is not "normal" for third-party devices. Compatible for
devices has model number. If the block has specification, then v2 would
have sense, otherwise you would invent own versioning...

I would say that firmware implementation can easily change. How much of
your code is tied to it, I don't know, but argument "I don't expect
Qualcomm to change something in their firmware" is not the correct argument.

> 
> Then again, if you prefer to name everything based on
> "qcom,<device>-<soc>" I don't have any strong arguments against it and
> I'm happy to change that. I just think it will unnecessarily introduce
> a bunch of compatibles and doesn't reflect the interface "versioning"
> situation as I see it.

Why bunch? All devices could bind to one specific compatible, as they
are compatible.

> 
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    firmware {
>>>>> +        scm {
>>>>> +            compatible = "qcom,scm-sc8180x", "qcom,scm";
>>>>> +        };
>>>>> +        tee-uefisecapp {
>>>>> +            compatible = "qcom,tee-uefisecapp";
>>>>
>>>> You did not model here any dependency on SCM. This is not full
>>>> description of the firmware/hardware
>>>
>>> How would I do that? A lot of other stuff also depends on SCM being
>>> present (e.g. qcom_q6v5_pas for loading mdt files) and I don't see them
>>> declare this in the device tree. As far as I can tell, SCM is pretty
>>> much expected to be there at all times (i.e. can't be unloaded) and
>>> drivers check for it when probing via qcom_scm_is_available(),
>>> deferring probe if not.
>>
>> It seems this will be opening a can of worms...
> 
> Indeed.
> 
>> The problem with existing approach is:
>> 1. Lack of any probe ordering or probe deferral support.
>> 2. Lack of any other dependencies, e.g. for PM.
> 
> I'm not entirely sure what you mean by "lack of probe deferral support".
> We have qcom_scm_is_available() and defer probe if that fails. So
> deferral works, unless I'm misunderstanding something.

And how do you differentiate that qcom_scm_is_available() failed because
it is not yet available (defer probe) or it is broken and will never
load? All regular consumer-provider interfaces have it sorted out.

> 
> But yes, correct on the other points.
> 
>> Unloading is "solved" only by disallowing the unload, not by proper
>> device links and module get/put.
>>
>> I understand that SCM must be there, but the same for several other
>> components and for these others we have ways to pass reference around
>> (e.g. syscon regmap, PHYs handles).
>>
>>>
>>> Don't take this as an excuse as in "I want to leave that out", it's just
>>> that I don't know how one would declare such a dependency explicitly. If
>>> you can tell me how to fix it, I'll include that for v2.
>>
>> I think there are no dedicated subsystem helpers for this (like for
>> provider/consumer of resets/power domains/clocks etc), so one way would
>> be something like nvidia,bpmp is doing.
> 
> I assume you're referring to tegra_bpmp_get()? Does this correctly
> handle PM dependencies? At least as far as I can tell it doesn't
> explicitly establish a device link, it only gets a reference to the
> device, which doesn't guarantee the presence of a driver. Nor correct PM
> ordering. Please correct me if I'm wrong. As far as I know automatic
> creation of device links only works with certain props defined in
> of_supplier_bindings, right?

The Tegra choice is not complete, but it implements at least parts of it
and firmware dependencies are modeled in DTS. Other way would be to add
your device as child of SMC firmware and then you do not need bindings
at all...

> 
> So unless I'm wrong there is also a bunch of other stuff that may be
> subtly broken. (Again, not a justification to include these changes,
> just wondering whether there should be a conscious approach to find and
> fix these things... rather than discover them patch-by-patch).
> 
>> meson_sm_get is a bit similar - looking by compatible. This is less
>> portable and I would prefer the bpmp way (just like syscon phandles).
> 
> I have another example (that could be improved via a phandle in DT): For
> the Surface System Aggregator (in ACPI-land), we have ssam_client_bind().
> This function 1) checks if the controller is available and ready, 2) if
> it is gets a reference to it, and 3) establishes a device link for
> PM-ordering, before 4) returning the reference to that controller to the
> client. This combined with deferring probe ensures that we will always
> have a valid reference. And since we're in DT-land, we could hook that
> up with a phandle reference to SCM and load that instead of having to
> use a global static.

Yes, that's better example than Tegra BPMP.

>> The qcom_q6v5_pas could be converted later to use similar approach and
>> eventually the "tatic struct qcom_scm *__scm;" can be entirely removed.
>>
>> Any comments on this approach from Konrad, Bjorn, Dmitry, Vinod and
>> anyone else?
> 
> Regards,
> Max


Best regards,
Krzysztof

Powered by blists - more mailing lists