[PATCH 2/8] CMDLINE: drivers: of: ifdef out cmdline section

[PATCH 2/8] CMDLINE: drivers: of: ifdef out cmdline section

[Daniel Walker]

It looks like there's some seepage of cmdline stuff into
the generic device tree code. This conflicts with the
generic cmdline implementation so I remove it in the case
when that's enabled.

Cc: xe-linux-external@cisco.com
Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
Signed-off-by: Daniel Walker <danielwa@cisco.com>
---
 drivers/of/fdt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index dcc1dd96911a..d8805cd9717a 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -25,6 +25,7 @@
 #include <linux/serial_core.h>
 #include <linux/sysfs.h>
 #include <linux/random.h>
+#include <linux/cmdline.h>
 
 #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
 #include <asm/page.h>
@@ -1050,6 +1051,18 @@ 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 defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_GENERIC_CMDLINE_OF)
+	/*
+	 * The builtin command line will be added here, or it can override
+	 * with the DT bootargs.
+	 */
+	cmdline_add_builtin(data,
+			    (l > 0 ? p : NULL), /* This is sanity checking */
+			    COMMAND_LINE_SIZE);
+#elif defined(CONFIG_GENERIC_CMDLINE)
+	strlcpy(data, p, COMMAND_LINE_SIZE);
+#else
 	if (p != NULL && l > 0)
 		strlcpy(data, p, min(l, COMMAND_LINE_SIZE));
 
@@ -1070,6 +1083,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
 		strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
 #endif
 #endif /* CONFIG_CMDLINE */
+#endif /* CONFIG_GENERIC_CMDLINE */
 
 	pr_debug("Command line is: %s\n", (char *)data);
 
-- 
2.25.1

Re: [PATCH 2/8] CMDLINE: drivers: of: ifdef out cmdline section

[Rob Herring]

On Tue, Mar 30, 2021 at 12:57 PM Daniel Walker <danielwa@cisco.com> wrote:
>
> It looks like there's some seepage of cmdline stuff into
> the generic device tree code. This conflicts with the
> generic cmdline implementation so I remove it in the case
> when that's enabled.
>
> Cc: xe-linux-external@cisco.com
> Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> Signed-off-by: Daniel Walker <danielwa@cisco.com>
> ---
>  drivers/of/fdt.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index dcc1dd96911a..d8805cd9717a 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -25,6 +25,7 @@
>  #include <linux/serial_core.h>
>  #include <linux/sysfs.h>
>  #include <linux/random.h>
> +#include <linux/cmdline.h>
>
>  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>  #include <asm/page.h>
> @@ -1050,6 +1051,18 @@ 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 defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_GENERIC_CMDLINE_OF)

Moving in the wrong direction... This code already has too many
#ifdef's. I like Christophe's version as it gets rid of all the code
here.

> +       /*
> +        * The builtin command line will be added here, or it can override
> +        * with the DT bootargs.
> +        */
> +       cmdline_add_builtin(data,
> +                           (l > 0 ? p : NULL), /* This is sanity checking */
> +                           COMMAND_LINE_SIZE);
> +#elif defined(CONFIG_GENERIC_CMDLINE)
> +       strlcpy(data, p, COMMAND_LINE_SIZE);
> +#else
>         if (p != NULL && l > 0)
>                 strlcpy(data, p, min(l, COMMAND_LINE_SIZE));
>
> @@ -1070,6 +1083,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>                 strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
>  #endif
>  #endif /* CONFIG_CMDLINE */
> +#endif /* CONFIG_GENERIC_CMDLINE */
>
>         pr_debug("Command line is: %s\n", (char *)data);
>
> --
> 2.25.1
>

Re: [PATCH 2/8] CMDLINE: drivers: of: ifdef out cmdline section

[Daniel Walker]

On Tue, Mar 30, 2021 at 02:49:13PM -0500, Rob Herring wrote:
> On Tue, Mar 30, 2021 at 12:57 PM Daniel Walker <danielwa@cisco.com> wrote:
> >
> > It looks like there's some seepage of cmdline stuff into
> > the generic device tree code. This conflicts with the
> > generic cmdline implementation so I remove it in the case
> > when that's enabled.
> >
> > Cc: xe-linux-external@cisco.com
> > Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > ---
> >  drivers/of/fdt.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index dcc1dd96911a..d8805cd9717a 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/serial_core.h>
> >  #include <linux/sysfs.h>
> >  #include <linux/random.h>
> > +#include <linux/cmdline.h>
> >
> >  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> >  #include <asm/page.h>
> > @@ -1050,6 +1051,18 @@ 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 defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_GENERIC_CMDLINE_OF)
> 
> Moving in the wrong direction... This code already has too many
> #ifdef's. I like Christophe's version as it gets rid of all the code
> here.
 
It's temporary .. Notice CONFIG_GENERIC_CMDLINE_OF is only used on PowerPC. I
experienced doubling on arm64 when this was used (i.e. the append and prepend
was added twice).

I don't think there are any other users which can't be moved outside the device
tree code, but powerpc uses this function three times during boot up plus the
prom_init user. It's possible to use the generic command line in all four places,
but it become space inefficient.

So the plan would be make the other architectures call the generic cmdline
directly without this code, then powerpc would need to be reworked to call the
generic commandline in it's own code hopefully just once.

The end results would be this section would reduce down to one string copy and
no command line stuff.

Maybe you would rather I just use the generic command line in those three place
and reduce this at the space code of powerpc ?

Daniel

Re: [PATCH 2/8] CMDLINE: drivers: of: ifdef out cmdline section

[Christophe Leroy]

Le 30/03/2021 à 19:56, Daniel Walker a écrit :
> It looks like there's some seepage of cmdline stuff into
> the generic device tree code. This conflicts with the
> generic cmdline implementation so I remove it in the case
> when that's enabled.
> 
> Cc: xe-linux-external@cisco.com
> Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> Signed-off-by: Daniel Walker <danielwa@cisco.com>
> ---
>   drivers/of/fdt.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index dcc1dd96911a..d8805cd9717a 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -25,6 +25,7 @@
>   #include <linux/serial_core.h>
>   #include <linux/sysfs.h>
>   #include <linux/random.h>
> +#include <linux/cmdline.h>
>   
>   #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
>   #include <asm/page.h>
> @@ -1050,6 +1051,18 @@ 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 defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_GENERIC_CMDLINE_OF)
> +	/*
> +	 * The builtin command line will be added here, or it can override
> +	 * with the DT bootargs.
> +	 */
> +	cmdline_add_builtin(data,
> +			    (l > 0 ? p : NULL), /* This is sanity checking */
> +			    COMMAND_LINE_SIZE);
> +#elif defined(CONFIG_GENERIC_CMDLINE)
> +	strlcpy(data, p, COMMAND_LINE_SIZE);
> +#else

Ugly.

Linux codying style recommend to limit the use of #ifdefs to headers as much as possible.

Why do we need so many alternatives ? Allthough they are temporary, can we order the changes in 
another way to reduce that ?

>   	if (p != NULL && l > 0)
>   		strlcpy(data, p, min(l, COMMAND_LINE_SIZE));
>   
> @@ -1070,6 +1083,7 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>   		strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
>   #endif
>   #endif /* CONFIG_CMDLINE */
> +#endif /* CONFIG_GENERIC_CMDLINE */
>   
>   	pr_debug("Command line is: %s\n", (char *)data);
>   
> 

Re: [PATCH 2/8] CMDLINE: drivers: of: ifdef out cmdline section

[Daniel Walker]

On Fri, Apr 02, 2021 at 07:32:08PM +0200, Christophe Leroy wrote:
> 
> 
> Le 30/03/2021 à 19:56, Daniel Walker a écrit :
> > It looks like there's some seepage of cmdline stuff into
> > the generic device tree code. This conflicts with the
> > generic cmdline implementation so I remove it in the case
> > when that's enabled.
> > 
> > Cc: xe-linux-external@cisco.com
> > Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > ---
> >   drivers/of/fdt.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index dcc1dd96911a..d8805cd9717a 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -25,6 +25,7 @@
> >   #include <linux/serial_core.h>
> >   #include <linux/sysfs.h>
> >   #include <linux/random.h>
> > +#include <linux/cmdline.h>
> >   #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> >   #include <asm/page.h>
> > @@ -1050,6 +1051,18 @@ 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 defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_GENERIC_CMDLINE_OF)
> > +	/*
> > +	 * The builtin command line will be added here, or it can override
> > +	 * with the DT bootargs.
> > +	 */
> > +	cmdline_add_builtin(data,
> > +			    (l > 0 ? p : NULL), /* This is sanity checking */
> > +			    COMMAND_LINE_SIZE);
> > +#elif defined(CONFIG_GENERIC_CMDLINE)
> > +	strlcpy(data, p, COMMAND_LINE_SIZE);
> > +#else
> 
> Ugly.
> 
> Linux codying style recommend to limit the use of #ifdefs to headers as much as possible.
> 
> Why do we need so many alternatives ? Allthough they are temporary, can we
> order the changes in another way to reduce that ?

I think this whole section can be removed down even all the CMDLINE ifdef's ..
The only architecture which needs this is powerpc because it calls this function
three times.

If powerpc were made to call this only once , and then call the generic handling
for the command line then this whole section would get removed.

Daniel

Re: [PATCH 2/8] CMDLINE: drivers: of: ifdef out cmdline section

[Rob Herring]

On Tue, Mar 30, 2021 at 04:17:53PM -0700, Daniel Walker wrote:
> On Tue, Mar 30, 2021 at 02:49:13PM -0500, Rob Herring wrote:
> > On Tue, Mar 30, 2021 at 12:57 PM Daniel Walker <danielwa@cisco.com> wrote:
> > >
> > > It looks like there's some seepage of cmdline stuff into
> > > the generic device tree code. This conflicts with the
> > > generic cmdline implementation so I remove it in the case
> > > when that's enabled.
> > >
> > > Cc: xe-linux-external@cisco.com
> > > Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> > > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > > ---
> > >  drivers/of/fdt.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index dcc1dd96911a..d8805cd9717a 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -25,6 +25,7 @@
> > >  #include <linux/serial_core.h>
> > >  #include <linux/sysfs.h>
> > >  #include <linux/random.h>
> > > +#include <linux/cmdline.h>
> > >
> > >  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> > >  #include <asm/page.h>
> > > @@ -1050,6 +1051,18 @@ 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 defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_GENERIC_CMDLINE_OF)
> > 
> > Moving in the wrong direction... This code already has too many
> > #ifdef's. I like Christophe's version as it gets rid of all the code
> > here.
>  
> It's temporary .. Notice CONFIG_GENERIC_CMDLINE_OF is only used on PowerPC. I
> experienced doubling on arm64 when this was used (i.e. the append and prepend
> was added twice).
> 
> I don't think there are any other users which can't be moved outside the device
> tree code, but powerpc uses this function three times during boot up plus the
> prom_init user. It's possible to use the generic command line in all four places,
> but it become space inefficient.

What's the 3rd use? I count kaslr code and in 
early_init_dt_scan_chosen_ppc. Do we need to build the command line for 
kaslr seed? Getting any build time value from the kernel is pointless.

Rob

Re: [PATCH 2/8] CMDLINE: drivers: of: ifdef out cmdline section

[Daniel Walker]

On Wed, Apr 07, 2021 at 05:59:15PM -0500, Rob Herring wrote:
> On Tue, Mar 30, 2021 at 04:17:53PM -0700, Daniel Walker wrote:
> > On Tue, Mar 30, 2021 at 02:49:13PM -0500, Rob Herring wrote:
> > > On Tue, Mar 30, 2021 at 12:57 PM Daniel Walker <danielwa@cisco.com> wrote:
> > > >
> > > > It looks like there's some seepage of cmdline stuff into
> > > > the generic device tree code. This conflicts with the
> > > > generic cmdline implementation so I remove it in the case
> > > > when that's enabled.
> > > >
> > > > Cc: xe-linux-external@cisco.com
> > > > Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> > > > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > > > ---
> > > >  drivers/of/fdt.c | 14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > index dcc1dd96911a..d8805cd9717a 100644
> > > > --- a/drivers/of/fdt.c
> > > > +++ b/drivers/of/fdt.c
> > > > @@ -25,6 +25,7 @@
> > > >  #include <linux/serial_core.h>
> > > >  #include <linux/sysfs.h>
> > > >  #include <linux/random.h>
> > > > +#include <linux/cmdline.h>
> > > >
> > > >  #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
> > > >  #include <asm/page.h>
> > > > @@ -1050,6 +1051,18 @@ 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 defined(CONFIG_GENERIC_CMDLINE) && defined(CONFIG_GENERIC_CMDLINE_OF)
> > > 
> > > Moving in the wrong direction... This code already has too many
> > > #ifdef's. I like Christophe's version as it gets rid of all the code
> > > here.
> >  
> > It's temporary .. Notice CONFIG_GENERIC_CMDLINE_OF is only used on PowerPC. I
> > experienced doubling on arm64 when this was used (i.e. the append and prepend
> > was added twice).
> > 
> > I don't think there are any other users which can't be moved outside the device
> > tree code, but powerpc uses this function three times during boot up plus the
> > prom_init user. It's possible to use the generic command line in all four places,
> > but it become space inefficient.
> 
> What's the 3rd use? I count kaslr code and in 
> early_init_dt_scan_chosen_ppc. Do we need to build the command line for 
> kaslr seed? Getting any build time value from the kernel is pointless.

I think I may have been mistaken. I added a dump_stack() , but there may have
been other stack traces during bootup on prior -rcX's I was testing.

I re-ran the test and I only see one user on powerpc and powerpc64,

powerpc64,

[    T0] Call Trace:
[    T0] [c000000001517d00] [c00000000077e910] dump_stack+0xc4/0x114 (unreliable)
[    T0] [c000000001517d50] [c000000001186fb4] early_init_dt_scan_chosen+0x238/0x324
[    T0] [c000000001517de0] [c000000001138b00] early_init_dt_scan_chosen_ppc+0x20/0x194
[    T0] [c000000001517e10] [c000000001186ae0] of_scan_flat_dt+0xc8/0x130
[    T0] [c000000001517e70] [c000000001139404] early_init_devtree+0xa4/0x48c
[    T0] [c000000001517f10] [c00000000113ac90] early_setup+0xc8/0x254
[    T0] [c000000001517f90] [000000000000c754] 0xc754

powerpc32,

Call Trace:
[c06bbee0] [c067e334] early_init_dt_scan_chosen+0xf8/0x1dc (unreliable)
[c06bbf10] [c0666ec4] early_init_dt_scan_chosen_ppc+0x18/0x6c
[c06bbf30] [c067e048] of_scan_flat_dt+0x98/0xf4
[c06bbf70] [c0667234] early_init_devtree+0x48/0x2d0
[c06bbfb0] [c06679cc] machine_init+0x98/0xcc
[c06bbff0] [c0000398] set_ivor+0x114/0x154

I think it would be possible to just move the generic handling entire into
architecture code.

Daniel