[PATCH 0/2] of: Fix handling CONFIG_CMDLINE* even without /chosen node

[PATCH 0/2] of: Fix handling CONFIG_CMDLINE* even without /chosen node

[Rob Herring]

Commit a7d550f82b445cf218b47a2c1a9c56e97ecb8c7a (of: fdt: Honor CONFIG_CMDLINE*
even without /chosen node) moved the processing of the kernel built-in command
line (CONFIG_CMDLINE) from the early_init_dt_scan_chosen routine to the
early_init_dt_scan_nodes routine.

The current powerpc startup code does not call into early_init_dt_scan_nodes, so
processing of CONFIG_CMDLINE never happens, even if CONFIG_CMDLINE_FORCE=y.
The result is an empty kernel command line, and mounting of the root file system
then fails with a kernel panic (not syncing: VFS: Unable to mount root fs).

Let's revert the above commit and redo the missing /chosen node handling 
within early_init_dt_scan_chosen().

Signed-off-by: Rob Herring <robh@kernel.org>
---
Rob Herring (2):
      Revert "of: fdt: Honor CONFIG_CMDLINE* even without /chosen node"
      of: fdt: Honor CONFIG_CMDLINE* even without /chosen node, take 2

 drivers/of/fdt.c | 54 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 26 deletions(-)
---
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
change-id: 20230103-dt-cmdline-fix-e36eb7e39b75

Best regards,
-- 
Rob Herring <robh@kernel.org>

[PATCH 1/2] Revert "of: fdt: Honor CONFIG_CMDLINE* even without /chosen node"

[Rob Herring]

This reverts commit a7d550f82b445cf218b47a2c1a9c56e97ecb8c7a.

Some arches (PPC at least) don't call early_init_dt_scan_nodes(), so
moving the cmdline processing there breaks them.

Reported-by: Geoff Levand <geoff@infradead.org>
Cc: Alexander Sverdlin <alexander.sverdlin@gmail.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/fdt.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index b2272bccf85c..7b571a631639 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1173,6 +1173,26 @@ int __init early_init_dt_scan_chosen(char *cmdline)
 	if (p != NULL && l > 0)
 		strscpy(cmdline, p, min(l, COMMAND_LINE_SIZE));
 
+	/*
+	 * CONFIG_CMDLINE is meant to be a default in case nothing else
+	 * managed to set the command line, unless CONFIG_CMDLINE_FORCE
+	 * is set in which case we override whatever was found earlier.
+	 */
+#ifdef CONFIG_CMDLINE
+#if defined(CONFIG_CMDLINE_EXTEND)
+	strlcat(cmdline, " ", COMMAND_LINE_SIZE);
+	strlcat(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
+#elif defined(CONFIG_CMDLINE_FORCE)
+	strscpy(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
+#else
+	/* No arguments from boot loader, use kernel's  cmdl*/
+	if (!((char *)cmdline)[0])
+		strscpy(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
+#endif
+#endif /* CONFIG_CMDLINE */
+
+	pr_debug("Command line is: %s\n", (char *)cmdline);
+
 	rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
 	if (rng_seed && l > 0) {
 		add_bootloader_randomness(rng_seed, l);
@@ -1277,26 +1297,6 @@ void __init early_init_dt_scan_nodes(void)
 	if (rc)
 		pr_warn("No chosen node found, continuing without\n");
 
-	/*
-	 * CONFIG_CMDLINE is meant to be a default in case nothing else
-	 * managed to set the command line, unless CONFIG_CMDLINE_FORCE
-	 * is set in which case we override whatever was found earlier.
-	 */
-#ifdef CONFIG_CMDLINE
-#if defined(CONFIG_CMDLINE_EXTEND)
-	strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
-	strlcat(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#elif defined(CONFIG_CMDLINE_FORCE)
-	strscpy(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#else
-	/* No arguments from boot loader, use kernel's cmdl */
-	if (!boot_command_line[0])
-		strscpy(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#endif
-#endif /* CONFIG_CMDLINE */
-
-	pr_debug("Command line is: %s\n", boot_command_line);
-
 	/* Setup memory, calling early_init_dt_add_memory_arch */
 	early_init_dt_scan_memory();
 

-- 
2.39.0

[PATCH 2/2] of: fdt: Honor CONFIG_CMDLINE* even without /chosen node, take 2

[Rob Herring]

I do not read a strict requirement on /chosen node in either ePAPR or in
Documentation/devicetree. Help text for CONFIG_CMDLINE and
CONFIG_CMDLINE_EXTEND doesn't make their behavior explicitly dependent on
the presence of /chosen or the presense of /chosen/bootargs.

However the early check for /chosen and bailing out in
early_init_dt_scan_chosen() skips CONFIG_CMDLINE handling which is not
really related to /chosen node or the particular method of passing cmdline
from bootloader.

This leads to counterintuitive combinations (assuming
CONFIG_CMDLINE_EXTEND=y):

a) bootargs="foo", CONFIG_CMDLINE="bar" => cmdline=="foo bar"
b) /chosen missing, CONFIG_CMDLINE="bar" => cmdline==""
c) bootargs="", CONFIG_CMDLINE="bar" => cmdline==" bar"

Rework early_init_dt_scan_chosen() so that the cmdline config options are
always handled.

[commit msg written by Alexander Sverdlin]
Cc: Alexander Sverdlin <alexander.sverdlin@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/fdt.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 7b571a631639..4b94bb52671e 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1161,18 +1161,32 @@ int __init early_init_dt_scan_chosen(char *cmdline)
 	if (node < 0)
 		node = fdt_path_offset(fdt, "/chosen@0");
 	if (node < 0)
-		return -ENOENT;
+		/* Handle the cmdline config options even if no /chosen node */
+		goto handle_cmdline;
 
 	chosen_node_offset = node;
 
 	early_init_dt_check_for_initrd(node);
 	early_init_dt_check_for_elfcorehdr(node);
 
+	rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
+	if (rng_seed && l > 0) {
+		add_bootloader_randomness(rng_seed, l);
+
+		/* try to clear seed so it won't be found. */
+		fdt_nop_property(initial_boot_params, node, "rng-seed");
+
+		/* update CRC check value */
+		of_fdt_crc32 = crc32_be(~0, initial_boot_params,
+				fdt_totalsize(initial_boot_params));
+	}
+
 	/* Retrieve command line */
 	p = of_get_flat_dt_prop(node, "bootargs", &l);
 	if (p != NULL && l > 0)
 		strscpy(cmdline, p, min(l, COMMAND_LINE_SIZE));
 
+handle_cmdline:
 	/*
 	 * CONFIG_CMDLINE is meant to be a default in case nothing else
 	 * managed to set the command line, unless CONFIG_CMDLINE_FORCE
@@ -1193,18 +1207,6 @@ int __init early_init_dt_scan_chosen(char *cmdline)
 
 	pr_debug("Command line is: %s\n", (char *)cmdline);
 
-	rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
-	if (rng_seed && l > 0) {
-		add_bootloader_randomness(rng_seed, l);
-
-		/* try to clear seed so it won't be found. */
-		fdt_nop_property(initial_boot_params, node, "rng-seed");
-
-		/* update CRC check value */
-		of_fdt_crc32 = crc32_be(~0, initial_boot_params,
-				fdt_totalsize(initial_boot_params));
-	}
-
 	return 0;
 }
 

-- 
2.39.0

Re: [PATCH 0/2] of: Fix handling CONFIG_CMDLINE* even without /chosen node

[Geoff Levand]

Hi Rob,

On 1/3/23 10:00, Rob Herring wrote:
> 
> Let's revert the above commit and redo the missing /chosen node handling 
> within early_init_dt_scan_chosen().
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Rob Herring (2):
>       Revert "of: fdt: Honor CONFIG_CMDLINE* even without /chosen node"
>       of: fdt: Honor CONFIG_CMDLINE* even without /chosen node, take 2
> 
>  drivers/of/fdt.c | 54 ++++++++++++++++++++++++++++--------------------------
>  1 file changed, 28 insertions(+), 26 deletions(-)

I applied these two patches to v6.2-rc2 and tested on PS3. I used a kernel
config file that had a CONFIG_CMDLINE, and CONFIG_CMDLINE_FORCE=y set.
Everything seemed to work OK.  

Tested-by: Geoff Levand <geoff@infradead.org>

Re: [PATCH 0/2] of: Fix handling CONFIG_CMDLINE* even without /chosen node

[Alexander Sverdlin]

Hi Rob,

thanks for looking into it!

On Tue, 2023-01-03 at 12:00 -0600, Rob Herring wrote:
> Commit a7d550f82b445cf218b47a2c1a9c56e97ecb8c7a (of: fdt: Honor CONFIG_CMDLINE*
> even without /chosen node) moved the processing of the kernel built-in command
> line (CONFIG_CMDLINE) from the early_init_dt_scan_chosen routine to the
> early_init_dt_scan_nodes routine.
> 
> The current powerpc startup code does not call into early_init_dt_scan_nodes, so
> processing of CONFIG_CMDLINE never happens, even if CONFIG_CMDLINE_FORCE=y.
> The result is an empty kernel command line, and mounting of the root file system
> then fails with a kernel panic (not syncing: VFS: Unable to mount root fs).
> 
> Let's revert the above commit and redo the missing /chosen node handling 
> within early_init_dt_scan_chosen().
> 
> Signed-off-by: Rob Herring <robh@kernel.org>

The series looks good to me,
Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

-- 
Alexander Sverdlin.