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);
 
 			if (size == 0)
 				continue;
-- 
2.51.0

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

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

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

Hi Rob,

On Fri, 14 Nov 2025 at 16:11, Rob Herring <robh@kernel.org> wrote:
> On Thu, Nov 13, 2025 at 9:56 PM Yuntao Wang <yuntao.wang@linux.dev> wrote:
> > 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).

You mean running multi_v7_defconfig on anything found by
'git grep "#address-cells\s*=\s*<2>" -- arch/arm/boot/dts'?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Fri, 14 Nov 2025 09:11:18 -0600, Rob Herring <robh@kernel.org> wrote:

> 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

Hi Rob,

The link to the v3 patch series:

https://lore.kernel.org/linux-devicetree/20251115134753.179931-1-yuntao.wang@linux.dev/

Thanks,
Yuntao