[PATCH v2 0/7] of/fdt: Some bug fixes and cleanups

[PATCH v2 0/7] of/fdt: Some bug fixes and cleanups

[Yuntao Wang]

This patch series fixes several bugs related to dt_root_addr_cells and
dt_root_size_cells, and performs some cleanup.

Changelog:

v1: Consolidate duplicate code into helper functions

Links:

v1: https://lore.kernel.org/linux-devicetree/20251112143520.233870-11-yuntao.wang@linux.dev/t/

Yuntao Wang (7):
  of/fdt: Consolidate duplicate code into helper functions
  of/fdt: Fix the len check in early_init_dt_check_for_elfcorehdr()
  of/fdt: Fix the len check in
    early_init_dt_check_for_usable_mem_range()
  of/fdt: Fix incorrect use of dt_root_addr_cells in
    early_init_dt_check_kho()
  of/fdt: Simplify the logic of early_init_dt_scan_memory()
  of/reserved_mem: Simplify the logic of __reserved_mem_reserve_reg()
  of/reserved_mem: Simplify the logic of
    fdt_scan_reserved_mem_reg_nodes()

 drivers/of/fdt.c             | 93 ++++++++++++++++++++++--------------
 drivers/of/of_reserved_mem.c | 37 ++++----------
 include/linux/of_fdt.h       |  5 ++
 3 files changed, 72 insertions(+), 63 deletions(-)

-- 
2.51.0

[PATCH v2 1/7] of/fdt: Consolidate duplicate code into helper functions

[Yuntao Wang]

Currently, there are many pieces of nearly identical code scattered across
different places. Consolidate the duplicate code into helper functions to
improve maintainability and reduce the likelihood of errors.

Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
---
 drivers/of/fdt.c       | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/of_fdt.h |  5 +++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 0edd639898a6..5e0eabc1449f 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -625,6 +625,47 @@ const void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
 	return fdt_getprop(initial_boot_params, node, name, size);
 }
 
+const __be32 *__init of_fdt_get_addr_size_prop(unsigned long node,
+                                               const char *name, int *entries)
+{
+	const __be32 *prop;
+	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+
+	prop = of_get_flat_dt_prop(node, name, &len);
+	if (!prop) {
+		*entries = 0;
+		return NULL;
+	}
+
+	if (len % elen) {
+		*entries = -1;
+		return NULL;
+	}
+
+	*entries = len / elen;
+	return prop;
+}
+
+bool __init of_fdt_get_addr_size(unsigned long node, const char *name,
+                                 u64 *addr, u64 *size)
+{
+	const __be32 *prop;
+	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+
+	prop = of_get_flat_dt_prop(node, name, &len);
+	if (!prop || len < elen)
+		return false;
+
+	of_fdt_read_addr_size(prop, addr, size);
+	return true;
+}
+
+void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
+{
+	*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
+	*size = dt_mem_next_cell(dt_root_size_cells, &prop);
+}
+
 /**
  * of_fdt_is_compatible - Return true if given node from the given blob has
  * compat in its compatible list
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index b8d6c0c20876..3a0805ff6c7b 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -55,6 +55,11 @@ extern int of_get_flat_dt_subnode_by_name(unsigned long node,
 					  const char *uname);
 extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
 				       int *size);
+extern const __be32 *of_fdt_get_addr_size_prop(unsigned long node,
+                                               const char *name, int *entries);
+extern bool of_fdt_get_addr_size(unsigned long node, const char *name,
+                                 u64 *addr, u64 *size);
+extern void of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size);
 extern int of_flat_dt_is_compatible(unsigned long node, const char *name);
 extern unsigned long of_get_flat_dt_root(void);
 extern uint32_t of_get_flat_dt_phandle(unsigned long node);
-- 
2.51.0

[PATCH v2 2/7] of/fdt: Fix the len check in early_init_dt_check_for_elfcorehdr()

[Yuntao Wang]

The len value is in bytes, while `dt_root_addr_cells + dt_root_size_cells`
is in cells (4 bytes per cell). Comparing them directly is incorrect.

Use a helper function to simplify the code and address this issue.

Fixes: f7e7ce93aac1 ("of: fdt: Add generic support for handling elf core headers property")
Fixes: e62aaeac426ab1dd ("arm64: kdump: provide /proc/vmcore file")
Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
---
 drivers/of/fdt.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 5e0eabc1449f..80b7236efdc6 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -853,21 +853,15 @@ static void __init early_init_dt_check_for_initrd(unsigned long node)
  */
 static void __init early_init_dt_check_for_elfcorehdr(unsigned long node)
 {
-	const __be32 *prop;
-	int len;
-
 	if (!IS_ENABLED(CONFIG_CRASH_DUMP))
 		return;
 
 	pr_debug("Looking for elfcorehdr property... ");
 
-	prop = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
-	if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
+	if (!of_fdt_get_addr_size(node, "linux,elfcorehdr",
+	                          &elfcorehdr_addr, &elfcorehdr_size))
 		return;
 
-	elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
-	elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &prop);
-
 	pr_debug("elfcorehdr_start=0x%llx elfcorehdr_size=0x%llx\n",
 		 elfcorehdr_addr, elfcorehdr_size);
 }
-- 
2.51.0

[PATCH v2 3/7] of/fdt: Fix the len check in early_init_dt_check_for_usable_mem_range()

[Yuntao Wang]

The len value is in bytes, while `dt_root_addr_cells + dt_root_size_cells`
is in cells (4 bytes per cell). Modulo calculation between them is
incorrect, the units must be converted first.

Use helper functions to simplify the code and fix this issue.

Fixes: fb319e77a0e7 ("of: fdt: Add memory for devices by DT property "linux,usable-memory-range"")
Fixes: 2af2b50acf9b9c38 ("of: fdt: Add generic support for handling usable memory range property")
Fixes: 8f579b1c4e347b23 ("arm64: limit memory regions based on DT property, usable-memory-range")
Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
---
 drivers/of/fdt.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 80b7236efdc6..d0caaab42aa7 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -884,7 +884,7 @@ static unsigned long chosen_node_offset = -FDT_ERR_NOTFOUND;
 void __init early_init_dt_check_for_usable_mem_range(void)
 {
 	struct memblock_region rgn[MAX_USABLE_RANGES] = {0};
-	const __be32 *prop, *endp;
+	const __be32 *prop;
 	int len, i;
 	unsigned long node = chosen_node_offset;
 
@@ -893,14 +893,14 @@ void __init early_init_dt_check_for_usable_mem_range(void)
 
 	pr_debug("Looking for usable-memory-range property... ");
 
-	prop = of_get_flat_dt_prop(node, "linux,usable-memory-range", &len);
-	if (!prop || (len % (dt_root_addr_cells + dt_root_size_cells)))
+	prop = of_fdt_get_addr_size_prop(node, "linux,usable-memory-range", &len);
+	if (!prop)
 		return;
 
-	endp = prop + (len / sizeof(__be32));
-	for (i = 0; i < MAX_USABLE_RANGES && prop < endp; i++) {
-		rgn[i].base = dt_mem_next_cell(dt_root_addr_cells, &prop);
-		rgn[i].size = dt_mem_next_cell(dt_root_size_cells, &prop);
+	len = min(len, MAX_USABLE_RANGES);
+
+	for (i = 0; i < len; i++) {
+		of_fdt_read_addr_size(prop, &rgn[i].base, &rgn[i].size);
 
 		pr_debug("cap_mem_regions[%d]: base=%pa, size=%pa\n",
 			 i, &rgn[i].base, &rgn[i].size);
-- 
2.51.0

[PATCH v2 4/7] of/fdt: Fix incorrect use of dt_root_addr_cells in early_init_dt_check_kho()

[Yuntao Wang]

When reading the fdt_size value, the argument passed to dt_mem_next_cell()
is dt_root_addr_cells, but it should be dt_root_size_cells.

The same issue occurs when reading the scratch_size value.

Use a helper function to simplify the code and fix these issues.

Fixes: 274cdcb1c004 ("arm64: add KHO support")
Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
---
 drivers/of/fdt.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index d0caaab42aa7..4c45a97d6652 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -918,26 +918,16 @@ static void __init early_init_dt_check_kho(void)
 {
 	unsigned long node = chosen_node_offset;
 	u64 fdt_start, fdt_size, scratch_start, scratch_size;
-	const __be32 *p;
-	int l;
 
 	if (!IS_ENABLED(CONFIG_KEXEC_HANDOVER) || (long)node < 0)
 		return;
 
-	p = of_get_flat_dt_prop(node, "linux,kho-fdt", &l);
-	if (l != (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32))
+	if (!of_fdt_get_addr_size(node, "linux,kho-fdt", &fdt_start, &fdt_size))
 		return;
 
-	fdt_start = dt_mem_next_cell(dt_root_addr_cells, &p);
-	fdt_size = dt_mem_next_cell(dt_root_addr_cells, &p);
-
-	p = of_get_flat_dt_prop(node, "linux,kho-scratch", &l);
-	if (l != (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32))
+	if (!of_fdt_get_addr_size(node, "linux,kho-scratch", &scratch_start, &scratch_size))
 		return;
 
-	scratch_start = dt_mem_next_cell(dt_root_addr_cells, &p);
-	scratch_size = dt_mem_next_cell(dt_root_addr_cells, &p);
-
 	kho_populate(fdt_start, fdt_size, scratch_start, scratch_size);
 }
 
-- 
2.51.0

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

[Yuntao Wang]

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

[PATCH v2 6/7] of/reserved_mem: Simplify the logic of __reserved_mem_reserve_reg()

[Yuntao Wang]

Use the existing helper functions to simplify the logic of
__reserved_mem_reserve_reg()

Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
---
 drivers/of/of_reserved_mem.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 2e9ea751ed2d..b8527f3e335e 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -154,17 +154,16 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
 static int __init __reserved_mem_reserve_reg(unsigned long node,
 					     const char *uname)
 {
-	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
 	phys_addr_t base, size;
 	int len;
 	const __be32 *prop;
 	bool nomap;
 
-	prop = of_get_flat_dt_prop(node, "reg", &len);
-	if (!prop)
+	prop = of_fdt_get_addr_size_prop(node, "reg", &len);
+	if (!len)
 		return -ENOENT;
 
-	if (len && len % t_len != 0) {
+	if (len < 0) {
 		pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
 		       uname);
 		return -EINVAL;
@@ -172,9 +171,8 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
 
 	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
 
-	while (len >= t_len) {
-		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
-		size = dt_mem_next_cell(dt_root_size_cells, &prop);
+	while (len-- > 0) {
+		of_fdt_read_addr_size(prop, &base, &size);
 
 		if (size && early_init_dt_reserve_memory(base, size, nomap) == 0) {
 			/* Architecture specific contiguous memory fixup. */
@@ -187,8 +185,6 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
 			pr_err("Reserved memory: failed to reserve memory for node '%s': base %pa, size %lu MiB\n",
 			       uname, &base, (unsigned long)(size / SZ_1M));
 		}
-
-		len -= t_len;
 	}
 	return 0;
 }
-- 
2.51.0

[PATCH v2 7/7] of/reserved_mem: Simplify the logic of fdt_scan_reserved_mem_reg_nodes()

[Yuntao Wang]

Use the existing helper functions to simplify the logic of
fdt_scan_reserved_mem_reg_nodes()

Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
---
 drivers/of/of_reserved_mem.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index b8527f3e335e..96771ab073c0 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -226,12 +226,9 @@ static void __init __rmem_check_for_overlap(void);
  */
 void __init fdt_scan_reserved_mem_reg_nodes(void)
 {
-	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
 	const void *fdt = initial_boot_params;
 	phys_addr_t base, size;
-	const __be32 *prop;
 	int node, child;
-	int len;
 
 	if (!fdt)
 		return;
@@ -253,28 +250,16 @@ void __init fdt_scan_reserved_mem_reg_nodes(void)
 	fdt_for_each_subnode(child, fdt, node) {
 		const char *uname;
 
-		prop = of_get_flat_dt_prop(child, "reg", &len);
-		if (!prop)
-			continue;
 		if (!of_fdt_device_is_available(fdt, child))
 			continue;
 
-		uname = fdt_get_name(fdt, child, NULL);
-		if (len && len % t_len != 0) {
-			pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
-			       uname);
+		if (!of_fdt_get_addr_size(child, "reg", &base, &size))
 			continue;
-		}
-
-		if (len > t_len)
-			pr_warn("%s() ignores %d regions in node '%s'\n",
-				__func__, len / t_len - 1, uname);
 
-		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
-		size = dt_mem_next_cell(dt_root_size_cells, &prop);
-
-		if (size)
+		if (size) {
+			uname = fdt_get_name(fdt, child, NULL);
 			fdt_reserved_mem_save_node(child, uname, base, size);
+		}
 	}
 
 	/* check for overlapping reserved regions */
-- 
2.51.0

Re: [PATCH v2 3/7] of/fdt: Fix the len check in early_init_dt_check_for_usable_mem_range()

[kernel test robot]

Hi Yuntao,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.18-rc5 next-20251113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yuntao-Wang/of-fdt-Consolidate-duplicate-code-into-helper-functions/20251114-004121
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20251113155104.226617-4-yuntao.wang%40linux.dev
patch subject: [PATCH v2 3/7] of/fdt: Fix the len check in early_init_dt_check_for_usable_mem_range()
config: arm-allnoconfig (https://download.01.org/0day-ci/archive/20251114/202511140236.zLyckeBA-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 0bba1e76581bad04e7d7f09f5115ae5e2989e0d9)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251114/202511140236.zLyckeBA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511140236.zLyckeBA-lkp@intel.com/

All errors (new ones prefixed by >>):

>> 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]
     903 |                 of_fdt_read_addr_size(prop, &rgn[i].base, &rgn[i].size);
         |                                             ^~~~~~~~~~~~
   drivers/of/fdt.c:663:60: note: passing argument to parameter 'addr' here
     663 | void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
         |                                                            ^
   drivers/of/fdt.c:903:45: error: incompatible pointer types passing 'phys_addr_t *' (aka 'unsigned int *') to parameter of type 'u64 *' (aka 'unsigned long long *') [-Wincompatible-pointer-types]
     903 |                 of_fdt_read_addr_size(prop, &rgn[i].base, &rgn[i].size);
         |                                                           ^~~~~~~~~~~~
   drivers/of/fdt.c:663:71: note: passing argument to parameter 'size' here
     663 | void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
         |                                                                       ^
   2 errors generated.


vim +903 drivers/of/fdt.c

   879	
   880	/**
   881	 * early_init_dt_check_for_usable_mem_range - Decode usable memory range
   882	 * location from flat tree
   883	 */
   884	void __init early_init_dt_check_for_usable_mem_range(void)
   885	{
   886		struct memblock_region rgn[MAX_USABLE_RANGES] = {0};
   887		const __be32 *prop;
   888		int len, i;
   889		unsigned long node = chosen_node_offset;
   890	
   891		if ((long)node < 0)
   892			return;
   893	
   894		pr_debug("Looking for usable-memory-range property... ");
   895	
   896		prop = of_fdt_get_addr_size_prop(node, "linux,usable-memory-range", &len);
   897		if (!prop)
   898			return;
   899	
   900		len = min(len, MAX_USABLE_RANGES);
   901	
   902		for (i = 0; i < len; i++) {
 > 903			of_fdt_read_addr_size(prop, &rgn[i].base, &rgn[i].size);
   904	
   905			pr_debug("cap_mem_regions[%d]: base=%pa, size=%pa\n",
   906				 i, &rgn[i].base, &rgn[i].size);
   907		}
   908	
   909		memblock_cap_memory_range(rgn[0].base, rgn[0].size);
   910		for (i = 1; i < MAX_USABLE_RANGES && rgn[i].size; i++)
   911			memblock_add(rgn[i].base, rgn[i].size);
   912	}
   913	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 3/7] of/fdt: Fix the len check in early_init_dt_check_for_usable_mem_range()

[kernel test robot]

Hi Yuntao,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.18-rc5 next-20251113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yuntao-Wang/of-fdt-Consolidate-duplicate-code-into-helper-functions/20251114-004121
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20251113155104.226617-4-yuntao.wang%40linux.dev
patch subject: [PATCH v2 3/7] of/fdt: Fix the len check in early_init_dt_check_for_usable_mem_range()
config: arc-allnoconfig (https://download.01.org/0day-ci/archive/20251114/202511140206.XDGIw3J1-lkp@intel.com/config)
compiler: arc-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251114/202511140206.XDGIw3J1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511140206.XDGIw3J1-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/of/fdt.c: In function 'early_init_dt_check_for_usable_mem_range':
>> drivers/of/fdt.c:903:45: error: passing argument 2 of 'of_fdt_read_addr_size' from incompatible pointer type [-Wincompatible-pointer-types]
     903 |                 of_fdt_read_addr_size(prop, &rgn[i].base, &rgn[i].size);
         |                                             ^~~~~~~~~~~~
         |                                             |
         |                                             phys_addr_t * {aka unsigned int *}
   drivers/of/fdt.c:663:60: note: expected 'u64 *' {aka 'long long unsigned int *'} but argument is of type 'phys_addr_t *' {aka 'unsigned int *'}
     663 | void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
         |                                                       ~~~~~^~~~
   drivers/of/fdt.c:903:59: error: passing argument 3 of 'of_fdt_read_addr_size' from incompatible pointer type [-Wincompatible-pointer-types]
     903 |                 of_fdt_read_addr_size(prop, &rgn[i].base, &rgn[i].size);
         |                                                           ^~~~~~~~~~~~
         |                                                           |
         |                                                           phys_addr_t * {aka unsigned int *}
   drivers/of/fdt.c:663:71: note: expected 'u64 *' {aka 'long long unsigned int *'} but argument is of type 'phys_addr_t *' {aka 'unsigned int *'}
     663 | void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
         |                                                                  ~~~~~^~~~


vim +/of_fdt_read_addr_size +903 drivers/of/fdt.c

   879	
   880	/**
   881	 * early_init_dt_check_for_usable_mem_range - Decode usable memory range
   882	 * location from flat tree
   883	 */
   884	void __init early_init_dt_check_for_usable_mem_range(void)
   885	{
   886		struct memblock_region rgn[MAX_USABLE_RANGES] = {0};
   887		const __be32 *prop;
   888		int len, i;
   889		unsigned long node = chosen_node_offset;
   890	
   891		if ((long)node < 0)
   892			return;
   893	
   894		pr_debug("Looking for usable-memory-range property... ");
   895	
   896		prop = of_fdt_get_addr_size_prop(node, "linux,usable-memory-range", &len);
   897		if (!prop)
   898			return;
   899	
   900		len = min(len, MAX_USABLE_RANGES);
   901	
   902		for (i = 0; i < len; i++) {
 > 903			of_fdt_read_addr_size(prop, &rgn[i].base, &rgn[i].size);
   904	
   905			pr_debug("cap_mem_regions[%d]: base=%pa, size=%pa\n",
   906				 i, &rgn[i].base, &rgn[i].size);
   907		}
   908	
   909		memblock_cap_memory_range(rgn[0].base, rgn[0].size);
   910		for (i = 1; i < MAX_USABLE_RANGES && rgn[i].size; i++)
   911			memblock_add(rgn[i].base, rgn[i].size);
   912	}
   913	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 6/7] of/reserved_mem: Simplify the logic of __reserved_mem_reserve_reg()

[kernel test robot]

Hi Yuntao,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.18-rc5 next-20251113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yuntao-Wang/of-fdt-Consolidate-duplicate-code-into-helper-functions/20251114-004121
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20251113155104.226617-7-yuntao.wang%40linux.dev
patch subject: [PATCH v2 6/7] of/reserved_mem: Simplify the logic of __reserved_mem_reserve_reg()
config: arm-allnoconfig (https://download.01.org/0day-ci/archive/20251114/202511140307.Th0UqUd9-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 0bba1e76581bad04e7d7f09f5115ae5e2989e0d9)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251114/202511140307.Th0UqUd9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511140307.Th0UqUd9-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/of/of_reserved_mem.c:175:31: error: incompatible pointer types passing 'phys_addr_t *' (aka 'unsigned int *') to parameter of type 'u64 *' (aka 'unsigned long long *') [-Wincompatible-pointer-types]
     175 |                 of_fdt_read_addr_size(prop, &base, &size);
         |                                             ^~~~~
   include/linux/of_fdt.h:62:60: note: passing argument to parameter 'addr' here
      62 | extern void of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size);
         |                                                            ^
   drivers/of/of_reserved_mem.c:175:38: error: incompatible pointer types passing 'phys_addr_t *' (aka 'unsigned int *') to parameter of type 'u64 *' (aka 'unsigned long long *') [-Wincompatible-pointer-types]
     175 |                 of_fdt_read_addr_size(prop, &base, &size);
         |                                                    ^~~~~
   include/linux/of_fdt.h:62:71: note: passing argument to parameter 'size' here
      62 | extern void of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size);
         |                                                                       ^
   2 errors generated.


vim +175 drivers/of/of_reserved_mem.c

   150	
   151	/*
   152	 * __reserved_mem_reserve_reg() - reserve all memory described in 'reg' property
   153	 */
   154	static int __init __reserved_mem_reserve_reg(unsigned long node,
   155						     const char *uname)
   156	{
   157		phys_addr_t base, size;
   158		int len;
   159		const __be32 *prop;
   160		bool nomap;
   161	
   162		prop = of_fdt_get_addr_size_prop(node, "reg", &len);
   163		if (!len)
   164			return -ENOENT;
   165	
   166		if (len < 0) {
   167			pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
   168			       uname);
   169			return -EINVAL;
   170		}
   171	
   172		nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
   173	
   174		while (len-- > 0) {
 > 175			of_fdt_read_addr_size(prop, &base, &size);
   176	
   177			if (size && early_init_dt_reserve_memory(base, size, nomap) == 0) {
   178				/* Architecture specific contiguous memory fixup. */
   179				if (of_flat_dt_is_compatible(node, "shared-dma-pool") &&
   180				    of_get_flat_dt_prop(node, "reusable", NULL))
   181					dma_contiguous_early_fixup(base, size);
   182				pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
   183					uname, &base, (unsigned long)(size / SZ_1M));
   184			} else {
   185				pr_err("Reserved memory: failed to reserve memory for node '%s': base %pa, size %lu MiB\n",
   186				       uname, &base, (unsigned long)(size / SZ_1M));
   187			}
   188		}
   189		return 0;
   190	}
   191	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Rob Herring]

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

Re: [PATCH v2 1/7] of/fdt: Consolidate duplicate code into helper functions

[Rob Herring]

On Thu, Nov 13, 2025 at 11:50:58PM +0800, Yuntao Wang wrote:
> Currently, there are many pieces of nearly identical code scattered across
> different places. Consolidate the duplicate code into helper functions to
> improve maintainability and reduce the likelihood of errors.
> 
> Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
> ---
>  drivers/of/fdt.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_fdt.h |  5 +++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 0edd639898a6..5e0eabc1449f 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -625,6 +625,47 @@ const void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
>  	return fdt_getprop(initial_boot_params, node, name, size);
>  }
>  
> +const __be32 *__init of_fdt_get_addr_size_prop(unsigned long node,
> +                                               const char *name, int *entries)
> +{
> +	const __be32 *prop;
> +	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> +
> +	prop = of_get_flat_dt_prop(node, name, &len);
> +	if (!prop) {
> +		*entries = 0;
> +		return NULL;
> +	}
> +
> +	if (len % elen) {
> +		*entries = -1;

I don't think it's really important to distinguish a length error from 
any other error. Either we can read the property or we can't.

> +		return NULL;
> +	}
> +
> +	*entries = len / elen;
> +	return prop;
> +}
> +
> +bool __init of_fdt_get_addr_size(unsigned long node, const char *name,
> +                                 u64 *addr, u64 *size)
> +{
> +	const __be32 *prop;
> +	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);

Still have 2 locations to get the same calculation wrong...

> +
> +	prop = of_get_flat_dt_prop(node, name, &len);
> +	if (!prop || len < elen)
> +		return false;

Why doesn't calling of_fdt_get_addr_size_prop() work here? If 'len < 
elen', then 'len % elen' will also be true except in the 0 length case. 
For that case, of_fdt_get_addr_size_prop() needs to handle it too.

> +
> +	of_fdt_read_addr_size(prop, addr, size);
> +	return true;
> +}
> +
> +void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
> +{
> +	*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +	*size = dt_mem_next_cell(dt_root_size_cells, &prop);
> +}
> +
>  /**
>   * of_fdt_is_compatible - Return true if given node from the given blob has
>   * compat in its compatible list
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index b8d6c0c20876..3a0805ff6c7b 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -55,6 +55,11 @@ extern int of_get_flat_dt_subnode_by_name(unsigned long node,
>  					  const char *uname);
>  extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
>  				       int *size);
> +extern const __be32 *of_fdt_get_addr_size_prop(unsigned long node,
> +                                               const char *name, int *entries);
> +extern bool of_fdt_get_addr_size(unsigned long node, const char *name,
> +                                 u64 *addr, u64 *size);
> +extern void of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size);
>  extern int of_flat_dt_is_compatible(unsigned long node, const char *name);

Looks like of_flat_dt_* would be more consistent with existing naming.

>  extern unsigned long of_get_flat_dt_root(void);
>  extern uint32_t of_get_flat_dt_phandle(unsigned long node);
> -- 
> 2.51.0
> 

Re: [PATCH v2 1/7] of/fdt: Consolidate duplicate code into helper functions

[Yuntao Wang]

On Thu, 13 Nov 2025 16:38:59 -0600, Rob Herring <robh@kernel.org> wrote:

> On Thu, Nov 13, 2025 at 11:50:58PM +0800, Yuntao Wang wrote:
> > Currently, there are many pieces of nearly identical code scattered across
> > different places. Consolidate the duplicate code into helper functions to
> > improve maintainability and reduce the likelihood of errors.
> > 
> > Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
> > ---
> >  drivers/of/fdt.c       | 41 +++++++++++++++++++++++++++++++++++++++++
> >  include/linux/of_fdt.h |  5 +++++
> >  2 files changed, 46 insertions(+)
> > 
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 0edd639898a6..5e0eabc1449f 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -625,6 +625,47 @@ const void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
> >  	return fdt_getprop(initial_boot_params, node, name, size);
> >  }
> >  
> > +const __be32 *__init of_fdt_get_addr_size_prop(unsigned long node,
> > +                                               const char *name, int *entries)
> > +{
> > +	const __be32 *prop;
> > +	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> > +
> > +	prop = of_get_flat_dt_prop(node, name, &len);
> > +	if (!prop) {
> > +		*entries = 0;
> > +		return NULL;
> > +	}
> > +
> > +	if (len % elen) {
> > +		*entries = -1;
> 
> I don't think it's really important to distinguish a length error from 
> any other error. Either we can read the property or we can't.

Hi Rob,

I didn't originally split it into two checks, but later I noticed that in
__reserved_mem_reserve_reg(), the two error conditions return different
error codes. I was concerned about breaking compatibility, so I made the
change this way.

If compatibility isn't an issue, I'd be happy to merge the two checks into one.

> 
> > +		return NULL;
> > +	}
> > +
> > +	*entries = len / elen;
> > +	return prop;
> > +}
> > +
> > +bool __init of_fdt_get_addr_size(unsigned long node, const char *name,
> > +                                 u64 *addr, u64 *size)
> > +{
> > +	const __be32 *prop;
> > +	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> 
> Still have 2 locations to get the same calculation wrong...
> 
> > +
> > +	prop = of_get_flat_dt_prop(node, name, &len);
> > +	if (!prop || len < elen)
> > +		return false;
> 
> Why doesn't calling of_fdt_get_addr_size_prop() work here? If 'len < 
> elen', then 'len % elen' will also be true except in the 0 length case. 
> For that case, of_fdt_get_addr_size_prop() needs to handle it too.

I'm fully in favor of calling of_fdt_get_addr_size_prop() directly here,
that was my original intention as well, which is also why I placed this
function right after of_fdt_get_addr_size_prop().

But again, due to compatibility concerns, I had to implement it this way.

For example, suppose prop points to data like:

[addr, size, other data]

With the previous `len < elen` check, addr and size could still be read
successfully. But if we switch to the `len % elen` check, this type of
data may fail.

If compatibility is not a concern, I can certainly change it to something
like the following:

prop = of_fdt_get_addr_size_prop(node, name, &len);
if (!prop || len != 1)
  return false;

> 
> > +
> > +	of_fdt_read_addr_size(prop, addr, size);
> > +	return true;
> > +}
> > +
> > +void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
> > +{
> > +	*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> > +	*size = dt_mem_next_cell(dt_root_size_cells, &prop);
> > +}
> > +
> >  /**
> >   * of_fdt_is_compatible - Return true if given node from the given blob has
> >   * compat in its compatible list
> > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > index b8d6c0c20876..3a0805ff6c7b 100644
> > --- a/include/linux/of_fdt.h
> > +++ b/include/linux/of_fdt.h
> > @@ -55,6 +55,11 @@ extern int of_get_flat_dt_subnode_by_name(unsigned long node,
> >  					  const char *uname);
> >  extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
> >  				       int *size);
> > +extern const __be32 *of_fdt_get_addr_size_prop(unsigned long node,
> > +                                               const char *name, int *entries);
> > +extern bool of_fdt_get_addr_size(unsigned long node, const char *name,
> > +                                 u64 *addr, u64 *size);
> > +extern void of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size);
> >  extern int of_flat_dt_is_compatible(unsigned long node, const char *name);
> 
> Looks like of_flat_dt_* would be more consistent with existing naming.

Naming is hard :-)

I spent quite a while thinking about the names of these functions.

In drivers/of/fdt.c and include/linux/of_fdt.h, there are several naming
styles in use, such as of_fdt_, of_flat_dt_, and others.

I chose of_fdt_ as the prefix, or namespace, for these functions mainly
because:

1. Compared to of_flat_dt_, it's simpler and shorter, and fdt can represent
   flat_dt, or flattened device tree.

2. of_fdt_ matches the file names drivers/of/fdt.c and include/linux/of_fdt.h better.

3. In the libfdt library, functions consistently use the fdt_ prefix, so using
   a similar of_fdt_ prefix in of/fdt.c seems reasonable.

But if you prefer the of_flat_dt_ nameing convention, I can change it.

> 
> >  extern unsigned long of_get_flat_dt_root(void);
> >  extern uint32_t of_get_flat_dt_phandle(unsigned long node);
> > -- 
> > 2.51.0
> > 

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

[Yuntao Wang]

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

Re: [PATCH v2 0/7] of/fdt: Some bug fixes and cleanups

[Krzysztof Kozlowski]

On Thu, Nov 13, 2025 at 11:50:57PM +0800, Yuntao Wang wrote:
> This patch series fixes several bugs related to dt_root_addr_cells and
> dt_root_size_cells, and performs some cleanup.
> 
> Changelog:
> 
> v1: Consolidate duplicate code into helper functions

Your patchset has multiple checkpatch warnings and errors. In multiple
patches.

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

Please run standard kernel tools for static analysis, like coccinelle,
smatch and sparse, and fix reported warnings. Also please check for
warnings when building with W=1 for gcc and clang. Most of these
commands (checks or W=1 build) can build specific targets, like some
directory, to narrow the scope to only your code. The code here looks
like it needs a fix. Feel free to get in touch if the warning is not
clear.

Best regards,
Krzysztof

Re: [PATCH v2 1/7] of/fdt: Consolidate duplicate code into helper functions

[Rob Herring]

On Thu, Nov 13, 2025 at 9:10 PM Yuntao Wang <yuntao.wang@linux.dev> wrote:
>
> On Thu, 13 Nov 2025 16:38:59 -0600, Rob Herring <robh@kernel.org> wrote:
>
> > On Thu, Nov 13, 2025 at 11:50:58PM +0800, Yuntao Wang wrote:
> > > Currently, there are many pieces of nearly identical code scattered across
> > > different places. Consolidate the duplicate code into helper functions to
> > > improve maintainability and reduce the likelihood of errors.
> > >
> > > Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
> > > ---
> > >  drivers/of/fdt.c       | 41 +++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/of_fdt.h |  5 +++++
> > >  2 files changed, 46 insertions(+)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 0edd639898a6..5e0eabc1449f 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -625,6 +625,47 @@ const void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
> > >     return fdt_getprop(initial_boot_params, node, name, size);
> > >  }
> > >
> > > +const __be32 *__init of_fdt_get_addr_size_prop(unsigned long node,
> > > +                                               const char *name, int *entries)
> > > +{
> > > +   const __be32 *prop;
> > > +   int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> > > +
> > > +   prop = of_get_flat_dt_prop(node, name, &len);
> > > +   if (!prop) {
> > > +           *entries = 0;
> > > +           return NULL;
> > > +   }
> > > +
> > > +   if (len % elen) {
> > > +           *entries = -1;
> >
> > I don't think it's really important to distinguish a length error from
> > any other error. Either we can read the property or we can't.
>
> Hi Rob,
>
> I didn't originally split it into two checks, but later I noticed that in
> __reserved_mem_reserve_reg(), the two error conditions return different
> error codes. I was concerned about breaking compatibility, so I made the
> change this way.
>
> If compatibility isn't an issue, I'd be happy to merge the two checks into one.

You'll have to adjust the handling of -ENOENT case, but yes I think
that is fine. IMO, the kernel can either read and parse a property or
it can't. The exact reason it can't is generally not important.

> > > +           return NULL;
> > > +   }
> > > +
> > > +   *entries = len / elen;
> > > +   return prop;
> > > +}
> > > +
> > > +bool __init of_fdt_get_addr_size(unsigned long node, const char *name,
> > > +                                 u64 *addr, u64 *size)
> > > +{
> > > +   const __be32 *prop;
> > > +   int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> >
> > Still have 2 locations to get the same calculation wrong...
> >
> > > +
> > > +   prop = of_get_flat_dt_prop(node, name, &len);
> > > +   if (!prop || len < elen)
> > > +           return false;
> >
> > Why doesn't calling of_fdt_get_addr_size_prop() work here? If 'len <
> > elen', then 'len % elen' will also be true except in the 0 length case.
> > For that case, of_fdt_get_addr_size_prop() needs to handle it too.
>
> I'm fully in favor of calling of_fdt_get_addr_size_prop() directly here,
> that was my original intention as well, which is also why I placed this
> function right after of_fdt_get_addr_size_prop().
>
> But again, due to compatibility concerns, I had to implement it this way.
>
> For example, suppose prop points to data like:
>
> [addr, size, other data]
>
> With the previous `len < elen` check, addr and size could still be read
> successfully. But if we switch to the `len % elen` check, this type of
> data may fail.

Only if 'other data' is not a multiple of [addr,size], but that's
completely invalid*.

(*The dts format does allow something as complex as '<0x12345678>,
"string", /bits 8/ <0xab>', but you would have to be completely insane
to do that when there's no type information in the DTB.)

> If compatibility is not a concern, I can certainly change it to something
> like the following:
>
> prop = of_fdt_get_addr_size_prop(node, name, &len);
> if (!prop || len != 1)
>   return false;
> >
> > > +
> > > +   of_fdt_read_addr_size(prop, addr, size);
> > > +   return true;
> > > +}
> > > +
> > > +void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
> > > +{
> > > +   *addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> > > +   *size = dt_mem_next_cell(dt_root_size_cells, &prop);
> > > +}
> > > +
> > >  /**
> > >   * of_fdt_is_compatible - Return true if given node from the given blob has
> > >   * compat in its compatible list
> > > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > > index b8d6c0c20876..3a0805ff6c7b 100644
> > > --- a/include/linux/of_fdt.h
> > > +++ b/include/linux/of_fdt.h
> > > @@ -55,6 +55,11 @@ extern int of_get_flat_dt_subnode_by_name(unsigned long node,
> > >                                       const char *uname);
> > >  extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
> > >                                    int *size);
> > > +extern const __be32 *of_fdt_get_addr_size_prop(unsigned long node,
> > > +                                               const char *name, int *entries);
> > > +extern bool of_fdt_get_addr_size(unsigned long node, const char *name,
> > > +                                 u64 *addr, u64 *size);
> > > +extern void of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size);
> > >  extern int of_flat_dt_is_compatible(unsigned long node, const char *name);
> >
> > Looks like of_flat_dt_* would be more consistent with existing naming.
>
> Naming is hard :-)

Indeed.

> I spent quite a while thinking about the names of these functions.
>
> In drivers/of/fdt.c and include/linux/of_fdt.h, there are several naming
> styles in use, such as of_fdt_, of_flat_dt_, and others.

It's a bit of a mess...

> I chose of_fdt_ as the prefix, or namespace, for these functions mainly
> because:
>
> 1. Compared to of_flat_dt_, it's simpler and shorter, and fdt can represent
>    flat_dt, or flattened device tree.
>
> 2. of_fdt_ matches the file names drivers/of/fdt.c and include/linux/of_fdt.h better.
>
> 3. In the libfdt library, functions consistently use the fdt_ prefix, so using
>    a similar of_fdt_ prefix in of/fdt.c seems reasonable.

If we started fresh, I would agree with all of this.

> But if you prefer the of_flat_dt_ nameing convention, I can change it.

I do primarily because that aligns with the other functions which read
specific properties (e.g. of_flat_dt_is_compatible(),
of_flat_dt_translate_address()).

Rob

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

[Rob Herring]

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

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

[Yuntao Wang]

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

Re: [PATCH v2 0/7] of/fdt: Some bug fixes and cleanups

[Yuntao Wang]

On Fri, 14 Nov 2025 09:04:30 +0100, Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On Thu, Nov 13, 2025 at 11:50:57PM +0800, Yuntao Wang wrote:
> > This patch series fixes several bugs related to dt_root_addr_cells and
> > dt_root_size_cells, and performs some cleanup.
> >
> > Changelog:
> >
> > v1: Consolidate duplicate code into helper functions
>
> Your patchset has multiple checkpatch warnings and errors. In multiple
> patches.
>
> Please run scripts/checkpatch.pl on the patches and fix reported
> warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
> patches and (probably) fix more warnings. Some warnings can be ignored,
> especially from --strict run, but the code here looks like it needs a
> fix. Feel free to get in touch if the warning is not clear.
>
> Please run standard kernel tools for static analysis, like coccinelle,
> smatch and sparse, and fix reported warnings. Also please check for
> warnings when building with W=1 for gcc and clang. Most of these
> commands (checks or W=1 build) can build specific targets, like some
> directory, to narrow the scope to only your code. The code here looks
> like it needs a fix. Feel free to get in touch if the warning is not
> clear.
>
> Best regards,
> Krzysztof

Hi Krzysztof,

Many thanks for pointing out these issues, they have been fixed in the new patch series:

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

Thanks,
Yuntao

Re: [PATCH v2 0/7] of/fdt: Some bug fixes and cleanups

[Krzysztof Kozlowski]

On 15/11/2025 15:07, Yuntao Wang wrote:
> On Fri, 14 Nov 2025 09:04:30 +0100, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
>> On Thu, Nov 13, 2025 at 11:50:57PM +0800, Yuntao Wang wrote:
>>> This patch series fixes several bugs related to dt_root_addr_cells and
>>> dt_root_size_cells, and performs some cleanup.
>>>
>>> Changelog:
>>>
>>> v1: Consolidate duplicate code into helper functions
>>
>> Your patchset has multiple checkpatch warnings and errors. In multiple
>> patches.
>>
>> Please run scripts/checkpatch.pl on the patches and fix reported
>> warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
>> patches and (probably) fix more warnings. Some warnings can be ignored,
>> especially from --strict run, but the code here looks like it needs a
>> fix. Feel free to get in touch if the warning is not clear.
>>
>> Please run standard kernel tools for static analysis, like coccinelle,
>> smatch and sparse, and fix reported warnings. Also please check for
>> warnings when building with W=1 for gcc and clang. Most of these
>> commands (checks or W=1 build) can build specific targets, like some
>> directory, to narrow the scope to only your code. The code here looks
>> like it needs a fix. Feel free to get in touch if the warning is not
>> clear.
>>
>> Best regards,
>> Krzysztof
> 
> Hi Krzysztof,
> 
> Many thanks for pointing out these issues, they have been fixed in the new patch series:
> 
> https://lore.kernel.org/linux-devicetree/20251115134753.179931-1-yuntao.wang@linux.dev/

Not really. I don't think you run checkpatch.

Best regards,
Krzysztof

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

[Geert Uytterhoeven]

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