[PATCH v2] of/fdt: implement a "merge-cmdline" property

[PATCH v2] of/fdt: implement a "merge-cmdline" property

[Daniel Gimpelevich]

Currently, "bootargs" supplied via the "chosen" node can be used only to
supply a kernel command line as a whole. No mechanism exists in DT to add
bootargs to the existing command line instead. This is needed in order to
avoid having to update the bootloader or default bootloader config when
upgrading to a DTB and kernel pair that requires bootargs not previously
needed.

One example use case is that OpenWrt currently supports four ARM devices by
means of locally applying the previously rejected edition of this patch. So
far, the patch has been used in production only on ARM, but architecture is
not a distinction in the design.

On MIPS, Commit 951d223 ("MIPS: Fix CONFIG_CMDLINE handling") currently
prevents support of such a mechanism, so I am including a workaround, in
anticipation of upcoming changes.

Signed-off-by: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us>
Fixes: 951d223 ("MIPS: Fix CONFIG_CMDLINE handling")
References: https://patchwork.linux-mips.org/patch/17659/
---
 arch/mips/kernel/setup.c | 12 ++++++++----
 drivers/of/fdt.c         |  9 +++++++--
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index ab349d2..9ce58f2 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -725,7 +725,10 @@ static void __init arch_mem_init(char **cmdline_p)
 	 * CONFIG_CMDLINE ourselves below & don't want to duplicate its
 	 * content because repeating arguments can be problematic.
 	 */
-	strlcpy(boot_command_line, " ", COMMAND_LINE_SIZE);
+	if (USE_DTB_CMDLINE)
+		strlcpy(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
+	else
+		strlcpy(boot_command_line, " ", COMMAND_LINE_SIZE);
 
 	/* call board setup routine */
 	plat_mem_setup();
@@ -753,9 +756,10 @@ static void __init arch_mem_init(char **cmdline_p)
 #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]))
+	if (USE_PROM_CMDLINE)
 		strlcpy(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
+	else if (!strcmp(boot_command_line, " "))
+		boot_command_line[0] = '\0';
 
 	if (EXTEND_WITH_PROM && arcs_cmdline[0]) {
 		if (boot_command_line[0])
@@ -764,7 +768,7 @@ static void __init arch_mem_init(char **cmdline_p)
 	}
 
 #if defined(CONFIG_CMDLINE_BOOL)
-	if (builtin_cmdline[0]) {
+	if (builtin_cmdline[0] && strcmp(boot_command_line, builtin_cmdline)) {
 		if (boot_command_line[0])
 			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
 		strlcat(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 9cdf14b..08c25eb 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1055,8 +1055,13 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
 
 	/* 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));
+	if (p != NULL && l > 0) {
+		if (!of_get_flat_dt_prop(node, "merge-cmdline", NULL))
+			*(char *)data = '\0';
+		if (*(char *)data)
+			strlcat(data, " ", COMMAND_LINE_SIZE);
+		strlcat(data, p, min(l + strlen(data), COMMAND_LINE_SIZE));
+	}
 
 	/*
 	 * CONFIG_CMDLINE is meant to be a default in case nothing else
-- 
1.9.1

Re: [PATCH v2] of/fdt: implement a "merge-cmdline" property

[Rob Herring]

On Mon, Aug 5, 2019 at 9:53 AM Daniel Gimpelevich
<daniel@gimpelevich.san-francisco.ca.us> wrote:
>
> Currently, "bootargs" supplied via the "chosen" node can be used only to
> supply a kernel command line as a whole. No mechanism exists in DT to add
> bootargs to the existing command line instead. This is needed in order to
> avoid having to update the bootloader or default bootloader config when
> upgrading to a DTB and kernel pair that requires bootargs not previously
> needed.

The DTB and kernel are not a pair. Or at least they shouldn't be. If
anything, the bootloader and DTB are a pair as those are the h/w
specific parts. If the DTB and kernel are a pair, then anything you
want to put into DTB can just be in the kernel. There's been some
attempts to rework the command line handling to be common and support
prepending and appending which could be useful here.

> One example use case is that OpenWrt currently supports four ARM devices by
> means of locally applying the previously rejected edition of this patch. So
> far, the patch has been used in production only on ARM, but architecture is
> not a distinction in the design.

Other distros support dozens of boards, so why haven't they ever
needed such a change?

> On MIPS, Commit 951d223 ("MIPS: Fix CONFIG_CMDLINE handling") currently
> prevents support of such a mechanism, so I am including a workaround, in
> anticipation of upcoming changes.

Like I said on irc, mixing this with the mess that is MIPS
bootloader-kernel interface generally and command line handling
specifically is not a path to upstream.

> Signed-off-by: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us>
> Fixes: 951d223 ("MIPS: Fix CONFIG_CMDLINE handling")
> References: https://patchwork.linux-mips.org/patch/17659/
> ---
>  arch/mips/kernel/setup.c | 12 ++++++++----
>  drivers/of/fdt.c         |  9 +++++++--
>  2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index ab349d2..9ce58f2 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -725,7 +725,10 @@ static void __init arch_mem_init(char **cmdline_p)
>          * CONFIG_CMDLINE ourselves below & don't want to duplicate its
>          * content because repeating arguments can be problematic.
>          */
> -       strlcpy(boot_command_line, " ", COMMAND_LINE_SIZE);
> +       if (USE_DTB_CMDLINE)
> +               strlcpy(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
> +       else
> +               strlcpy(boot_command_line, " ", COMMAND_LINE_SIZE);
>
>         /* call board setup routine */
>         plat_mem_setup();
> @@ -753,9 +756,10 @@ static void __init arch_mem_init(char **cmdline_p)
>  #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]))
> +       if (USE_PROM_CMDLINE)
>                 strlcpy(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
> +       else if (!strcmp(boot_command_line, " "))
> +               boot_command_line[0] = '\0';
>
>         if (EXTEND_WITH_PROM && arcs_cmdline[0]) {
>                 if (boot_command_line[0])
> @@ -764,7 +768,7 @@ static void __init arch_mem_init(char **cmdline_p)
>         }
>
>  #if defined(CONFIG_CMDLINE_BOOL)
> -       if (builtin_cmdline[0]) {
> +       if (builtin_cmdline[0] && strcmp(boot_command_line, builtin_cmdline)) {
>                 if (boot_command_line[0])
>                         strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
>                 strlcat(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 9cdf14b..08c25eb 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1055,8 +1055,13 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>
>         /* 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));
> +       if (p != NULL && l > 0) {
> +               if (!of_get_flat_dt_prop(node, "merge-cmdline", NULL))

This is worse than the original proposal because 'merge' is not clear
what it does compared to 'append' and how does 'cmdline' relate to
'bootargs'.

> +                       *(char *)data = '\0';
> +               if (*(char *)data)
> +                       strlcat(data, " ", COMMAND_LINE_SIZE);
> +               strlcat(data, p, min(l + strlen(data), COMMAND_LINE_SIZE));
> +       }
>
>         /*
>          * CONFIG_CMDLINE is meant to be a default in case nothing else
> --
> 1.9.1
>

Re: [PATCH v2] of/fdt: implement a "merge-cmdline" property

[Daniel Gimpelevich]

On Mon, 2019-08-05 at 10:29 -0600, Rob Herring wrote:
> On Mon, Aug 5, 2019 at 9:53 AM Daniel Gimpelevich
> <daniel@gimpelevich.san-francisco.ca.us> wrote:
> >
> > Currently, "bootargs" supplied via the "chosen" node can be used only to
> > supply a kernel command line as a whole. No mechanism exists in DT to add
> > bootargs to the existing command line instead. This is needed in order to
> > avoid having to update the bootloader or default bootloader config when
> > upgrading to a DTB and kernel pair that requires bootargs not previously
> > needed.
> 
> The DTB and kernel are not a pair. Or at least they shouldn't be. If
> anything, the bootloader and DTB are a pair as those are the h/w
> specific parts. If the DTB and kernel are a pair, then anything you
> want to put into DTB can just be in the kernel.

They would only be a pair in the sense that DTB changes using newer
bindings would also require a newer kernel.

> There's been some
> attempts to rework the command line handling to be common and support
> prepending and appending which could be useful here.

Sounds good. Can you reference any commit hash for this?

> > One example use case is that OpenWrt currently supports four ARM devices by
> > means of locally applying the previously rejected edition of this patch. So
> > far, the patch has been used in production only on ARM, but architecture is
> > not a distinction in the design.
> 
> Other distros support dozens of boards, so why haven't they ever
> needed such a change?

IMHO, each of those four boards could be supported in some other way.
That said, some other devices have bootloaders implementing dual boot
with fallback on failure, and the kernel should always have access to
any bootarg that reflects which one is being booted.

> > On MIPS, Commit 951d223 ("MIPS: Fix CONFIG_CMDLINE handling") currently
> > prevents support of such a mechanism, so I am including a workaround, in
> > anticipation of upcoming changes.
> 
> Like I said on irc, mixing this with the mess that is MIPS
> bootloader-kernel interface generally and command line handling
> specifically is not a path to upstream.

The DT proposal can be separated from the MIPS mess.

> This is worse than the original proposal because 'merge' is not clear
> what it does compared to 'append' and how does 'cmdline' relate to
> 'bootargs'.

Can there be a different property name that would be better instead of
worse?

Re: [PATCH v2] of/fdt: implement a "merge-cmdline" property

[Frank Rowand]

Hi Daniel,

Please include me on the distribution list for devicetree patches.

-Frank

On 8/5/19 8:53 AM, Daniel Gimpelevich wrote:
> Currently, "bootargs" supplied via the "chosen" node can be used only to
> supply a kernel command line as a whole. No mechanism exists in DT to add
> bootargs to the existing command line instead. This is needed in order to
> avoid having to update the bootloader or default bootloader config when
> upgrading to a DTB and kernel pair that requires bootargs not previously
> needed.
> 
> One example use case is that OpenWrt currently supports four ARM devices by
> means of locally applying the previously rejected edition of this patch. So
> far, the patch has been used in production only on ARM, but architecture is
> not a distinction in the design.
> 
> On MIPS, Commit 951d223 ("MIPS: Fix CONFIG_CMDLINE handling") currently
> prevents support of such a mechanism, so I am including a workaround, in
> anticipation of upcoming changes.
> 
> Signed-off-by: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us>
> Fixes: 951d223 ("MIPS: Fix CONFIG_CMDLINE handling")
> References: https://patchwork.linux-mips.org/patch/17659/
> ---
>  arch/mips/kernel/setup.c | 12 ++++++++----
>  drivers/of/fdt.c         |  9 +++++++--
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index ab349d2..9ce58f2 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -725,7 +725,10 @@ static void __init arch_mem_init(char **cmdline_p)
>  	 * CONFIG_CMDLINE ourselves below & don't want to duplicate its
>  	 * content because repeating arguments can be problematic.
>  	 */
> -	strlcpy(boot_command_line, " ", COMMAND_LINE_SIZE);
> +	if (USE_DTB_CMDLINE)
> +		strlcpy(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
> +	else
> +		strlcpy(boot_command_line, " ", COMMAND_LINE_SIZE);
>  
>  	/* call board setup routine */
>  	plat_mem_setup();
> @@ -753,9 +756,10 @@ static void __init arch_mem_init(char **cmdline_p)
>  #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]))
> +	if (USE_PROM_CMDLINE)
>  		strlcpy(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
> +	else if (!strcmp(boot_command_line, " "))
> +		boot_command_line[0] = '\0';
>  
>  	if (EXTEND_WITH_PROM && arcs_cmdline[0]) {
>  		if (boot_command_line[0])
> @@ -764,7 +768,7 @@ static void __init arch_mem_init(char **cmdline_p)
>  	}
>  
>  #if defined(CONFIG_CMDLINE_BOOL)
> -	if (builtin_cmdline[0]) {
> +	if (builtin_cmdline[0] && strcmp(boot_command_line, builtin_cmdline)) {
>  		if (boot_command_line[0])
>  			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
>  		strlcat(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 9cdf14b..08c25eb 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1055,8 +1055,13 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>  
>  	/* 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));
> +	if (p != NULL && l > 0) {
> +		if (!of_get_flat_dt_prop(node, "merge-cmdline", NULL))
> +			*(char *)data = '\0';
> +		if (*(char *)data)
> +			strlcat(data, " ", COMMAND_LINE_SIZE);
> +		strlcat(data, p, min(l + strlen(data), COMMAND_LINE_SIZE));
> +	}
>  
>  	/*
>  	 * CONFIG_CMDLINE is meant to be a default in case nothing else
> 

Re: [PATCH v2] of/fdt: implement a "merge-cmdline" property

[Frank Rowand]

+Frank (me)

On 8/5/19 8:53 AM, Daniel Gimpelevich wrote:
> Currently, "bootargs" supplied via the "chosen" node can be used only to
> supply a kernel command line as a whole. No mechanism exists in DT to add
> bootargs to the existing command line instead. This is needed in order to
> avoid having to update the bootloader or default bootloader config when
> upgrading to a DTB and kernel pair that requires bootargs not previously
> needed.
> 
> One example use case is that OpenWrt currently supports four ARM devices by
> means of locally applying the previously rejected edition of this patch. So
> far, the patch has been used in production only on ARM, but architecture is
> not a distinction in the design.
> 
> On MIPS, Commit 951d223 ("MIPS: Fix CONFIG_CMDLINE handling") currently
> prevents support of such a mechanism, so I am including a workaround, in
> anticipation of upcoming changes.
> 
> Signed-off-by: Daniel Gimpelevich <daniel@gimpelevich.san-francisco.ca.us>
> Fixes: 951d223 ("MIPS: Fix CONFIG_CMDLINE handling")
> References: https://patchwork.linux-mips.org/patch/17659/
> ---
>  arch/mips/kernel/setup.c | 12 ++++++++----
>  drivers/of/fdt.c         |  9 +++++++--
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index ab349d2..9ce58f2 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -725,7 +725,10 @@ static void __init arch_mem_init(char **cmdline_p)
>  	 * CONFIG_CMDLINE ourselves below & don't want to duplicate its
>  	 * content because repeating arguments can be problematic.
>  	 */
> -	strlcpy(boot_command_line, " ", COMMAND_LINE_SIZE);
> +	if (USE_DTB_CMDLINE)
> +		strlcpy(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
> +	else
> +		strlcpy(boot_command_line, " ", COMMAND_LINE_SIZE);
>  
>  	/* call board setup routine */
>  	plat_mem_setup();
> @@ -753,9 +756,10 @@ static void __init arch_mem_init(char **cmdline_p)
>  #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]))
> +	if (USE_PROM_CMDLINE)
>  		strlcpy(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE);
> +	else if (!strcmp(boot_command_line, " "))
> +		boot_command_line[0] = '\0';
>  
>  	if (EXTEND_WITH_PROM && arcs_cmdline[0]) {
>  		if (boot_command_line[0])
> @@ -764,7 +768,7 @@ static void __init arch_mem_init(char **cmdline_p)
>  	}
>  
>  #if defined(CONFIG_CMDLINE_BOOL)
> -	if (builtin_cmdline[0]) {
> +	if (builtin_cmdline[0] && strcmp(boot_command_line, builtin_cmdline)) {
>  		if (boot_command_line[0])
>  			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
>  		strlcat(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 9cdf14b..08c25eb 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1055,8 +1055,13 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>  
>  	/* 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));
> +	if (p != NULL && l > 0) {
> +		if (!of_get_flat_dt_prop(node, "merge-cmdline", NULL))
> +			*(char *)data = '\0';
> +		if (*(char *)data)
> +			strlcat(data, " ", COMMAND_LINE_SIZE);
> +		strlcat(data, p, min(l + strlen(data), COMMAND_LINE_SIZE));
> +	}
>  
>  	/*
>  	 * CONFIG_CMDLINE is meant to be a default in case nothing else
>