Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <946f9db2-41a0-b58c-771a-5a82fdf28039@kernel.org>
Date:   Tue, 4 Jul 2023 08:05:21 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops
 memory regions
Content-Language: en-US
To:     "Isaac J. Manjarres" <isaacmanjarres@google.com>,
        Kees Cook <keescook@chromium.org>,
        Tony Luck <tony.luck@intel.com>,
        "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
        Rob Herring <robh+dt@kernel.org>,
        Conor Dooley <conor+dt@kernel.org>
Cc:     "Isaac J. Manjarres" <isaacm@codeaurora.org>,
        Mukesh Ojha <mojha@codeaurora.org>,
        Prasad Sodagudi <psodagud@codeaurora.org>,
        kernel-team@android.com, linux-hardening@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230622005213.458236-1-isaacmanjarres@google.com>
From:   Krzysztof Kozlowski <krzk@kernel.org>
In-Reply-To: <20230622005213.458236-1-isaacmanjarres@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 22/06/2023 02:52, Isaac J. Manjarres wrote:
> From: "Isaac J. Manjarres" <isaacm@codeaurora.org>
> 
> 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 environments where it is preferred for the region to be dynamically
> allocated early during boot (i.e. the memory region is defined with
> the "alloc-ranges" property instead of the "reg" property).
> 
> If the location of the ramoops region is not fixed via the "reg"
> devicetree property, the call to platform_get_resource() will fail
> because resources of type IORESOURCE_MEM must be described with the
> "reg" property.
> 
> Since ramoops regions are part of the reserved-memory devicetree
> node, they exist in the reserved_mem array. This means that the
> of_reserved_mem_lookup() function can be used to retrieve the
> reserved_mem structure for the ramoops region, and that structure
> contains the base and size of the region, even if it has been
> dynamically allocated.
> 
> Thus invoke of_reserved_mem_lookup() in case the call to
> platform_get_resource() fails in order to support dynamically
> allocated ramoops memory regions.

You missed change to Devicetree binding, so the reg is still required
for all DT-based platforms. For such, this patch is just half-baked.

You also did not CC DT maintainers, which would be nice considering some
improper advises for minidump like here:
https://lore.kernel.org/lkml/e25723bf-be85-b458-a84c-1a45392683bb@gmail.com/

DT is not for some dynamically allocated properties or dynamic system
configuration. You don't need DT for that. Therefore if you remove the
reg, I question the entire point of this binding.

See also:
https://lore.kernel.org/lkml/CAL_JsqJ_TTnGjjB2d8_FKHpWBRG5GHLoWnabCKjsdeZ4QFdNEg@mail.gmail.com/

https://lore.kernel.org/lkml/20230202235916.GA2931100-robh@kernel.org/


Best regards,
Krzysztof