linux-kernel - Re: [PATCH v2 5/7] of/fdt: Simplify the logic of early_init_dt_scan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20251115140031.181716-1-yuntao.wang@linux.dev>
Date: Sat, 15 Nov 2025 22:00:29 +0800
From: Yuntao Wang <yuntao.wang@...ux.dev>
To: robh@...nel.org
Cc: akpm@...ux-foundation.org,
	ardb@...nel.org,
	bhe@...hat.com,
	catalin.marinas@....com,
	changyuanl@...gle.com,
	devicetree@...r.kernel.org,
	geert+renesas@...der.be,
	geoff@...radead.org,
	graf@...zon.com,
	james.morse@....com,
	linux-kernel@...r.kernel.org,
	mark.rutland@....com,
	rppt@...nel.org,
	saravanak@...gle.com,
	thunder.leizhen@...wei.com,
	yuntao.wang@...ux.dev
Subject: Re: [PATCH v2 5/7] of/fdt: Simplify the logic of early_init_dt_scan_memory()

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

> On Thu, Nov 13, 2025 at 9:56 PM Yuntao Wang <yuntao.wang@...ux.dev> wrote:
> >
> > On Thu, 13 Nov 2025 16:03:56 -0600, Rob Herring <robh@...nel.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@...ux.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