Message-ID: <f26ffb41-f542-2809-9818-903947dd6eb4@gmail.com>
Date:   Wed, 1 Mar 2023 10:49:16 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v3 1/2] of: create of_root if no dtb provided
Content-Language: en-US
To:     Rob Herring <robh@kernel.org>
Cc:     =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <clement.leger@bootlin.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Lizhi Hou <lizhi.hou@xilinx.com>,
        Allan Nielsen <allan.nielsen@microchip.com>,
        Horatiu Vultur <horatiu.vultur@microchip.com>,
        Steen Hegelund <steen.hegelund@microchip.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
References: <20230223213418.891942-1-frowand.list@gmail.com>
 <20230223213418.891942-2-frowand.list@gmail.com>
 <CAL_JsqLR9sm+GRU8EP4eO_Ln2UhD=ztdAU834CzP8RSv2s2jQg@mail.gmail.com>
 <cbf76155-4355-5241-d7a5-816e6721ce1b@gmail.com>
 <20230301030003.GA263873-robh@kernel.org>
From:   Frank Rowand <frowand.list@gmail.com>
In-Reply-To: <20230301030003.GA263873-robh@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 2/28/23 21:00, Rob Herring wrote:
> On Tue, Feb 28, 2023 at 08:05:58PM -0600, Frank Rowand wrote:
>> On 2/27/23 11:17, Rob Herring wrote:
>>> On Thu, Feb 23, 2023 at 3:34 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> When enabling CONFIG_OF on a platform where of_root is not populated by
>>>> firmware, we end up without a root node. In order to apply overlays and
>>>> create subnodes of the root node, we need one. Create this root node
>>>> by unflattening an empty builtin dtb.
>>>>
>>>> If firmware provides a flattened device tree (FDT) then the FDT is
>>>> unflattened via setup_arch().  Otherwise, setup_of() which is called
>>>> immediately after setup_arch(), and will create the default root node
>>>> if it does not exist.
>>>
>>> Why do we need a hook after setup_arch() rather than an initcall?
>>>
>>> Rob
>>
>> It might work as an initcall today.  Maybe not in the future as other
>> initcalls are added.
> 
> That's an argument for never using initcalls (not a bad one either). But 
> we have them and we have little reason not to use them. Also, it's 
> better to do things as late as possible I've found. The earlier you do 
> things, the more architecture specific stuff you hit. That's a big 
> reason for the remaining differences in FDT init across architectures. 
> Maybe after setup_arch is late enough. IDK.
> 
>> But my main stream of thinking is that before the patch "we know" that
>> the device tree data structure exists when setup_arch() returns.
>> Adding setup_of() immediately after setup_arch() retains that
>> guarantee, but one line later in start_kernel().
> 
> I get the logic. I'd just rather not add another hook between the DT 
> code and the core/arch code. Especially for this niche usecase.
> 
> We already have the secondary init when sysfs is up. Can't we just do 
> this there?

In general, I agree with your sentiments about an initcall being a preferred
solution.

But when I was looking at the suggested alternatives, I noticed one sticking
point.  The new setup_of() calls unflatten_device_tree(), which calls
unittest_unflatten_overlay_base().  The call to unittest_unflatten_overlay_base()
is deliberately very early in the boot, so that the memory allocator used
for this very small portion of the devicetree nodes created for unittest
is the same early boot allocator that is used to unflatten an FDT passed
to the kernel from a bootloader.

Digging through this led me to another issue.  I have not tested this patch
series on a user mode linux kernel (on my todo list...).  For user mode linux,
unittest_data_add() is called directly from the late initcall of_unittest().
So for user mode linux, unittest_data_add() will be called a second time - I
need to remove that second call and make sure unittest still works on user
mode linux.

> 
>> I could have instead put the call to setup_of() into each architectures'
>> setup_arch(), but that would just be duplicating the same code for each
>> architecture, which did not seem like a good choice.
> 

> Uhh, no!

Agreed, I guess I was too subtle with "did not seem like a good choice". :-)

> 
> Rob