diff mbox series

ARM: Fix support for CMDLINE_EXTEND

Message ID tencent_3E8155B4A33D48D6637F16CFE5ED293F0E08@qq.com (mailing list archive)
State New
Headers show
Series ARM: Fix support for CMDLINE_EXTEND | expand

Commit Message

谢致邦 (XIE Zhibang) March 28, 2025, 10:20 a.m. UTC 
From: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>

It should be the default command line appended to the bootloader command
line, not the bootloader command line appended to the default command
line.

This will be consistent with the behavior of FDT, EFI, and other
platforms.

Fixes: 4394c1244249 ("ARM: 6893/1: Allow for kernel command line concatenation")
Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
---
 arch/arm/Kconfig              |  4 ++--
 arch/arm/kernel/atags_parse.c | 10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Russell King (Oracle) March 28, 2025, 12:29 p.m. UTC | #1 
On Fri, Mar 28, 2025 at 10:20:17AM +0000, Yeking@Red54.com wrote:
> From: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
> 
> It should be the default command line appended to the bootloader command
> line, not the bootloader command line appended to the default command
> line.

Later arguments override earlier arguments. Any compiled-in command line
needs to be overridable by user supplied input from the boot loader. The
current behaviour is correct.

Sorry, but no.
Russell King (Oracle) March 31, 2025, 12:55 p.m. UTC | #2 
On Mon, Mar 31, 2025 at 08:29:21AM +0000, 谢致邦 (XIE Zhibang) wrote:
> > Later arguments override earlier arguments. Any compiled-in command line
> > needs to be overridable by user supplied input from the boot loader. The
> > current behaviour is correct.
> 
> CMDLINE_EXTEND is for the built-in command line to "Extend bootloader kernel
> arguments", not for the bootloader command line to extend/override built-in
> command line.
> 
> Don't forget that the default empty CMDLINE already gives the bootloader the
> ability to override everything. Not to mention that even if CMDLINE is not
> empty, the default CMDLINE_FROM_BOOTLOADER continues to give the bootloader the
> ability to override all.

The help text:

+         The command-line arguments provided by the boot loader will be
+         appended to the default kernel command string.

so, you end up with default kernel command string _followed by_ the
boot loader arguments. This allows the boot loader arguments to
override anything that is in the default kernel command string.
That's why it is a _default_ - it can be overrided.

If the boot loader command string is extended by the default kernel
command string, then that doesn't happen.

CMDLINE_EXTEND is default _followed by_ boot loader.

You're wanting it to be the other way around. That's broken.

No.

Sorry.
Russell King (Oracle) March 31, 2025, 1:34 p.m. UTC | #3 
On Mon, Mar 31, 2025 at 01:55:23PM +0100, Russell King (Oracle) wrote:
> On Mon, Mar 31, 2025 at 08:29:21AM +0000, 谢致邦 (XIE Zhibang) wrote:
> > > Later arguments override earlier arguments. Any compiled-in command line
> > > needs to be overridable by user supplied input from the boot loader. The
> > > current behaviour is correct.
> > 
> > CMDLINE_EXTEND is for the built-in command line to "Extend bootloader kernel
> > arguments", not for the bootloader command line to extend/override built-in
> > command line.
> > 
> > Don't forget that the default empty CMDLINE already gives the bootloader the
> > ability to override everything. Not to mention that even if CMDLINE is not
> > empty, the default CMDLINE_FROM_BOOTLOADER continues to give the bootloader the
> > ability to override all.
> 
> The help text:
> 
> +         The command-line arguments provided by the boot loader will be
> +         appended to the default kernel command string.
> 
> so, you end up with default kernel command string _followed by_ the
> boot loader arguments. This allows the boot loader arguments to
> override anything that is in the default kernel command string.
> That's why it is a _default_ - it can be overrided.
> 
> If the boot loader command string is extended by the default kernel
> command string, then that doesn't happen.
> 
> CMDLINE_EXTEND is default _followed by_ boot loader.
> 
> You're wanting it to be the other way around. That's broken.

To add further weight:

ARM gained support for CMDLINE_EXTEND in commit 4394c1244249 ("ARM:
6893/1: Allow for kernel command line concatenation") dated 4 May
2011. In this commit, CONFIG_CMDLINE _prefixes_ the boot loader
supplied arguments.

In commit 34b82026a507 ("fdt: fix extend of cmd line") dated 13
April 2016, which _post_ _dates_ the introduction on ARM, and the
commit even states that it's fixing the lack of appending compared
to ARM, this adds code to drivers/of to _append_ CONFIG_CMDLINE
to the FDT arguments which come from the boot loader.

It is DT that implemented this wrongly.

No, we are not going to change arch/arm to conform to something
that was implemented in a broken way. drivers/of needs fixing
to actually implement it as it was *originally* intended - and
there is five years of arch/arm doing this *before* DT started
to do it.

If drivers/of maintainers also don't want to change, then I'm
sorry, but you have to then put up with the fact that it got
wrongly implemented by drivers/of and thus has a different
behaviour there.
Maxim Uvarov March 31, 2025, 3:14 p.m. UTC | #4 
It will be good  to have some constance here with other arches:
./arch/arm/kernel/atags_parse.c:#if defined(CONFIG_CMDLINE_EXTEND)
./arch/sh/kernel/setup.c:#ifdef CONFIG_CMDLINE_EXTEND
./arch/powerpc/kernel/prom_init.c:    if
(IS_ENABLED(CONFIG_CMDLINE_EXTEND) || l <= 0 || p[0] == '\0')
./arch/loongarch/kernel/setup.c:     * CONFIG_CMDLINE_EXTEND is enabled.
./arch/loongarch/kernel/setup.c:    if
(IS_ENABLED(CONFIG_CMDLINE_EXTEND) && CONFIG_CMDLINE[0]) {
./arch/riscv/kernel/pi/cmdline_early.c:    if
(IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||

Or drop CONFIG_CMDLINE_EXTEND completely or make arm64 work as others.

BR,
Maxim.

пн, 31 мар. 2025 г. в 16:35, Russell King (Oracle) <linux@armlinux.org.uk>:
>
> On Mon, Mar 31, 2025 at 01:55:23PM +0100, Russell King (Oracle) wrote:
> > On Mon, Mar 31, 2025 at 08:29:21AM +0000, 谢致邦 (XIE Zhibang) wrote:
> > > > Later arguments override earlier arguments. Any compiled-in command line
> > > > needs to be overridable by user supplied input from the boot loader. The
> > > > current behaviour is correct.
> > >
> > > CMDLINE_EXTEND is for the built-in command line to "Extend bootloader kernel
> > > arguments", not for the bootloader command line to extend/override built-in
> > > command line.
> > >
> > > Don't forget that the default empty CMDLINE already gives the bootloader the
> > > ability to override everything. Not to mention that even if CMDLINE is not
> > > empty, the default CMDLINE_FROM_BOOTLOADER continues to give the bootloader the
> > > ability to override all.
> >
> > The help text:
> >
> > +         The command-line arguments provided by the boot loader will be
> > +         appended to the default kernel command string.
> >
> > so, you end up with default kernel command string _followed by_ the
> > boot loader arguments. This allows the boot loader arguments to
> > override anything that is in the default kernel command string.
> > That's why it is a _default_ - it can be overrided.
> >
> > If the boot loader command string is extended by the default kernel
> > command string, then that doesn't happen.
> >
> > CMDLINE_EXTEND is default _followed by_ boot loader.
> >
> > You're wanting it to be the other way around. That's broken.
>
> To add further weight:
>
> ARM gained support for CMDLINE_EXTEND in commit 4394c1244249 ("ARM:
> 6893/1: Allow for kernel command line concatenation") dated 4 May
> 2011. In this commit, CONFIG_CMDLINE _prefixes_ the boot loader
> supplied arguments.
>
> In commit 34b82026a507 ("fdt: fix extend of cmd line") dated 13
> April 2016, which _post_ _dates_ the introduction on ARM, and the
> commit even states that it's fixing the lack of appending compared
> to ARM, this adds code to drivers/of to _append_ CONFIG_CMDLINE
> to the FDT arguments which come from the boot loader.
>
> It is DT that implemented this wrongly.
>
> No, we are not going to change arch/arm to conform to something
> that was implemented in a broken way. drivers/of needs fixing
> to actually implement it as it was *originally* intended - and
> there is five years of arch/arm doing this *before* DT started
> to do it.
>
> If drivers/of maintainers also don't want to change, then I'm
> sorry, but you have to then put up with the fact that it got
> wrongly implemented by drivers/of and thus has a different
> behaviour there.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Russell King (Oracle) March 31, 2025, 3:18 p.m. UTC | #5 
On Mon, Mar 31, 2025 at 06:14:27PM +0300, Maxim Uvarov wrote:
> It will be good  to have some constance here with other arches:
> ./arch/arm/kernel/atags_parse.c:#if defined(CONFIG_CMDLINE_EXTEND)
> ./arch/sh/kernel/setup.c:#ifdef CONFIG_CMDLINE_EXTEND
> ./arch/powerpc/kernel/prom_init.c:    if
> (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || l <= 0 || p[0] == '\0')
> ./arch/loongarch/kernel/setup.c:     * CONFIG_CMDLINE_EXTEND is enabled.
> ./arch/loongarch/kernel/setup.c:    if
> (IS_ENABLED(CONFIG_CMDLINE_EXTEND) && CONFIG_CMDLINE[0]) {
> ./arch/riscv/kernel/pi/cmdline_early.c:    if
> (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
> 
> Or drop CONFIG_CMDLINE_EXTEND completely or make arm64 work as others.

I'm not changing something that has had 14 years of being the way it's
been - people will have come to rely upon it. Changing it now is way
too late.
diff mbox series

Patch

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 25ed6f1a7c7a..635e4da33fff 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1534,8 +1534,8 @@  config CMDLINE_FROM_BOOTLOADER
 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.
+	  The default kernel command string will be appended to the
+	  command-line arguments provided by the boot loader.
 
 config CMDLINE_FORCE
 	bool "Always use the default kernel command string"
diff --git a/arch/arm/kernel/atags_parse.c b/arch/arm/kernel/atags_parse.c
index 4ec591bde3df..bd163f00602e 100644
--- a/arch/arm/kernel/atags_parse.c
+++ b/arch/arm/kernel/atags_parse.c
@@ -120,15 +120,15 @@  __tagtable(ATAG_REVISION, parse_tag_revision);
 
 static int __init parse_tag_cmdline(const struct tag *tag)
 {
-#if defined(CONFIG_CMDLINE_EXTEND)
-	strlcat(default_command_line, " ", COMMAND_LINE_SIZE);
-	strlcat(default_command_line, tag->u.cmdline.cmdline,
-		COMMAND_LINE_SIZE);
-#elif defined(CONFIG_CMDLINE_FORCE)
+#if defined(CONFIG_CMDLINE_FORCE)
 	pr_warn("Ignoring tag cmdline (using the default kernel command line)\n");
 #else
 	strscpy(default_command_line, tag->u.cmdline.cmdline,
 		COMMAND_LINE_SIZE);
+#if defined(CONFIG_CMDLINE_EXTEND)
+	strlcat(default_command_line, " ", COMMAND_LINE_SIZE);
+	strlcat(default_command_line, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
+#endif
 #endif
 	return 0;
 }