From: Marc Zyngier <maz@kernel.org>
To: Will Deacon <will@kernel.org>
Cc: Rob Herring <robh@kernel.org>,
kernel-team@android.com, Arnd Bergmann <arnd@arndb.de>,
devicetree@vger.kernel.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Doug Anderson <dianders@chromium.org>,
linux-kernel@vger.kernel.org,
Tyler Hicks <tyhicks@linux.microsoft.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Max Uvarov <muvarov@gmail.com>,
Frank Rowand <frowand.list@gmail.com>,
Ard Biesheuvel <ardb@kernel.org>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/2] of/fdt: Append bootloader arguments when CMDLINE_EXTEND=y
Date: Thu, 25 Feb 2021 14:08:11 +0000 [thread overview]
Message-ID: <87y2fcz1dg.wl-maz@kernel.org> (raw)
In-Reply-To: <20210225125921.13147-3-will@kernel.org>
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.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-02-25 14:09 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-25 12:59 [PATCH 0/2] Fix CMDLINE_EXTEND handling for FDT "bootargs" Will Deacon
2021-02-25 12:59 ` [PATCH 1/2] arm64: cpufeatures: Fix handling of CONFIG_CMDLINE for idreg overrides Will Deacon
2021-02-25 13:53 ` Marc Zyngier
2021-02-25 14:04 ` Will Deacon
2021-02-25 12:59 ` [PATCH 2/2] of/fdt: Append bootloader arguments when CMDLINE_EXTEND=y Will Deacon
2021-02-25 14:08 ` Marc Zyngier [this message]
2021-03-01 14:19 ` [PATCH 0/2] Fix CMDLINE_EXTEND handling for FDT "bootargs" Rob Herring
2021-03-01 14:41 ` Will Deacon
2021-03-01 17:26 ` Rob Herring
2021-03-01 17:45 ` Christophe Leroy
2021-03-02 14:56 ` Rob Herring
2021-03-02 15:16 ` Christophe Leroy
2021-03-02 17:12 ` Daniel Walker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y2fcz1dg.wl-maz@kernel.org \
--to=maz@kernel.org \
--cc=ardb@kernel.org \
--cc=arnd@arndb.de \
--cc=catalin.marinas@arm.com \
--cc=devicetree@vger.kernel.org \
--cc=dianders@chromium.org \
--cc=frowand.list@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=kernel-team@android.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=muvarov@gmail.com \
--cc=palmer@dabbelt.com \
--cc=robh@kernel.org \
--cc=tyhicks@linux.microsoft.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).