Re: [PATCH v2 5/7] of/fdt: Simplify the logic of early_init_dt_scan_memory()

[Rob Herring <robh@kernel.org>]

On Thu, Nov 13, 2025 at 9:56 PM Yuntao Wang <yuntao.wang@linux.dev> wrote:
>
> On Thu, 13 Nov 2025 16:03:56 -0600, Rob Herring <robh@kernel.org> wrote:
>
> > On Thu, Nov 13, 2025 at 11:51:02PM +0800, Yuntao Wang wrote:
> > > Use the existing helper functions to simplify the logic of
> > > early_init_dt_scan_memory()
> > >
> > > Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
> > > ---
> > >  drivers/of/fdt.c | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 4c45a97d6652..b6b059960fc2 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -1027,7 +1027,7 @@ int __init early_init_dt_scan_memory(void)
> > >
> > >     fdt_for_each_subnode(node, fdt, 0) {
> > >             const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
> > > -           const __be32 *reg, *endp;
> > > +           const __be32 *reg;
> > >             int l;
> > >             bool hotpluggable;
> > >
> > > @@ -1038,23 +1038,21 @@ int __init early_init_dt_scan_memory(void)
> > >             if (!of_fdt_device_is_available(fdt, node))
> > >                     continue;
> > >
> > > -           reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
> > > +           reg = of_fdt_get_addr_size_prop(node, "linux,usable-memory", &l);
> > >             if (reg == NULL)
> > > -                   reg = of_get_flat_dt_prop(node, "reg", &l);
> > > +                   reg = of_fdt_get_addr_size_prop(node, "reg", &l);
> > >             if (reg == NULL)
> > >                     continue;
> > >
> > > -           endp = reg + (l / sizeof(__be32));
> > >             hotpluggable = of_get_flat_dt_prop(node, "hotpluggable", NULL);
> > >
> > > -           pr_debug("memory scan node %s, reg size %d,\n",
> > > +           pr_debug("memory scan node %s, reg {addr,size} entries %d,\n",
> > >                      fdt_get_name(fdt, node, NULL), l);
> > >
> > > -           while ((endp - reg) >= (dt_root_addr_cells + dt_root_size_cells)) {
> > > +           while (l-- > 0) {
> > >                     u64 base, size;
> > >
> > > -                   base = dt_mem_next_cell(dt_root_addr_cells, &reg);
> > > -                   size = dt_mem_next_cell(dt_root_size_cells, &reg);
> > > +                   of_fdt_read_addr_size(reg, &base, &size);
> >
> > This doesn't work. of_fdt_read_addr_size() needs to take an entry index
> > to read each entry.
> >
> > Rob
>
> Hi Rob,
>
> This was entirely my mistake. I intended to pass &reg rather than reg,
> just like how dt_mem_next_cell() works.
>
> So the correct definition of of_fdt_read_addr_size() should be:
>
> void __init of_fdt_read_addr_size(const __be32 **prop, u64 *addr, u64 *size);
>
> And the correct usage should be:
>
> of_fdt_read_addr_size(&reg, &base, &size);
>
> This bug was caused by my oversight — apologies for that.
>
> I didn’t choose an interface like `of_fdt_read_addr_size(reg, i, &base, &size)`
> because in normal cases the data in prop is consumed sequentially, and I felt
> there was no need to introduce an entry index parameter, which would increase
> the API’s complexity.

Yes, but giving the index mirrors how the unflattened of_property APIs
work. Not so much with the FDT, but we're trying to eliminate giving
out raw pointers (with no lifetime) to the DT data. That doesn't work
well with overlays and dynamic DTs.

> There is another issue reported by kernel test robot:
>
> drivers/of/fdt.c:903:31: error: incompatible pointer types passing 'phys_addr_t *' (aka 'unsigned int *') to parameter of type 'u64 *' (aka 'unsigned long long *') [-Wincompatible-pointer-types]
>
> Given this, the problem exists regardless of which implementation we choose.
>
> I’m considering two possible solutions:
>
> 1. Convert of_fdt_read_addr_size() into a macro.
> 2. Split it into two functions: of_fdt_read_addr() and of_fdt_read_size().
>
> I’m leaning toward the second option.
>
> What do you think? Or do you have a better approach?

Just use local u64 variables and then assign the values to the struct.
This will not warn:

a_phys_addr = a_u64;

(It could silently truncate values, but I'm pretty sure no one runs
32-bit LPAE systems with a non-LPAE kernel on the very few systems
that even still exist).

Rob