| Message ID | 11af73e05bad75e4ef49067515e3214f6d944b3d.camel@gmail.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | of: fdt: Honor CONFIG_CMDLINE* even without /chosen node | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
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
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
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/
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.
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!
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();
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(-)