Re: [PATCH v2 6/7] cmdline: Gives architectures opportunity to use generically defined boot cmdline manipulation

[Will Deacon <will@kernel.org>]

On Tue, Mar 02, 2021 at 05:25:22PM +0000, Christophe Leroy wrote:
> Most architectures have similar boot command line manipulation
> options. This patchs adds the definition in init/Kconfig, gated by
> CONFIG_HAVE_CMDLINE that the architectures can select to use them.
> 
> In order to use this, a few architectures will have to change their
> CONFIG options:
> - riscv has to replace CMDLINE_FALLBACK by CMDLINE_FROM_BOOTLOADER
> - architectures using CONFIG_CMDLINE_OVERRIDE or
> CONFIG_CMDLINE_OVERWRITE have to replace them by CONFIG_CMDLINE_FORCE.
> 
> Architectures also have to define CONFIG_DEFAULT_CMDLINE.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  init/Kconfig | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index 22946fe5ded9..a0f2ad9467df 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -117,6 +117,62 @@ config INIT_ENV_ARG_LIMIT
>  	  Maximum of each of the number of arguments and environment
>  	  variables passed to init from the kernel command line.
>  
> +config HAVE_CMDLINE
> +	bool
> +
> +config CMDLINE_BOOL
> +	bool "Default bootloader kernel arguments"
> +	depends on HAVE_CMDLINE
> +	help
> +	  On some platforms, there is currently no way for the boot loader to
> +	  pass arguments to the kernel. For these platforms, you can supply
> +	  some command-line options at build time by entering them here.  In
> +	  most cases you will need to specify the root device here.

Why is this needed as well as CMDLINE_FROM_BOOTLOADER? IIUC, the latter
will use CONFIG_CMDLINE if it fails to get anything from the bootloader,
which sounds like the same scenario.

> +config CMDLINE
> +	string "Initial kernel command string"

s/Initial/Default

which is then consistent with the rest of the text here.

> +	depends on CMDLINE_BOOL

Ah, so this is a bit different and I don't think lines-up with the
CMDLINE_BOOL help text.

> +	default DEFAULT_CMDLINE
> +	help
> +	  On some platforms, there is currently no way for the boot loader to
> +	  pass arguments to the kernel. For these platforms, you can supply
> +	  some command-line options at build time by entering them here.  In
> +	  most cases you will need to specify the root device here.

(same stale text)

> +choice
> +	prompt "Kernel command line type" if CMDLINE != ""
> +	default CMDLINE_FROM_BOOTLOADER
> +	help
> +	  Selects the way you want to use the default kernel arguments.

How about:

"Determines how the default kernel arguments are combined with any
 arguments passed by the bootloader"

> +config CMDLINE_FROM_BOOTLOADER
> +	bool "Use bootloader kernel arguments if available"
> +	help
> +	  Uses the command-line options passed by the boot loader. If
> +	  the boot loader doesn't provide any, the default kernel command
> +	  string provided in CMDLINE will be used.
> +
> +config CMDLINE_EXTEND

Can we rename this to CMDLINE_APPEND, please? There is code in the tree
which disagrees about what CMDLINE_EXTEND means, so that will need be
to be updated to be consistent (e.g. the EFI stub parsing order). Having
the generic option with a different name means we won't accidentally end
up with the same inconsistent behaviours.

> +	bool "Extend bootloader kernel arguments"

"Append to the bootloader kernel arguments"

> +	help
> +	  The default kernel command string will be appended to the
> +	  command-line arguments provided during boot.

s/provided during boot/provided by the bootloader/

> +
> +config CMDLINE_PREPEND
> +	bool "Prepend bootloader kernel arguments"

"Prepend to the bootloader kernel arguments"

> +	help
> +	  The default kernel command string will be prepend to the
> +	  command-line arguments provided during boot.

s/prepend/prepended/
s/provided during boot/provided by the bootloader/

> +
> +config CMDLINE_FORCE
> +	bool "Always use the default kernel command string"
> +	help
> +	  Always use the default kernel command string, even if the boot
> +	  loader passes other arguments to the kernel.
> +	  This is useful if you cannot or don't want to change the
> +	  command-line options your boot loader passes to the kernel.

I find the "This is useful if ..." sentence really confusing, perhaps just
remove it? I'd then tweak it to be:

  "Always use the default kernel command string, ignoring any arguments
   provided by the bootloader."

Will