[PATCH 0/2] powerpc/prom, of/fdt: fix chosen node handling and cleanup

[PATCH 0/2] powerpc/prom, of/fdt: fix chosen node handling and cleanup

[Sang-Heon Jeon]

Hello,

While looking into boot information, I found one bug fix and one
minor cleanup around /chosen node handling.

Patch 1 fixes a bug in powerpc where /chosen properties are read
from the wrong node.

This bug has existed for a while without any report, so I don't
think backporting is necessary and I did not add Cc: stable.
Please let me know if you think otherwise.

Patch 2 is a follow-up cleanup that drops the now-unused return
value of early_init_dt_scan_chosen() and the dead checks at its
callers.

If I missunderstood anything, please feel free to let me know.

Best Regards,
Sang-Heon Jeon

Sang-Heon Jeon (2):
  powerpc/prom: fix /chosen properties read from wrong node
  of/fdt: make early_init_dt_scan_chosen() return void

 arch/powerpc/kernel/prom.c | 20 ++++++++++----------
 drivers/of/fdt.c           | 10 ++--------
 include/linux/of_fdt.h     |  2 +-
 3 files changed, 13 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH 1/2] powerpc/prom: fix /chosen properties read from wrong node

[Sang-Heon Jeon]

Commit 60f20d84dc81 ("of/fdt: Rework early_init_dt_scan_chosen() to
call directly") changed early_init_dt_scan_chosen() to be called
directly instead of via of_scan_flat_dt(). After this change,
early_init_dt_scan_chosen_ppc() does not behave as intended in either
case below:

- /chosen(or /chosen@0) exists: early_init_dt_scan_chosen() always 
  succeeds, so early_init_dt_scan_chosen_ppc() then reads properties
  from the root node (first iteration) instead of chosen node.

- /chosen does not exist:
  - Until commit 064e32dc5b03 ("of: fdt: Honor CONFIG_CMDLINE* even without
    /chosen node, take 2"), early_init_dt_scan_chosen() returns -ENOENT
    and early_init_dt_scan_chosen_ppc() returns 0. So of_scan_flat_dt()
    iterates over all remaining nodes. Not a bug but unnecessary.
  - After above commit, early_init_dt_scan_chosen() returns 0 and
    early_init_dt_scan_chosen_ppc() returns 1. So it reads properties
    from the root node (first iteration) instead of chosen node, same as
    the chosen node exist case above.

Instead of using of_scan_flat_dt() for chosen node handling, first call
early_init_dt_scan_chosen() directly to handle common chosen node
properties. Then call early_init_dt_scan_chosen_ppc(), which is updated
to handle powerpc-specific chosen node properties.

Both now look up chosen node directly to avoid reading from the wrong node.

Fixes: 60f20d84dc81 ("of/fdt: Rework early_init_dt_scan_chosen() to call directly")
Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
---
QEMU-based test results

$ fdtput -t bx chosen.dtb /chosen linux,crashkernel-base 00 00 00 10 00 00 00 00
$ fdtput -t bx chosen.dtb /chosen linux,crashkernel-size 00 00 00 02 00 00 00 00
$ qemu-system-ppc64 -M powernv9,dtb=chosen.dtb ...

1) AS-IS (before fix)
# cat /sys/kernel/kexec_crash_size
0

2) TO-BE (after fix)
# cat /sys/kernel/kexec_crash_size
33554432

---
 arch/powerpc/kernel/prom.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 9ed9dde7d231..34318ddb4a73 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -450,15 +450,17 @@ static int __init early_init_dt_scan_cpus(unsigned long node,
 	return 0;
 }
 
-static int __init early_init_dt_scan_chosen_ppc(unsigned long node,
-						const char *uname,
-						int depth, void *data)
+static void __init early_init_dt_scan_chosen_ppc(void)
 {
+	const void *fdt = initial_boot_params;
 	const unsigned long *lprop; /* All these set by kernel, so no need to convert endian */
+	int node;
 
-	/* Use common scan routine to determine if this is the chosen node */
-	if (early_init_dt_scan_chosen(data) < 0)
-		return 0;
+	node = fdt_path_offset(fdt, "/chosen");
+	if (node < 0)
+		node = fdt_path_offset(fdt, "/chosen@0");
+	if (node < 0)
+		return;
 
 #ifdef CONFIG_PPC64
 	/* check if iommu is forced on or off */
@@ -491,9 +493,6 @@ static int __init early_init_dt_scan_chosen_ppc(unsigned long node,
 	if (lprop)
 		crashk_res.end = crashk_res.start + *lprop - 1;
 #endif
-
-	/* break now */
-	return 1;
 }
 
 /*
@@ -818,7 +817,8 @@ void __init early_init_devtree(void *params)
 	 * device-tree, including the platform type, initrd location and
 	 * size, TCE reserve, and more ...
 	 */
-	of_scan_flat_dt(early_init_dt_scan_chosen_ppc, boot_command_line);
+	early_init_dt_scan_chosen(boot_command_line);
+	early_init_dt_scan_chosen_ppc();
 
 	/* Append additional parameters passed for fadump capture kernel */
 	fadump_append_bootargs();
-- 
2.43.0

[PATCH 2/2] of/fdt: make early_init_dt_scan_chosen() return void

[Sang-Heon Jeon]

Since commit 064e32dc5b03 ("of: fdt: Honor CONFIG_CMDLINE* even
without /chosen node, take 2"), early_init_dt_scan_chosen() always
returns 0, so the return value checks at its callers have become
dead code.

Drop the return value, change the function to return void, and
update its declaration and callers accordingly. No functional
change.

Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
---
dt-test result

[    0.684136] ### dt-test ### start of unittest - you will see error messages
[    0.685849] ### dt-test ### EXPECT \ : Duplicate name in testcase-data, renamed to "duplicate-name#1"
[    0.685870] Duplicate name in testcase-data, renamed to "duplicate-name#1"
[    0.689496] ### dt-test ### EXPECT / : Duplicate name in testcase-data, renamed to "duplicate-name#1"

...

[    1.693384] ### dt-test ### EXPECT / : OF: resolver: overlay phandle fixup failed: -22
[    1.693386] ### dt-test ### EXPECT / : OF: resolver: node label 'this_label_does_not_exist' not found in live devicetree symbols table
[    1.694968] ### dt-test ### end of unittest - 405 passed, 0 failed
---
 drivers/of/fdt.c       | 10 ++--------
 include/linux/of_fdt.h |  2 +-
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index ba65e36e183c..1b2cf5b29926 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1087,7 +1087,7 @@ int __init early_init_dt_scan_memory(void)
 	return found_memory;
 }
 
-int __init early_init_dt_scan_chosen(char *cmdline)
+void __init early_init_dt_scan_chosen(char *cmdline)
 {
 	int l, node;
 	const char *p;
@@ -1144,8 +1144,6 @@ int __init early_init_dt_scan_chosen(char *cmdline)
 #endif /* CONFIG_CMDLINE */
 
 	pr_debug("Command line is: %s\n", (char *)cmdline);
-
-	return 0;
 }
 
 #ifndef MIN_MEMBLOCK_ADDR
@@ -1226,12 +1224,8 @@ bool __init early_init_dt_verify(void *dt_virt, phys_addr_t dt_phys)
 
 void __init early_init_dt_scan_nodes(void)
 {
-	int rc;
-
 	/* Retrieve various information from the /chosen node */
-	rc = early_init_dt_scan_chosen(boot_command_line);
-	if (rc)
-		pr_warn("No chosen node found, continuing without\n");
+	early_init_dt_scan_chosen(boot_command_line);
 
 	/* Setup memory, calling early_init_dt_add_memory_arch */
 	early_init_dt_scan_memory();
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index 51dadbaa3d63..09e9d45f432b 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -68,7 +68,7 @@ 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);
 
-extern int early_init_dt_scan_chosen(char *cmdline);
+extern void early_init_dt_scan_chosen(char *cmdline);
 extern int early_init_dt_scan_memory(void);
 extern void early_init_dt_check_for_usable_mem_range(void);
 extern int early_init_dt_scan_chosen_stdout(void);
-- 
2.43.0