On Thu, 25 Feb 2021 12:59:21 +0000,
Will Deacon <will@xxxxxxxxxx> 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@xxxxxxxxx>
> Cc: Rob Herring <robh@xxxxxxxxxx>
> Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: Doug Anderson <dianders@xxxxxxxxxxxx>
> Cc: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx>
> Cc: Frank Rowand <frowand.list@xxxxxxxxx>
> Fixes: 34b82026a507 ("fdt: fix extend of cmd line")
> Signed-off-by: Will Deacon <will@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
