Message-ID: <f0609361-6fb6-a446-4e23-646201943923@linaro.org>
Date:   Tue, 4 Jul 2023 07:57:21 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Mukesh Ojha <quic_mojha@...cinc.com>
Cc:     Rob Herring <robh+dt@...nel.org>, corbet@....net,
        agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
        conor+dt@...nel.org, keescook@...omium.org, tony.luck@...el.com,
        gpiccoli@...lia.com, mathieu.poirier@...aro.org,
        catalin.marinas@....com, will@...nel.org, linus.walleij@...aro.org,
        andy.shevchenko@...il.com, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-hardening@...r.kernel.org,
        linux-remoteproc@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v4 08/21] dt-bindings: reserved-memory: Add qcom,ramoops
 binding

On 03/07/2023 17:55, Mukesh Ojha wrote:
> 
> 
> On 7/3/2023 12:50 PM, Krzysztof Kozlowski wrote:
>> On Mon, 3 Jul 2023 at 08:22, Mukesh Ojha <quic_mojha@...cinc.com> wrote:
>>> On 7/2/2023 1:42 PM, Krzysztof Kozlowski wrote:
>>>>>> The big difference is if firmware is not deciding where this log
>>>>>> lives, then it doesn't need to be in DT. How does anything except the
>>>>>> kernel that allocates the log find the logs?
>>>>>
>>>>> Yes, you are correct, firmware is not deciding where the logs lives
>>>>> instead here, Kernel has reserved the region where the ramoops region
>>>>> lives and later with the minidump registration where, physical
>>>>> address/size/virtual address(for parsing) are passed and that is how
>>>>> firmware is able to know and dump those region before triggering system
>>>>> reset.
>>>>
>>>> Your explanation does not justify storing all this in DT. Kernel can
>>>> allocate any memory it wishes, store there logs and pass the address to
>>>> the firmware. That's it, no need for DT.
>>>
>>> If you go through the driver, you will know that what it does, is
>>
>> We talk about bindings and I should not be forced to look at the
>> driver to be able to understand them. Bindings should stand on their
>> own.
> 
> Why can't ramoops binding have one more feature where it can add a flag 
> *dynamic* to indicate the regions are dynamic and it is for platforms
> where there is another entity 'minidump' who is interested in these
> regions.

Because we do not define dynamic stuff in Devicetree. Dynamic means
defined by SW or runtime configurable. It is against the entire idea of
Devicetree which is for non-discoverable hardware.

> 
>>
>>> just create platform device for actual ramoops driver to probe and to
>>
>> Not really justification for Devicetree anyway. Whatever your driver
>> is doing, is driver's business, not bindings.
>>
>>> provide this it needs exact set of parameters of input what original
>>> ramoops DT provides, we need to keep it in DT as maintaining this in
>>> driver will not scale well with different size/parameter size
>>> requirement for different targets.
>>
>> Really? Why? I don't see a problem in scaling. At all.
> 
> I had attempted it here,
> 
> https://lore.kernel.org/lkml/1683133352-10046-10-git-send-email-quic_mojha@quicinc.com/
> 
> but got comments related to hard coding and some in favor of having
> the same set of properties what ramoops has/provides
> 
> https://lore.kernel.org/lkml/e25723bf-be85-b458-a84c-1a45392683bb@gmail.com/
> 
> https://lore.kernel.org/lkml/202305161347.80204C1A0E@keescook/

Then you were tricked. I don't get why someone else suggests that
non-hardware property should be part of Devicetree, but anyway it's the
call of Devicetree binding maintainers, not someone else. DT is not
dumping ground for all the system configuration variables.


>>
>>>
>>>>
>>>>>
>>>>> A part of this registration code you can find in 11/21
>>>>>
>>>>>> I'm pretty sure I already said all this before.
>>>>>
>>>>> Yes, you said this before but that's the reason i came up with vendor
>>>>> ramoops instead of changing traditional ramoops binding.
>>>>
>>>> That's unexpected conclusion. Adding more bindings is not the answer to
>>>> comment that it should not be in the DTS in the first place.
>>>
>>> Please suggest, what is the other way being above text as requirement..
>>
>> I do not see any requirement for us there. Forcing me to figure out
>> how to add non-hardware property to DT is not the way to convince
>> reviewers. But if you insist - we have ABI for this, called sysfs. If
>> it is debugging feature, then debugfs.
> 
> ramoops already support module params and a way to pass these parameters
> from bootargs but it also need to know the hard-codes addresses, so, 
> doing something in sysfs will be again duplication with ramoops driver..

Why do you need hard-coded addresses?

> 
> If this can be accommodated under ramoops, this will be very small 
> change, like this
> 
> https://lore.kernel.org/lkml/20230622005213.458236-1-isaacmanjarres@google.com/

That's also funny patch - missing bindings updated, missing CC DT
maintainers.

Best regards,
Krzysztof

Powered by blists - more mailing lists