| Message ID | 20210225125921.13147-3-will@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Fix CMDLINE_EXTEND handling for FDT "bootargs" | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 5 warnings, 80 lines checked |
On Thu, 25 Feb 2021 12:59:21 +0000, Will Deacon <will@kernel.org> wrote: > > The Kconfig help text for CMDLINE_EXTEND is sadly duplicated across all > architectures that implement it (arm, arm64, powerpc, riscv and sh), but > they all seem to agree that the bootloader arguments will be appended to > the CONFIG_CMDLINE. For example, on arm64: > > | The command-line arguments provided by the boot loader will be > | appended to the default kernel command string. > > This also matches the behaviour of the EFI stub, which parses the > bootloader arguments first if CMDLINE_EXTEND is set, as well as the > out-of-tree CMDLINE_EXTEND implementation in Android. > > However, the behaviour in the upstream fdt code appears to be the other > way around: CONFIG_CMDLINE is appended to the bootloader arguments. > > Fix the code to follow the documentation by moving the cmdline > processing out into a new function, early_init_dt_retrieve_cmdline(), > and copying CONFIG_CMDLINE to the beginning of the cmdline buffer rather > than concatenating it onto the end. > > Cc: Max Uvarov <muvarov@gmail.com> > Cc: Rob Herring <robh@kernel.org> > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Doug Anderson <dianders@chromium.org> > Cc: Tyler Hicks <tyhicks@linux.microsoft.com> > Cc: Frank Rowand <frowand.list@gmail.com> > Fixes: 34b82026a507 ("fdt: fix extend of cmd line") > Signed-off-by: Will Deacon <will@kernel.org> > --- > drivers/of/fdt.c | 64 +++++++++++++++++++++++++++++------------------- > 1 file changed, 39 insertions(+), 25 deletions(-) > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index dcc1dd96911a..83b9d065e58d 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -1033,11 +1033,48 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname, > return 0; > } > > +static int __init cmdline_from_bootargs(unsigned long node, void *dst, int sz) > +{ > + int l; > + const char *p = of_get_flat_dt_prop(node, "bootargs", &l); > + > + if (!p || l <= 0) > + return -EINVAL; > + > + return strlcpy(dst, p, min(l, sz)); > +} > + > +/* dst is a zero-initialised buffer of COMMAND_LINE_SIZE bytes */ > +static void __init early_init_dt_retrieve_cmdline(unsigned long node, char *dst) > +{ > + if (IS_ENABLED(CONFIG_CMDLINE_EXTEND)) { > + /* Copy CONFIG_CMDLINE to the start of destination buffer */ > + size_t idx = strlcpy(dst, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > + > + /* Check that we have enough space to concatenate */ > + if (idx + 1 >= COMMAND_LINE_SIZE) > + return; > + > + /* Append the bootloader arguments */ > + dst[idx++] = ' '; > + cmdline_from_bootargs(node, &dst[idx], COMMAND_LINE_SIZE - idx); > + } else if (IS_ENABLED(CONFIG_CMDLINE_FORCE)) { > + /* Just use CONFIG_CMDLINE */ > + strlcpy(dst, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > + } else if (IS_ENABLED(CONFIG_CMDLINE_FROM_BOOTLOADER)) { > + /* Use CONFIG_CMDLINE if no arguments from bootloader. */ > + if (cmdline_from_bootargs(node, dst, COMMAND_LINE_SIZE) <= 0) > + strlcpy(dst, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > + } else { Do we have any arch that can end-up not defining any of the 3 above cases? We should be able to just have the above case as the catch-all, and drop the one below. > + /* Just use bootloader arguments */ > + cmdline_from_bootargs(node, dst, COMMAND_LINE_SIZE); > + } > +} > + > int __init early_init_dt_scan_chosen(unsigned long node, const char *uname, > int depth, void *data) > { > int l; > - const char *p; > const void *rng_seed; > > pr_debug("search \"chosen\", depth: %d, uname: %s\n", depth, uname); > @@ -1047,30 +1084,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname, > return 0; > > early_init_dt_check_for_initrd(node); > - > - /* Retrieve command line */ > - p = of_get_flat_dt_prop(node, "bootargs", &l); > - if (p != NULL && l > 0) > - strlcpy(data, 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(data, " ", COMMAND_LINE_SIZE); > - strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > -#elif defined(CONFIG_CMDLINE_FORCE) > - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > -#else > - /* No arguments from boot loader, use kernel's cmdl*/ > - if (!((char *)data)[0]) > - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > -#endif > -#endif /* CONFIG_CMDLINE */ > - > + early_init_dt_retrieve_cmdline(node, data); > pr_debug("Command line is: %s\n", (char *)data); > > rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l); > -- > 2.30.1.766.gb4fecdf3b7-goog > > Other than the above nit: Reviewed-by: Marc Zyngier <maz@kernel.org> Thanks, M.
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index dcc1dd96911a..83b9d065e58d 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -1033,11 +1033,48 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname, return 0; } +static int __init cmdline_from_bootargs(unsigned long node, void *dst, int sz) +{ + int l; + const char *p = of_get_flat_dt_prop(node, "bootargs", &l); + + if (!p || l <= 0) + return -EINVAL; + + return strlcpy(dst, p, min(l, sz)); +} + +/* dst is a zero-initialised buffer of COMMAND_LINE_SIZE bytes */ +static void __init early_init_dt_retrieve_cmdline(unsigned long node, char *dst) +{ + if (IS_ENABLED(CONFIG_CMDLINE_EXTEND)) { + /* Copy CONFIG_CMDLINE to the start of destination buffer */ + size_t idx = strlcpy(dst, CONFIG_CMDLINE, COMMAND_LINE_SIZE); + + /* Check that we have enough space to concatenate */ + if (idx + 1 >= COMMAND_LINE_SIZE) + return; + + /* Append the bootloader arguments */ + dst[idx++] = ' '; + cmdline_from_bootargs(node, &dst[idx], COMMAND_LINE_SIZE - idx); + } else if (IS_ENABLED(CONFIG_CMDLINE_FORCE)) { + /* Just use CONFIG_CMDLINE */ + strlcpy(dst, CONFIG_CMDLINE, COMMAND_LINE_SIZE); + } else if (IS_ENABLED(CONFIG_CMDLINE_FROM_BOOTLOADER)) { + /* Use CONFIG_CMDLINE if no arguments from bootloader. */ + if (cmdline_from_bootargs(node, dst, COMMAND_LINE_SIZE) <= 0) + strlcpy(dst, CONFIG_CMDLINE, COMMAND_LINE_SIZE); + } else { + /* Just use bootloader arguments */ + cmdline_from_bootargs(node, dst, COMMAND_LINE_SIZE); + } +} + int __init early_init_dt_scan_chosen(unsigned long node, const char *uname, int depth, void *data) { int l; - const char *p; const void *rng_seed; pr_debug("search \"chosen\", depth: %d, uname: %s\n", depth, uname); @@ -1047,30 +1084,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname, return 0; early_init_dt_check_for_initrd(node); - - /* Retrieve command line */ - p = of_get_flat_dt_prop(node, "bootargs", &l); - if (p != NULL && l > 0) - strlcpy(data, 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(data, " ", COMMAND_LINE_SIZE); - strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE); -#elif defined(CONFIG_CMDLINE_FORCE) - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE); -#else - /* No arguments from boot loader, use kernel's cmdl*/ - if (!((char *)data)[0]) - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE); -#endif -#endif /* CONFIG_CMDLINE */ - + early_init_dt_retrieve_cmdline(node, data); pr_debug("Command line is: %s\n", (char *)data); rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
The Kconfig help text for CMDLINE_EXTEND is sadly duplicated across all architectures that implement it (arm, arm64, powerpc, riscv and sh), but they all seem to agree that the bootloader arguments will be appended to the CONFIG_CMDLINE. For example, on arm64: | The command-line arguments provided by the boot loader will be | appended to the default kernel command string. This also matches the behaviour of the EFI stub, which parses the bootloader arguments first if CMDLINE_EXTEND is set, as well as the out-of-tree CMDLINE_EXTEND implementation in Android. However, the behaviour in the upstream fdt code appears to be the other way around: CONFIG_CMDLINE is appended to the bootloader arguments. Fix the code to follow the documentation by moving the cmdline processing out into a new function, early_init_dt_retrieve_cmdline(), and copying CONFIG_CMDLINE to the beginning of the cmdline buffer rather than concatenating it onto the end. Cc: Max Uvarov <muvarov@gmail.com> Cc: Rob Herring <robh@kernel.org> Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Marc Zyngier <maz@kernel.org> Cc: Doug Anderson <dianders@chromium.org> Cc: Tyler Hicks <tyhicks@linux.microsoft.com> Cc: Frank Rowand <frowand.list@gmail.com> Fixes: 34b82026a507 ("fdt: fix extend of cmd line") Signed-off-by: Will Deacon <will@kernel.org> --- drivers/of/fdt.c | 64 +++++++++++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 25 deletions(-)