RISC-V: Implement built-in command line feature
PATCH 1/1 RISC-V: Implement built-in command line feature
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.
--- arch/riscv/kernel/setup.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
arch/riscv/kernel/setup.c +49 -0
@@ -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 ();
Signature · 1 line
2.16.4
PATCH 1/1 RISC-V: Implement built-in command line feature
arch/riscv/kernel/setup.c line 49
Quoted 7 lines
arch/riscv/kernel/setup.c +7
-0+#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.
arch/riscv/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 228
Quoted 1 line
arch/riscv/kernel/setup.c +1
-0+/* 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.
Quoted 7 lines
arch/riscv/kernel/setup.c +7
-0+#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
arch/riscv/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 265
Quoted 10 lines
arch/riscv/kernel/setup.c +10
-0+/* 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
PATCH 1/1 RISC-V: Implement built-in command line feature
???? 2018-10-02 17:34, Christoph Hellwig ??????:
Quoted 11 lines
arch/riscv/kernel/setup.c line 49
Quoted 7 lines
arch/riscv/kernel/setup.c +7
-0+#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_cmdlinecommand 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.
Quoted 2 lines
Also please don't indent code inside cpp conditionals.
ACK (although there is nothing against it on the kernel coding guidelines)
Quoted 5 lines
arch/riscv/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 228
Quoted 1 line
arch/riscv/kernel/setup.c +1
-0+/* 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.
Quoted 29 lines
Quoted 7 lines
arch/riscv/kernel/setup.c +7
-0+#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 arch/riscv/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 265
Quoted 10 lines
arch/riscv/kernel/setup.c +10
-0+/* 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); }
Quoted 1 line
#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.
PATCH 1/1 RISC-V: Implement built-in command line feature
In reply to Nick KossifidisQuoted 4 lines · mick at ics.forth.gr
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 <[email protected]>
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!
Quoted 78 lines
--- arch/riscv/kernel/setup.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)arch/riscv/kernel/setup.c +49
-0@@ -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 ();
PATCH 1/1 RISC-V: Implement built-in command line feature
???? 2018-10-02 17:56, Palmer Dabbelt ??????:
Quoted 20 lines · mick at ics.forth.gr
Quoted 5 lines
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 <[email protected]>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()indrivers/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 setboot_command_line) when there is no /chosen node. The fix might be as simple as checking for a /chosen node inearly_init_dt_scan_nodes(), and calling theCONFIG_CMDLINEhandling if there's no /chosen node.
Quoted 7 lines
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 ?
PATCH 1/1 RISC-V: Implement built-in command line feature
Quoted 26 lines · mick at ics.forth.gr
???? 2018-10-02 17:56, Palmer Dabbelt ??????:
Quoted 25 lines · mick at ics.forth.gr
Quoted 5 lines
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 <[email protected]>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()indrivers/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 setboot_command_line) when there is no /chosen node. The fix might be as simple as checking for a /chosen node inearly_init_dt_scan_nodes(), and calling theCONFIG_CMDLINEhandling 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.
Quoted 8 lines
Quoted 2 lines
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_EXTENDfor this to work as expected so I guess I'll send a series to also tweakKconfigagain. Do you want me to work on top of my previousKconfigpatch 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!