| Message ID | 0c4b839f023f87c451c8aa3c4f7a8d92729c2f02.1617126961.git.danielwa@cisco.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/8] CMDLINE: add generic builtin command line | expand |
| Related | show |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (87d76f542a24ecfa797e9bd3bb56c0f19aabff57) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 32 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
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 >
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
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); > >
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
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
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
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);