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

[Yuntao Wang <yuntao.wang@linux.dev>]

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.

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?

Thanks,
Yuntao