From:   Michael Ellerman <mpe@ellerman.id.au>
To:     Rob Herring <robh@kernel.org>
Cc:     John Crispin <john@phrozen.org>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        Frank Rowand <frowand.list@gmail.com>,
        "open list:MIPS" <linux-mips@vger.kernel.org>,
        linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        devicetree@vger.kernel.org
Subject: Re: [PATCH v2] of/fdt: Rework early_init_dt_scan_memory() to call
 directly
In-Reply-To: <CAL_JsqKbaRgivZMxEj6Mjdny2LNeSA1GQyDW-nQe7E2irPc-Fw@mail.gmail.com>
References: <20211208155839.4084795-1-robh@kernel.org>
 <87fsqwn03o.fsf@mpe.ellerman.id.au>
 <CAL_JsqLpq7fx0pyQiJFa0P5C3JXijiVe_fr84x9RML1aDJ7vDQ@mail.gmail.com>
 <877dc7mo3o.fsf@mpe.ellerman.id.au>
 <CAL_JsqKbaRgivZMxEj6Mjdny2LNeSA1GQyDW-nQe7E2irPc-Fw@mail.gmail.com>
Date:   Wed, 15 Dec 2021 21:25:27 +1100
Message-ID: <87zgp2kvvs.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Rob Herring <robh@kernel.org> writes:
> On Tue, Dec 14, 2021 at 5:18 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>
>> Rob Herring <robh@kernel.org> writes:
>> > On Mon, Dec 13, 2021 at 6:47 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>> >> Rob Herring <robh@kernel.org> writes:
>> >> > Use of the of_scan_flat_dt() function predates libfdt and is discouraged
>> >> > as libfdt provides a nicer set of APIs. Rework
>> >> > early_init_dt_scan_memory() to be called directly and use libfdt.
>> >> ...
>> >> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> >> > index 6e1a106f02eb..63762a3b75e8 100644
>> >> > --- a/arch/powerpc/kernel/prom.c
>> >> > +++ b/arch/powerpc/kernel/prom.c
>> >> > @@ -532,19 +532,19 @@ static int  __init early_init_drmem_lmb(struct drmem_lmb *lmb,
>> >> >  }
>> >> >  #endif /* CONFIG_PPC_PSERIES */
>> >> >
>> >> > -static int __init early_init_dt_scan_memory_ppc(unsigned long node,
>> >> > -                                             const char *uname,
>> >> > -                                             int depth, void *data)
>> >> > +static int __init early_init_dt_scan_memory_ppc(void)
>> >> >  {
>> >> >  #ifdef CONFIG_PPC_PSERIES
>> >> > -     if (depth == 1 &&
>> >> > -         strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
>> >> > +     const void *fdt = initial_boot_params;
>> >> > +     int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
>> >> > +
>> >> > +     if (node > 0) {
>> >> >               walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
>> >> >               return 0;
>> >> >       }
>>
>> It's that return that is the problem.
>>
>> Now that early_init_dt_scan_memory_ppc() is only called once, that
>> return causes us to skip scanning regular memory nodes if there is an
>> "ibm,dynamic-reconfiguration-memory" property present.
>>
>> So the fix is just:
>>
>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index 1098de3b172f..125661e5fcf3 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -538,10 +538,8 @@ static int __init early_init_dt_scan_memory_ppc(void)
>>         const void *fdt = initial_boot_params;
>>         int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
>>
>> -       if (node > 0) {
>> +       if (node > 0)
>>                 walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
>> -               return 0;
>> -       }
>>  #endif
>>
>>         return early_init_dt_scan_memory();
>
> Thanks! I've rolled that in.
>
>> > The only thing I see is now there is an assumption that 'memory' nodes
>> > are off the root node only. Before they could be anywhere.
>>
>> I don't know of any machines where that would be a problem. But given
>> all the wild and wonderful device trees out there, who really knows :)
>>
>> Maybe we should continue to allow memory nodes to be anywhere, and print
>> a warning for any that aren't at the root. Then if no one reports any
>> hits for the warning we could switch to only allowing them at the root?
>
> I really doubt there's any case. I just have the least visibility into
> what IBM DTs look like. I checked some old DT files I have and also
> u-boot only supports off the root node.

The IBM ones are pretty standard, it's other embedded things I'd be more
worried about.

I have a collection of device trees, but they were given to me by
various random people over the years and I'm not comfortable putting
them up in public. I looked through those and didn't see anything odd.

I'll try and get a collection of device trees from machines of mine and
put them somewhere public.

cheers