* [PATCH v2] MIPS: cmdline: Clean up boot_command_line initialization
@ 2019-10-09 23:09 Paul Burton
2019-10-10 21:04 ` Paul Burton
2019-10-11 16:54 ` Nathan Chancellor
0 siblings, 2 replies; 3+ messages in thread
From: Paul Burton @ 2019-10-09 23:09 UTC (permalink / raw)
To: linux-mips@vger.kernel.org; +Cc: Paul Burton
Our current code to initialize boot_command_line is a mess. Some of this
is due to the addition of too many options over the years, and some of
this is due to workarounds for early_init_dt_scan_chosen() performing
actions specific to options from other architectures that probably
shouldn't be in generic code.
Clean this up by introducing a new bootcmdline_init() function that
simplifies the initialization somewhat. The major changes are:
- Because bootcmdline_init() is a function it can return early in the
CONFIG_CMDLINE_OVERRIDE case.
- We clear boot_command_line rather than inheriting whatever
early_init_dt_scan_chosen() may have left us. This means we no longer
need to set boot_command_line to a space character in an attempt to
prevent early_init_dt_scan_chosen() from copying CONFIG_CMDLINE into
boot_command_line without us knowing about it.
- Indirection via USE_PROM_CMDLINE, USE_DTB_CMDLINE, EXTEND_WITH_PROM &
BUILTIN_EXTEND_WITH_PROM macros is removed; they seemingly served only
to obfuscate the code.
- The logic is cleaner, clearer & commented.
Two minor drawbacks of this approach are:
1) We call of_scan_flat_dt(), which means we scan through the DT again.
The overhead is fairly minimal & shouldn't be noticeable.
2) cmdline_scan_chosen() duplicates a small amount of the logic from
early_init_dt_scan_chosen(). Alternatives might be to allow the
generic FDT code to keep & expose a copy of the arguments taken from
the /chosen node's bootargs property, or to introduce a function like
early_init_dt_scan_chosen() that retrieves them without modification
to handle CONFIG_CMDLINE. Neither of these sounds particularly
cleaner though, and this way we at least keep the extra work in
arch/mips.
Signed-off-by: Paul Burton <paul.burton@mips.com>
---
Changes in v2:
- Keep CONFIG_MIPS_CMDLINE_DTB_EXTEND &
CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND for now.
- Hoist bootcmdline_append() into a function to avoid duplicating its
logic multiple times.
arch/mips/kernel/setup.c | 125 ++++++++++++++++++++++++++-------------
1 file changed, 83 insertions(+), 42 deletions(-)
diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 5eec13b8d222..4aeba3122972 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -538,11 +538,88 @@ static void __init check_kernel_sections_mem(void)
}
}
-#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 bootcmdline_append(const char *s, size_t max)
+{
+ if (!s[0] || !max)
+ return;
+
+ if (boot_command_line[0])
+ strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
+
+ strlcat(boot_command_line, s, max);
+}
+
+static int __init bootcmdline_scan_chosen(unsigned long node, const char *uname,
+ int depth, void *data)
+{
+ bool *dt_bootargs = data;
+ const char *p;
+ int l;
+
+ if (depth != 1 || !data ||
+ (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
+ return 0;
+
+ p = of_get_flat_dt_prop(node, "bootargs", &l);
+ if (p != NULL && l > 0) {
+ bootcmdline_append(p, min(l, COMMAND_LINE_SIZE));
+ *dt_bootargs = true;
+ }
+
+ return 1;
+}
+
+static void __init bootcmdline_init(char **cmdline_p)
+{
+ bool dt_bootargs = false;
+
+ /*
+ * If CMDLINE_OVERRIDE is enabled then initializing the command line is
+ * trivial - we simply use the built-in command line unconditionally &
+ * unmodified.
+ */
+ if (IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
+ strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+ return;
+ }
+
+ /*
+ * If the user specified a built-in command line &
+ * MIPS_CMDLINE_BUILTIN_EXTEND, then the built-in command line is
+ * prepended to arguments from the bootloader or DT so we'll copy them
+ * to the start of boot_command_line here. Otherwise, empty
+ * boot_command_line to undo anything early_init_dt_scan_chosen() did.
+ */
+ if (IS_ENABLED(CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND))
+ strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+ else
+ boot_command_line[0] = 0;
+
+ /*
+ * If we're configured to take boot arguments from DT, look for those
+ * now.
+ */
+ if (IS_ENABLED(CONFIG_MIPS_CMDLINE_FROM_DTB))
+ of_scan_flat_dt(bootcmdline_scan_chosen, &dt_bootargs);
+
+ /*
+ * If we didn't get any arguments from DT (regardless of whether that's
+ * because we weren't configured to look for them, or because we looked
+ * & found none) then we'll take arguments from the bootloader.
+ * plat_mem_setup() should have filled arcs_cmdline with arguments from
+ * the bootloader.
+ */
+ if (IS_ENABLED(CONFIG_MIPS_CMDLINE_DTB_EXTEND) || !dt_bootargs)
+ bootcmdline_append(arcs_cmdline, COMMAND_LINE_SIZE);
+
+ /*
+ * If the user specified a built-in command line & we didn't already
+ * prepend it, we append it to boot_command_line here.
+ */
+ if (IS_ENABLED(CONFIG_CMDLINE_BOOL) &&
+ !IS_ENABLED(CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND))
+ bootcmdline_append(builtin_cmdline, COMMAND_LINE_SIZE);
+}
/*
* arch_mem_init - initialize memory management subsystem
@@ -570,48 +647,12 @@ static void __init arch_mem_init(char **cmdline_p)
{
extern void plat_mem_setup(void);
- /*
- * Initialize boot_command_line to an innocuous but non-empty string in
- * order to prevent early_init_dt_scan_chosen() from copying
- * CONFIG_CMDLINE into it without our knowledge. We handle
- * CONFIG_CMDLINE ourselves below & don't want to duplicate its
- * content because repeating arguments can be problematic.
- */
- strlcpy(boot_command_line, " ", COMMAND_LINE_SIZE);
-
/* call board setup routine */
plat_mem_setup();
memblock_set_bottom_up(true);
-#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
+ bootcmdline_init(cmdline_p);
strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
-
*cmdline_p = command_line;
parse_early_param();
--
2.23.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] MIPS: cmdline: Clean up boot_command_line initialization
2019-10-09 23:09 [PATCH v2] MIPS: cmdline: Clean up boot_command_line initialization Paul Burton
@ 2019-10-10 21:04 ` Paul Burton
2019-10-11 16:54 ` Nathan Chancellor
1 sibling, 0 replies; 3+ messages in thread
From: Paul Burton @ 2019-10-10 21:04 UTC (permalink / raw)
To: Paul Burton
Cc: linux-mips@vger.kernel.org, Paul Burton,
linux-mips@vger.kernel.org
Hello,
Paul Burton wrote:
> Our current code to initialize boot_command_line is a mess. Some of this
> is due to the addition of too many options over the years, and some of
> this is due to workarounds for early_init_dt_scan_chosen() performing
> actions specific to options from other architectures that probably
> shouldn't be in generic code.
>
> Clean this up by introducing a new bootcmdline_init() function that
> simplifies the initialization somewhat. The major changes are:
>
> - Because bootcmdline_init() is a function it can return early in the
> CONFIG_CMDLINE_OVERRIDE case.
>
> - We clear boot_command_line rather than inheriting whatever
> early_init_dt_scan_chosen() may have left us. This means we no longer
> need to set boot_command_line to a space character in an attempt to
> prevent early_init_dt_scan_chosen() from copying CONFIG_CMDLINE into
> boot_command_line without us knowing about it.
>
> - Indirection via USE_PROM_CMDLINE, USE_DTB_CMDLINE, EXTEND_WITH_PROM &
> BUILTIN_EXTEND_WITH_PROM macros is removed; they seemingly served only
> to obfuscate the code.
>
> - The logic is cleaner, clearer & commented.
>
> Two minor drawbacks of this approach are:
>
> 1) We call of_scan_flat_dt(), which means we scan through the DT again.
> The overhead is fairly minimal & shouldn't be noticeable.
>
> 2) cmdline_scan_chosen() duplicates a small amount of the logic from
> early_init_dt_scan_chosen(). Alternatives might be to allow the
> generic FDT code to keep & expose a copy of the arguments taken from
> the /chosen node's bootargs property, or to introduce a function like
> early_init_dt_scan_chosen() that retrieves them without modification
> to handle CONFIG_CMDLINE. Neither of these sounds particularly
> cleaner though, and this way we at least keep the extra work in
> arch/mips.
Applied to mips-next.
> commit 7784cac69735
> https://git.kernel.org/mips/c/7784cac69735
>
> Signed-off-by: Paul Burton <paul.burton@mips.com>
Thanks,
Paul
[ This message was auto-generated; if you believe anything is incorrect
then please email paul.burton@mips.com to report it. ]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] MIPS: cmdline: Clean up boot_command_line initialization
2019-10-09 23:09 [PATCH v2] MIPS: cmdline: Clean up boot_command_line initialization Paul Burton
2019-10-10 21:04 ` Paul Burton
@ 2019-10-11 16:54 ` Nathan Chancellor
1 sibling, 0 replies; 3+ messages in thread
From: Nathan Chancellor @ 2019-10-11 16:54 UTC (permalink / raw)
To: Paul Burton; +Cc: linux-mips@vger.kernel.org, Paul Burton
On Wed, Oct 09, 2019 at 11:09:45PM +0000, Paul Burton wrote:
> Our current code to initialize boot_command_line is a mess. Some of this
> is due to the addition of too many options over the years, and some of
> this is due to workarounds for early_init_dt_scan_chosen() performing
> actions specific to options from other architectures that probably
> shouldn't be in generic code.
>
> Clean this up by introducing a new bootcmdline_init() function that
> simplifies the initialization somewhat. The major changes are:
>
> - Because bootcmdline_init() is a function it can return early in the
> CONFIG_CMDLINE_OVERRIDE case.
>
> - We clear boot_command_line rather than inheriting whatever
> early_init_dt_scan_chosen() may have left us. This means we no longer
> need to set boot_command_line to a space character in an attempt to
> prevent early_init_dt_scan_chosen() from copying CONFIG_CMDLINE into
> boot_command_line without us knowing about it.
>
> - Indirection via USE_PROM_CMDLINE, USE_DTB_CMDLINE, EXTEND_WITH_PROM &
> BUILTIN_EXTEND_WITH_PROM macros is removed; they seemingly served only
> to obfuscate the code.
>
> - The logic is cleaner, clearer & commented.
>
> Two minor drawbacks of this approach are:
>
> 1) We call of_scan_flat_dt(), which means we scan through the DT again.
> The overhead is fairly minimal & shouldn't be noticeable.
>
> 2) cmdline_scan_chosen() duplicates a small amount of the logic from
> early_init_dt_scan_chosen(). Alternatives might be to allow the
> generic FDT code to keep & expose a copy of the arguments taken from
> the /chosen node's bootargs property, or to introduce a function like
> early_init_dt_scan_chosen() that retrieves them without modification
> to handle CONFIG_CMDLINE. Neither of these sounds particularly
> cleaner though, and this way we at least keep the extra work in
> arch/mips.
>
> Signed-off-by: Paul Burton <paul.burton@mips.com>
>
> ---
>
> Changes in v2:
> - Keep CONFIG_MIPS_CMDLINE_DTB_EXTEND &
> CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND for now.
> - Hoist bootcmdline_append() into a function to avoid duplicating its
> logic multiple times.
>
> arch/mips/kernel/setup.c | 125 ++++++++++++++++++++++++++-------------
> 1 file changed, 83 insertions(+), 42 deletions(-)
>
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 5eec13b8d222..4aeba3122972 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -538,11 +538,88 @@ static void __init check_kernel_sections_mem(void)
> }
> }
>
> -#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 bootcmdline_append(const char *s, size_t max)
> +{
> + if (!s[0] || !max)
> + return;
> +
> + if (boot_command_line[0])
> + strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> +
> + strlcat(boot_command_line, s, max);
> +}
> +
> +static int __init bootcmdline_scan_chosen(unsigned long node, const char *uname,
> + int depth, void *data)
> +{
> + bool *dt_bootargs = data;
> + const char *p;
> + int l;
> +
> + if (depth != 1 || !data ||
> + (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
> + return 0;
> +
> + p = of_get_flat_dt_prop(node, "bootargs", &l);
> + if (p != NULL && l > 0) {
> + bootcmdline_append(p, min(l, COMMAND_LINE_SIZE));
> + *dt_bootargs = true;
> + }
> +
> + return 1;
> +}
> +
> +static void __init bootcmdline_init(char **cmdline_p)
> +{
> + bool dt_bootargs = false;
> +
> + /*
> + * If CMDLINE_OVERRIDE is enabled then initializing the command line is
> + * trivial - we simply use the built-in command line unconditionally &
> + * unmodified.
> + */
> + if (IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
> + strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> + return;
> + }
> +
> + /*
> + * If the user specified a built-in command line &
> + * MIPS_CMDLINE_BUILTIN_EXTEND, then the built-in command line is
> + * prepended to arguments from the bootloader or DT so we'll copy them
> + * to the start of boot_command_line here. Otherwise, empty
> + * boot_command_line to undo anything early_init_dt_scan_chosen() did.
> + */
> + if (IS_ENABLED(CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND))
> + strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> + else
> + boot_command_line[0] = 0;
> +
> + /*
> + * If we're configured to take boot arguments from DT, look for those
> + * now.
> + */
> + if (IS_ENABLED(CONFIG_MIPS_CMDLINE_FROM_DTB))
> + of_scan_flat_dt(bootcmdline_scan_chosen, &dt_bootargs);
> +
> + /*
> + * If we didn't get any arguments from DT (regardless of whether that's
> + * because we weren't configured to look for them, or because we looked
> + * & found none) then we'll take arguments from the bootloader.
> + * plat_mem_setup() should have filled arcs_cmdline with arguments from
> + * the bootloader.
> + */
> + if (IS_ENABLED(CONFIG_MIPS_CMDLINE_DTB_EXTEND) || !dt_bootargs)
> + bootcmdline_append(arcs_cmdline, COMMAND_LINE_SIZE);
> +
> + /*
> + * If the user specified a built-in command line & we didn't already
> + * prepend it, we append it to boot_command_line here.
> + */
> + if (IS_ENABLED(CONFIG_CMDLINE_BOOL) &&
> + !IS_ENABLED(CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND))
> + bootcmdline_append(builtin_cmdline, COMMAND_LINE_SIZE);
> +}
>
> /*
> * arch_mem_init - initialize memory management subsystem
> @@ -570,48 +647,12 @@ static void __init arch_mem_init(char **cmdline_p)
> {
> extern void plat_mem_setup(void);
>
> - /*
> - * Initialize boot_command_line to an innocuous but non-empty string in
> - * order to prevent early_init_dt_scan_chosen() from copying
> - * CONFIG_CMDLINE into it without our knowledge. We handle
> - * CONFIG_CMDLINE ourselves below & don't want to duplicate its
> - * content because repeating arguments can be problematic.
> - */
> - strlcpy(boot_command_line, " ", COMMAND_LINE_SIZE);
> -
> /* call board setup routine */
> plat_mem_setup();
> memblock_set_bottom_up(true);
>
> -#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
> + bootcmdline_init(cmdline_p);
> strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> -
> *cmdline_p = command_line;
>
> parse_early_param();
> --
> 2.23.0
>
Hi Paul,
This causes a build error on malta_defconfig because CONFIG_CMDLINE_BOOL
is not set so builtin_cmdline is not defined.
https://travis-ci.com/ClangBuiltLinux/continuous-integration/jobs/244643074
../arch/mips/kernel/setup.c: In function 'bootcmdline_init':
../arch/mips/kernel/setup.c:582:30: error: 'builtin_cmdline' undeclared (first use in this function); did you mean 'builtin_driver'?
strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
^~~~~~~~~~~~~~~
builtin_driver
../arch/mips/kernel/setup.c:582:30: note: each undeclared identifier is reported only once for each function it appears in
Cheers,
Nathan
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-10-11 16:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-09 23:09 [PATCH v2] MIPS: cmdline: Clean up boot_command_line initialization Paul Burton
2019-10-10 21:04 ` Paul Burton
2019-10-11 16:54 ` Nathan Chancellor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).