mbox series

[v4,00/20] Implement GENERIC_CMDLINE

Message ID cover.1617375802.git.christophe.leroy@csgroup.eu
Headers show
Series Implement GENERIC_CMDLINE | expand

Message

Christophe Leroy April 2, 2021, 3:18 p.m. UTC
The purpose of this series is to improve and enhance the
handling of kernel boot arguments.

Current situation is that most if not all architectures are using
similar options to do some manupulation on command line arguments:
- Prepend built-in arguments in front of bootloader provided arguments
- Append built-in arguments after bootloader provided arguments
- Replace bootloader provided arguments by built-in arguments
- Use built-in arguments when none is provided by bootloader.

On some architectures, all the options are possible. On other ones,
only a subset are available.

The purpose of this series is to refactor and enhance the
handling of kernel boot arguments so that every architecture can
benefit from all possibilities.

It is first focussed on powerpc but also extends the capability
for other arches.

The work has been focussed on minimising the churn in architectures
by keeping the most commonly used namings.

Main changes in V4:
- Included patch from Daniel to replace powerpc's strcpy() by strlcpy()
- Using strlcpy() instead of zeroing first char + strlcat() (idea taken frm Daniel's series)
- Reworked the convertion of EFI which was wrong in V3
- Added "too long" command line handling
- Changed cmdline macro into a function
- Done a few fixes in arch (NIOS2, SH, ARM)
- Taken comments into account (see individual responses for details)
- Tested on powerpc, build tested on ARM64, X86_64.

Main changes in V3:
- Also accept destination equal to source in cmdline_build() by setting a tmp buffer in __initdata. Powerpc provides different source and destination and call __cmdline_build() directly.
- Taken comments received from Will and Rob
- Converted all architectures (Only tested on powerpc)

Christophe Leroy (19):
  cmdline: Add generic function to build command line.
  drivers: of: use cmdline building function
  x86/efi: Replace CONFIG_CMDLINE_OVERRIDE by CONFIG_CMDLINE_FORCE
  drivers: firmware: efi: use cmdline building function
  cmdline: Gives architectures opportunity to use generically defined
    boot cmdline manipulation
  powerpc: Convert to GENERIC_CMDLINE
  arm: Convert to GENERIC_CMDLINE
  arm64: Convert to GENERIC_CMDLINE
  hexagon: Convert to GENERIC_CMDLINE
  microblaze: Convert to GENERIC_CMDLINE
  nios2: Convert to GENERIC_CMDLINE
  openrisc: Convert to GENERIC_CMDLINE
  riscv: Convert to GENERIC_CMDLINE
  sh: Convert to GENERIC_CMDLINE
  sparc: Convert to GENERIC_CMDLINE
  xtensa: Convert to GENERIC_CMDLINE
  x86: Convert to GENERIC_CMDLINE
  mips: Convert to GENERIC_CMDLINE
  cmdline: Remove CONFIG_CMDLINE_EXTEND

Daniel Walker (1):
  powerpc: convert strcpy to strlcpy in prom_init

 arch/arm/Kconfig                              | 38 +--------
 arch/arm/kernel/atags_parse.c                 | 13 +--
 arch/arm64/Kconfig                            | 33 +-------
 arch/arm64/kernel/idreg-override.c            |  9 +--
 arch/hexagon/Kconfig                          | 11 +--
 arch/hexagon/kernel/setup.c                   | 10 +--
 arch/microblaze/Kconfig                       | 24 +-----
 arch/microblaze/configs/mmu_defconfig         |  2 +-
 arch/microblaze/kernel/head.S                 |  4 +-
 arch/mips/Kconfig                             |  1 +
 arch/mips/Kconfig.debug                       | 44 -----------
 arch/mips/configs/ar7_defconfig               |  1 -
 arch/mips/configs/bcm47xx_defconfig           |  1 -
 arch/mips/configs/bcm63xx_defconfig           |  1 -
 arch/mips/configs/bmips_be_defconfig          |  1 -
 arch/mips/configs/bmips_stb_defconfig         |  1 -
 arch/mips/configs/capcella_defconfig          |  1 -
 arch/mips/configs/ci20_defconfig              |  1 -
 arch/mips/configs/cu1000-neo_defconfig        |  1 -
 arch/mips/configs/cu1830-neo_defconfig        |  1 -
 arch/mips/configs/e55_defconfig               |  1 -
 arch/mips/configs/generic_defconfig           |  1 -
 arch/mips/configs/gpr_defconfig               |  1 -
 arch/mips/configs/loongson3_defconfig         |  1 -
 arch/mips/configs/mpc30x_defconfig            |  1 -
 arch/mips/configs/rt305x_defconfig            |  1 -
 arch/mips/configs/tb0219_defconfig            |  1 -
 arch/mips/configs/tb0226_defconfig            |  1 -
 arch/mips/configs/tb0287_defconfig            |  1 -
 arch/mips/configs/workpad_defconfig           |  1 -
 arch/mips/configs/xway_defconfig              |  1 -
 arch/mips/kernel/relocate.c                   |  4 +-
 arch/mips/kernel/setup.c                      | 40 +---------
 arch/mips/pic32/pic32mzda/early_console.c     |  2 +-
 arch/mips/pic32/pic32mzda/init.c              |  2 -
 arch/nios2/Kconfig                            | 25 +-----
 arch/nios2/kernel/setup.c                     | 13 +--
 arch/openrisc/Kconfig                         | 10 +--
 arch/powerpc/Kconfig                          | 37 +--------
 arch/powerpc/kernel/prom_init.c               | 46 ++++++-----
 arch/riscv/Kconfig                            | 44 +----------
 arch/riscv/kernel/setup.c                     |  7 +-
 arch/sh/Kconfig                               | 28 +------
 arch/sh/configs/ap325rxa_defconfig            |  2 +-
 arch/sh/configs/dreamcast_defconfig           |  2 +-
 arch/sh/configs/ecovec24-romimage_defconfig   |  2 +-
 arch/sh/configs/ecovec24_defconfig            |  2 +-
 arch/sh/configs/edosk7760_defconfig           |  2 +-
 arch/sh/configs/espt_defconfig                |  2 +-
 arch/sh/configs/j2_defconfig                  |  2 +-
 arch/sh/configs/kfr2r09-romimage_defconfig    |  2 +-
 arch/sh/configs/kfr2r09_defconfig             |  2 +-
 arch/sh/configs/lboxre2_defconfig             |  2 +-
 arch/sh/configs/microdev_defconfig            |  2 +-
 arch/sh/configs/migor_defconfig               |  2 +-
 arch/sh/configs/polaris_defconfig             |  2 +-
 arch/sh/configs/r7780mp_defconfig             |  2 +-
 arch/sh/configs/r7785rp_defconfig             |  2 +-
 arch/sh/configs/rsk7201_defconfig             |  2 +-
 arch/sh/configs/rsk7203_defconfig             |  2 +-
 arch/sh/configs/rts7751r2d1_defconfig         |  2 +-
 arch/sh/configs/rts7751r2dplus_defconfig      |  2 +-
 arch/sh/configs/sdk7780_defconfig             |  2 +-
 arch/sh/configs/sdk7786_defconfig             |  2 +-
 arch/sh/configs/se7206_defconfig              |  2 +-
 arch/sh/configs/se7343_defconfig              |  2 +-
 arch/sh/configs/se7712_defconfig              |  2 +-
 arch/sh/configs/se7721_defconfig              |  2 +-
 arch/sh/configs/se7724_defconfig              |  2 +-
 arch/sh/configs/se7751_defconfig              |  2 +-
 arch/sh/configs/se7780_defconfig              |  2 +-
 arch/sh/configs/sh03_defconfig                |  2 +-
 arch/sh/configs/sh2007_defconfig              |  2 +-
 arch/sh/configs/sh7757lcr_defconfig           |  2 +-
 arch/sh/configs/sh7763rdp_defconfig           |  2 +-
 arch/sh/configs/shmin_defconfig               |  2 +-
 arch/sh/configs/shx3_defconfig                |  2 +-
 arch/sh/configs/titan_defconfig               |  2 +-
 arch/sh/configs/ul2_defconfig                 |  2 +-
 arch/sh/kernel/setup.c                        | 11 +--
 arch/sparc/Kconfig                            | 18 +----
 arch/sparc/prom/bootstr_64.c                  |  2 +-
 arch/x86/Kconfig                              | 45 +----------
 arch/x86/kernel/setup.c                       | 17 +---
 arch/xtensa/Kconfig                           | 15 +---
 arch/xtensa/configs/audio_kc705_defconfig     |  1 -
 arch/xtensa/configs/common_defconfig          |  1 -
 arch/xtensa/configs/generic_kc705_defconfig   |  1 -
 arch/xtensa/configs/iss_defconfig             |  1 -
 arch/xtensa/configs/nommu_kc705_defconfig     |  1 -
 arch/xtensa/configs/smp_lx200_defconfig       |  1 -
 arch/xtensa/configs/virt_defconfig            |  1 -
 arch/xtensa/configs/xip_kc705_defconfig       |  1 -
 arch/xtensa/kernel/setup.c                    | 10 +--
 .../firmware/efi/libstub/efi-stub-helper.c    | 35 ++++----
 drivers/firmware/efi/libstub/efi-stub.c       | 23 ++----
 drivers/firmware/efi/libstub/efistub.h        |  2 +-
 drivers/firmware/efi/libstub/x86-stub.c       | 18 +----
 drivers/of/fdt.c                              | 23 +-----
 include/linux/cmdline.h                       | 79 +++++++++++++++++++
 init/Kconfig                                  | 46 +++++++++++
 usr/Kconfig                                   |  2 +-
 102 files changed, 265 insertions(+), 628 deletions(-)
 create mode 100644 include/linux/cmdline.h

Comments

Daniel Walker (danielwa) April 6, 2021, 4:56 p.m. UTC | #1
On Fri, Apr 02, 2021 at 03:18:01PM +0000, Christophe Leroy wrote:
> The purpose of this series is to improve and enhance the
> handling of kernel boot arguments.
> 
> Current situation is that most if not all architectures are using
> similar options to do some manupulation on command line arguments:
> - Prepend built-in arguments in front of bootloader provided arguments
> - Append built-in arguments after bootloader provided arguments
> - Replace bootloader provided arguments by built-in arguments
> - Use built-in arguments when none is provided by bootloader.
> 
> On some architectures, all the options are possible. On other ones,
> only a subset are available.
> 
> The purpose of this series is to refactor and enhance the
> handling of kernel boot arguments so that every architecture can
> benefit from all possibilities.
> 
> It is first focussed on powerpc but also extends the capability
> for other arches.
> 
> The work has been focussed on minimising the churn in architectures
> by keeping the most commonly used namings.
> 
> Main changes in V4:
> - Included patch from Daniel to replace powerpc's strcpy() by strlcpy()
> - Using strlcpy() instead of zeroing first char + strlcat() (idea taken frm Daniel's series)
> - Reworked the convertion of EFI which was wrong in V3
> - Added "too long" command line handling
> - Changed cmdline macro into a function
> - Done a few fixes in arch (NIOS2, SH, ARM)
> - Taken comments into account (see individual responses for details)
> - Tested on powerpc, build tested on ARM64, X86_64.
> 

Why submit your changes ? My changes have been around for almost 10 years, and
are more widely used. Your changes are very new and unstable, but don't really
solve the needs of people using my series.

I've tried to work with you and I take comments from you, but yet you insist to
submit your own series.

I would suggest this isn't going to go anyplace unless we work together.

I can't really support your changes because, honestly, your changes are really
ugly and they just look more and more like my changes with every passing
iteration .. As the maturity of your changes continue they will just become my
change set.

I've been thru every iteration of these changes, and I see those attempts in
your changes. Everything different in your changes I've tried, and found not to
be useful, then it falls away in later iterations.

When you give me comments on something which I haven't tried I typically
incorporate it.

Daniel
Daniel Walker (danielwa) April 6, 2021, 5:38 p.m. UTC | #2
On Fri, Apr 02, 2021 at 03:18:21PM +0000, Christophe Leroy wrote:
> -config CMDLINE_BOOL
> -	bool "Built-in kernel command line"
> -	help
> -	  For most systems, it is firmware or second stage bootloader that
> -	  by default specifies the kernel command line options.  However,
> -	  it might be necessary or advantageous to either override the
> -	  default kernel command line or add a few extra options to it.
> -	  For such cases, this option allows you to hardcode your own
> -	  command line options directly into the kernel.  For that, you
> -	  should choose 'Y' here, and fill in the extra boot arguments
> -	  in CONFIG_CMDLINE.
> -
> -	  The built-in options will be concatenated to the default command
> -	  line if CMDLINE_OVERRIDE is set to 'N'. Otherwise, the default
> -	  command line will be ignored and replaced by the built-in string.
> -
> -	  Most MIPS systems will normally expect 'N' here and rely upon
> -	  the command line from the firmware or the second-stage bootloader.
> -


See how you complained that I have CMDLINE_BOOL in my changed, and you think it
shouldn't exist.

Yet here mips has it, and you just deleted it with no feature parity in your
changes for this.

In my changes I tried to maintain as much feature parity as I could with the
architectures. I did the same huge conversion a long time ago you've done here to be sure all
platforms have the features needed.

Daniel
Rob Herring April 8, 2021, 7:04 p.m. UTC | #3
On Tue, Apr 06, 2021 at 10:38:36AM -0700, Daniel Walker wrote:
> On Fri, Apr 02, 2021 at 03:18:21PM +0000, Christophe Leroy wrote:
> > -config CMDLINE_BOOL
> > -	bool "Built-in kernel command line"
> > -	help
> > -	  For most systems, it is firmware or second stage bootloader that
> > -	  by default specifies the kernel command line options.  However,
> > -	  it might be necessary or advantageous to either override the
> > -	  default kernel command line or add a few extra options to it.
> > -	  For such cases, this option allows you to hardcode your own
> > -	  command line options directly into the kernel.  For that, you
> > -	  should choose 'Y' here, and fill in the extra boot arguments
> > -	  in CONFIG_CMDLINE.
> > -
> > -	  The built-in options will be concatenated to the default command
> > -	  line if CMDLINE_OVERRIDE is set to 'N'. Otherwise, the default
> > -	  command line will be ignored and replaced by the built-in string.
> > -
> > -	  Most MIPS systems will normally expect 'N' here and rely upon
> > -	  the command line from the firmware or the second-stage bootloader.
> > -
> 
> 
> See how you complained that I have CMDLINE_BOOL in my changed, and you think it
> shouldn't exist.
> 
> Yet here mips has it, and you just deleted it with no feature parity in your
> changes for this.

AFAICT, CMDLINE_BOOL equates to a non-empty or empty CONFIG_CMDLINE. You 
seem to need it just because you have CMDLINE_PREPEND and 
CMDLINE_APPEND. If that's not it, what feature is missing? CMDLINE_BOOL 
is not a feature, but an implementation detail.

Rob
Rob Herring April 8, 2021, 7:41 p.m. UTC | #4
On Fri, Apr 02, 2021 at 03:18:20PM +0000, Christophe Leroy wrote:
> This converts the architecture to GENERIC_CMDLINE.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/x86/Kconfig        | 45 ++---------------------------------------
>  arch/x86/kernel/setup.c | 17 ++--------------
>  2 files changed, 4 insertions(+), 58 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index a20684d56b4b..66b384228ca3 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -104,6 +104,7 @@ config X86
>  	select ARCH_USE_QUEUED_SPINLOCKS
>  	select ARCH_USE_SYM_ANNOTATIONS
>  	select ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
> +	select ARCH_WANT_CMDLINE_PREPEND_BY_DEFAULT

Seems to be non-existent kconfig option.

>  	select ARCH_WANT_DEFAULT_BPF_JIT	if X86_64
>  	select ARCH_WANTS_DYNAMIC_TASK_STRUCT
>  	select ARCH_WANT_HUGE_PMD_SHARE
> @@ -118,6 +119,7 @@ config X86
>  	select EDAC_SUPPORT
>  	select GENERIC_CLOCKEVENTS_BROADCAST	if X86_64 || (X86_32 && X86_LOCAL_APIC)
>  	select GENERIC_CLOCKEVENTS_MIN_ADJUST
> +	select GENERIC_CMDLINE
>  	select GENERIC_CMOS_UPDATE
>  	select GENERIC_CPU_AUTOPROBE
>  	select GENERIC_CPU_VULNERABILITIES
> @@ -2358,49 +2360,6 @@ choice
>  
>  endchoice
>  
> -config CMDLINE_BOOL
> -	bool "Built-in kernel command line"
> -	help
> -	  Allow for specifying boot arguments to the kernel at
> -	  build time.  On some systems (e.g. embedded ones), it is
> -	  necessary or convenient to provide some or all of the
> -	  kernel boot arguments with the kernel itself (that is,
> -	  to not rely on the boot loader to provide them.)
> -
> -	  To compile command line arguments into the kernel,
> -	  set this option to 'Y', then fill in the
> -	  boot arguments in CONFIG_CMDLINE.
> -
> -	  Systems with fully functional boot loaders (i.e. non-embedded)
> -	  should leave this option set to 'N'.
> -
> -config CMDLINE
> -	string "Built-in kernel command string"
> -	depends on CMDLINE_BOOL
> -	default ""
> -	help
> -	  Enter arguments here that should be compiled into the kernel
> -	  image and used at boot time.  If the boot loader provides a
> -	  command line at boot time, it is appended to this string to
> -	  form the full kernel command line, when the system boots.
> -
> -	  However, you can use the CONFIG_CMDLINE_FORCE option to
> -	  change this behavior.
> -
> -	  In most cases, the command line (whether built-in or provided
> -	  by the boot loader) should specify the device for the root
> -	  file system.
> -
> -config CMDLINE_FORCE
> -	bool "Built-in command line overrides boot loader arguments"
> -	depends on CMDLINE_BOOL && CMDLINE != ""
> -	help
> -	  Set this option to 'Y' to have the kernel ignore the boot loader
> -	  command line, and use ONLY the built-in command line.
> -
> -	  This is used to work around broken boot loaders.  This should
> -	  be set to 'N' under normal conditions.
> -
>  config MODIFY_LDT_SYSCALL
>  	bool "Enable the LDT (local descriptor table)" if EXPERT
>  	default y
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 6f2de58eeb54..3f274b02e51c 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -5,6 +5,7 @@
>   * This file contains the setup_arch() code, which handles the architecture-dependent
>   * parts of early kernel initialization.
>   */
> +#include <linux/cmdline.h>
>  #include <linux/console.h>
>  #include <linux/crash_dump.h>
>  #include <linux/dma-map-ops.h>
> @@ -161,9 +162,6 @@ unsigned long saved_video_mode;
>  #define RAMDISK_LOAD_FLAG		0x4000
>  
>  static char __initdata command_line[COMMAND_LINE_SIZE];
> -#ifdef CONFIG_CMDLINE_BOOL
> -static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
> -#endif
>  
>  #if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
>  struct edd edd;
> @@ -883,18 +881,7 @@ void __init setup_arch(char **cmdline_p)
>  	bss_resource.start = __pa_symbol(__bss_start);
>  	bss_resource.end = __pa_symbol(__bss_stop)-1;
>  
> -#ifdef CONFIG_CMDLINE_BOOL
> -#ifdef CONFIG_CMDLINE_FORCE
> -	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> -#else
> -	if (builtin_cmdline[0]) {
> -		/* append boot loader cmdline to builtin */
> -		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> -		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> -		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> -	}
> -#endif
> -#endif
> +	cmdline_build(boot_command_line, boot_command_line);
>  
>  	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
>  	*cmdline_p = command_line;

Once this is all done, I wonder if we can get rid of the strlcpy and 
perhaps also cmdline_p.

Rob
Daniel Walker (danielwa) April 9, 2021, 1:23 a.m. UTC | #5
On Thu, Apr 08, 2021 at 02:04:08PM -0500, Rob Herring wrote:
> On Tue, Apr 06, 2021 at 10:38:36AM -0700, Daniel Walker wrote:
> > On Fri, Apr 02, 2021 at 03:18:21PM +0000, Christophe Leroy wrote:
> > > -config CMDLINE_BOOL
> > > -	bool "Built-in kernel command line"
> > > -	help
> > > -	  For most systems, it is firmware or second stage bootloader that
> > > -	  by default specifies the kernel command line options.  However,
> > > -	  it might be necessary or advantageous to either override the
> > > -	  default kernel command line or add a few extra options to it.
> > > -	  For such cases, this option allows you to hardcode your own
> > > -	  command line options directly into the kernel.  For that, you
> > > -	  should choose 'Y' here, and fill in the extra boot arguments
> > > -	  in CONFIG_CMDLINE.
> > > -
> > > -	  The built-in options will be concatenated to the default command
> > > -	  line if CMDLINE_OVERRIDE is set to 'N'. Otherwise, the default
> > > -	  command line will be ignored and replaced by the built-in string.
> > > -
> > > -	  Most MIPS systems will normally expect 'N' here and rely upon
> > > -	  the command line from the firmware or the second-stage bootloader.
> > > -
> > 
> > 
> > See how you complained that I have CMDLINE_BOOL in my changed, and you think it
> > shouldn't exist.
> > 
> > Yet here mips has it, and you just deleted it with no feature parity in your
> > changes for this.
> 
> AFAICT, CMDLINE_BOOL equates to a non-empty or empty CONFIG_CMDLINE. You 
> seem to need it just because you have CMDLINE_PREPEND and 
> CMDLINE_APPEND. If that's not it, what feature is missing? CMDLINE_BOOL 
> is not a feature, but an implementation detail.

Not true.

It makes it easier to turn it all off inside the Kconfig , so it's for usability
and multiple architecture have it even with just CMDLINE as I was commenting
here.

Daniel
Christophe Leroy April 9, 2021, 10:20 a.m. UTC | #6
Le 08/04/2021 à 21:41, Rob Herring a écrit :
> On Fri, Apr 02, 2021 at 03:18:20PM +0000, Christophe Leroy wrote:
>> This converts the architecture to GENERIC_CMDLINE.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/x86/Kconfig        | 45 ++---------------------------------------
>>   arch/x86/kernel/setup.c | 17 ++--------------
>>   2 files changed, 4 insertions(+), 58 deletions(-)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index a20684d56b4b..66b384228ca3 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -104,6 +104,7 @@ config X86
>>   	select ARCH_USE_QUEUED_SPINLOCKS
>>   	select ARCH_USE_SYM_ANNOTATIONS
>>   	select ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
>> +	select ARCH_WANT_CMDLINE_PREPEND_BY_DEFAULT
> 
> Seems to be non-existent kconfig option.

Oops. Added in v5.

>> @@ -883,18 +881,7 @@ void __init setup_arch(char **cmdline_p)
>>   	bss_resource.start = __pa_symbol(__bss_start);
>>   	bss_resource.end = __pa_symbol(__bss_stop)-1;
>>   
>> -#ifdef CONFIG_CMDLINE_BOOL
>> -#ifdef CONFIG_CMDLINE_FORCE
>> -	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>> -#else
>> -	if (builtin_cmdline[0]) {
>> -		/* append boot loader cmdline to builtin */
>> -		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
>> -		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>> -		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>> -	}
>> -#endif
>> -#endif
>> +	cmdline_build(boot_command_line, boot_command_line);
>>   
>>   	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
>>   	*cmdline_p = command_line;
> 
> Once this is all done, I wonder if we can get rid of the strlcpy and
> perhaps also cmdline_p.
> 

It seems rather complicated, in init/main.c you have heavy manipulations of command lines which 
seems to be done in setup_command_line() which seems to add stuff in front of command lines, at the 
end we end up with several command lines:

/* Untouched saved command line (eg. for /proc) */
char *saved_command_line;
/* Command line for parameter parsing */
static char *static_command_line;
/* Untouched extra command line */
static char *extra_command_line;

Some of them come from the cmdline_p which others are from boot_command_line.

I think a cleanup on all that stuff would be worth it as a further step.
Christophe Leroy April 20, 2021, 4:05 p.m. UTC | #7
Le 09/04/2021 à 03:23, Daniel Walker a écrit :
> On Thu, Apr 08, 2021 at 02:04:08PM -0500, Rob Herring wrote:
>> On Tue, Apr 06, 2021 at 10:38:36AM -0700, Daniel Walker wrote:
>>> On Fri, Apr 02, 2021 at 03:18:21PM +0000, Christophe Leroy wrote:
>>>> -config CMDLINE_BOOL
>>>> -	bool "Built-in kernel command line"
>>>> -	help
>>>> -	  For most systems, it is firmware or second stage bootloader that
>>>> -	  by default specifies the kernel command line options.  However,
>>>> -	  it might be necessary or advantageous to either override the
>>>> -	  default kernel command line or add a few extra options to it.
>>>> -	  For such cases, this option allows you to hardcode your own
>>>> -	  command line options directly into the kernel.  For that, you
>>>> -	  should choose 'Y' here, and fill in the extra boot arguments
>>>> -	  in CONFIG_CMDLINE.
>>>> -
>>>> -	  The built-in options will be concatenated to the default command
>>>> -	  line if CMDLINE_OVERRIDE is set to 'N'. Otherwise, the default
>>>> -	  command line will be ignored and replaced by the built-in string.
>>>> -
>>>> -	  Most MIPS systems will normally expect 'N' here and rely upon
>>>> -	  the command line from the firmware or the second-stage bootloader.
>>>> -
>>>
>>>
>>> See how you complained that I have CMDLINE_BOOL in my changed, and you think it
>>> shouldn't exist.
>>>
>>> Yet here mips has it, and you just deleted it with no feature parity in your
>>> changes for this.
>>
>> AFAICT, CMDLINE_BOOL equates to a non-empty or empty CONFIG_CMDLINE. You
>> seem to need it just because you have CMDLINE_PREPEND and
>> CMDLINE_APPEND. If that's not it, what feature is missing? CMDLINE_BOOL
>> is not a feature, but an implementation detail.
> 
> Not true.
> 
> It makes it easier to turn it all off inside the Kconfig , so it's for usability
> and multiple architecture have it even with just CMDLINE as I was commenting
> here.
> 

Among the 13 architectures having CONFIG_CMDLINE, todayb only 6 have a CONFIG_CMDLINE_BOOL in addition:

arch/arm/Kconfig:config CMDLINE
arch/arm64/Kconfig:config CMDLINE
arch/hexagon/Kconfig:config CMDLINE
arch/microblaze/Kconfig:config CMDLINE
arch/mips/Kconfig.debug:config CMDLINE
arch/nios2/Kconfig:config CMDLINE
arch/openrisc/Kconfig:config CMDLINE
arch/powerpc/Kconfig:config CMDLINE
arch/riscv/Kconfig:config CMDLINE
arch/sh/Kconfig:config CMDLINE
arch/sparc/Kconfig:config CMDLINE
arch/x86/Kconfig:config CMDLINE
arch/xtensa/Kconfig:config CMDLINE

arch/microblaze/Kconfig:config CMDLINE_BOOL
arch/mips/Kconfig.debug:config CMDLINE_BOOL
arch/nios2/Kconfig:config CMDLINE_BOOL
arch/sparc/Kconfig:config CMDLINE_BOOL
arch/x86/Kconfig:config CMDLINE_BOOL
arch/xtensa/Kconfig:config CMDLINE_BOOL


In the begining I hesitated about the CMDLINE_BOOL, at the end I decided to go the same way as what 
is done today in the kernel for initramfs with CONFIG_INITRAMFS_SOURCE.

The problem I see within adding CONFIG_CMDLINE_BOOL for every architecture which don't have it today 
is that when doing a "make oldconfig" on their custom configs, thousands of users will loose their 
CMDLINE without notice.

When we do the other way round, removing CONFIG_CMDLINE_BOOL on the 6 architectures that have it 
today will have no impact on existing config.

Also, in order to avoid tons of #ifdefs in the code as mandated by Kernel Codying Style §21, we have 
to have CONFIG_CMDLINE defined at all time, so at the end CONFIG_CMDLINE_BOOL is really redundant 
with an empty CONFIG_CMDLINE.

Unlike you, the approach I took for my series is to minimise the impact on existing implementation 
and existing configurations as much as possible.

I know you have a different approach where you break every existing config anyway.

https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation

Christophe