Re: [PATCH 2/2] of/fdt: Append bootloader arguments when CMDLINE_EXTEND=y

[Marc Zyngier]

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.