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

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

[Maksym Kokhan]

From: Daniel Walker <danielwa@cisco.com>

This updates the mips code to use the CONFIG_GENERIC_CMDLINE
option.

[maksym.kokhan@globallogic.com: remove new mips arch-specific
command line implementation]
Cc: Daniel Walker <dwalker@fifo99.com>
Cc: Daniel Walker <danielwa@cisco.com>
Signed-off-by: Daniel Walker <danielwa@cisco.com>
Signed-off-by: Maksym Kokhan <maksym.kokhan@globallogic.com>
---
 arch/mips/Kconfig        | 24 +-----------------------
 arch/mips/Kconfig.debug  | 47 -----------------------------------------------
 arch/mips/kernel/setup.c | 41 +++--------------------------------------
 3 files changed, 4 insertions(+), 108 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 3551199..642e31b 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -78,6 +78,7 @@ config MIPS
 	select RTC_LIB if !MACH_LOONGSON64
 	select SYSCTL_EXCEPTION_TRACE
 	select VIRT_TO_BUS
+	select GENERIC_CMDLINE
 
 menu "Machine selection"
 
@@ -2942,29 +2943,6 @@ choice
 		  if you don't intend to always append a DTB.
 endchoice
 
-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
-
 endmenu
 
 config LOCKDEP_SUPPORT
diff --git a/arch/mips/Kconfig.debug b/arch/mips/Kconfig.debug
index 0c86b2a..bcf11c2 100644
--- a/arch/mips/Kconfig.debug
+++ b/arch/mips/Kconfig.debug
@@ -30,53 +30,6 @@ config EARLY_PRINTK_8250
 config USE_GENERIC_EARLY_PRINTK_8250
 	bool
 
-config CMDLINE_BOOL
-	bool "Built-in kernel command line"
-	default n
-	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.
-
-config CMDLINE
-	string "Default kernel command string"
-	depends on CMDLINE_BOOL
-	default ""
-	help
-	  On some platforms, there is currently no way for the boot loader to
-	  pass arguments to the kernel.  For these platforms, and for the cases
-	  when you want to add some extra options to the command line or ignore
-	  the default command line, you can supply some command-line options at
-	  build time by entering them here.  In other cases you can specify
-	  kernel args so that you don't have to set them up in board prom
-	  initialization routines.
-
-	  For more information, see the CMDLINE_BOOL and CMDLINE_OVERRIDE
-	  options.
-
-config CMDLINE_OVERRIDE
-	bool "Built-in command line overrides firmware arguments"
-	default n
-	depends on CMDLINE_BOOL
-	help
-	  By setting this option to 'Y' you will have your kernel ignore
-	  command line arguments from firmware or second stage bootloader.
-	  Instead, the built-in command line will be used exclusively.
-
-	  Normally, you will choose 'N' here.
-
 config SB1XXX_CORELIS
 	bool "Corelis Debugger"
 	depends on SIBYTE_SB1xxx_SOC
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index c71d1eb..60638dd 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -28,6 +28,7 @@
 #include <linux/dma-contiguous.h>
 #include <linux/decompress/generic.h>
 #include <linux/of_fdt.h>
+#include <linux/cmdline.h>
 
 #include <asm/addrspace.h>
 #include <asm/bootinfo.h>
@@ -68,10 +69,6 @@ struct boot_mem_map boot_mem_map;
 static char __initdata command_line[COMMAND_LINE_SIZE];
 char __initdata arcs_cmdline[COMMAND_LINE_SIZE];
 
-#ifdef CONFIG_CMDLINE_BOOL
-static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
-#endif
-
 /*
  * mips_io_port_base is the begin of the address space to which x86 style
  * I/O ports are mapped.
@@ -835,45 +832,11 @@ static void __init request_crashkernel(struct resource *res)
 }
 #endif /* !defined(CONFIG_KEXEC)  */
 
-#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;
-- 
2.7.4

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

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

[Maksym Kokhan]

Hi, Paul,

On Thu, Sep 27, 2018 at 9:56 PM Paul Burton <paul.burton@mips.com> wrote:
>
> 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).

You are right, there is a mistake in our implementation, thank you for
your observation. This bug can be easily fixed in 2 ways.
First - modify mips-specific code adding if statement with
processing bootloader cmdline:
------------------------------8<-----------------------------------

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 60638dd..7d11ef5 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -856,7 +856,11 @@ 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);
+       if (arcs_cmdline[0]) {
+               strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
+               strlcat(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
+       }
+       cmdline_add_builtin(boot_command_line, NULL, COMMAND_LINE_SIZE);

        strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);

------------------------------8<-----------------------------------
This solution was tested under QEMU using malta emulation and it
works fine. Another way is to modify common code in
include/linux/cmdline.h, but I am not sure, which approach is better.

> 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.

Yes, this generic command line implementation lacks some of the
features, existing in the current mips command line code, and we
are going to expand functionality of generic command line code to
correspond it, but it would be easier to initially merge this simple
implementation and then develop it step by step.

Thanks,
Maksym

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

[Paul Burton]

Hi Maksym,

On Mon, Oct 08, 2018 at 08:56:25PM +0300, Maksym Kokhan wrote:
> On Thu, Sep 27, 2018 at 9:56 PM Paul Burton <paul.burton@mips.com> wrote:
> > 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.
> 
> Yes, this generic command line implementation lacks some of the
> features, existing in the current mips command line code, and we
> are going to expand functionality of generic command line code to
> correspond it, but it would be easier to initially merge this simple
> implementation and then develop it step by step.

The problem occurs if merging the simple implementation breaks currently
working systems. That is a no-go, and that is what I believe will happen
with the current patchset.

"Knowingly break it now & say we'll fix it later" is not an acceptable
approach.

Thanks,
    Paul