| Message ID | 20231110013817.2378507-8-danielwa@cisco.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | generic command line v6 | expand |
On Thu, Nov 09, 2023 at 05:38:11PM -0800, Daniel Walker wrote: > Rob Herring has complained about this section of code. I removed the > command line handling code to the cmdline.h header. This hopefully makes > it easier for Rob to maintain it (at least he doesn't have to look at it > directly anymore). Well, my goal is to eliminate drivers/of/, but no. > I would like to add a Kconfig option called > OF_DEPRECATED_CMDLINE which an architecture would set if it uses this code. Which architecture needs this code? Do we wait and see who complains their platform broke and then go set this option? In the meantime, new platforms started depending on the new behavior and setting the option may break them. So we can't have a kconfig option. > This would allow a platform to use the cmdline.h and the added function > directly and remove the Kconfig option. This change would be in a subsequent > patch. Per platform code handling the cmdline is completely the wrong direction. Per arch behavior is bad enough. Rob
On Thu, Nov 16, 2023 at 10:09:36AM -0600, Rob Herring wrote: > On Thu, Nov 09, 2023 at 05:38:11PM -0800, Daniel Walker wrote: > > Rob Herring has complained about this section of code. I removed the > > command line handling code to the cmdline.h header. This hopefully makes > > it easier for Rob to maintain it (at least he doesn't have to look at it > > directly anymore). > > Well, my goal is to eliminate drivers/of/, but no. > > > I would like to add a Kconfig option called > > OF_DEPRECATED_CMDLINE which an architecture would set if it uses this code. > > Which architecture needs this code? Do we wait and see who complains > their platform broke and then go set this option? In the meantime, new > platforms started depending on the new behavior and setting the option > may break them. So we can't have a kconfig option. I think you misunderstood the comments. The objective is, 1) Remove the code in drivers/of/ 2) Remove platform specific command line handling > > This would allow a platform to use the cmdline.h and the added function > > directly and remove the Kconfig option. This change would be in a subsequent > > patch. > > Per platform code handling the cmdline is completely the wrong > direction. Per arch behavior is bad enough. This code unifies the handling into generic code outside drivers/of/ . Daniel
Le 10/11/2023 à 02:38, Daniel Walker a écrit : > Rob Herring has complained about this section of code. I removed the This kind of considerations shouldn't appear in the commit message, it may appear after the --- so that it gets dropped when applying. > command line handling code to the cmdline.h header. This hopefully makes > it easier for Rob to maintain it (at least he doesn't have to look at it > directly anymore). I would like to add a Kconfig option called > OF_DEPRECATED_CMDLINE which an architecture would set if it uses this code. Not sure to understand what you want to do and why that is needed. Why can't that work for everyone ? Avoid unnecessary options. > This would allow a platform to use the cmdline.h and the added function > directly and remove the Kconfig option. This change would be in a subsequent > patch. would or will ? Not sure it is worth mentioning if it won't. > > This code was boot tested on powerpc 32bit, powerpc 64bit without > any generic command line conversion. > > Cc: xe-linux-external@cisco.com > Signed-off-by: Daniel Walker <danielwa@cisco.com> > --- > drivers/of/fdt.c | 22 +++------------------- > include/linux/cmdline.h | 31 +++++++++++++++++++++++++++++++ > 2 files changed, 34 insertions(+), 19 deletions(-) > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index bf502ba8da95..1fc1b17d04dc 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -26,6 +26,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> > @@ -1183,27 +1184,10 @@ int __init early_init_dt_scan_chosen(char *cmdline) > > /* Retrieve command line */ > p = of_get_flat_dt_prop(node, "bootargs", &l); > - if (p != NULL && l > 0) > - strscpy(cmdline, p, min(l, COMMAND_LINE_SIZE)); > > handle_cmdline: > - /* > - * CONFIG_CMDLINE is meant to be a default in case nothing else > - * managed to set the command line, unless CONFIG_CMDLINE_FORCE > - * is set in which case we override whatever was found earlier. > - */ > -#ifdef CONFIG_CMDLINE > -#if defined(CONFIG_CMDLINE_EXTEND) > - strlcat(cmdline, " ", COMMAND_LINE_SIZE); > - strlcat(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > -#elif defined(CONFIG_CMDLINE_FORCE) > - strscpy(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > -#else > - /* No arguments from boot loader, use kernel's cmdl*/ > - if (!((char *)cmdline)[0]) > - strscpy(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > -#endif > -#endif /* CONFIG_CMDLINE */ > + > + of_deprecated_cmdline_update(cmdline, p, l); > > pr_debug("Command line is: %s\n", (char *)cmdline); > > diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h > index a94758a0f257..c772afb7340f 100644 > --- a/include/linux/cmdline.h > +++ b/include/linux/cmdline.h > @@ -103,4 +103,35 @@ __cmdline_add_builtin( > > #define cmdline_get_static_builtin(dest) \ > (CMDLINE_STATIC_PREPEND CMDLINE_STATIC_APPEND) > + > +#ifndef CONFIG_GENERIC_CMDLINE > +static inline bool of_deprecated_cmdline_update(char *cmdline, const char *dt_bootargs, int length) Add a comment explaining why it is deprecated. > +{ > + if (dt_bootargs != NULL && length > 0) > + strlcpy(cmdline, dt_bootargs, min(length, COMMAND_LINE_SIZE)); > + /* > + * CONFIG_CMDLINE is meant to be a default in case nothing else > + * managed to set the command line, unless CONFIG_CMDLINE_FORCE > + * is set in which case we override whatever was found earlier. > + */ > + > +#ifdef CONFIG_CMDLINE > +#if defined(CONFIG_CMDLINE_EXTEND) > + strlcat(cmdline, " ", COMMAND_LINE_SIZE); > + strlcat(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > +#elif defined(CONFIG_CMDLINE_FORCE) > + strscpy(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > +#else > + /* No arguments from boot loader, use kernel's cmdl*/ > + if (!((char *)cmdline)[0]) > + strscpy(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > #endif > +#endif /* CONFIG_CMDLINE */ > + return true; > +} > +#else > +static inline bool of_deprecated_cmdline_update(char *cmdline, const char *dt_bootargs, int length) { return false; } > +#endif /* CONFIG_GENERIC_CMDLINE */ > + > + > +#endif /* _LINUX_CMDLINE_H */
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index bf502ba8da95..1fc1b17d04dc 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -26,6 +26,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> @@ -1183,27 +1184,10 @@ int __init early_init_dt_scan_chosen(char *cmdline) /* Retrieve command line */ p = of_get_flat_dt_prop(node, "bootargs", &l); - if (p != NULL && l > 0) - strscpy(cmdline, p, min(l, COMMAND_LINE_SIZE)); handle_cmdline: - /* - * CONFIG_CMDLINE is meant to be a default in case nothing else - * managed to set the command line, unless CONFIG_CMDLINE_FORCE - * is set in which case we override whatever was found earlier. - */ -#ifdef CONFIG_CMDLINE -#if defined(CONFIG_CMDLINE_EXTEND) - strlcat(cmdline, " ", COMMAND_LINE_SIZE); - strlcat(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); -#elif defined(CONFIG_CMDLINE_FORCE) - strscpy(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); -#else - /* No arguments from boot loader, use kernel's cmdl*/ - if (!((char *)cmdline)[0]) - strscpy(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); -#endif -#endif /* CONFIG_CMDLINE */ + + of_deprecated_cmdline_update(cmdline, p, l); pr_debug("Command line is: %s\n", (char *)cmdline); diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h index a94758a0f257..c772afb7340f 100644 --- a/include/linux/cmdline.h +++ b/include/linux/cmdline.h @@ -103,4 +103,35 @@ __cmdline_add_builtin( #define cmdline_get_static_builtin(dest) \ (CMDLINE_STATIC_PREPEND CMDLINE_STATIC_APPEND) + +#ifndef CONFIG_GENERIC_CMDLINE +static inline bool of_deprecated_cmdline_update(char *cmdline, const char *dt_bootargs, int length) +{ + if (dt_bootargs != NULL && length > 0) + strlcpy(cmdline, dt_bootargs, min(length, COMMAND_LINE_SIZE)); + /* + * CONFIG_CMDLINE is meant to be a default in case nothing else + * managed to set the command line, unless CONFIG_CMDLINE_FORCE + * is set in which case we override whatever was found earlier. + */ + +#ifdef CONFIG_CMDLINE +#if defined(CONFIG_CMDLINE_EXTEND) + strlcat(cmdline, " ", COMMAND_LINE_SIZE); + strlcat(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); +#elif defined(CONFIG_CMDLINE_FORCE) + strscpy(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); +#else + /* No arguments from boot loader, use kernel's cmdl*/ + if (!((char *)cmdline)[0]) + strscpy(cmdline, CONFIG_CMDLINE, COMMAND_LINE_SIZE); #endif +#endif /* CONFIG_CMDLINE */ + return true; +} +#else +static inline bool of_deprecated_cmdline_update(char *cmdline, const char *dt_bootargs, int length) { return false; } +#endif /* CONFIG_GENERIC_CMDLINE */ + + +#endif /* _LINUX_CMDLINE_H */
Rob Herring has complained about this section of code. I removed the command line handling code to the cmdline.h header. This hopefully makes it easier for Rob to maintain it (at least he doesn't have to look at it directly anymore). I would like to add a Kconfig option called OF_DEPRECATED_CMDLINE which an architecture would set if it uses this code. This would allow a platform to use the cmdline.h and the added function directly and remove the Kconfig option. This change would be in a subsequent patch. This code was boot tested on powerpc 32bit, powerpc 64bit without any generic command line conversion. Cc: xe-linux-external@cisco.com Signed-off-by: Daniel Walker <danielwa@cisco.com> --- drivers/of/fdt.c | 22 +++------------------- include/linux/cmdline.h | 31 +++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 19 deletions(-)