[PATCH] bootconfig: Update prototype of setup_boot_config()

[PATCH] bootconfig: Update prototype of setup_boot_config()

[Cao jin]

Parameter "cmdline" has no use, drop it.

Signed-off-by: Cao jin <jojing64@gmail.com>
---
 init/main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/init/main.c b/init/main.c
index c68d784376ca..621a11ed18fb 100644
--- a/init/main.c
+++ b/init/main.c
@@ -404,7 +404,7 @@ static int __init bootconfig_params(char *param, char *val,
 	return 0;
 }
 
-static void __init setup_boot_config(const char *cmdline)
+static void __init setup_boot_config(void)
 {
 	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
 	const char *msg;
@@ -471,7 +471,7 @@ static void __init setup_boot_config(const char *cmdline)
 
 #else
 
-static void __init setup_boot_config(const char *cmdline)
+static void __init setup_boot_config(void)
 {
 	/* Remove bootconfig data from initrd */
 	get_boot_config_from_initrd(NULL, NULL);
@@ -869,7 +869,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
 	pr_notice("%s", linux_banner);
 	early_security_init();
 	setup_arch(&command_line);
-	setup_boot_config(command_line);
+	setup_boot_config();
 	setup_command_line(command_line);
 	setup_nr_cpu_ids();
 	setup_per_cpu_areas();
-- 
2.29.2

Re: [PATCH] bootconfig: Update prototype of setup_boot_config()

[Masami Hiramatsu]

On Thu, 11 Mar 2021 16:52:13 +0800
Cao jin <jojing64@gmail.com> wrote:

> Parameter "cmdline" has no use, drop it.

OK, this looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> 
> Signed-off-by: Cao jin <jojing64@gmail.com>
> ---
>  init/main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index c68d784376ca..621a11ed18fb 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -404,7 +404,7 @@ static int __init bootconfig_params(char *param, char *val,
>  	return 0;
>  }
>  
> -static void __init setup_boot_config(const char *cmdline)
> +static void __init setup_boot_config(void)
>  {
>  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
>  	const char *msg;
> @@ -471,7 +471,7 @@ static void __init setup_boot_config(const char *cmdline)
>  
>  #else
>  
> -static void __init setup_boot_config(const char *cmdline)
> +static void __init setup_boot_config(void)
>  {
>  	/* Remove bootconfig data from initrd */
>  	get_boot_config_from_initrd(NULL, NULL);
> @@ -869,7 +869,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
>  	pr_notice("%s", linux_banner);
>  	early_security_init();
>  	setup_arch(&command_line);
> -	setup_boot_config(command_line);
> +	setup_boot_config();
>  	setup_command_line(command_line);
>  	setup_nr_cpu_ids();
>  	setup_per_cpu_areas();
> -- 
> 2.29.2
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] bootconfig: Update prototype of setup_boot_config()

[Steven Rostedt]

On Thu, 11 Mar 2021 16:52:13 +0800
Cao jin <jojing64@gmail.com> wrote:

> Parameter "cmdline" has no use, drop it.

Actually, I wonder if it should be using that instead of boot_command_line,
as the cmdline passed in is boot_command_line plus anything that the
architecture itself modified.

Masami?

-- Steve

Re: [PATCH] bootconfig: Update prototype of setup_boot_config()

[Steven Rostedt]

On Thu, 11 Mar 2021 18:50:21 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 11 Mar 2021 16:52:13 +0800
> Cao jin <jojing64@gmail.com> wrote:
> 
> > Parameter "cmdline" has no use, drop it.  
> 
> OK, this looks good to me.

Why is this using boot_command_line instead of what is passed in, which
might be different?

-- Steve

Re: [PATCH] bootconfig: Update prototype of setup_boot_config()

[Masami Hiramatsu]

Hi Steve,

On Thu, 11 Mar 2021 09:04:39 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 11 Mar 2021 18:50:21 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Thu, 11 Mar 2021 16:52:13 +0800
> > Cao jin <jojing64@gmail.com> wrote:
> > 
> > > Parameter "cmdline" has no use, drop it.  
> > 
> > OK, this looks good to me.
> 
> Why is this using boot_command_line instead of what is passed in, which
> might be different?

I think you may know the reason...

commit f61872bb58a1cd8f0422aab1940eeee8be579d38
Author: Steven Rostedt (VMware) <rostedt@goodmis.org>
Date:   Fri Feb 7 19:07:37 2020 -0500

    bootconfig: Use parse_args() to find bootconfig and '--'
...
-       p = strstr(cmdline, "bootconfig");
-       if (!p || (p != cmdline && !isspace(*(p-1))) ||
-           (p[10] && !isspace(p[10])))
+       strlcpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
+       parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
+                  bootconfig_params);
+
+       if (!bootconfig_found)

I guess since the boot_command_line has fixed length, it is safer to
allocate fixed length memory for tmp_cmdline. Is that correct?

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] bootconfig: Update prototype of setup_boot_config()

[Masami Hiramatsu]

On Thu, 11 Mar 2021 09:03:21 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 11 Mar 2021 16:52:13 +0800
> Cao jin <jojing64@gmail.com> wrote:
> 
> > Parameter "cmdline" has no use, drop it.
> 
> Actually, I wonder if it should be using that instead of boot_command_line,
> as the cmdline passed in is boot_command_line plus anything that the
> architecture itself modified.
> 
> Masami?

I think the policy is that boot_command_line is the reference command line 
string passed from user, and the passed cmdline is modified by architecutre code.
But those are usually same in most cases, because boot_command_line was also
modified in setup_arch().

For example, x86 setup_arch() overwrites/modifies boot_command_line itself
by builtin_cmdline, and copy boot_command_line into the passed cmdline.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] bootconfig: Update prototype of setup_boot_config()

[Steven Rostedt]

On Fri, 12 Mar 2021 10:44:23 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > Why is this using boot_command_line instead of what is passed in, which
> > might be different?  
> 
> I think you may know the reason...
> 
> commit f61872bb58a1cd8f0422aab1940eeee8be579d38
> Author: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Date:   Fri Feb 7 19:07:37 2020 -0500
> 
>     bootconfig: Use parse_args() to find bootconfig and '--'
> ...
> -       p = strstr(cmdline, "bootconfig");
> -       if (!p || (p != cmdline && !isspace(*(p-1))) ||
> -           (p[10] && !isspace(p[10])))
> +       strlcpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> +       parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
> +                  bootconfig_params);

I knew I shouldn't have trusted that code, based on the author :-p

> +
> +       if (!bootconfig_found)
> 
> I guess since the boot_command_line has fixed length, it is safer to
> allocate fixed length memory for tmp_cmdline. Is that correct?

Yeah, I guess it would be safer. If an arch wants to add bootconfig options
in the setup code, it could then modify how this works in the future. No
need to worry about it now.

Thanks for the review, I'll take this patch then.

-- Steve