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

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

[Alexander Sverdlin]

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"

Move CONFIG_CMDLINE handling outside of early_init_dt_scan_chosen() so that
cases b and c above result in the same cmdline.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
---
 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 7b571a631639..b2272bccf85c 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1173,26 +1173,6 @@ 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);
@@ -1297,6 +1277,26 @@ 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.37.3

Re: [PATCH] of: fdt: Honor CONFIG_CMDLINE* even without /chosen node

[Arnd Bergmann]

On Mon, Dec 12, 2022, at 00:58, Alexander Sverdlin wrote:
> 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"
>
> Move CONFIG_CMDLINE handling outside of early_init_dt_scan_chosen() so that
> cases b and c above result in the same cmdline.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

Thanks for debugging this and coming up with a fix. We probably want
to have an empty /chosen node in most dts files to stay compatible with
existing kernels, but fixing the kernel is a good idea regardless.

Acked-by: Arnd Bergmann <arnd@arndb.de>

and probably 

Cc: stable@vger.kernel.org

Re: [PATCH] of: fdt: Honor CONFIG_CMDLINE* even without /chosen node

[Linus Walleij]

On Mon, Dec 12, 2022 at 7:01 AM Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:

> 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"
>
> Move CONFIG_CMDLINE handling outside of early_init_dt_scan_chosen() so that
> cases b and c above result in the same cmdline.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

Excellent debugging Alexander!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I also think this should go to stable.

Yours,
Linus Walleij

Re: [PATCH] of: fdt: Honor CONFIG_CMDLINE* even without /chosen node

[Rob Herring]

On Tue, Dec 13, 2022 at 09:51:33AM +0100, Linus Walleij wrote:
> On Mon, Dec 12, 2022 at 7:01 AM Alexander Sverdlin
> <alexander.sverdlin@gmail.com> wrote:
> 
> > 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"
> >
> > Move CONFIG_CMDLINE handling outside of early_init_dt_scan_chosen() so that
> > cases b and c above result in the same cmdline.
> >
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> 
> Excellent debugging Alexander!
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> I also think this should go to stable.

We have to be careful there. This could change behavior on a working 
system. A system taking the cmdline entirely from a built kernel and 
no initrd is going to be pretty customized already, I think they can 
carry a patch. What platform is this anyways?

This has actually been known for some time[1][2]. My concern in the past 
(besides wanting all the cmdline manipulation being common) was MIPS. 
MIPS in particular has lots of sources for cmdline and ways to combine 
it. However, MIPS has since stopped using this code and does their own 
parsing (not great either IMO).

Rob

[1] https://lore.kernel.org/all/CAL_Jsq+CgxWbCMz_qwLbMJS3fYwKyCMBGVS501-5ShXywyDAXA@mail.gmail.com/
[2] https://lore.kernel.org/all/CAL_Jsq+n4e5_ptuh89CJibViGS_bgHz0A+Ki-uwtcGU38+mHXQ@mail.gmail.com/

Re: [PATCH] of: fdt: Honor CONFIG_CMDLINE* even without /chosen node

[Alexander Sverdlin]

Hello Rob,

On Tue, 2022-12-13 at 09:29 -0600, Rob Herring wrote:
> On Tue, Dec 13, 2022 at 09:51:33AM +0100, Linus Walleij wrote:
> > On Mon, Dec 12, 2022 at 7:01 AM Alexander Sverdlin
> > <alexander.sverdlin@gmail.com> wrote:
> > 
> > > 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"
> > > 
> > > Move CONFIG_CMDLINE handling outside of early_init_dt_scan_chosen() so that
> > > cases b and c above result in the same cmdline.
> > > 
> > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> > 
> > Excellent debugging Alexander!
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > 
> > I also think this should go to stable.
> 
> We have to be careful there. This could change behavior on a working 
> system. A system taking the cmdline entirely from a built kernel and 
> no initrd is going to be pretty customized already, I think they can 
> carry a patch. What platform is this anyways?

I've stumbled upon this testing first DT conversion patches for EP93xx (ARM).

> This has actually been known for some time[1][2]. My concern in the past 
> (besides wanting all the cmdline manipulation being common) was MIPS. 

This "change of behavior" actually changes one exact corner case:
no /chosen node + CONFIG_CMDLINE!="" + CONFIG_CMDLINE_EXTEND=y

If someone was intentionally hiding something in the config file
under CONFIG_CMDLINE but didn't want it to appear on the kernel command
line in the past, he could just reconfigure new kernel version after
the change and remove the above configs.

> MIPS in particular has lots of sources for cmdline and ways to combine 
> it. However, MIPS has since stopped using this code and does their own 
> parsing (not great either IMO).

I agree, this code screams to be common.

-- 
Alexander Sverdlin.

Re: [PATCH] of: fdt: Honor CONFIG_CMDLINE* even without /chosen node

[Rob Herring]

On Mon, 12 Dec 2022 00:58:17 +0100, Alexander Sverdlin wrote:
> 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"
> 
> Move CONFIG_CMDLINE handling outside of early_init_dt_scan_chosen() so that
> cases b and c above result in the same cmdline.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> ---
>  drivers/of/fdt.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 

Applied, thanks!

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

[Alexander Sverdlin]

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"

Move CONFIG_CMDLINE handling outside of early_init_dt_scan_chosen() so that
cases b and c above result in the same cmdline.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
---
 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 7b571a631639..b2272bccf85c 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1173,26 +1173,6 @@ 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);
@@ -1297,6 +1277,26 @@ 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.37.3