Re: [PATCH 7/8] mips: convert to generic builtin command line

[Paul Burton]

Hi Maksym,

On Thu, Sep 27, 2018 at 07:56:57PM +0300, Maksym Kokhan wrote:
> -choice
> -	prompt "Kernel command line type" if !CMDLINE_OVERRIDE
> -	default MIPS_CMDLINE_FROM_DTB if USE_OF && !ATH79 && !MACH_INGENIC && \
> -					 !MIPS_MALTA && \
> -					 !CAVIUM_OCTEON_SOC
> -	default MIPS_CMDLINE_FROM_BOOTLOADER
> -
> -	config MIPS_CMDLINE_FROM_DTB
> -		depends on USE_OF
> -		bool "Dtb kernel arguments if available"
> -
> -	config MIPS_CMDLINE_DTB_EXTEND
> -		depends on USE_OF
> -		bool "Extend dtb kernel arguments with bootloader arguments"
> -
> -	config MIPS_CMDLINE_FROM_BOOTLOADER
> -		bool "Bootloader kernel arguments if available"
> -
> -	config MIPS_CMDLINE_BUILTIN_EXTEND
> -		depends on CMDLINE_BOOL
> -		bool "Extend builtin kernel arguments with bootloader arguments"
> -endchoice
>%
> -#define USE_PROM_CMDLINE	IS_ENABLED(CONFIG_MIPS_CMDLINE_FROM_BOOTLOADER)
> -#define USE_DTB_CMDLINE		IS_ENABLED(CONFIG_MIPS_CMDLINE_FROM_DTB)
> -#define EXTEND_WITH_PROM	IS_ENABLED(CONFIG_MIPS_CMDLINE_DTB_EXTEND)
> -#define BUILTIN_EXTEND_WITH_PROM	\
> -	IS_ENABLED(CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND)
> -
>  static void __init arch_mem_init(char **cmdline_p)
>  {
>  	struct memblock_region *reg;
>  	extern void plat_mem_setup(void);
>  
> -#if defined(CONFIG_CMDLINE_BOOL) && defined(CONFIG_CMDLINE_OVERRIDE)
> -	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> -#else
> -	if ((USE_PROM_CMDLINE && arcs_cmdline[0]) ||
> -	    (USE_DTB_CMDLINE && !boot_command_line[0]))
> -		strlcpy(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
> -
> -	if (EXTEND_WITH_PROM && arcs_cmdline[0]) {
> -		if (boot_command_line[0])
> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> -		strlcat(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
> -	}
> -
> -#if defined(CONFIG_CMDLINE_BOOL)
> -	if (builtin_cmdline[0]) {
> -		if (boot_command_line[0])
> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> -		strlcat(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> -	}
> -
> -	if (BUILTIN_EXTEND_WITH_PROM && arcs_cmdline[0]) {
> -		if (boot_command_line[0])
> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> -		strlcat(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
> -	}
> -#endif
> -#endif
> -
>  	/* call board setup routine */
>  	plat_mem_setup();
>  
> @@ -893,6 +856,8 @@ static void __init arch_mem_init(char **cmdline_p)
>  	pr_info("Determined physical RAM map:\n");
>  	print_memory_map();
>  
> +	cmdline_add_builtin(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
> +
>  	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
>  
>  	*cmdline_p = command_line;

I love the idea of simplifying this & sharing code with other
architectures, but unfortunately I believe the above will be problematic
for systems using arguments from device tree.

At the point you call cmdline_add_builtin we should expect that:

  - boot_command_line contains arguments from the DT, if any, and
    otherwise may contain CONFIG_CMDLINE copied there by
    early_init_dt_scan_chosen().

  - arcs_cmdline contains arguments from the bootloader, if any.

If I understand correctly you overwrite boot_command_line with the
concatenation of CONFIG_CMDLINE_PREPEND, arcs_cmdline &
CONFIG_CMDLINE_APPEND. This will clobber/lose the DT arguments.

I'd expect this to be reproducible under QEMU using its boston emulation
(ie. -M boston) and a kernel built for the generic platform that
includes boston support (eg. 64r6el_defconfig).

It also doesn't allow for the various Kconfig options which allow us to
ignore some of the sources of command line arguments, nor does it honor
the ordering that those existing options allow. In practice perhaps we
can cut down on some of this configurability anyway, but if we do that
it needs to be thought through & the commit message should describe the
changes in behaviour.

Thanks,
    Paul