* [PATCH] RISC-V: Implement built-in command line feature
@ 2018-10-02 14:04 ` Nick Kossifidis
0 siblings, 0 replies; 12+ messages in thread
From: Nick Kossifidis @ 2018-10-02 14:04 UTC (permalink / raw)
To: linux-riscv; +Cc: Nick Kossifidis, palmer, aou
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH] RISC-V: Implement built-in command line feature
@ 2018-10-02 14:34 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2018-10-02 14:34 UTC (permalink / raw)
To: linux-riscv
> +#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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] RISC-V: Implement built-in command line feature
@ 2018-10-02 14:34 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2018-10-02 14:34 UTC (permalink / raw)
To: Nick Kossifidis; +Cc: linux-riscv, palmer, aou
> +#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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH] RISC-V: Implement built-in command line feature
@ 2018-10-02 15:21 ` Nick Kossifidis
0 siblings, 0 replies; 12+ messages in thread
From: Nick Kossifidis @ 2018-10-02 15:21 UTC (permalink / raw)
To: linux-riscv
???? 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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] RISC-V: Implement built-in command line feature
@ 2018-10-02 15:21 ` Nick Kossifidis
0 siblings, 0 replies; 12+ messages in thread
From: Nick Kossifidis @ 2018-10-02 15:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Nick Kossifidis, linux-riscv, palmer, aou
Στις 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] RISC-V: Implement built-in command line feature
@ 2018-10-02 14:56 ` Palmer Dabbelt
0 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2018-10-02 14:56 UTC (permalink / raw)
To: linux-riscv
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();
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] RISC-V: Implement built-in command line feature
@ 2018-10-02 14:56 ` Palmer Dabbelt
0 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2018-10-02 14:56 UTC (permalink / raw)
To: mick; +Cc: mick, linux-riscv, aou
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH] RISC-V: Implement built-in command line feature
@ 2018-10-02 16:43 ` Nick Kossifidis
0 siblings, 0 replies; 12+ messages in thread
From: Nick Kossifidis @ 2018-10-02 16:43 UTC (permalink / raw)
To: linux-riscv
???? 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 ?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] RISC-V: Implement built-in command line feature
@ 2018-10-02 16:43 ` Nick Kossifidis
0 siblings, 0 replies; 12+ messages in thread
From: Nick Kossifidis @ 2018-10-02 16:43 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: mick, linux-riscv, aou
Στις 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] RISC-V: Implement built-in command line feature
@ 2018-10-02 16:56 ` Palmer Dabbelt
0 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2018-10-02 16:56 UTC (permalink / raw)
To: linux-riscv
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!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] RISC-V: Implement built-in command line feature
@ 2018-10-02 16:56 ` Palmer Dabbelt
0 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2018-10-02 16:56 UTC (permalink / raw)
To: mick; +Cc: mick, linux-riscv, aou
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
^ permalink raw reply [flat|nested] 12+ messages in thread