Re: [PATCH] OF: Handle CMDLINE when /chosen node is not present

[Nick Kossifidis <mick@ics.forth.gr>]

Στις 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