diff mbox

arm: dt: Initialize boot_command_line from CONFIG_CMDLINE in case DT does not provide /chosen/bootargs

Message ID 20161215100931.GJ14217@n2100.armlinux.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Russell King (Oracle) Dec. 15, 2016, 10:09 a.m. UTC 
On Thu, Dec 15, 2016 at 01:09:20AM +0100, Pali Rohár wrote:
> If cmdline is not in DT, but /chosen exists, then function 
> early_init_dt_scan_chosen() use cmdline from CONFIG_CMDLINE.

Ah, yes.  Looks to me then as if the bug exists there, and not in arch
code then.  early_init_dt_scan_chosen() completely ignores the CMDLINE
options if the chosen node is not found.

> What is reason that CONFIG_CMDLINE is not supported for DT?

Sorry, that's my mistake - as you've pointed out above, it is supported
but via generic code.  I was only looking at arch code when I made the
statement.

This patch (untested) should solve it:

 drivers/of/fdt.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

Comments

Pali Rohár Dec. 16, 2016, 11:42 a.m. UTC | #1 
On Thursday 15 December 2016 11:09:32 Russell King - ARM Linux wrote:
> On Thu, Dec 15, 2016 at 01:09:20AM +0100, Pali Rohár wrote:
> > If cmdline is not in DT, but /chosen exists, then function
> > early_init_dt_scan_chosen() use cmdline from CONFIG_CMDLINE.
> 
> Ah, yes.  Looks to me then as if the bug exists there, and not in
> arch code then.  early_init_dt_scan_chosen() completely ignores the
> CMDLINE options if the chosen node is not found.

Yes... but question is: shoud function named early_init_dt_scan_chosen() 
use CONFIG_CMDLINE if there is DT entry? From name dt_scan_chosen I 
would expect that it scan /chosen and set some fields from /chosen...

At least name of that function is confusing.

> > What is reason that CONFIG_CMDLINE is not supported for DT?
> 
> Sorry, that's my mistake - as you've pointed out above, it is
> supported but via generic code.  I was only looking at arch code
> when I made the statement.
> 
> This patch (untested) should solve it:
> 
>  drivers/of/fdt.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index c89d5d231a0e..fb89157332c6 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1073,26 +1073,6 @@ int __init early_init_dt_scan_chosen(unsigned
> long node, const char *uname, 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);
> -
>  	/* break now */
>  	return 1;
>  }
> @@ -1205,6 +1185,26 @@ void __init early_init_dt_scan_nodes(void)
>  	/* Retrieve various information from the /chosen node */
>  	of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
> 
> +	/*
> +	 * 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(boot_command_line, " ", COMMAND_LINE_SIZE);
> +	strlcat(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> +#elif defined(CONFIG_CMDLINE_FORCE)
> +	strlcpy(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> +#else
> +	/* No arguments from boot loader, use kernel's cmdline */
> +	if (!boot_command_line[0])
> +		strlcpy(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> +#endif
> +#endif /* CONFIG_CMDLINE */
> +
> +	pr_debug("Command line is: %s\n", boot_command_line);
> +
>  	/* Initialize {size,address}-cells info */
>  	of_scan_flat_dt(early_init_dt_scan_root, NULL);

Patch is working fine and fixes my problem. You can add my Tested-by.
Pali Rohár Dec. 25, 2016, 10:08 p.m. UTC | #2 
On Friday 16 December 2016 12:42:34 Pali Rohár wrote:
> On Thursday 15 December 2016 11:09:32 Russell King - ARM Linux wrote:
> > > What is reason that CONFIG_CMDLINE is not supported for DT?
> > 
> > Sorry, that's my mistake - as you've pointed out above, it is
> > supported but via generic code.  I was only looking at arch code
> > when I made the statement.
> > 
> > This patch (untested) should solve it:
> >  drivers/of/fdt.c | 40 ++++++++++++++++++++--------------------
> >  1 file changed, 20 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index c89d5d231a0e..fb89157332c6 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -1073,26 +1073,6 @@ int __init
> > early_init_dt_scan_chosen(unsigned long node, const char *uname,
> > 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);
> > -
> > 
> >  	/* break now */
> >  	return 1;
> >  
> >  }
> > 
> > @@ -1205,6 +1185,26 @@ void __init early_init_dt_scan_nodes(void)
> > 
> >  	/* Retrieve various information from the /chosen node */
> >  	of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
> > 
> > +	/*
> > +	 * 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(boot_command_line, " ", COMMAND_LINE_SIZE);
> > +	strlcat(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> > +#elif defined(CONFIG_CMDLINE_FORCE)
> > +	strlcpy(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> > +#else
> > +	/* No arguments from boot loader, use kernel's cmdline */
> > +	if (!boot_command_line[0])
> > +		strlcpy(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> > +#endif
> > +#endif /* CONFIG_CMDLINE */
> > +
> > +	pr_debug("Command line is: %s\n", boot_command_line);
> > +
> > 
> >  	/* Initialize {size,address}-cells info */
> >  	of_scan_flat_dt(early_init_dt_scan_root, NULL);
> 
> Patch is working fine and fixes my problem. You can add my Tested-by.

Russell, are you going to use your patch? Or original my? Or any other?
If you need more testing or other data, let me know.

I would like to see this problem (non working CONFIG_CMDLINE) fixed...
Russell King (Oracle) Jan. 2, 2017, 1:54 p.m. UTC | #3 
On Sun, Dec 25, 2016 at 11:08:15PM +0100, Pali Rohár wrote:
> On Friday 16 December 2016 12:42:34 Pali Rohár wrote:
> > On Thursday 15 December 2016 11:09:32 Russell King - ARM Linux wrote:
> > > > What is reason that CONFIG_CMDLINE is not supported for DT?
> > > 
> > > Sorry, that's my mistake - as you've pointed out above, it is
> > > supported but via generic code.  I was only looking at arch code
> > > when I made the statement.
> > > 
> > > This patch (untested) should solve it:
> > >  drivers/of/fdt.c | 40 ++++++++++++++++++++--------------------
> > >  1 file changed, 20 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index c89d5d231a0e..fb89157332c6 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -1073,26 +1073,6 @@ int __init
> > > early_init_dt_scan_chosen(unsigned long node, const char *uname,
> > > 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);
> > > -
> > > 
> > >  	/* break now */
> > >  	return 1;
> > >  
> > >  }
> > > 
> > > @@ -1205,6 +1185,26 @@ void __init early_init_dt_scan_nodes(void)
> > > 
> > >  	/* Retrieve various information from the /chosen node */
> > >  	of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
> > > 
> > > +	/*
> > > +	 * 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(boot_command_line, " ", COMMAND_LINE_SIZE);
> > > +	strlcat(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> > > +#elif defined(CONFIG_CMDLINE_FORCE)
> > > +	strlcpy(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> > > +#else
> > > +	/* No arguments from boot loader, use kernel's cmdline */
> > > +	if (!boot_command_line[0])
> > > +		strlcpy(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> > > +#endif
> > > +#endif /* CONFIG_CMDLINE */
> > > +
> > > +	pr_debug("Command line is: %s\n", boot_command_line);
> > > +
> > > 
> > >  	/* Initialize {size,address}-cells info */
> > >  	of_scan_flat_dt(early_init_dt_scan_root, NULL);
> > 
> > Patch is working fine and fixes my problem. You can add my Tested-by.
> 
> Russell, are you going to use your patch? Or original my? Or any other?
> If you need more testing or other data, let me know.
> 
> I would like to see this problem (non working CONFIG_CMDLINE) fixed...

If the DT people would like to comment on it and give me an ack or take
the patch... I guess that's not going to happen as they're not on the
Cc... I'll resend later today.
diff mbox

Patch

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index c89d5d231a0e..fb89157332c6 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1073,26 +1073,6 @@  int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
 	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);
-
 	/* break now */
 	return 1;
 }
@@ -1205,6 +1185,26 @@  void __init early_init_dt_scan_nodes(void)
 	/* Retrieve various information from the /chosen node */
 	of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
 
+	/*
+	 * 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(boot_command_line, " ", COMMAND_LINE_SIZE);
+	strlcat(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
+#elif defined(CONFIG_CMDLINE_FORCE)
+	strlcpy(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
+#else
+	/* No arguments from boot loader, use kernel's cmdline */
+	if (!boot_command_line[0])
+		strlcpy(boot_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
+#endif
+#endif /* CONFIG_CMDLINE */
+
+	pr_debug("Command line is: %s\n", boot_command_line);
+
 	/* Initialize {size,address}-cells info */
 	of_scan_flat_dt(early_init_dt_scan_root, NULL);