diff mbox series

[v2,4/7] CMDLINE: powerpc: convert to generic builtin command line

Message ID 20210309000247.2989531-5-danielwa@cisco.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/7] CMDLINE: add generic builtin command line | expand

Commit Message

Daniel Walker (danielwa) March 9, 2021, 12:02 a.m. UTC 
This updates the powerpc code to use the CONFIG_GENERIC_CMDLINE
option.

Cc: xe-linux-external@cisco.com
Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com>
Signed-off-by: Daniel Walker <danielwa@cisco.com>
---
 arch/powerpc/Kconfig            | 37 +--------------------------------
 arch/powerpc/kernel/prom.c      |  1 +
 arch/powerpc/kernel/prom_init.c | 35 ++++++++++++++++++-------------
 3 files changed, 23 insertions(+), 50 deletions(-)

Comments

Christophe Leroy March 9, 2021, 7:56 a.m. UTC | #1 
Le 09/03/2021 à 01:02, Daniel Walker a écrit :
> This updates the powerpc code to use the CONFIG_GENERIC_CMDLINE
> option.
> 
> Cc: xe-linux-external@cisco.com
> Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com>
> Signed-off-by: Daniel Walker <danielwa@cisco.com>
> ---
>   arch/powerpc/Kconfig            | 37 +--------------------------------
>   arch/powerpc/kernel/prom.c      |  1 +
>   arch/powerpc/kernel/prom_init.c | 35 ++++++++++++++++++-------------
>   3 files changed, 23 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 107bb4319e0e..276b06d5c961 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -167,6 +167,7 @@ config PPC
>   	select EDAC_SUPPORT
>   	select GENERIC_ATOMIC64			if PPC32
>   	select GENERIC_CLOCKEVENTS_BROADCAST	if SMP
> +	select GENERIC_CMDLINE
>   	select GENERIC_CMOS_UPDATE
>   	select GENERIC_CPU_AUTOPROBE
>   	select GENERIC_CPU_VULNERABILITIES	if PPC_BARRIER_NOSPEC
> @@ -906,42 +907,6 @@ config PPC_DENORMALISATION
>   	  Add support for handling denormalisation of single precision
>   	  values.  Useful for bare metal only.  If unsure say Y here.
>   
> -config CMDLINE
> -	string "Initial kernel command string"
> -	default ""
> -	help
> -	  On some platforms, there is currently no way for the boot loader to
> -	  pass arguments to the kernel. For these platforms, you can supply
> -	  some command-line options at build time by entering them here.  In
> -	  most cases you will need to specify the root device here.
> -
> -choice
> -	prompt "Kernel command line type" if CMDLINE != ""
> -	default CMDLINE_FROM_BOOTLOADER
> -
> -config CMDLINE_FROM_BOOTLOADER
> -	bool "Use bootloader kernel arguments if available"
> -	help
> -	  Uses the command-line options passed by the boot loader. If
> -	  the boot loader doesn't provide any, the default kernel command
> -	  string provided in CMDLINE will be used.
> -
> -config CMDLINE_EXTEND
> -	bool "Extend bootloader kernel arguments"
> -	help
> -	  The command-line arguments provided by the boot loader will be
> -	  appended to the default kernel command string.
> -
> -config CMDLINE_FORCE
> -	bool "Always use the default kernel command string"
> -	help
> -	  Always use the default kernel command string, even if the boot
> -	  loader passes other arguments to the kernel.
> -	  This is useful if you cannot or don't want to change the
> -	  command-line options your boot loader passes to the kernel.
> -
> -endchoice
> -
>   config EXTRA_TARGETS
>   	string "Additional default image types"
>   	help
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index ae3c41730367..96d0a01be1b4 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -27,6 +27,7 @@
>   #include <linux/irq.h>
>   #include <linux/memblock.h>
>   #include <linux/of.h>
> +#include <linux/cmdline.h>

Why is this needed in prom.c ?

>   #include <linux/of_fdt.h>
>   #include <linux/libfdt.h>
>   #include <linux/cpu.h>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index e9d4eb6144e1..657241534d69 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -27,6 +27,7 @@
>   #include <linux/initrd.h>
>   #include <linux/bitops.h>
>   #include <linux/pgtable.h>
> +#include <linux/cmdline.h>
>   #include <asm/prom.h>
>   #include <asm/rtas.h>
>   #include <asm/page.h>
> @@ -242,15 +243,6 @@ static int __init prom_strcmp(const char *cs, const char *ct)
>   	return 0;
>   }
>   
> -static char __init *prom_strcpy(char *dest, const char *src)
> -{
> -	char *tmp = dest;
> -
> -	while ((*dest++ = *src++) != '\0')
> -		/* nothing */;
> -	return tmp;
> -}
> -

This game with prom_strcpy() should go a separate preceeding patch.

Also, it looks like checkpatch.pl recommends to use strscpy() instead of strlcpy().

>   static int __init prom_strncmp(const char *cs, const char *ct, size_t count)
>   {
>   	unsigned char c1, c2;
> @@ -276,6 +268,20 @@ static size_t __init prom_strlen(const char *s)
>   	return sc - s;
>   }
>   
> +static size_t __init prom_strlcpy(char *dest, const char *src, size_t size)
> +{
> +	size_t ret = prom_strlen(src);
> +
> +	if (size) {
> +		size_t len = (ret >= size) ? size - 1 : ret;
> +
> +		memcpy(dest, src, len);
> +		dest[len] = '\0';
> +	}
> +	return ret;
> +}
> +
> +
>   static int __init prom_memcmp(const void *cs, const void *ct, size_t count)
>   {
>   	const unsigned char *su1, *su2;
> @@ -304,6 +310,7 @@ static char __init *prom_strstr(const char *s1, const char *s2)
>   	return NULL;
>   }
>   
> +#ifdef GENERIC_CMDLINE_NEED_STRLCAT
>   static size_t __init prom_strlcat(char *dest, const char *src, size_t count)
>   {
>   	size_t dsize = prom_strlen(dest);
> @@ -323,6 +330,7 @@ static size_t __init prom_strlcat(char *dest, const char *src, size_t count)
>   	return res;
>   
>   }
> +#endif
>   
>   #ifdef CONFIG_PPC_PSERIES
>   static int __init prom_strtobool(const char *s, bool *res)
> @@ -775,12 +783,11 @@ static void __init early_cmdline_parse(void)
>   	prom_cmd_line[0] = 0;
>   	p = prom_cmd_line;
>   
> -	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && (long)prom.chosen > 0)
> +	if ((long)prom.chosen > 0)
>   		l = prom_getprop(prom.chosen, "bootargs", p, COMMAND_LINE_SIZE-1);
>   
> -	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || l <= 0 || p[0] == '\0')
> -		prom_strlcat(prom_cmd_line, " " CONFIG_CMDLINE,
> -			     sizeof(prom_cmd_line));
> +	cmdline_add_builtin_custom(prom_cmd_line, (l > 0 ? p : NULL), sizeof(prom_cmd_line),
> +					__prombss, prom_strlcpy, prom_strlcat);

So we are referencing a function that doesn't exist (namely prom_strlcat).
But it works because cmdline_add_builtin_custom() looks like a function but is in fact an obscure 
macro that doesn't use prom_strlcat() unless GENERIC_CMDLINE_NEED_STRLCAT is defined.

IMHO that's awful for readability and code maintenance.

>   
>   	prom_printf("command line: %s\n", prom_cmd_line);
>   
> @@ -2706,7 +2713,7 @@ static void __init flatten_device_tree(void)
>   
>   	/* Add "phandle" in there, we'll need it */
>   	namep = make_room(&mem_start, &mem_end, 16, 1);
> -	prom_strcpy(namep, "phandle");
> +	prom_strlcpy(namep, "phandle", 8);

Should be in a separate patch.

>   	mem_start = (unsigned long)namep + prom_strlen(namep) + 1;
>   
>   	/* Build string array */
> 

Christophe
Daniel Walker (danielwa) March 9, 2021, 9:40 p.m. UTC | #2 
On Tue, Mar 09, 2021 at 08:56:47AM +0100, Christophe Leroy wrote:
> 
> 
> Le 09/03/2021 à 01:02, Daniel Walker a écrit :
> > This updates the powerpc code to use the CONFIG_GENERIC_CMDLINE
> > option.
> > 
> > Cc: xe-linux-external@cisco.com
> > Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> > Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com>
> > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > ---
> >   arch/powerpc/Kconfig            | 37 +--------------------------------
> >   arch/powerpc/kernel/prom.c      |  1 +
> >   arch/powerpc/kernel/prom_init.c | 35 ++++++++++++++++++-------------
> >   3 files changed, 23 insertions(+), 50 deletions(-)
> > 
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index 107bb4319e0e..276b06d5c961 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -167,6 +167,7 @@ config PPC
> >   	select EDAC_SUPPORT
> >   	select GENERIC_ATOMIC64			if PPC32
> >   	select GENERIC_CLOCKEVENTS_BROADCAST	if SMP
> > +	select GENERIC_CMDLINE
> >   	select GENERIC_CMOS_UPDATE
> >   	select GENERIC_CPU_AUTOPROBE
> >   	select GENERIC_CPU_VULNERABILITIES	if PPC_BARRIER_NOSPEC
> > @@ -906,42 +907,6 @@ config PPC_DENORMALISATION
> >   	  Add support for handling denormalisation of single precision
> >   	  values.  Useful for bare metal only.  If unsure say Y here.
> > -config CMDLINE
> > -	string "Initial kernel command string"
> > -	default ""
> > -	help
> > -	  On some platforms, there is currently no way for the boot loader to
> > -	  pass arguments to the kernel. For these platforms, you can supply
> > -	  some command-line options at build time by entering them here.  In
> > -	  most cases you will need to specify the root device here.
> > -
> > -choice
> > -	prompt "Kernel command line type" if CMDLINE != ""
> > -	default CMDLINE_FROM_BOOTLOADER
> > -
> > -config CMDLINE_FROM_BOOTLOADER
> > -	bool "Use bootloader kernel arguments if available"
> > -	help
> > -	  Uses the command-line options passed by the boot loader. If
> > -	  the boot loader doesn't provide any, the default kernel command
> > -	  string provided in CMDLINE will be used.
> > -
> > -config CMDLINE_EXTEND
> > -	bool "Extend bootloader kernel arguments"
> > -	help
> > -	  The command-line arguments provided by the boot loader will be
> > -	  appended to the default kernel command string.
> > -
> > -config CMDLINE_FORCE
> > -	bool "Always use the default kernel command string"
> > -	help
> > -	  Always use the default kernel command string, even if the boot
> > -	  loader passes other arguments to the kernel.
> > -	  This is useful if you cannot or don't want to change the
> > -	  command-line options your boot loader passes to the kernel.
> > -
> > -endchoice
> > -
> >   config EXTRA_TARGETS
> >   	string "Additional default image types"
> >   	help
> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> > index ae3c41730367..96d0a01be1b4 100644
> > --- a/arch/powerpc/kernel/prom.c
> > +++ b/arch/powerpc/kernel/prom.c
> > @@ -27,6 +27,7 @@
> >   #include <linux/irq.h>
> >   #include <linux/memblock.h>
> >   #include <linux/of.h>
> > +#include <linux/cmdline.h>
> 
> Why is this needed in prom.c ?
 
Must have been a mistake, I don't think it's needed.


> >   #include <linux/of_fdt.h>
> >   #include <linux/libfdt.h>
> >   #include <linux/cpu.h>
> > diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> > index e9d4eb6144e1..657241534d69 100644
> > --- a/arch/powerpc/kernel/prom_init.c
> > +++ b/arch/powerpc/kernel/prom_init.c
> > @@ -27,6 +27,7 @@
> >   #include <linux/initrd.h>
> >   #include <linux/bitops.h>
> >   #include <linux/pgtable.h>
> > +#include <linux/cmdline.h>
> >   #include <asm/prom.h>
> >   #include <asm/rtas.h>
> >   #include <asm/page.h>
> > @@ -242,15 +243,6 @@ static int __init prom_strcmp(const char *cs, const char *ct)
> >   	return 0;
> >   }
> > -static char __init *prom_strcpy(char *dest, const char *src)
> > -{
> > -	char *tmp = dest;
> > -
> > -	while ((*dest++ = *src++) != '\0')
> > -		/* nothing */;
> > -	return tmp;
> > -}
> > -
> 
> This game with prom_strcpy() should go a separate preceeding patch.
> 
> Also, it looks like checkpatch.pl recommends to use strscpy() instead of strlcpy().

strscpy() is very large. I'm not sure it's compatible with this prom_init.c
environment.

> >   static int __init prom_strncmp(const char *cs, const char *ct, size_t count)
> >   {
> >   	unsigned char c1, c2;
> > @@ -276,6 +268,20 @@ static size_t __init prom_strlen(const char *s)
> >   	return sc - s;
> >   }
> > +static size_t __init prom_strlcpy(char *dest, const char *src, size_t size)
> > +{
> > +	size_t ret = prom_strlen(src);
> > +
> > +	if (size) {
> > +		size_t len = (ret >= size) ? size - 1 : ret;
> > +
> > +		memcpy(dest, src, len);
> > +		dest[len] = '\0';
> > +	}
> > +	return ret;
> > +}
> > +
> > +
> >   static int __init prom_memcmp(const void *cs, const void *ct, size_t count)
> >   {
> >   	const unsigned char *su1, *su2;
> > @@ -304,6 +310,7 @@ static char __init *prom_strstr(const char *s1, const char *s2)
> >   	return NULL;
> >   }
> > +#ifdef GENERIC_CMDLINE_NEED_STRLCAT
> >   static size_t __init prom_strlcat(char *dest, const char *src, size_t count)
> >   {
> >   	size_t dsize = prom_strlen(dest);
> > @@ -323,6 +330,7 @@ static size_t __init prom_strlcat(char *dest, const char *src, size_t count)
> >   	return res;
> >   }
> > +#endif
> >   #ifdef CONFIG_PPC_PSERIES
> >   static int __init prom_strtobool(const char *s, bool *res)
> > @@ -775,12 +783,11 @@ static void __init early_cmdline_parse(void)
> >   	prom_cmd_line[0] = 0;
> >   	p = prom_cmd_line;
> > -	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && (long)prom.chosen > 0)
> > +	if ((long)prom.chosen > 0)
> >   		l = prom_getprop(prom.chosen, "bootargs", p, COMMAND_LINE_SIZE-1);
> > -	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || l <= 0 || p[0] == '\0')
> > -		prom_strlcat(prom_cmd_line, " " CONFIG_CMDLINE,
> > -			     sizeof(prom_cmd_line));
> > +	cmdline_add_builtin_custom(prom_cmd_line, (l > 0 ? p : NULL), sizeof(prom_cmd_line),
> > +					__prombss, prom_strlcpy, prom_strlcat);
> 
> So we are referencing a function that doesn't exist (namely prom_strlcat).
> But it works because cmdline_add_builtin_custom() looks like a function but
> is in fact an obscure macro that doesn't use prom_strlcat() unless
> GENERIC_CMDLINE_NEED_STRLCAT is defined.
> 
> IMHO that's awful for readability and code maintenance.

powerpc is a special case, there's no other users like this. The reason is
because of all the difficulty in this prom_init.c code. A lot of the generic
code has similar kind of changes to work across architectures.


> >   	prom_printf("command line: %s\n", prom_cmd_line);
> > @@ -2706,7 +2713,7 @@ static void __init flatten_device_tree(void)
> >   	/* Add "phandle" in there, we'll need it */
> >   	namep = make_room(&mem_start, &mem_end, 16, 1);
> > -	prom_strcpy(namep, "phandle");
> > +	prom_strlcpy(namep, "phandle", 8);
> 
> Should be in a separate patch.

I can move it, I missed that from the first round.

Daniel
Christophe Leroy March 13, 2021, 9:29 a.m. UTC | #3 
Le 09/03/2021 à 22:40, Daniel Walker a écrit :
> On Tue, Mar 09, 2021 at 08:56:47AM +0100, Christophe Leroy wrote:
>>
>> So we are referencing a function that doesn't exist (namely prom_strlcat).
>> But it works because cmdline_add_builtin_custom() looks like a function but
>> is in fact an obscure macro that doesn't use prom_strlcat() unless
>> GENERIC_CMDLINE_NEED_STRLCAT is defined.
>>
>> IMHO that's awful for readability and code maintenance.
> 
> powerpc is a special case, there's no other users like this. The reason is
> because of all the difficulty in this prom_init.c code. A lot of the generic
> code has similar kind of changes to work across architectures.
> 

I'd suggest the following (sorry if Thunderbird damages whitespaces, you'll get the idea anyway)



diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
new file mode 100644
index 000000000000..30b9eefc802f
--- /dev/null
+++ b/include/linux/cmdline.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CMDLINE_H
+#define _LINUX_CMDLINE_H
+
+#ifdef CONFIG_GENERIC_CMDLINE
+
+#ifndef cmdline_strlcpy
+#define cmdline_strlcpy strlcpy
+#endif
+#ifndef cmdline_strlcat
+#define cmdline_strlcat strlcat
+#endif
+
+static __always_inline void
+cmdline_add_builtin_custom(char *dest, const char *src, char *tmp, unsigned long length)
+{
+	if (WARN_ON(sizeof(CONFIG_CMDLINE_PREPEND) > 1 &&
+		    !IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) &&
+		    !tmp && src == dest))
+		return;
+
+	if (sizeof(CONFIG_CMDLINE_PREPEND) > 1 &&
+	    !IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) && src == dest)
+		cmdline_strlcpy(tmp, src, length);
+	else
+		tmp = (char *)src;
+
+	cmdline_strlcpy(dest, CONFIG_CMDLINE_PREPEND " ", length);
+	if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) && tmp)
+		cmdline_strlcat(dest, tmp, length);
+	cmdline_strlcat(dest, " " CONFIG_CMDLINE_APPEND, length);
+}
+
+#define cmdline_add_builtin(dest, src, length) do {			\
+	static __init char cmdline_tmp[length];				\
+									\
+	cmdline_add_builtin_custom(dest, src, cmdline_tmp, length);	\
+} while (0)
+
+#endif /* CONFIG_GENERIC_CMDLINE */
+
+#endif /* _LINUX_CMDLINE_H */
diff --git a/init/Kconfig b/init/Kconfig
index 22946fe5ded9..aeb134f0703b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2035,6 +2035,27 @@ config PROFILING
  config TRACEPOINTS
  	bool

+config GENERIC_CMDLINE
+	bool
+
+if GENERIC_CMDLINE
+
+config CMDLINE_BOOL
+	bool "Built-in kernel command line"
+
+config CMDLINE_APPEND
+	string "Built-in kernel command string append" if CMDLINE_BOOL
+	default ""
+
+config CMDLINE_PREPEND
+	string "Built-in kernel command string prepend" if CMDLINE_BOOL
+	default ""
+
+config CMDLINE_OVERRIDE
+	bool "Built-in command line overrides boot loader arguments" if CMDLINE_BOOL
+
+endif
+
  endmenu		# General setup

  source "arch/Kconfig"
Christophe Leroy March 24, 2021, 3:31 p.m. UTC | #4 
Le 09/03/2021 à 22:40, Daniel Walker a écrit :
> On Tue, Mar 09, 2021 at 08:56:47AM +0100, Christophe Leroy wrote:
>>
>>
>> Le 09/03/2021 à 01:02, Daniel Walker a écrit :
>>> This updates the powerpc code to use the CONFIG_GENERIC_CMDLINE
>>> option.
>>>
>>> Cc: xe-linux-external@cisco.com
>>> Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
>>> Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com>
>>> Signed-off-by: Daniel Walker <danielwa@cisco.com>
>>> ---
>>>    arch/powerpc/Kconfig            | 37 +--------------------------------
>>>    arch/powerpc/kernel/prom.c      |  1 +
>>>    arch/powerpc/kernel/prom_init.c | 35 ++++++++++++++++++-------------
>>>    3 files changed, 23 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index 107bb4319e0e..276b06d5c961 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -167,6 +167,7 @@ config PPC
>>>    	select EDAC_SUPPORT
>>>    	select GENERIC_ATOMIC64			if PPC32
>>>    	select GENERIC_CLOCKEVENTS_BROADCAST	if SMP
>>> +	select GENERIC_CMDLINE
>>>    	select GENERIC_CMOS_UPDATE
>>>    	select GENERIC_CPU_AUTOPROBE
>>>    	select GENERIC_CPU_VULNERABILITIES	if PPC_BARRIER_NOSPEC
>>> @@ -906,42 +907,6 @@ config PPC_DENORMALISATION
>>>    	  Add support for handling denormalisation of single precision
>>>    	  values.  Useful for bare metal only.  If unsure say Y here.
>>> -config CMDLINE
>>> -	string "Initial kernel command string"
>>> -	default ""
>>> -	help
>>> -	  On some platforms, there is currently no way for the boot loader to
>>> -	  pass arguments to the kernel. For these platforms, you can supply
>>> -	  some command-line options at build time by entering them here.  In
>>> -	  most cases you will need to specify the root device here.
>>> -
>>> -choice
>>> -	prompt "Kernel command line type" if CMDLINE != ""
>>> -	default CMDLINE_FROM_BOOTLOADER
>>> -
>>> -config CMDLINE_FROM_BOOTLOADER
>>> -	bool "Use bootloader kernel arguments if available"
>>> -	help
>>> -	  Uses the command-line options passed by the boot loader. If
>>> -	  the boot loader doesn't provide any, the default kernel command
>>> -	  string provided in CMDLINE will be used.

I can't see how the above is supported in the generic builtin.

Taking into account that it is the default on powerpc, I'm having hardtime with that.

>>> -
>>> -config CMDLINE_EXTEND
>>> -	bool "Extend bootloader kernel arguments"
>>> -	help
>>> -	  The command-line arguments provided by the boot loader will be
>>> -	  appended to the default kernel command string.
>>> -
>>> -config CMDLINE_FORCE
>>> -	bool "Always use the default kernel command string"
>>> -	help
>>> -	  Always use the default kernel command string, even if the boot
>>> -	  loader passes other arguments to the kernel.
>>> -	  This is useful if you cannot or don't want to change the
>>> -	  command-line options your boot loader passes to the kernel.
>>> -
>>> -endchoice
>>> -
>>>    config EXTRA_TARGETS
>>>    	string "Additional default image types"
>>>    	help
>>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>>> index ae3c41730367..96d0a01be1b4 100644
>>> --- a/arch/powerpc/kernel/prom.c
>>> +++ b/arch/powerpc/kernel/prom.c
>>> @@ -27,6 +27,7 @@
>>>    #include <linux/irq.h>
>>>    #include <linux/memblock.h>
>>>    #include <linux/of.h>
>>> +#include <linux/cmdline.h>
>>
>> Why is this needed in prom.c ?
>   
> Must have been a mistake, I don't think it's needed.
> 
> 
>>>    #include <linux/of_fdt.h>
>>>    #include <linux/libfdt.h>
>>>    #include <linux/cpu.h>
>>> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
>>> index e9d4eb6144e1..657241534d69 100644
>>> --- a/arch/powerpc/kernel/prom_init.c
>>> +++ b/arch/powerpc/kernel/prom_init.c
>>> @@ -27,6 +27,7 @@
>>>    #include <linux/initrd.h>
>>>    #include <linux/bitops.h>
>>>    #include <linux/pgtable.h>
>>> +#include <linux/cmdline.h>
>>>    #include <asm/prom.h>
>>>    #include <asm/rtas.h>
>>>    #include <asm/page.h>
>>> @@ -242,15 +243,6 @@ static int __init prom_strcmp(const char *cs, const char *ct)
>>>    	return 0;
>>>    }
>>> -static char __init *prom_strcpy(char *dest, const char *src)
>>> -{
>>> -	char *tmp = dest;
>>> -
>>> -	while ((*dest++ = *src++) != '\0')
>>> -		/* nothing */;
>>> -	return tmp;
>>> -}
>>> -
>>
>> This game with prom_strcpy() should go a separate preceeding patch.
>>
>> Also, it looks like checkpatch.pl recommends to use strscpy() instead of strlcpy().
> 
> strscpy() is very large. I'm not sure it's compatible with this prom_init.c
> environment.

Ok you are right, lets keep strlcpy() and ignore checkpatch complaints

> 
>>>    static int __init prom_strncmp(const char *cs, const char *ct, size_t count)
>>>    {
>>>    	unsigned char c1, c2;
>>> @@ -276,6 +268,20 @@ static size_t __init prom_strlen(const char *s)
>>>    	return sc - s;
>>>    }
>>> +static size_t __init prom_strlcpy(char *dest, const char *src, size_t size)
>>> +{
>>> +	size_t ret = prom_strlen(src);
>>> +
>>> +	if (size) {
>>> +		size_t len = (ret >= size) ? size - 1 : ret;
>>> +
>>> +		memcpy(dest, src, len);
>>> +		dest[len] = '\0';
>>> +	}
>>> +	return ret;
>>> +}
>>> +
>>> +
>>>    static int __init prom_memcmp(const void *cs, const void *ct, size_t count)
>>>    {
>>>    	const unsigned char *su1, *su2;
>>> @@ -304,6 +310,7 @@ static char __init *prom_strstr(const char *s1, const char *s2)
>>>    	return NULL;
>>>    }
>>> +#ifdef GENERIC_CMDLINE_NEED_STRLCAT
>>>    static size_t __init prom_strlcat(char *dest, const char *src, size_t count)
>>>    {
>>>    	size_t dsize = prom_strlen(dest);
>>> @@ -323,6 +330,7 @@ static size_t __init prom_strlcat(char *dest, const char *src, size_t count)
>>>    	return res;
>>>    }
>>> +#endif
>>>    #ifdef CONFIG_PPC_PSERIES
>>>    static int __init prom_strtobool(const char *s, bool *res)
>>> @@ -775,12 +783,11 @@ static void __init early_cmdline_parse(void)
>>>    	prom_cmd_line[0] = 0;
>>>    	p = prom_cmd_line;
>>> -	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && (long)prom.chosen > 0)
>>> +	if ((long)prom.chosen > 0)
>>>    		l = prom_getprop(prom.chosen, "bootargs", p, COMMAND_LINE_SIZE-1);
>>> -	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || l <= 0 || p[0] == '\0')
>>> -		prom_strlcat(prom_cmd_line, " " CONFIG_CMDLINE,
>>> -			     sizeof(prom_cmd_line));
>>> +	cmdline_add_builtin_custom(prom_cmd_line, (l > 0 ? p : NULL), sizeof(prom_cmd_line),
>>> +					__prombss, prom_strlcpy, prom_strlcat);
>>
>> So we are referencing a function that doesn't exist (namely prom_strlcat).
>> But it works because cmdline_add_builtin_custom() looks like a function but
>> is in fact an obscure macro that doesn't use prom_strlcat() unless
>> GENERIC_CMDLINE_NEED_STRLCAT is defined.
>>
>> IMHO that's awful for readability and code maintenance.
> 
> powerpc is a special case, there's no other users like this. The reason is
> because of all the difficulty in this prom_init.c code. A lot of the generic
> code has similar kind of changes to work across architectures.

Any feedback on the proposed changes I made on the 13th ? I know it is partly buggy but that was 
more for the principle. I can make clean working patch if it helps.

> 
> 
>>>    	prom_printf("command line: %s\n", prom_cmd_line);
>>> @@ -2706,7 +2713,7 @@ static void __init flatten_device_tree(void)
>>>    	/* Add "phandle" in there, we'll need it */
>>>    	namep = make_room(&mem_start, &mem_end, 16, 1);
>>> -	prom_strcpy(namep, "phandle");
>>> +	prom_strlcpy(namep, "phandle", 8);
>>
>> Should be in a separate patch.
> 
> I can move it, I missed that from the first round.
> 
> Daniel
> 

Christophe
Daniel Walker (danielwa) March 25, 2021, 8:03 p.m. UTC | #5 
On Wed, Mar 24, 2021 at 04:31:35PM +0100, Christophe Leroy wrote:
> 
> 
> Le 09/03/2021 à 22:40, Daniel Walker a écrit :
> > On Tue, Mar 09, 2021 at 08:56:47AM +0100, Christophe Leroy wrote:
> > > 
> > > 
> > > Le 09/03/2021 à 01:02, Daniel Walker a écrit :
> > > > This updates the powerpc code to use the CONFIG_GENERIC_CMDLINE
> > > > option.
> > > > 
> > > > Cc: xe-linux-external@cisco.com
> > > > Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> > > > Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com>
> > > > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > > > ---
> > > >    arch/powerpc/Kconfig            | 37 +--------------------------------
> > > >    arch/powerpc/kernel/prom.c      |  1 +
> > > >    arch/powerpc/kernel/prom_init.c | 35 ++++++++++++++++++-------------
> > > >    3 files changed, 23 insertions(+), 50 deletions(-)
> > > > 
> > > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > > > index 107bb4319e0e..276b06d5c961 100644
> > > > --- a/arch/powerpc/Kconfig
> > > > +++ b/arch/powerpc/Kconfig
> > > > @@ -167,6 +167,7 @@ config PPC
> > > >    	select EDAC_SUPPORT
> > > >    	select GENERIC_ATOMIC64			if PPC32
> > > >    	select GENERIC_CLOCKEVENTS_BROADCAST	if SMP
> > > > +	select GENERIC_CMDLINE
> > > >    	select GENERIC_CMOS_UPDATE
> > > >    	select GENERIC_CPU_AUTOPROBE
> > > >    	select GENERIC_CPU_VULNERABILITIES	if PPC_BARRIER_NOSPEC
> > > > @@ -906,42 +907,6 @@ config PPC_DENORMALISATION
> > > >    	  Add support for handling denormalisation of single precision
> > > >    	  values.  Useful for bare metal only.  If unsure say Y here.
> > > > -config CMDLINE
> > > > -	string "Initial kernel command string"
> > > > -	default ""
> > > > -	help
> > > > -	  On some platforms, there is currently no way for the boot loader to
> > > > -	  pass arguments to the kernel. For these platforms, you can supply
> > > > -	  some command-line options at build time by entering them here.  In
> > > > -	  most cases you will need to specify the root device here.
> > > > -
> > > > -choice
> > > > -	prompt "Kernel command line type" if CMDLINE != ""
> > > > -	default CMDLINE_FROM_BOOTLOADER
> > > > -
> > > > -config CMDLINE_FROM_BOOTLOADER
> > > > -	bool "Use bootloader kernel arguments if available"
> > > > -	help
> > > > -	  Uses the command-line options passed by the boot loader. If
> > > > -	  the boot loader doesn't provide any, the default kernel command
> > > > -	  string provided in CMDLINE will be used.
> 
> I can't see how the above is supported in the generic builtin.
> 
> Taking into account that it is the default on powerpc, I'm having hardtime with that.

Hmm, so this ignores the built in changes. You just don't enable it, or you
don't add PREPEND or APPEND.

> Any feedback on the proposed changes I made on the 13th ? I know it is
> partly buggy but that was more for the principle. I can make clean working
> patch if it helps.


The reason I added it into the function parameters is because I can get free
type checking on the functions. If you use macro's then you don't know if the
function is compatible.

Daniel
diff mbox series

Patch

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 107bb4319e0e..276b06d5c961 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -167,6 +167,7 @@  config PPC
 	select EDAC_SUPPORT
 	select GENERIC_ATOMIC64			if PPC32
 	select GENERIC_CLOCKEVENTS_BROADCAST	if SMP
+	select GENERIC_CMDLINE
 	select GENERIC_CMOS_UPDATE
 	select GENERIC_CPU_AUTOPROBE
 	select GENERIC_CPU_VULNERABILITIES	if PPC_BARRIER_NOSPEC
@@ -906,42 +907,6 @@  config PPC_DENORMALISATION
 	  Add support for handling denormalisation of single precision
 	  values.  Useful for bare metal only.  If unsure say Y here.
 
-config CMDLINE
-	string "Initial kernel command string"
-	default ""
-	help
-	  On some platforms, there is currently no way for the boot loader to
-	  pass arguments to the kernel. For these platforms, you can supply
-	  some command-line options at build time by entering them here.  In
-	  most cases you will need to specify the root device here.
-
-choice
-	prompt "Kernel command line type" if CMDLINE != ""
-	default CMDLINE_FROM_BOOTLOADER
-
-config CMDLINE_FROM_BOOTLOADER
-	bool "Use bootloader kernel arguments if available"
-	help
-	  Uses the command-line options passed by the boot loader. If
-	  the boot loader doesn't provide any, the default kernel command
-	  string provided in CMDLINE will be used.
-
-config CMDLINE_EXTEND
-	bool "Extend bootloader kernel arguments"
-	help
-	  The command-line arguments provided by the boot loader will be
-	  appended to the default kernel command string.
-
-config CMDLINE_FORCE
-	bool "Always use the default kernel command string"
-	help
-	  Always use the default kernel command string, even if the boot
-	  loader passes other arguments to the kernel.
-	  This is useful if you cannot or don't want to change the
-	  command-line options your boot loader passes to the kernel.
-
-endchoice
-
 config EXTRA_TARGETS
 	string "Additional default image types"
 	help
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index ae3c41730367..96d0a01be1b4 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -27,6 +27,7 @@ 
 #include <linux/irq.h>
 #include <linux/memblock.h>
 #include <linux/of.h>
+#include <linux/cmdline.h>
 #include <linux/of_fdt.h>
 #include <linux/libfdt.h>
 #include <linux/cpu.h>
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index e9d4eb6144e1..657241534d69 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -27,6 +27,7 @@ 
 #include <linux/initrd.h>
 #include <linux/bitops.h>
 #include <linux/pgtable.h>
+#include <linux/cmdline.h>
 #include <asm/prom.h>
 #include <asm/rtas.h>
 #include <asm/page.h>
@@ -242,15 +243,6 @@  static int __init prom_strcmp(const char *cs, const char *ct)
 	return 0;
 }
 
-static char __init *prom_strcpy(char *dest, const char *src)
-{
-	char *tmp = dest;
-
-	while ((*dest++ = *src++) != '\0')
-		/* nothing */;
-	return tmp;
-}
-
 static int __init prom_strncmp(const char *cs, const char *ct, size_t count)
 {
 	unsigned char c1, c2;
@@ -276,6 +268,20 @@  static size_t __init prom_strlen(const char *s)
 	return sc - s;
 }
 
+static size_t __init prom_strlcpy(char *dest, const char *src, size_t size)
+{
+	size_t ret = prom_strlen(src);
+
+	if (size) {
+		size_t len = (ret >= size) ? size - 1 : ret;
+
+		memcpy(dest, src, len);
+		dest[len] = '\0';
+	}
+	return ret;
+}
+
+
 static int __init prom_memcmp(const void *cs, const void *ct, size_t count)
 {
 	const unsigned char *su1, *su2;
@@ -304,6 +310,7 @@  static char __init *prom_strstr(const char *s1, const char *s2)
 	return NULL;
 }
 
+#ifdef GENERIC_CMDLINE_NEED_STRLCAT
 static size_t __init prom_strlcat(char *dest, const char *src, size_t count)
 {
 	size_t dsize = prom_strlen(dest);
@@ -323,6 +330,7 @@  static size_t __init prom_strlcat(char *dest, const char *src, size_t count)
 	return res;
 
 }
+#endif
 
 #ifdef CONFIG_PPC_PSERIES
 static int __init prom_strtobool(const char *s, bool *res)
@@ -775,12 +783,11 @@  static void __init early_cmdline_parse(void)
 	prom_cmd_line[0] = 0;
 	p = prom_cmd_line;
 
-	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && (long)prom.chosen > 0)
+	if ((long)prom.chosen > 0)
 		l = prom_getprop(prom.chosen, "bootargs", p, COMMAND_LINE_SIZE-1);
 
-	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || l <= 0 || p[0] == '\0')
-		prom_strlcat(prom_cmd_line, " " CONFIG_CMDLINE,
-			     sizeof(prom_cmd_line));
+	cmdline_add_builtin_custom(prom_cmd_line, (l > 0 ? p : NULL), sizeof(prom_cmd_line),
+					__prombss, prom_strlcpy, prom_strlcat);
 
 	prom_printf("command line: %s\n", prom_cmd_line);
 
@@ -2706,7 +2713,7 @@  static void __init flatten_device_tree(void)
 
 	/* Add "phandle" in there, we'll need it */
 	namep = make_room(&mem_start, &mem_end, 16, 1);
-	prom_strcpy(namep, "phandle");
+	prom_strlcpy(namep, "phandle", 8);
 	mem_start = (unsigned long)namep + prom_strlen(namep) + 1;
 
 	/* Build string array */