[PATCH] RISC-V: Implement built-in command line feature

[PATCH] RISC-V: Implement built-in command line feature

[Nick Kossifidis]

This patch enables the use of a built-in kernel command line, which can
optionaly also override the command line provided by the boot loader.

Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
---
 arch/riscv/kernel/setup.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index aee603123..fd171ca7f 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -30,6 +30,7 @@
 #include <linux/of_platform.h>
 #include <linux/sched/task.h>
 #include <linux/swiotlb.h>
+#include <linux/string.h>
 
 #include <asm/setup.h>
 #include <asm/sections.h>
@@ -39,6 +40,14 @@
 #include <asm/tlbflush.h>
 #include <asm/thread_info.h>
 
+#ifdef CONFIG_CMDLINE_BOOL
+#ifdef CONFIG_CMDLINE_FORCE
+	static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
+#else
+	static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
+#endif
+#endif
+
 #ifdef CONFIG_EARLY_PRINTK
 static void sbi_console_write(struct console *co, const char *buf,
 			      unsigned int n)
@@ -215,6 +224,46 @@ void __init setup_arch(char **cmdline_p)
                register_console(early_console);
        }
 #endif
+
+	/* When we return, cmdline_p should point to a temporary
+	 * buffer on the kernel's init section (that gets cleaned up
+	 * later on) to be saved on static_command_line and parsed for
+	 * parameters.
+	 *
+	 * That string may get tweaked during parameter parsing so we
+	 * also want to keep an untouched version of the command line
+	 * to display on dmesg, /proc/cmdline etc.
+	 *
+	 * At this point boot_command_line is a temporary buffer
+	 * (also on the init section) that contains the command line
+	 * string passed from the boot loader. After we return,
+	 * it will be saved to saved_command_line that holds the
+	 * untouched version of the command line.
+	 *
+	 * Since we won't be doing any parameter parsing here, we'll
+	 * hold the initial/untouched command line string on
+	 * boot_command_line and make cmdline_p point to it as well.
+	 * This way boot_command_line will be copied to both
+	 * saved_command_line and static_command_line.
+	 */
+#ifdef CONFIG_CMDLINE_BOOL
+#ifdef CONFIG_CMDLINE_FORCE
+	/* Built-in args override boot loader args and
+	 * boot loader args are being ignored.
+	 */
+	strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
+#else
+	/* Boot loader args override built-in args so we
+	 * need to put built-in args before boot loader args.
+	 */
+	if (builtin_cmdline[0]) {
+		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
+		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
+		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+	}
+#endif
+#endif
+
 	*cmdline_p = boot_command_line;
 
 	parse_early_param();
-- 
2.16.4

[PATCH] RISC-V: Implement built-in command line feature

[Nick Kossifidis]

This patch enables the use of a built-in kernel command line, which can
optionaly also override the command line provided by the boot loader.

Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
---
 arch/riscv/kernel/setup.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index aee603123..fd171ca7f 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -30,6 +30,7 @@
 #include <linux/of_platform.h>
 #include <linux/sched/task.h>
 #include <linux/swiotlb.h>
+#include <linux/string.h>
 
 #include <asm/setup.h>
 #include <asm/sections.h>
@@ -39,6 +40,14 @@
 #include <asm/tlbflush.h>
 #include <asm/thread_info.h>
 
+#ifdef CONFIG_CMDLINE_BOOL
+#ifdef CONFIG_CMDLINE_FORCE
+	static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
+#else
+	static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
+#endif
+#endif
+
 #ifdef CONFIG_EARLY_PRINTK
 static void sbi_console_write(struct console *co, const char *buf,
 			      unsigned int n)
@@ -215,6 +224,46 @@ void __init setup_arch(char **cmdline_p)
                register_console(early_console);
        }
 #endif
+
+	/* When we return, cmdline_p should point to a temporary
+	 * buffer on the kernel's init section (that gets cleaned up
+	 * later on) to be saved on static_command_line and parsed for
+	 * parameters.
+	 *
+	 * That string may get tweaked during parameter parsing so we
+	 * also want to keep an untouched version of the command line
+	 * to display on dmesg, /proc/cmdline etc.
+	 *
+	 * At this point boot_command_line is a temporary buffer
+	 * (also on the init section) that contains the command line
+	 * string passed from the boot loader. After we return,
+	 * it will be saved to saved_command_line that holds the
+	 * untouched version of the command line.
+	 *
+	 * Since we won't be doing any parameter parsing here, we'll
+	 * hold the initial/untouched command line string on
+	 * boot_command_line and make cmdline_p point to it as well.
+	 * This way boot_command_line will be copied to both
+	 * saved_command_line and static_command_line.
+	 */
+#ifdef CONFIG_CMDLINE_BOOL
+#ifdef CONFIG_CMDLINE_FORCE
+	/* Built-in args override boot loader args and
+	 * boot loader args are being ignored.
+	 */
+	strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
+#else
+	/* Boot loader args override built-in args so we
+	 * need to put built-in args before boot loader args.
+	 */
+	if (builtin_cmdline[0]) {
+		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
+		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
+		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+	}
+#endif
+#endif
+
 	*cmdline_p = boot_command_line;
 
 	parse_early_param();
-- 
2.16.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH] RISC-V: Implement built-in command line feature

[Christoph Hellwig]

> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
> +#else
> +	static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
> +#endif
> +#endif

Why do you need two variables here?  x86, where this same logic is used
seems to get away with just a builtin_cmdline command line one.

Also please don't indent code inside cpp conditionals.

> +	/* When we return, cmdline_p should point to a temporary

This is not the normal kernel comment style.  Please keep the
/* on a line of its own.

> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	/* Built-in args override boot loader args and
> +	 * boot loader args are being ignored.
> +	 */
> +	strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
> +#else
> +	/* Boot loader args override built-in args so we
> +	 * need to put built-in args before boot loader args.
> +	 */
> +	if (builtin_cmdline[0]) {
> +		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> +		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> +		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> +	}
> +#endif
> +#endif

Even if this seems mostly copied - it probably should be #if/#elif
instead of this convoluted magic:

#if defined(CONFIG_CMDLINE_FORCE)
	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
#elif defined(CONFIG_CMDLINE_BOOL)
	if (builtin_cmdline[0]) {
		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
	}
#endif

Re: [PATCH] RISC-V: Implement built-in command line feature

[Christoph Hellwig]

> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
> +#else
> +	static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
> +#endif
> +#endif

Why do you need two variables here?  x86, where this same logic is used
seems to get away with just a builtin_cmdline command line one.

Also please don't indent code inside cpp conditionals.

> +	/* When we return, cmdline_p should point to a temporary

This is not the normal kernel comment style.  Please keep the
/* on a line of its own.

> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	/* Built-in args override boot loader args and
> +	 * boot loader args are being ignored.
> +	 */
> +	strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
> +#else
> +	/* Boot loader args override built-in args so we
> +	 * need to put built-in args before boot loader args.
> +	 */
> +	if (builtin_cmdline[0]) {
> +		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> +		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> +		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> +	}
> +#endif
> +#endif

Even if this seems mostly copied - it probably should be #if/#elif
instead of this convoluted magic:

#if defined(CONFIG_CMDLINE_FORCE)
	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
#elif defined(CONFIG_CMDLINE_BOOL)
	if (builtin_cmdline[0]) {
		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
	}
#endif

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH] RISC-V: Implement built-in command line feature

[Palmer Dabbelt]

On Tue, 02 Oct 2018 07:04:49 PDT (-0700), mick at ics.forth.gr wrote:
> This patch enables the use of a built-in kernel command line, which can
> optionaly also override the command line provided by the boot loader.
>
> Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>

Christoph's comments are valid, but I have a bigger one: our original plan was 
to fix the generic support for CONFIG_CMDLINE, and while I'd still prefer to do 
that our original attempt got hung up.  A working implementation trumps a clean 
one, but I'd still prefer the clean one if you have time to take a look.

The offending function is early_init_dt_scan_chosen() in drivers/of/fdt.c.  The 
issue is that this is only called when a chosen node is present, which doesn't 
get called (and therefor doesn't set boot_command_line) when there is no 
/chosen node.  The fix might be as simple as checking for a /chosen node in 
early_init_dt_scan_nodes(), and calling the CONFIG_CMDLINE handling if there's 
no /chosen node.

If that's too much work I can add it to my TODO list, but that never gets 
shorter :).  Given that last time we tried messing with this we broke things 
multiple times, I'd prefer to have this on for-next for a bit first either way, 
so there's no big rush on my end.

Thanks for the patch!

> ---
>  arch/riscv/kernel/setup.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index aee603123..fd171ca7f 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -30,6 +30,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/sched/task.h>
>  #include <linux/swiotlb.h>
> +#include <linux/string.h>
>
>  #include <asm/setup.h>
>  #include <asm/sections.h>
> @@ -39,6 +40,14 @@
>  #include <asm/tlbflush.h>
>  #include <asm/thread_info.h>
>
> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
> +#else
> +	static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
> +#endif
> +#endif
> +
>  #ifdef CONFIG_EARLY_PRINTK
>  static void sbi_console_write(struct console *co, const char *buf,
>  			      unsigned int n)
> @@ -215,6 +224,46 @@ void __init setup_arch(char **cmdline_p)
>                 register_console(early_console);
>         }
>  #endif
> +
> +	/* When we return, cmdline_p should point to a temporary
> +	 * buffer on the kernel's init section (that gets cleaned up
> +	 * later on) to be saved on static_command_line and parsed for
> +	 * parameters.
> +	 *
> +	 * That string may get tweaked during parameter parsing so we
> +	 * also want to keep an untouched version of the command line
> +	 * to display on dmesg, /proc/cmdline etc.
> +	 *
> +	 * At this point boot_command_line is a temporary buffer
> +	 * (also on the init section) that contains the command line
> +	 * string passed from the boot loader. After we return,
> +	 * it will be saved to saved_command_line that holds the
> +	 * untouched version of the command line.
> +	 *
> +	 * Since we won't be doing any parameter parsing here, we'll
> +	 * hold the initial/untouched command line string on
> +	 * boot_command_line and make cmdline_p point to it as well.
> +	 * This way boot_command_line will be copied to both
> +	 * saved_command_line and static_command_line.
> +	 */
> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	/* Built-in args override boot loader args and
> +	 * boot loader args are being ignored.
> +	 */
> +	strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
> +#else
> +	/* Boot loader args override built-in args so we
> +	 * need to put built-in args before boot loader args.
> +	 */
> +	if (builtin_cmdline[0]) {
> +		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> +		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> +		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> +	}
> +#endif
> +#endif
> +
>  	*cmdline_p = boot_command_line;
>
>  	parse_early_param();

Re: [PATCH] RISC-V: Implement built-in command line feature

[Palmer Dabbelt]

On Tue, 02 Oct 2018 07:04:49 PDT (-0700), mick@ics.forth.gr wrote:
> This patch enables the use of a built-in kernel command line, which can
> optionaly also override the command line provided by the boot loader.
>
> Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>

Christoph's comments are valid, but I have a bigger one: our original plan was 
to fix the generic support for CONFIG_CMDLINE, and while I'd still prefer to do 
that our original attempt got hung up.  A working implementation trumps a clean 
one, but I'd still prefer the clean one if you have time to take a look.

The offending function is early_init_dt_scan_chosen() in drivers/of/fdt.c.  The 
issue is that this is only called when a chosen node is present, which doesn't 
get called (and therefor doesn't set boot_command_line) when there is no 
/chosen node.  The fix might be as simple as checking for a /chosen node in 
early_init_dt_scan_nodes(), and calling the CONFIG_CMDLINE handling if there's 
no /chosen node.

If that's too much work I can add it to my TODO list, but that never gets 
shorter :).  Given that last time we tried messing with this we broke things 
multiple times, I'd prefer to have this on for-next for a bit first either way, 
so there's no big rush on my end.

Thanks for the patch!

> ---
>  arch/riscv/kernel/setup.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index aee603123..fd171ca7f 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -30,6 +30,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/sched/task.h>
>  #include <linux/swiotlb.h>
> +#include <linux/string.h>
>
>  #include <asm/setup.h>
>  #include <asm/sections.h>
> @@ -39,6 +40,14 @@
>  #include <asm/tlbflush.h>
>  #include <asm/thread_info.h>
>
> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
> +#else
> +	static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
> +#endif
> +#endif
> +
>  #ifdef CONFIG_EARLY_PRINTK
>  static void sbi_console_write(struct console *co, const char *buf,
>  			      unsigned int n)
> @@ -215,6 +224,46 @@ void __init setup_arch(char **cmdline_p)
>                 register_console(early_console);
>         }
>  #endif
> +
> +	/* When we return, cmdline_p should point to a temporary
> +	 * buffer on the kernel's init section (that gets cleaned up
> +	 * later on) to be saved on static_command_line and parsed for
> +	 * parameters.
> +	 *
> +	 * That string may get tweaked during parameter parsing so we
> +	 * also want to keep an untouched version of the command line
> +	 * to display on dmesg, /proc/cmdline etc.
> +	 *
> +	 * At this point boot_command_line is a temporary buffer
> +	 * (also on the init section) that contains the command line
> +	 * string passed from the boot loader. After we return,
> +	 * it will be saved to saved_command_line that holds the
> +	 * untouched version of the command line.
> +	 *
> +	 * Since we won't be doing any parameter parsing here, we'll
> +	 * hold the initial/untouched command line string on
> +	 * boot_command_line and make cmdline_p point to it as well.
> +	 * This way boot_command_line will be copied to both
> +	 * saved_command_line and static_command_line.
> +	 */
> +#ifdef CONFIG_CMDLINE_BOOL
> +#ifdef CONFIG_CMDLINE_FORCE
> +	/* Built-in args override boot loader args and
> +	 * boot loader args are being ignored.
> +	 */
> +	strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
> +#else
> +	/* Boot loader args override built-in args so we
> +	 * need to put built-in args before boot loader args.
> +	 */
> +	if (builtin_cmdline[0]) {
> +		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> +		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> +		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> +	}
> +#endif
> +#endif
> +
>  	*cmdline_p = boot_command_line;
>
>  	parse_early_param();

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH] RISC-V: Implement built-in command line feature

[Nick Kossifidis]

???? 2018-10-02 17:34, Christoph Hellwig ??????:
>> +#ifdef CONFIG_CMDLINE_BOOL
>> +#ifdef CONFIG_CMDLINE_FORCE
>> +	static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
>> +#else
>> +	static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
>> +#endif
>> +#endif
> 
> Why do you need two variables here?  x86, where this same logic is used
> seems to get away with just a builtin_cmdline command line one.
> 

x86 actually uses an extra buffer named command_line, copies 
boot_command_line
there and returns a pointer to command_line (instead of just returning a 
pointer
to boot_command_line). What I do here is in case we have a forced 
command line,
I prefer having it as const (since the buffer won't get modified) 
instead of
having it as rw. I believe it's cleaner and the name is more 
representative.

> Also please don't indent code inside cpp conditionals.
> 

ACK (although there is nothing against it on the kernel coding 
guidelines)

>> +	/* When we return, cmdline_p should point to a temporary
> 
> This is not the normal kernel comment style.  Please keep the
> /* on a line of its own.
> 

It is for drivers/net where I've mostly worked (drivers/net/wireless) 
but you are
right, I'll fix it and re-send.

>> +#ifdef CONFIG_CMDLINE_BOOL
>> +#ifdef CONFIG_CMDLINE_FORCE
>> +	/* Built-in args override boot loader args and
>> +	 * boot loader args are being ignored.
>> +	 */
>> +	strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
>> +#else
>> +	/* Boot loader args override built-in args so we
>> +	 * need to put built-in args before boot loader args.
>> +	 */
>> +	if (builtin_cmdline[0]) {
>> +		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
>> +		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>> +		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>> +	}
>> +#endif
>> +#endif
> 
> Even if this seems mostly copied - it probably should be #if/#elif
> instead of this convoluted magic:
> 
> #if defined(CONFIG_CMDLINE_FORCE)
> 	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> #elif defined(CONFIG_CMDLINE_BOOL)
> 	if (builtin_cmdline[0]) {
> 		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> 		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> 		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> 	}
> #endif

There is a reason behind this "magic" IMHO, in your approach the reader 
doesn't realize
that CONFIG_CMDLINE_FORCE depends on CONFIG_CMDLINE_BOOL.

Re: [PATCH] RISC-V: Implement built-in command line feature

[Nick Kossifidis]

Στις 2018-10-02 17:34, Christoph Hellwig έγραψε:
>> +#ifdef CONFIG_CMDLINE_BOOL
>> +#ifdef CONFIG_CMDLINE_FORCE
>> +	static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
>> +#else
>> +	static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
>> +#endif
>> +#endif
> 
> Why do you need two variables here?  x86, where this same logic is used
> seems to get away with just a builtin_cmdline command line one.
> 

x86 actually uses an extra buffer named command_line, copies 
boot_command_line
there and returns a pointer to command_line (instead of just returning a 
pointer
to boot_command_line). What I do here is in case we have a forced 
command line,
I prefer having it as const (since the buffer won't get modified) 
instead of
having it as rw. I believe it's cleaner and the name is more 
representative.

> Also please don't indent code inside cpp conditionals.
> 

ACK (although there is nothing against it on the kernel coding 
guidelines)

>> +	/* When we return, cmdline_p should point to a temporary
> 
> This is not the normal kernel comment style.  Please keep the
> /* on a line of its own.
> 

It is for drivers/net where I've mostly worked (drivers/net/wireless) 
but you are
right, I'll fix it and re-send.

>> +#ifdef CONFIG_CMDLINE_BOOL
>> +#ifdef CONFIG_CMDLINE_FORCE
>> +	/* Built-in args override boot loader args and
>> +	 * boot loader args are being ignored.
>> +	 */
>> +	strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
>> +#else
>> +	/* Boot loader args override built-in args so we
>> +	 * need to put built-in args before boot loader args.
>> +	 */
>> +	if (builtin_cmdline[0]) {
>> +		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
>> +		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>> +		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>> +	}
>> +#endif
>> +#endif
> 
> Even if this seems mostly copied - it probably should be #if/#elif
> instead of this convoluted magic:
> 
> #if defined(CONFIG_CMDLINE_FORCE)
> 	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> #elif defined(CONFIG_CMDLINE_BOOL)
> 	if (builtin_cmdline[0]) {
> 		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
> 		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> 		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> 	}
> #endif

There is a reason behind this "magic" IMHO, in your approach the reader 
doesn't realize
that CONFIG_CMDLINE_FORCE depends on CONFIG_CMDLINE_BOOL.


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH] RISC-V: Implement built-in command line feature

[Nick Kossifidis]

???? 2018-10-02 17:56, Palmer Dabbelt ??????:
> On Tue, 02 Oct 2018 07:04:49 PDT (-0700), mick at ics.forth.gr wrote:
>> This patch enables the use of a built-in kernel command line, which 
>> can
>> optionaly also override the command line provided by the boot loader.
>> 
>> Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
> 
> Christoph's comments are valid, but I have a bigger one: our original
> plan was to fix the generic support for CONFIG_CMDLINE, and while I'd
> still prefer to do that our original attempt got hung up.  A working
> implementation trumps a clean one, but I'd still prefer the clean one
> if you have time to take a look.
> 
> The offending function is early_init_dt_scan_chosen() in
> drivers/of/fdt.c.  The issue is that this is only called when a chosen
> node is present, which doesn't get called (and therefor doesn't set
> boot_command_line) when there is no /chosen node.  The fix might be as
> simple as checking for a /chosen node in early_init_dt_scan_nodes(),
> and calling the CONFIG_CMDLINE handling if there's no /chosen node.
> 
> If that's too much work I can add it to my TODO list, but that never
> gets shorter :).  Given that last time we tried messing with this we
> broke things multiple times, I'd prefer to have this on for-next for a
> bit first either way, so there's no big rush on my end.
> 
> Thanks for the patch!
> 

ACK I'll work on that instead, it makes more sense if we only get the
boot arguments through the device tree. We'll also need to define
CONFIG_CMDLINE_EXTEND for this to work as expected so I guess I'll send
a series to also tweak Kconfig again. Do you want me to work on top of
my previous Kconfig patch or should I send a new series from scratch ?

Re: [PATCH] RISC-V: Implement built-in command line feature

[Nick Kossifidis]

Στις 2018-10-02 17:56, Palmer Dabbelt έγραψε:
> On Tue, 02 Oct 2018 07:04:49 PDT (-0700), mick@ics.forth.gr wrote:
>> This patch enables the use of a built-in kernel command line, which 
>> can
>> optionaly also override the command line provided by the boot loader.
>> 
>> Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
> 
> Christoph's comments are valid, but I have a bigger one: our original
> plan was to fix the generic support for CONFIG_CMDLINE, and while I'd
> still prefer to do that our original attempt got hung up.  A working
> implementation trumps a clean one, but I'd still prefer the clean one
> if you have time to take a look.
> 
> The offending function is early_init_dt_scan_chosen() in
> drivers/of/fdt.c.  The issue is that this is only called when a chosen
> node is present, which doesn't get called (and therefor doesn't set
> boot_command_line) when there is no /chosen node.  The fix might be as
> simple as checking for a /chosen node in early_init_dt_scan_nodes(),
> and calling the CONFIG_CMDLINE handling if there's no /chosen node.
> 
> If that's too much work I can add it to my TODO list, but that never
> gets shorter :).  Given that last time we tried messing with this we
> broke things multiple times, I'd prefer to have this on for-next for a
> bit first either way, so there's no big rush on my end.
> 
> Thanks for the patch!
> 

ACK I'll work on that instead, it makes more sense if we only get the
boot arguments through the device tree. We'll also need to define
CONFIG_CMDLINE_EXTEND for this to work as expected so I guess I'll send
a series to also tweak Kconfig again. Do you want me to work on top of
my previous Kconfig patch or should I send a new series from scratch ?

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH] RISC-V: Implement built-in command line feature

[Palmer Dabbelt]

On Tue, 02 Oct 2018 09:43:48 PDT (-0700), mick at ics.forth.gr wrote:
> ???? 2018-10-02 17:56, Palmer Dabbelt ??????:
>> On Tue, 02 Oct 2018 07:04:49 PDT (-0700), mick at ics.forth.gr wrote:
>>> This patch enables the use of a built-in kernel command line, which
>>> can
>>> optionaly also override the command line provided by the boot loader.
>>>
>>> Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
>>
>> Christoph's comments are valid, but I have a bigger one: our original
>> plan was to fix the generic support for CONFIG_CMDLINE, and while I'd
>> still prefer to do that our original attempt got hung up.  A working
>> implementation trumps a clean one, but I'd still prefer the clean one
>> if you have time to take a look.
>>
>> The offending function is early_init_dt_scan_chosen() in
>> drivers/of/fdt.c.  The issue is that this is only called when a chosen
>> node is present, which doesn't get called (and therefor doesn't set
>> boot_command_line) when there is no /chosen node.  The fix might be as
>> simple as checking for a /chosen node in early_init_dt_scan_nodes(),
>> and calling the CONFIG_CMDLINE handling if there's no /chosen node.
>>
>> If that's too much work I can add it to my TODO list, but that never
>> gets shorter :).  Given that last time we tried messing with this we
>> broke things multiple times, I'd prefer to have this on for-next for a
>> bit first either way, so there's no big rush on my end.
>>
>> Thanks for the patch!
>>
>
> ACK I'll work on that instead, it makes more sense if we only get the
> boot arguments through the device tree. We'll also need to define
> CONFIG_CMDLINE_EXTEND for this to work as expected so I guess I'll send
> a series to also tweak Kconfig again. Do you want me to work on top of
> my previous Kconfig patch or should I send a new series from scratch ?

I think you should work on top of your kconfig patch, as it's in for-next now 
so it'll get pulled in even if this doesn't.

Thanks!

Re: [PATCH] RISC-V: Implement built-in command line feature

[Palmer Dabbelt]

On Tue, 02 Oct 2018 09:43:48 PDT (-0700), mick@ics.forth.gr wrote:
> Στις 2018-10-02 17:56, Palmer Dabbelt έγραψε:
>> On Tue, 02 Oct 2018 07:04:49 PDT (-0700), mick@ics.forth.gr wrote:
>>> This patch enables the use of a built-in kernel command line, which
>>> can
>>> optionaly also override the command line provided by the boot loader.
>>>
>>> Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
>>
>> Christoph's comments are valid, but I have a bigger one: our original
>> plan was to fix the generic support for CONFIG_CMDLINE, and while I'd
>> still prefer to do that our original attempt got hung up.  A working
>> implementation trumps a clean one, but I'd still prefer the clean one
>> if you have time to take a look.
>>
>> The offending function is early_init_dt_scan_chosen() in
>> drivers/of/fdt.c.  The issue is that this is only called when a chosen
>> node is present, which doesn't get called (and therefor doesn't set
>> boot_command_line) when there is no /chosen node.  The fix might be as
>> simple as checking for a /chosen node in early_init_dt_scan_nodes(),
>> and calling the CONFIG_CMDLINE handling if there's no /chosen node.
>>
>> If that's too much work I can add it to my TODO list, but that never
>> gets shorter :).  Given that last time we tried messing with this we
>> broke things multiple times, I'd prefer to have this on for-next for a
>> bit first either way, so there's no big rush on my end.
>>
>> Thanks for the patch!
>>
>
> ACK I'll work on that instead, it makes more sense if we only get the
> boot arguments through the device tree. We'll also need to define
> CONFIG_CMDLINE_EXTEND for this to work as expected so I guess I'll send
> a series to also tweak Kconfig again. Do you want me to work on top of
> my previous Kconfig patch or should I send a new series from scratch ?

I think you should work on top of your kconfig patch, as it's in for-next now 
so it'll get pulled in even if this doesn't.

Thanks!

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv