Message-ID: <8a4d1e88-56d0-fb71-7172-75b3ecf6e4b6@quicinc.com>
Date:   Mon, 30 Jan 2023 13:43:10 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH v4 1/2] dt-bindings: ramoops: Inherit reserve memory
 property
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <linux-hardening@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>
CC:     Kees Cook <keescook@chromium.org>, Tony Luck <tony.luck@intel.com>,
        "Guilherme G . Piccoli" <gpiccoli@igalia.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
References: <1674835252-31954-1-git-send-email-quic_mojha@quicinc.com>
 <637fe4cd-f9e9-ae22-0085-30ea3f1dd5af@linaro.org>
From:   Mukesh Ojha <quic_mojha@quicinc.com>
In-Reply-To: <637fe4cd-f9e9-ae22-0085-30ea3f1dd5af@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/28/2023 1:33 AM, Krzysztof Kozlowski wrote:
> On 27/01/2023 17:00, Mukesh Ojha wrote:
>> The reserved memory region for ramoops is assumed to be at a
>> fixed and known location when read from the devicetree. This
>> is not desirable in an environment where it is preferred the
>> region to be dynamically allocated at runtime, as opposed to
>> being fixed at compile time.
>>
>> So, update the ramoops binding by inheriting some reserve memory
>> property to allocate the ramoops region dynamically.
> 
> Where is the update which adds "inheriting"?

By inheriting, i meant using reserve memory properties..

Probably rephrase above as.

"dt-bindings: ramoops: Support dynamic ramoops region allocation

The reserved memory region for ramoops is assumed to be at a
fixed and known location when read from the devicetree. This
is not desirable in an environment where it is preferred the
region to be dynamically allocated at runtime, as opposed to
being fixed at compile time. This can be done with minor update
in ramoops binding as it inherit reserve memory property
(.yaml) in the binding.

Dynamic region could be used by providing size(region size) and
alloc-ranges(allowed ddr region to allocate the size from) instead
of mentioning regs"

Does it sound reasonable ?

-Mukesh
> 
>>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Tony Luck <tony.luck@intel.com>
>> Cc: Guilherme G. Piccoli <gpiccoli@igalia.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>> Changes in v4:
>>   - Addressed comment made by Krzysztof on ramoops node name.
>>
>> Changes in v3:
>>   - Fixed yaml error and updated commit text as per comment.
>>
>> Change in v2:
>>    - Added this patch as per changes going to be done in patch 3/3
>>
>>   .../bindings/reserved-memory/ramoops.yaml          | 34 ++++++++++++++++++++--
>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>> index 0391871..8741626 100644
>> --- a/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>> +++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
>> @@ -10,7 +10,8 @@ description: |
>>     ramoops provides persistent RAM storage for oops and panics, so they can be
>>     recovered after a reboot. This is a child-node of "/reserved-memory", and
>>     is named "ramoops" after the backend, rather than "pstore" which is the
>> -  subsystem.
>> +  subsystem. This region can be reserved both statically or dynamically by
>> +  using appropriate property in device tree.
>>   
>>     Parts of this storage may be set aside for other persistent log buffers, such
>>     as kernel log messages, or for optional ECC error-correction data.  The total
>> @@ -112,7 +113,13 @@ unevaluatedProperties: false
>>   
>>   required:
>>     - compatible
>> -  - reg
> 
> This is okay, but:
> 
>> +
>> +oneOf:
>> +  - required:
>> +      - reg
>> +
>> +  - required:
>> +      - size
> 
> I now keep wondering - why do you need this?

This should be same as..
Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml

> 
>>   
>>   anyOf:
>>     - required: [record-size]
>> @@ -142,3 +149,26 @@ examples:
>>               };
>>           };
>>       };
>> +
>> +  - |
>> +    / {
>> +        compatible = "foo";
>> +        model = "foo";
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +
>> +        reserved-memory {
>> +            #address-cells = <1>;
>> +            #size-cells = <1>;
>> +            ranges;
>> +
>> +            ramoops_region: ramoops {
>> +                compatible = "ramoops";
>> +                alloc-ranges = <0x00000000 0xffffffff>;
>> +                size = <0x0 0x10000>;       /* 64kB */
>> +                console-size = <0x8000>;    /* 32kB */
>> +                record-size = <0x400>;      /*  1kB */
>> +                ecc-size = <16>;
>> +            };
>> +        };
>> +    };
> 
> This example does not bring anything new for the ramoops. It's an
> example for reserved-memory to show usage with alloc-ranges. There is
> nothing useful here in terms of ramoops, so I think it should be dropped.
> 

will drop this..

> Best regards,
> Krzysztof
>