[PATCH v2] of/fdt: Rework early_init_dt_scan_memory() to call directly

[PATCH v2] of/fdt: Rework early_init_dt_scan_memory() to call directly

[Rob Herring]

Use of the of_scan_flat_dt() function predates libfdt and is discouraged
as libfdt provides a nicer set of APIs. Rework
early_init_dt_scan_memory() to be called directly and use libfdt.

Cc: John Crispin <john@phrozen.org>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: linux-mips@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
v2:
 - ralink: Use 'if' instead of 'else if'
 - early_init_dt_scan_memory: continue instead of return on no reg
 - Fix indentation
---
 arch/mips/ralink/of.c      | 19 +++--------
 arch/powerpc/kernel/prom.c | 16 ++++-----
 drivers/of/fdt.c           | 68 ++++++++++++++++++++------------------
 include/linux/of_fdt.h     |  3 +-
 4 files changed, 49 insertions(+), 57 deletions(-)

diff --git a/arch/mips/ralink/of.c b/arch/mips/ralink/of.c
index 0135376c5de5..35a87a2da10b 100644
--- a/arch/mips/ralink/of.c
+++ b/arch/mips/ralink/of.c
@@ -53,17 +53,6 @@ void __init device_tree_init(void)
 	unflatten_and_copy_device_tree();
 }
 
-static int memory_dtb;
-
-static int __init early_init_dt_find_memory(unsigned long node,
-				const char *uname, int depth, void *data)
-{
-	if (depth == 1 && !strcmp(uname, "memory@0"))
-		memory_dtb = 1;
-
-	return 0;
-}
-
 void __init plat_mem_setup(void)
 {
 	void *dtb;
@@ -77,10 +66,10 @@ void __init plat_mem_setup(void)
 	dtb = get_fdt();
 	__dt_setup_arch(dtb);
 
-	of_scan_flat_dt(early_init_dt_find_memory, NULL);
-	if (memory_dtb)
-		of_scan_flat_dt(early_init_dt_scan_memory, NULL);
-	else if (soc_info.mem_detect)
+	if (!early_init_dt_scan_memory())
+		return;
+
+	if (soc_info.mem_detect)
 		soc_info.mem_detect();
 	else if (soc_info.mem_size)
 		memblock_add(soc_info.mem_base, soc_info.mem_size * SZ_1M);
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 6e1a106f02eb..63762a3b75e8 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -532,19 +532,19 @@ static int  __init early_init_drmem_lmb(struct drmem_lmb *lmb,
 }
 #endif /* CONFIG_PPC_PSERIES */
 
-static int __init early_init_dt_scan_memory_ppc(unsigned long node,
-						const char *uname,
-						int depth, void *data)
+static int __init early_init_dt_scan_memory_ppc(void)
 {
 #ifdef CONFIG_PPC_PSERIES
-	if (depth == 1 &&
-	    strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
+	const void *fdt = initial_boot_params;
+	int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
+
+	if (node > 0) {
 		walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
 		return 0;
 	}
 #endif
 	
-	return early_init_dt_scan_memory(node, uname, depth, data);
+	return early_init_dt_scan_memory();
 }
 
 /*
@@ -749,7 +749,7 @@ void __init early_init_devtree(void *params)
 
 	/* Scan memory nodes and rebuild MEMBLOCKs */
 	early_init_dt_scan_root();
-	of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
+	early_init_dt_scan_memory_ppc();
 
 	parse_early_param();
 
@@ -858,7 +858,7 @@ void __init early_get_first_memblock_info(void *params, phys_addr_t *size)
 	 */
 	add_mem_to_memblock = 0;
 	early_init_dt_scan_root();
-	of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
+	early_init_dt_scan_memory_ppc();
 	add_mem_to_memblock = 1;
 
 	if (size)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 5e216555fe4f..a835c458f50a 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1078,49 +1078,53 @@ u64 __init dt_mem_next_cell(int s, const __be32 **cellp)
 /*
  * early_init_dt_scan_memory - Look for and parse memory nodes
  */
-int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
-				     int depth, void *data)
+int __init early_init_dt_scan_memory(void)
 {
-	const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
-	const __be32 *reg, *endp;
-	int l;
-	bool hotpluggable;
+	int node;
+	const void *fdt = initial_boot_params;
 
-	/* We are scanning "memory" nodes only */
-	if (type == NULL || strcmp(type, "memory") != 0)
-		return 0;
+	fdt_for_each_subnode(node, fdt, 0) {
+		const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
+		const __be32 *reg, *endp;
+		int l;
+		bool hotpluggable;
 
-	reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
-	if (reg == NULL)
-		reg = of_get_flat_dt_prop(node, "reg", &l);
-	if (reg == NULL)
-		return 0;
+		/* We are scanning "memory" nodes only */
+		if (type == NULL || strcmp(type, "memory") != 0)
+			continue;
 
-	endp = reg + (l / sizeof(__be32));
-	hotpluggable = of_get_flat_dt_prop(node, "hotpluggable", NULL);
+		reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
+		if (reg == NULL)
+			reg = of_get_flat_dt_prop(node, "reg", &l);
+		if (reg == NULL)
+			continue;
 
-	pr_debug("memory scan node %s, reg size %d,\n", uname, l);
+		endp = reg + (l / sizeof(__be32));
+		hotpluggable = of_get_flat_dt_prop(node, "hotpluggable", NULL);
 
-	while ((endp - reg) >= (dt_root_addr_cells + dt_root_size_cells)) {
-		u64 base, size;
+		pr_debug("memory scan node %s, reg size %d,\n",
+			 fdt_get_name(fdt, node, NULL), l);
 
-		base = dt_mem_next_cell(dt_root_addr_cells, &reg);
-		size = dt_mem_next_cell(dt_root_size_cells, &reg);
+		while ((endp - reg) >= (dt_root_addr_cells + dt_root_size_cells)) {
+			u64 base, size;
 
-		if (size == 0)
-			continue;
-		pr_debug(" - %llx, %llx\n", base, size);
+			base = dt_mem_next_cell(dt_root_addr_cells, &reg);
+			size = dt_mem_next_cell(dt_root_size_cells, &reg);
 
-		early_init_dt_add_memory_arch(base, size);
+			if (size == 0)
+				continue;
+			pr_debug(" - %llx, %llx\n", base, size);
 
-		if (!hotpluggable)
-			continue;
+			early_init_dt_add_memory_arch(base, size);
 
-		if (memblock_mark_hotplug(base, size))
-			pr_warn("failed to mark hotplug range 0x%llx - 0x%llx\n",
-				base, base + size);
-	}
+			if (!hotpluggable)
+				continue;
 
+			if (memblock_mark_hotplug(base, size))
+				pr_warn("failed to mark hotplug range 0x%llx - 0x%llx\n",
+					base, base + size);
+		}
+	}
 	return 0;
 }
 
@@ -1271,7 +1275,7 @@ void __init early_init_dt_scan_nodes(void)
 		pr_warn("No chosen node found, continuing without\n");
 
 	/* Setup memory, calling early_init_dt_add_memory_arch */
-	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
+	early_init_dt_scan_memory();
 
 	/* Handle linux,usable-memory-range property */
 	memblock_cap_memory_range(cap_mem_addr, cap_mem_size);
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index df3d31926c3c..914739f3c192 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -59,8 +59,7 @@ extern unsigned long of_get_flat_dt_root(void);
 extern uint32_t of_get_flat_dt_phandle(unsigned long node);
 
 extern int early_init_dt_scan_chosen(char *cmdline);
-extern int early_init_dt_scan_memory(unsigned long node, const char *uname,
-				     int depth, void *data);
+extern int early_init_dt_scan_memory(void);
 extern int early_init_dt_scan_chosen_stdout(void);
 extern void early_init_fdt_scan_reserved_mem(void);
 extern void early_init_fdt_reserve_self(void);
-- 
2.32.0

Re: [PATCH v2] of/fdt: Rework early_init_dt_scan_memory() to call directly

[Frank Rowand]

On 12/8/21 10:58 AM, Rob Herring wrote:
> Use of the of_scan_flat_dt() function predates libfdt and is discouraged
> as libfdt provides a nicer set of APIs. Rework
> early_init_dt_scan_memory() to be called directly and use libfdt.
> 
> Cc: John Crispin <john@phrozen.org>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: linux-mips@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v2:
>  - ralink: Use 'if' instead of 'else if'
>  - early_init_dt_scan_memory: continue instead of return on no reg
>  - Fix indentation
> ---
>  arch/mips/ralink/of.c      | 19 +++--------
>  arch/powerpc/kernel/prom.c | 16 ++++-----
>  drivers/of/fdt.c           | 68 ++++++++++++++++++++------------------
>  include/linux/of_fdt.h     |  3 +-
>  4 files changed, 49 insertions(+), 57 deletions(-)

Reviewed-by: Frank Rowand <frank.rowand@sony.com>

-Frank

> 
> diff --git a/arch/mips/ralink/of.c b/arch/mips/ralink/of.c
> index 0135376c5de5..35a87a2da10b 100644
> --- a/arch/mips/ralink/of.c
> +++ b/arch/mips/ralink/of.c
> @@ -53,17 +53,6 @@ void __init device_tree_init(void)
>  	unflatten_and_copy_device_tree();
>  }
>  
> -static int memory_dtb;
> -
> -static int __init early_init_dt_find_memory(unsigned long node,
> -				const char *uname, int depth, void *data)
> -{
> -	if (depth == 1 && !strcmp(uname, "memory@0"))
> -		memory_dtb = 1;
> -
> -	return 0;
> -}
> -
>  void __init plat_mem_setup(void)
>  {
>  	void *dtb;
> @@ -77,10 +66,10 @@ void __init plat_mem_setup(void)
>  	dtb = get_fdt();
>  	__dt_setup_arch(dtb);
>  
> -	of_scan_flat_dt(early_init_dt_find_memory, NULL);
> -	if (memory_dtb)
> -		of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> -	else if (soc_info.mem_detect)
> +	if (!early_init_dt_scan_memory())
> +		return;
> +
> +	if (soc_info.mem_detect)
>  		soc_info.mem_detect();
>  	else if (soc_info.mem_size)
>  		memblock_add(soc_info.mem_base, soc_info.mem_size * SZ_1M);
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 6e1a106f02eb..63762a3b75e8 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -532,19 +532,19 @@ static int  __init early_init_drmem_lmb(struct drmem_lmb *lmb,
>  }
>  #endif /* CONFIG_PPC_PSERIES */
>  
> -static int __init early_init_dt_scan_memory_ppc(unsigned long node,
> -						const char *uname,
> -						int depth, void *data)
> +static int __init early_init_dt_scan_memory_ppc(void)
>  {
>  #ifdef CONFIG_PPC_PSERIES
> -	if (depth == 1 &&
> -	    strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
> +	const void *fdt = initial_boot_params;
> +	int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
> +
> +	if (node > 0) {
>  		walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
>  		return 0;
>  	}
>  #endif
>  	
> -	return early_init_dt_scan_memory(node, uname, depth, data);
> +	return early_init_dt_scan_memory();
>  }
>  
>  /*
> @@ -749,7 +749,7 @@ void __init early_init_devtree(void *params)
>  
>  	/* Scan memory nodes and rebuild MEMBLOCKs */
>  	early_init_dt_scan_root();
> -	of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
> +	early_init_dt_scan_memory_ppc();
>  
>  	parse_early_param();
>  
> @@ -858,7 +858,7 @@ void __init early_get_first_memblock_info(void *params, phys_addr_t *size)
>  	 */
>  	add_mem_to_memblock = 0;
>  	early_init_dt_scan_root();
> -	of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
> +	early_init_dt_scan_memory_ppc();
>  	add_mem_to_memblock = 1;
>  
>  	if (size)
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 5e216555fe4f..a835c458f50a 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1078,49 +1078,53 @@ u64 __init dt_mem_next_cell(int s, const __be32 **cellp)
>  /*
>   * early_init_dt_scan_memory - Look for and parse memory nodes
>   */
> -int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
> -				     int depth, void *data)
> +int __init early_init_dt_scan_memory(void)
>  {
> -	const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
> -	const __be32 *reg, *endp;
> -	int l;
> -	bool hotpluggable;
> +	int node;
> +	const void *fdt = initial_boot_params;
>  
> -	/* We are scanning "memory" nodes only */
> -	if (type == NULL || strcmp(type, "memory") != 0)
> -		return 0;
> +	fdt_for_each_subnode(node, fdt, 0) {
> +		const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
> +		const __be32 *reg, *endp;
> +		int l;
> +		bool hotpluggable;
>  
> -	reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
> -	if (reg == NULL)
> -		reg = of_get_flat_dt_prop(node, "reg", &l);
> -	if (reg == NULL)
> -		return 0;
> +		/* We are scanning "memory" nodes only */
> +		if (type == NULL || strcmp(type, "memory") != 0)
> +			continue;
>  
> -	endp = reg + (l / sizeof(__be32));
> -	hotpluggable = of_get_flat_dt_prop(node, "hotpluggable", NULL);
> +		reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
> +		if (reg == NULL)
> +			reg = of_get_flat_dt_prop(node, "reg", &l);
> +		if (reg == NULL)
> +			continue;
>  
> -	pr_debug("memory scan node %s, reg size %d,\n", uname, l);
> +		endp = reg + (l / sizeof(__be32));
> +		hotpluggable = of_get_flat_dt_prop(node, "hotpluggable", NULL);
>  
> -	while ((endp - reg) >= (dt_root_addr_cells + dt_root_size_cells)) {
> -		u64 base, size;
> +		pr_debug("memory scan node %s, reg size %d,\n",
> +			 fdt_get_name(fdt, node, NULL), l);
>  
> -		base = dt_mem_next_cell(dt_root_addr_cells, &reg);
> -		size = dt_mem_next_cell(dt_root_size_cells, &reg);
> +		while ((endp - reg) >= (dt_root_addr_cells + dt_root_size_cells)) {
> +			u64 base, size;
>  
> -		if (size == 0)
> -			continue;
> -		pr_debug(" - %llx, %llx\n", base, size);
> +			base = dt_mem_next_cell(dt_root_addr_cells, &reg);
> +			size = dt_mem_next_cell(dt_root_size_cells, &reg);
>  
> -		early_init_dt_add_memory_arch(base, size);
> +			if (size == 0)
> +				continue;
> +			pr_debug(" - %llx, %llx\n", base, size);
>  
> -		if (!hotpluggable)
> -			continue;
> +			early_init_dt_add_memory_arch(base, size);
>  
> -		if (memblock_mark_hotplug(base, size))
> -			pr_warn("failed to mark hotplug range 0x%llx - 0x%llx\n",
> -				base, base + size);
> -	}
> +			if (!hotpluggable)
> +				continue;
>  
> +			if (memblock_mark_hotplug(base, size))
> +				pr_warn("failed to mark hotplug range 0x%llx - 0x%llx\n",
> +					base, base + size);
> +		}
> +	}
>  	return 0;
>  }
>  
> @@ -1271,7 +1275,7 @@ void __init early_init_dt_scan_nodes(void)
>  		pr_warn("No chosen node found, continuing without\n");
>  
>  	/* Setup memory, calling early_init_dt_add_memory_arch */
> -	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> +	early_init_dt_scan_memory();
>  
>  	/* Handle linux,usable-memory-range property */
>  	memblock_cap_memory_range(cap_mem_addr, cap_mem_size);
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index df3d31926c3c..914739f3c192 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -59,8 +59,7 @@ extern unsigned long of_get_flat_dt_root(void);
>  extern uint32_t of_get_flat_dt_phandle(unsigned long node);
>  
>  extern int early_init_dt_scan_chosen(char *cmdline);
> -extern int early_init_dt_scan_memory(unsigned long node, const char *uname,
> -				     int depth, void *data);
> +extern int early_init_dt_scan_memory(void);
>  extern int early_init_dt_scan_chosen_stdout(void);
>  extern void early_init_fdt_scan_reserved_mem(void);
>  extern void early_init_fdt_reserve_self(void);
> 

Re: [PATCH v2] of/fdt: Rework early_init_dt_scan_memory() to call directly

[Michael Ellerman]

Rob Herring <robh@kernel.org> writes:
> Use of the of_scan_flat_dt() function predates libfdt and is discouraged
> as libfdt provides a nicer set of APIs. Rework
> early_init_dt_scan_memory() to be called directly and use libfdt.
...
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 6e1a106f02eb..63762a3b75e8 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -532,19 +532,19 @@ static int  __init early_init_drmem_lmb(struct drmem_lmb *lmb,
>  }
>  #endif /* CONFIG_PPC_PSERIES */
>  
> -static int __init early_init_dt_scan_memory_ppc(unsigned long node,
> -						const char *uname,
> -						int depth, void *data)
> +static int __init early_init_dt_scan_memory_ppc(void)
>  {
>  #ifdef CONFIG_PPC_PSERIES
> -	if (depth == 1 &&
> -	    strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
> +	const void *fdt = initial_boot_params;
> +	int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
> +
> +	if (node > 0) {
>  		walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
>  		return 0;
>  	}
>  #endif
>  	
> -	return early_init_dt_scan_memory(node, uname, depth, data);
> +	return early_init_dt_scan_memory();
>  }
>  
>  /*
> @@ -749,7 +749,7 @@ void __init early_init_devtree(void *params)
>  
>  	/* Scan memory nodes and rebuild MEMBLOCKs */
>  	early_init_dt_scan_root();
> -	of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
> +	early_init_dt_scan_memory_ppc();
>  
>  	parse_early_param();
>  
> @@ -858,7 +858,7 @@ void __init early_get_first_memblock_info(void *params, phys_addr_t *size)
>  	 */
>  	add_mem_to_memblock = 0;
>  	early_init_dt_scan_root();
> -	of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
> +	early_init_dt_scan_memory_ppc();
>  	add_mem_to_memblock = 1;
>  
>  	if (size)


This blows up one of my machines with:

  [    0.000000][    T0] printk: bootconsole [udbg0] enabled
   -> early_setup(), dt_ptr: 0x1ec90000
  [    0.000000][    T0] ------------[ cut here ]------------
  [    0.000000][    T0] kernel BUG at arch/powerpc/mm/book3s64/hash_utils.c:2117!
  [    0.000000][    T0] Oops: Exception in kernel mode, sig: 5 [#1]
  [    0.000000][    T0] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA
  [    0.000000][    T0] Modules linked in:
  [    0.000000][    T0] CPU: 0 PID: 0 Comm: swapper Not tainted 5.16.0-rc4-00073-g81291383ffde-dirty #69
  [    0.000000][    T0] NIP:  c0000000000924d8 LR: c000000002009764 CTR: c0000000000924d0
  [    0.000000][    T0] REGS: c000000002833bc0 TRAP: 0700   Not tainted  (5.16.0-rc4-00073-g81291383ffde-dirty)
  [    0.000000][    T0] MSR:  8000000000021003 <SF,ME,RI,LE>  CR: 24000244  XER: 20000001
  [    0.000000][    T0] CFAR: 0000000000000730 IRQMASK: 1
  [    0.000000][    T0] GPR00: c000000002009764 c000000002833e60 c000000002834100 ffffffffffffffff
  [    0.000000][    T0] GPR04: 0000000000000000 c000000002080866 0000000000000000 0000000000000000
  [    0.000000][    T0] GPR08: c000000002080864 0000000000000001 c0000000028d4100 c000000000ffe598
  [    0.000000][    T0] GPR12: c0000000000924d0 c000000002082200 0000000000000000 0000000000000000
  [    0.000000][    T0] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
  [    0.000000][    T0] GPR20: 0000000000000001 0000000010004604 0000000000000000 0000000010004bfc
  [    0.000000][    T0] GPR24: 0000000000000000 c000000000000000 0000000002970000 c00000000008a480
  [    0.000000][    T0] GPR28: c0000000028e19f8 c00000001ec90000 c000000002865af8 000000001ec90000
  [    0.000000][    T0] NIP [c0000000000924d8] hash__setup_initial_memory_limit+0x18/0x110
  [    0.000000][    T0] LR [c000000002009764] early_init_devtree+0x13c/0x4ec
  [    0.000000][    T0] Call Trace:
  [    0.000000][    T0] [c000000002833e60] [c0000000020096fc] early_init_devtree+0xd4/0x4ec (unreliable)
  [    0.000000][    T0] [c000000002833f10] [c00000000200b008] early_setup+0xc8/0x22c
  [    0.000000][    T0] [c000000002833f90] [000000000000d368] 0xd368
  [    0.000000][    T0] Instruction dump:
  [    0.000000][    T0] 4bffff0c eaa10028 4bffff44 60000000 60000000 60420000 3c4c027a 38421c40
  [    0.000000][    T0] 7c0802a6 4bfe2e5d 3123ffff 7d291910 <0b090000> 3d220003 392919f8 e9290000
  [    0.000000][    T0] random: get_random_bytes called from oops_exit+0x54/0xa0 with crng_init=0
  [    0.000000][    T0] ---[ end trace 0000000000000000 ]---


It's complaining about memstart_addr being 0, which implies
early_init_dt_add_memory_arch() was never called.

Will try and get some more debug tomorrow.

cheers

Re: [PATCH v2] of/fdt: Rework early_init_dt_scan_memory() to call directly

[Rob Herring]

On Mon, Dec 13, 2021 at 6:47 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Rob Herring <robh@kernel.org> writes:
> > Use of the of_scan_flat_dt() function predates libfdt and is discouraged
> > as libfdt provides a nicer set of APIs. Rework
> > early_init_dt_scan_memory() to be called directly and use libfdt.
> ...
> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> > index 6e1a106f02eb..63762a3b75e8 100644
> > --- a/arch/powerpc/kernel/prom.c
> > +++ b/arch/powerpc/kernel/prom.c
> > @@ -532,19 +532,19 @@ static int  __init early_init_drmem_lmb(struct drmem_lmb *lmb,
> >  }
> >  #endif /* CONFIG_PPC_PSERIES */
> >
> > -static int __init early_init_dt_scan_memory_ppc(unsigned long node,
> > -                                             const char *uname,
> > -                                             int depth, void *data)
> > +static int __init early_init_dt_scan_memory_ppc(void)
> >  {
> >  #ifdef CONFIG_PPC_PSERIES
> > -     if (depth == 1 &&
> > -         strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
> > +     const void *fdt = initial_boot_params;
> > +     int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
> > +
> > +     if (node > 0) {
> >               walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
> >               return 0;
> >       }
> >  #endif
> >
> > -     return early_init_dt_scan_memory(node, uname, depth, data);
> > +     return early_init_dt_scan_memory();
> >  }
> >
> >  /*
> > @@ -749,7 +749,7 @@ void __init early_init_devtree(void *params)
> >
> >       /* Scan memory nodes and rebuild MEMBLOCKs */
> >       early_init_dt_scan_root();
> > -     of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
> > +     early_init_dt_scan_memory_ppc();
> >
> >       parse_early_param();
> >
> > @@ -858,7 +858,7 @@ void __init early_get_first_memblock_info(void *params, phys_addr_t *size)
> >        */
> >       add_mem_to_memblock = 0;
> >       early_init_dt_scan_root();
> > -     of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
> > +     early_init_dt_scan_memory_ppc();
> >       add_mem_to_memblock = 1;
> >
> >       if (size)
>
>
> This blows up one of my machines with:
>
>   [    0.000000][    T0] printk: bootconsole [udbg0] enabled
>    -> early_setup(), dt_ptr: 0x1ec90000
>   [    0.000000][    T0] ------------[ cut here ]------------
>   [    0.000000][    T0] kernel BUG at arch/powerpc/mm/book3s64/hash_utils.c:2117!
>   [    0.000000][    T0] Oops: Exception in kernel mode, sig: 5 [#1]
>   [    0.000000][    T0] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA
>   [    0.000000][    T0] Modules linked in:
>   [    0.000000][    T0] CPU: 0 PID: 0 Comm: swapper Not tainted 5.16.0-rc4-00073-g81291383ffde-dirty #69
>   [    0.000000][    T0] NIP:  c0000000000924d8 LR: c000000002009764 CTR: c0000000000924d0
>   [    0.000000][    T0] REGS: c000000002833bc0 TRAP: 0700   Not tainted  (5.16.0-rc4-00073-g81291383ffde-dirty)
>   [    0.000000][    T0] MSR:  8000000000021003 <SF,ME,RI,LE>  CR: 24000244  XER: 20000001
>   [    0.000000][    T0] CFAR: 0000000000000730 IRQMASK: 1
>   [    0.000000][    T0] GPR00: c000000002009764 c000000002833e60 c000000002834100 ffffffffffffffff
>   [    0.000000][    T0] GPR04: 0000000000000000 c000000002080866 0000000000000000 0000000000000000
>   [    0.000000][    T0] GPR08: c000000002080864 0000000000000001 c0000000028d4100 c000000000ffe598
>   [    0.000000][    T0] GPR12: c0000000000924d0 c000000002082200 0000000000000000 0000000000000000
>   [    0.000000][    T0] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>   [    0.000000][    T0] GPR20: 0000000000000001 0000000010004604 0000000000000000 0000000010004bfc
>   [    0.000000][    T0] GPR24: 0000000000000000 c000000000000000 0000000002970000 c00000000008a480
>   [    0.000000][    T0] GPR28: c0000000028e19f8 c00000001ec90000 c000000002865af8 000000001ec90000
>   [    0.000000][    T0] NIP [c0000000000924d8] hash__setup_initial_memory_limit+0x18/0x110
>   [    0.000000][    T0] LR [c000000002009764] early_init_devtree+0x13c/0x4ec
>   [    0.000000][    T0] Call Trace:
>   [    0.000000][    T0] [c000000002833e60] [c0000000020096fc] early_init_devtree+0xd4/0x4ec (unreliable)
>   [    0.000000][    T0] [c000000002833f10] [c00000000200b008] early_setup+0xc8/0x22c
>   [    0.000000][    T0] [c000000002833f90] [000000000000d368] 0xd368
>   [    0.000000][    T0] Instruction dump:
>   [    0.000000][    T0] 4bffff0c eaa10028 4bffff44 60000000 60000000 60420000 3c4c027a 38421c40
>   [    0.000000][    T0] 7c0802a6 4bfe2e5d 3123ffff 7d291910 <0b090000> 3d220003 392919f8 e9290000
>   [    0.000000][    T0] random: get_random_bytes called from oops_exit+0x54/0xa0 with crng_init=0
>   [    0.000000][    T0] ---[ end trace 0000000000000000 ]---
>
>
> It's complaining about memstart_addr being 0, which implies
> early_init_dt_add_memory_arch() was never called.

The only thing I see is now there is an assumption that 'memory' nodes
are off the root node only. Before they could be anywhere. If that's
the issue, then we need something like this (untested and WS
corrupted):

index a835c458f50a..97d7607625ec 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1083,16 +1083,13 @@ int __init early_init_dt_scan_memory(void)
        int node;
        const void *fdt = initial_boot_params;

-       fdt_for_each_subnode(node, fdt, 0) {
-               const char *type = of_get_flat_dt_prop(node,
"device_type", NULL);
+       for (node = fdt_node_offset_by_prop_value(fdt, -1,
"device_type", "memory", 6);
+            node != -FDT_ERR_NOTFOUND;
+            node = fdt_node_offset_by_prop_value(fdt, node,
"device_type", "memory", 6)) {
                const __be32 *reg, *endp;
                int l;
                bool hotpluggable;

-               /* We are scanning "memory" nodes only */
-               if (type == NULL || strcmp(type, "memory") != 0)
-                       continue;
-
                reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
                if (reg == NULL)
                        reg = of_get_flat_dt_prop(node, "reg", &l);

Rob

Re: [PATCH v2] of/fdt: Rework early_init_dt_scan_memory() to call directly

[Michael Ellerman]

Rob Herring <robh@kernel.org> writes:
> On Mon, Dec 13, 2021 at 6:47 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Rob Herring <robh@kernel.org> writes:
>> > Use of the of_scan_flat_dt() function predates libfdt and is discouraged
>> > as libfdt provides a nicer set of APIs. Rework
>> > early_init_dt_scan_memory() to be called directly and use libfdt.
>> ...
>> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> > index 6e1a106f02eb..63762a3b75e8 100644
>> > --- a/arch/powerpc/kernel/prom.c
>> > +++ b/arch/powerpc/kernel/prom.c
>> > @@ -532,19 +532,19 @@ static int  __init early_init_drmem_lmb(struct drmem_lmb *lmb,
>> >  }
>> >  #endif /* CONFIG_PPC_PSERIES */
>> >
>> > -static int __init early_init_dt_scan_memory_ppc(unsigned long node,
>> > -                                             const char *uname,
>> > -                                             int depth, void *data)
>> > +static int __init early_init_dt_scan_memory_ppc(void)
>> >  {
>> >  #ifdef CONFIG_PPC_PSERIES
>> > -     if (depth == 1 &&
>> > -         strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
>> > +     const void *fdt = initial_boot_params;
>> > +     int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
>> > +
>> > +     if (node > 0) {
>> >               walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
>> >               return 0;
>> >       }

It's that return that is the problem.

Now that early_init_dt_scan_memory_ppc() is only called once, that
return causes us to skip scanning regular memory nodes if there is an
"ibm,dynamic-reconfiguration-memory" property present.

So the fix is just:

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 1098de3b172f..125661e5fcf3 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -538,10 +538,8 @@ static int __init early_init_dt_scan_memory_ppc(void)
 	const void *fdt = initial_boot_params;
 	int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
 
-	if (node > 0) {
+	if (node > 0)
 		walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
-		return 0;
-	}
 #endif
 	
 	return early_init_dt_scan_memory();


> The only thing I see is now there is an assumption that 'memory' nodes
> are off the root node only. Before they could be anywhere.

I don't know of any machines where that would be a problem. But given
all the wild and wonderful device trees out there, who really knows :)

Maybe we should continue to allow memory nodes to be anywhere, and print
a warning for any that aren't at the root. Then if no one reports any
hits for the warning we could switch to only allowing them at the root?

cheers


> index a835c458f50a..97d7607625ec 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1083,16 +1083,13 @@ int __init early_init_dt_scan_memory(void)
>         int node;
>         const void *fdt = initial_boot_params;
>
> -       fdt_for_each_subnode(node, fdt, 0) {
> -               const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
> +       for (node = fdt_node_offset_by_prop_value(fdt, -1, "device_type", "memory", 6);
> +            node != -FDT_ERR_NOTFOUND;
> +            node = fdt_node_offset_by_prop_value(fdt, node, "device_type", "memory", 6)) {
>                 const __be32 *reg, *endp;
>                 int l;
>                 bool hotpluggable;
>
> -               /* We are scanning "memory" nodes only */
> -               if (type == NULL || strcmp(type, "memory") != 0)
> -                       continue;
> -
>                 reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
>                 if (reg == NULL)
>                         reg = of_get_flat_dt_prop(node, "reg", &l);
>
> Rob

Re: [PATCH v2] of/fdt: Rework early_init_dt_scan_memory() to call directly

[Rob Herring]

On Tue, Dec 14, 2021 at 5:18 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Rob Herring <robh@kernel.org> writes:
> > On Mon, Dec 13, 2021 at 6:47 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
> >> Rob Herring <robh@kernel.org> writes:
> >> > Use of the of_scan_flat_dt() function predates libfdt and is discouraged
> >> > as libfdt provides a nicer set of APIs. Rework
> >> > early_init_dt_scan_memory() to be called directly and use libfdt.
> >> ...
> >> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> >> > index 6e1a106f02eb..63762a3b75e8 100644
> >> > --- a/arch/powerpc/kernel/prom.c
> >> > +++ b/arch/powerpc/kernel/prom.c
> >> > @@ -532,19 +532,19 @@ static int  __init early_init_drmem_lmb(struct drmem_lmb *lmb,
> >> >  }
> >> >  #endif /* CONFIG_PPC_PSERIES */
> >> >
> >> > -static int __init early_init_dt_scan_memory_ppc(unsigned long node,
> >> > -                                             const char *uname,
> >> > -                                             int depth, void *data)
> >> > +static int __init early_init_dt_scan_memory_ppc(void)
> >> >  {
> >> >  #ifdef CONFIG_PPC_PSERIES
> >> > -     if (depth == 1 &&
> >> > -         strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
> >> > +     const void *fdt = initial_boot_params;
> >> > +     int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
> >> > +
> >> > +     if (node > 0) {
> >> >               walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
> >> >               return 0;
> >> >       }
>
> It's that return that is the problem.
>
> Now that early_init_dt_scan_memory_ppc() is only called once, that
> return causes us to skip scanning regular memory nodes if there is an
> "ibm,dynamic-reconfiguration-memory" property present.
>
> So the fix is just:
>
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 1098de3b172f..125661e5fcf3 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -538,10 +538,8 @@ static int __init early_init_dt_scan_memory_ppc(void)
>         const void *fdt = initial_boot_params;
>         int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
>
> -       if (node > 0) {
> +       if (node > 0)
>                 walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
> -               return 0;
> -       }
>  #endif
>
>         return early_init_dt_scan_memory();

Thanks! I've rolled that in.

> > The only thing I see is now there is an assumption that 'memory' nodes
> > are off the root node only. Before they could be anywhere.
>
> I don't know of any machines where that would be a problem. But given
> all the wild and wonderful device trees out there, who really knows :)
>
> Maybe we should continue to allow memory nodes to be anywhere, and print
> a warning for any that aren't at the root. Then if no one reports any
> hits for the warning we could switch to only allowing them at the root?

I really doubt there's any case. I just have the least visibility into
what IBM DTs look like. I checked some old DT files I have and also
u-boot only supports off the root node.


Rob

Re: [PATCH v2] of/fdt: Rework early_init_dt_scan_memory() to call directly

[Michael Ellerman]

Rob Herring <robh@kernel.org> writes:
> On Tue, Dec 14, 2021 at 5:18 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>
>> Rob Herring <robh@kernel.org> writes:
>> > On Mon, Dec 13, 2021 at 6:47 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>> >> Rob Herring <robh@kernel.org> writes:
>> >> > Use of the of_scan_flat_dt() function predates libfdt and is discouraged
>> >> > as libfdt provides a nicer set of APIs. Rework
>> >> > early_init_dt_scan_memory() to be called directly and use libfdt.
>> >> ...
>> >> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> >> > index 6e1a106f02eb..63762a3b75e8 100644
>> >> > --- a/arch/powerpc/kernel/prom.c
>> >> > +++ b/arch/powerpc/kernel/prom.c
>> >> > @@ -532,19 +532,19 @@ static int  __init early_init_drmem_lmb(struct drmem_lmb *lmb,
>> >> >  }
>> >> >  #endif /* CONFIG_PPC_PSERIES */
>> >> >
>> >> > -static int __init early_init_dt_scan_memory_ppc(unsigned long node,
>> >> > -                                             const char *uname,
>> >> > -                                             int depth, void *data)
>> >> > +static int __init early_init_dt_scan_memory_ppc(void)
>> >> >  {
>> >> >  #ifdef CONFIG_PPC_PSERIES
>> >> > -     if (depth == 1 &&
>> >> > -         strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
>> >> > +     const void *fdt = initial_boot_params;
>> >> > +     int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
>> >> > +
>> >> > +     if (node > 0) {
>> >> >               walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
>> >> >               return 0;
>> >> >       }
>>
>> It's that return that is the problem.
>>
>> Now that early_init_dt_scan_memory_ppc() is only called once, that
>> return causes us to skip scanning regular memory nodes if there is an
>> "ibm,dynamic-reconfiguration-memory" property present.
>>
>> So the fix is just:
>>
>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index 1098de3b172f..125661e5fcf3 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -538,10 +538,8 @@ static int __init early_init_dt_scan_memory_ppc(void)
>>         const void *fdt = initial_boot_params;
>>         int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
>>
>> -       if (node > 0) {
>> +       if (node > 0)
>>                 walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
>> -               return 0;
>> -       }
>>  #endif
>>
>>         return early_init_dt_scan_memory();
>
> Thanks! I've rolled that in.
>
>> > The only thing I see is now there is an assumption that 'memory' nodes
>> > are off the root node only. Before they could be anywhere.
>>
>> I don't know of any machines where that would be a problem. But given
>> all the wild and wonderful device trees out there, who really knows :)
>>
>> Maybe we should continue to allow memory nodes to be anywhere, and print
>> a warning for any that aren't at the root. Then if no one reports any
>> hits for the warning we could switch to only allowing them at the root?
>
> I really doubt there's any case. I just have the least visibility into
> what IBM DTs look like. I checked some old DT files I have and also
> u-boot only supports off the root node.

The IBM ones are pretty standard, it's other embedded things I'd be more
worried about.

I have a collection of device trees, but they were given to me by
various random people over the years and I'm not comfortable putting
them up in public. I looked through those and didn't see anything odd.

I'll try and get a collection of device trees from machines of mine and
put them somewhere public.

cheers