* [PATCH] OF: Handle CMDLINE when /chosen node is not present
@ 2018-10-04 12:31 Nick Kossifidis
2018-10-04 18:54 ` Atish Patra
0 siblings, 1 reply; 4+ messages in thread
From: Nick Kossifidis @ 2018-10-04 12:31 UTC (permalink / raw)
To: devicetree; +Cc: Nick Kossifidis, linux-riscv, robh+dt, frowand.list, palmer
The /chosen node is optional so we should handle CMDLINE regardless
the presence of /chosen/bootargs. Move handling of CMDLINE in
early_init_dt_scan() instead.
Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
---
drivers/of/fdt.c | 69 +++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 48 insertions(+), 21 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 800ad252c..868464b0b 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -31,6 +31,14 @@
#include "of_private.h"
+#ifdef CONFIG_CMDLINE
+#ifdef CONFIG_CMDLINE_FORCE
+static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
+#else
+static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
+#endif
+#endif
+
/*
* of_fdt_limit_memory - limit the number of regions in the /memory node
* @limit: maximum entries
@@ -1088,28 +1096,10 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
/* Retrieve command line */
p = of_get_flat_dt_prop(node, "bootargs", &l);
- if (p != NULL && l > 0)
+ if (p != NULL && l > 0) {
strlcpy(data, p, min((int)l, COMMAND_LINE_SIZE));
-
- /*
- * CONFIG_CMDLINE is meant to be a default in case nothing else
- * managed to set the command line, unless CONFIG_CMDLINE_FORCE
- * is set in which case we override whatever was found earlier.
- */
-#ifdef CONFIG_CMDLINE
-#if defined(CONFIG_CMDLINE_EXTEND)
- strlcat(data, " ", COMMAND_LINE_SIZE);
- strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#elif defined(CONFIG_CMDLINE_FORCE)
- strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#else
- /* No arguments from boot loader, use kernel's cmdl*/
- if (!((char *)data)[0])
- strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#endif
-#endif /* CONFIG_CMDLINE */
-
- pr_debug("Command line is: %s\n", (char*)data);
+ pr_debug("Got bootargs: %s\n", (char *) data);
+ }
/* break now */
return 1;
@@ -1240,6 +1230,43 @@ bool __init early_init_dt_scan(void *params)
return false;
early_init_dt_scan_nodes();
+
+ /*
+ * The /chosen node normaly contains the bootargs element
+ * that includes the kernel's command line parameters.
+ * However the presence of /chosen is not mandatory so
+ * in case we didn't get a command line when scanning
+ * nodes above, we should provide one here before we
+ * return, if possible.
+ *
+ * The built-in command line can be used as a default
+ * command line in case we received nothing from the
+ * device tree/bootloader. It can also be used for
+ * extending or replacing the received command line.
+ */
+#ifdef CONFIG_CMDLINE
+#if defined(CONFIG_CMDLINE_EXTEND)
+ /*
+ * Order of parameters shouldn't matter for most cases,
+ * so prepending or appending the built-in command line
+ * shouldn't make a difference. In cases where it does
+ * it's up to the user to configure the kernel and/or
+ * the bootloader properly.
+ */
+ if (builtin_cmdline[0]) {
+ strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
+ strlcat(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+ }
+#elif defined(CONFIG_CMDLINE_FORCE)
+ if (fixed_cmdline[0])
+ strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
+#else
+ /* No arguments from boot loader, use kernel's cmdline */
+ if (!boot_command_line[0] && builtin_cmdline[0])
+ strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+#endif
+#endif /* CONFIG_CMDLINE */
+
return true;
}
--
2.16.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] OF: Handle CMDLINE when /chosen node is not present
2018-10-04 12:31 [PATCH] OF: Handle CMDLINE when /chosen node is not present Nick Kossifidis
@ 2018-10-04 18:54 ` Atish Patra
2018-10-04 20:14 ` Nick Kossifidis
0 siblings, 1 reply; 4+ messages in thread
From: Atish Patra @ 2018-10-04 18:54 UTC (permalink / raw)
To: Nick Kossifidis, devicetree@vger.kernel.org
Cc: linux-riscv@lists.infradead.org, robh+dt@kernel.org,
frowand.list@gmail.com, palmer@sifive.com
On 10/4/18 5:46 AM, Nick Kossifidis wrote:
> The /chosen node is optional so we should handle CMDLINE regardless
> the presence of /chosen/bootargs. Move handling of CMDLINE in
> early_init_dt_scan() instead.
>
> Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
>
> ---
> drivers/of/fdt.c | 69 +++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 48 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 800ad252c..868464b0b 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -31,6 +31,14 @@
>
> #include "of_private.h"
>
> +#ifdef CONFIG_CMDLINE
> +#ifdef CONFIG_CMDLINE_FORCE
> +static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
> +#else
> +static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
> +#endif
> +#endif
> +
> /*
> * of_fdt_limit_memory - limit the number of regions in the /memory node
> * @limit: maximum entries
> @@ -1088,28 +1096,10 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>
> /* Retrieve command line */
> p = of_get_flat_dt_prop(node, "bootargs", &l);
> - if (p != NULL && l > 0)
> + if (p != NULL && l > 0) {
> strlcpy(data, p, min((int)l, COMMAND_LINE_SIZE));
> -
> - /*
> - * CONFIG_CMDLINE is meant to be a default in case nothing else
> - * managed to set the command line, unless CONFIG_CMDLINE_FORCE
> - * is set in which case we override whatever was found earlier.
> - */
> -#ifdef CONFIG_CMDLINE
> -#if defined(CONFIG_CMDLINE_EXTEND)
> - strlcat(data, " ", COMMAND_LINE_SIZE);
> - strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> -#elif defined(CONFIG_CMDLINE_FORCE)
> - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> -#else
> - /* No arguments from boot loader, use kernel's cmdl*/
> - if (!((char *)data)[0])
> - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> -#endif
> -#endif /* CONFIG_CMDLINE */
> -
> - pr_debug("Command line is: %s\n", (char*)data);
> + pr_debug("Got bootargs: %s\n", (char *) data);
> + }
>
> /* break now */
> return 1;
> @@ -1240,6 +1230,43 @@ bool __init early_init_dt_scan(void *params)
> return false;
>
> early_init_dt_scan_nodes();
> +
> + /*
> + * The /chosen node normaly contains the bootargs element
/s/normaly/normally
> + * that includes the kernel's command line parameters.
> + * However the presence of /chosen is not mandatory so
> + * in case we didn't get a command line when scanning
> + * nodes above, we should provide one here before we
> + * return, if possible.
> + *
> + * The built-in command line can be used as a default
> + * command line in case we received nothing from the
> + * device tree/bootloader. It can also be used for
> + * extending or replacing the received command line.
> + */
> +#ifdef CONFIG_CMDLINE
> +#if defined(CONFIG_CMDLINE_EXTEND)
> + /*
> + * Order of parameters shouldn't matter for most cases,
> + * so prepending or appending the built-in command line
> + * shouldn't make a difference. In cases where it does
> + * it's up to the user to configure the kernel and/or
> + * the bootloader properly.
> + */
> + if (builtin_cmdline[0]) {
> + strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> + strlcat(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE)
> + }
> +#elif defined(CONFIG_CMDLINE_FORCE)
> + if (fixed_cmdline[0])
> + strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
> +#else
> + /* No arguments from boot loader, use kernel's cmdline */
> + if (!boot_command_line[0] && builtin_cmdline[0])
> + strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
> +#endif
> +#endif /* CONFIG_CMDLINE */
> +
Any specific reason for doing this in early_init_dt_scan instead of
early_init_dt_scan_chosen?
I think keeping all boot command line modifications at one place makes
more sense.
Regards,
Atish
> return true;
> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] OF: Handle CMDLINE when /chosen node is not present
2018-10-04 18:54 ` Atish Patra
@ 2018-10-04 20:14 ` Nick Kossifidis
2018-10-04 20:42 ` Atish Patra
0 siblings, 1 reply; 4+ messages in thread
From: Nick Kossifidis @ 2018-10-04 20:14 UTC (permalink / raw)
To: Atish Patra
Cc: devicetree, palmer, robh+dt, Nick Kossifidis, linux-riscv,
frowand.list
Στις 2018-10-04 21:54, Atish Patra έγραψε:
> On 10/4/18 5:46 AM, Nick Kossifidis wrote:
>> The /chosen node is optional so we should handle CMDLINE regardless
>> the presence of /chosen/bootargs. Move handling of CMDLINE in
>> early_init_dt_scan() instead.
>>
>> Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
>>
>> ---
>> drivers/of/fdt.c | 69
>> +++++++++++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 48 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 800ad252c..868464b0b 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -31,6 +31,14 @@
>> #include "of_private.h"
>> +#ifdef CONFIG_CMDLINE
>> +#ifdef CONFIG_CMDLINE_FORCE
>> +static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
>> +#else
>> +static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
>> +#endif
>> +#endif
>> +
>> /*
>> * of_fdt_limit_memory - limit the number of regions in the /memory
>> node
>> * @limit: maximum entries
>> @@ -1088,28 +1096,10 @@ int __init early_init_dt_scan_chosen(unsigned
>> long node, const char *uname,
>> /* Retrieve command line */
>> p = of_get_flat_dt_prop(node, "bootargs", &l);
>> - if (p != NULL && l > 0)
>> + if (p != NULL && l > 0) {
>> strlcpy(data, p, min((int)l, COMMAND_LINE_SIZE));
>> -
>> - /*
>> - * CONFIG_CMDLINE is meant to be a default in case nothing else
>> - * managed to set the command line, unless CONFIG_CMDLINE_FORCE
>> - * is set in which case we override whatever was found earlier.
>> - */
>> -#ifdef CONFIG_CMDLINE
>> -#if defined(CONFIG_CMDLINE_EXTEND)
>> - strlcat(data, " ", COMMAND_LINE_SIZE);
>> - strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
>> -#elif defined(CONFIG_CMDLINE_FORCE)
>> - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
>> -#else
>> - /* No arguments from boot loader, use kernel's cmdl*/
>> - if (!((char *)data)[0])
>> - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
>> -#endif
>> -#endif /* CONFIG_CMDLINE */
>> -
>> - pr_debug("Command line is: %s\n", (char*)data);
>> + pr_debug("Got bootargs: %s\n", (char *) data);
>> + }
>> /* break now */
>> return 1;
>> @@ -1240,6 +1230,43 @@ bool __init early_init_dt_scan(void *params)
>> return false;
>> early_init_dt_scan_nodes();
>> +
>> + /*
>> + * The /chosen node normaly contains the bootargs element
>
> /s/normaly/normally
>
ACK
>> + * that includes the kernel's command line parameters.
>> + * However the presence of /chosen is not mandatory so
>> + * in case we didn't get a command line when scanning
>> + * nodes above, we should provide one here before we
>> + * return, if possible.
>> + *
>> + * The built-in command line can be used as a default
>> + * command line in case we received nothing from the
>> + * device tree/bootloader. It can also be used for
>> + * extending or replacing the received command line.
>> + */
>> +#ifdef CONFIG_CMDLINE
>> +#if defined(CONFIG_CMDLINE_EXTEND)
>> + /*
>> + * Order of parameters shouldn't matter for most cases,
>> + * so prepending or appending the built-in command line
>> + * shouldn't make a difference. In cases where it does
>> + * it's up to the user to configure the kernel and/or
>> + * the bootloader properly.
>> + */
>> + if (builtin_cmdline[0]) {
>> + strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
>> + strlcat(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE)
>> + }
>> +#elif defined(CONFIG_CMDLINE_FORCE)
>> + if (fixed_cmdline[0])
>> + strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
>> +#else
>> + /* No arguments from boot loader, use kernel's cmdline */
>> + if (!boot_command_line[0] && builtin_cmdline[0])
>> + strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>> +#endif
>> +#endif /* CONFIG_CMDLINE */
>> +
>
> Any specific reason for doing this in early_init_dt_scan instead of
> early_init_dt_scan_chosen?
> I think keeping all boot command line modifications at one place makes
> more sense.
>
> Regards,
> Atish
When parsing the device tree through early_init_dt_scan, after verifying
the fdt file's
integrity, the code will start traversing through the various nodes of
the tree using
of_scan_flat_dt (on the same file I patched). That function gets a
pointer to a search
function as its first argument and the second argument is an auxiliary
buffer. The
search function will run for all nodes on the tree until it reaches the
expected node
in which case it returns 1 and the search will end.
If we do the tweaking of the command line inside
early_init_dt_scan_chosen before the
check is done, we'll mess up the command line badly since we'll be
tweaking it
for every node on the tree (until we reach the /chosen node, if we reach
it). If we do
it after the check -which is what's happening now- we might not reach
the code since
the /chosen node is optional so the check may never succeed. So we need
to do the check
after the search finishes. I chose to do it on early_init_dt_scan since
it's something
that IMHO should be checked before that function returns.
Regards,
Nick
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] OF: Handle CMDLINE when /chosen node is not present
2018-10-04 20:14 ` Nick Kossifidis
@ 2018-10-04 20:42 ` Atish Patra
0 siblings, 0 replies; 4+ messages in thread
From: Atish Patra @ 2018-10-04 20:42 UTC (permalink / raw)
To: Nick Kossifidis
Cc: devicetree@vger.kernel.org, linux-riscv@lists.infradead.org,
robh+dt@kernel.org, frowand.list@gmail.com, palmer@sifive.com
On 10/4/18 1:15 PM, Nick Kossifidis wrote:
> Στις 2018-10-04 21:54, Atish Patra έγραψε:
>> On 10/4/18 5:46 AM, Nick Kossifidis wrote:
>>> The /chosen node is optional so we should handle CMDLINE regardless
>>> the presence of /chosen/bootargs. Move handling of CMDLINE in
>>> early_init_dt_scan() instead.
>>>
>>> Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
>>>
>>> ---
>>> drivers/of/fdt.c | 69
>>> +++++++++++++++++++++++++++++++++++++++-----------------
>>> 1 file changed, 48 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index 800ad252c..868464b0b 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -31,6 +31,14 @@
>>> #include "of_private.h"
>>> +#ifdef CONFIG_CMDLINE
>>> +#ifdef CONFIG_CMDLINE_FORCE
>>> +static const char fixed_cmdline[] __initconst = CONFIG_CMDLINE;
>>> +#else
>>> +static char builtin_cmdline[] __initdata = CONFIG_CMDLINE;
>>> +#endif
>>> +#endif
>>> +
>>> /*
>>> * of_fdt_limit_memory - limit the number of regions in the /memory
>>> node
>>> * @limit: maximum entries
>>> @@ -1088,28 +1096,10 @@ int __init early_init_dt_scan_chosen(unsigned
>>> long node, const char *uname,
>>> /* Retrieve command line */
>>> p = of_get_flat_dt_prop(node, "bootargs", &l);
>>> - if (p != NULL && l > 0)
>>> + if (p != NULL && l > 0) {
>>> strlcpy(data, p, min((int)l, COMMAND_LINE_SIZE));
>>> -
>>> - /*
>>> - * CONFIG_CMDLINE is meant to be a default in case nothing else
>>> - * managed to set the command line, unless CONFIG_CMDLINE_FORCE
>>> - * is set in which case we override whatever was found earlier.
>>> - */
>>> -#ifdef CONFIG_CMDLINE
>>> -#if defined(CONFIG_CMDLINE_EXTEND)
>>> - strlcat(data, " ", COMMAND_LINE_SIZE);
>>> - strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
>>> -#elif defined(CONFIG_CMDLINE_FORCE)
>>> - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
>>> -#else
>>> - /* No arguments from boot loader, use kernel's cmdl*/
>>> - if (!((char *)data)[0])
>>> - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
>>> -#endif
>>> -#endif /* CONFIG_CMDLINE */
>>> -
>>> - pr_debug("Command line is: %s\n", (char*)data);
>>> + pr_debug("Got bootargs: %s\n", (char *) data);
>>> + }
>>> /* break now */
>>> return 1;
>>> @@ -1240,6 +1230,43 @@ bool __init early_init_dt_scan(void *params)
>>> return false;
>>> early_init_dt_scan_nodes();
>>> +
>>> + /*
>>> + * The /chosen node normaly contains the bootargs element
>>
>> /s/normaly/normally
>>
>
> ACK
>
>>> + * that includes the kernel's command line parameters.
>>> + * However the presence of /chosen is not mandatory so
>>> + * in case we didn't get a command line when scanning
>>> + * nodes above, we should provide one here before we
>>> + * return, if possible.
>>> + *
>>> + * The built-in command line can be used as a default
>>> + * command line in case we received nothing from the
>>> + * device tree/bootloader. It can also be used for
>>> + * extending or replacing the received command line.
>>> + */
>>> +#ifdef CONFIG_CMDLINE
>>> +#if defined(CONFIG_CMDLINE_EXTEND)
>>> + /*
>>> + * Order of parameters shouldn't matter for most cases,
>>> + * so prepending or appending the built-in command line
>>> + * shouldn't make a difference. In cases where it does
>>> + * it's up to the user to configure the kernel and/or
>>> + * the bootloader properly.
>>> + */
>>> + if (builtin_cmdline[0]) {
>>> + strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
>>> + strlcat(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE)
>>> + }
>>> +#elif defined(CONFIG_CMDLINE_FORCE)
>>> + if (fixed_cmdline[0])
>>> + strlcpy(boot_command_line, fixed_cmdline, COMMAND_LINE_SIZE);
>>> +#else
>>> + /* No arguments from boot loader, use kernel's cmdline */
>>> + if (!boot_command_line[0] && builtin_cmdline[0])
>>> + strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>>> +#endif
>>> +#endif /* CONFIG_CMDLINE */
>>> +
>>
>> Any specific reason for doing this in early_init_dt_scan instead of
>> early_init_dt_scan_chosen?
>> I think keeping all boot command line modifications at one place makes
>> more sense.
>>
>> Regards,
>> Atish
>
> When parsing the device tree through early_init_dt_scan, after verifying
> the fdt file's
> integrity, the code will start traversing through the various nodes of
> the tree using
> of_scan_flat_dt (on the same file I patched). That function gets a
> pointer to a search
> function as its first argument and the second argument is an auxiliary
> buffer. The
> search function will run for all nodes on the tree until it reaches the
> expected node
> in which case it returns 1 and the search will end.
>
> If we do the tweaking of the command line inside
> early_init_dt_scan_chosen before the
> check is done, we'll mess up the command line badly since we'll be
> tweaking it
> for every node on the tree (until we reach the /chosen node, if we reach
> it). If we do
> it after the check -which is what's happening now- we might not reach
> the code since
> the /chosen node is optional so the check may never succeed. So we need
> to do the check
> after the search finishes. I chose to do it on early_init_dt_scan since
> it's something
> that IMHO should be checked before that function returns.
>
It makes sense. Thanks.
Regards,
Atish
> Regards,
> Nick
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-10-04 20:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-04 12:31 [PATCH] OF: Handle CMDLINE when /chosen node is not present Nick Kossifidis
2018-10-04 18:54 ` Atish Patra
2018-10-04 20:14 ` Nick Kossifidis
2018-10-04 20:42 ` Atish Patra
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).