[PATCH 1/2] of/fdt: Allow architectures to override CONFIG_CMDLINE logic

[PATCH 1/2] of/fdt: Allow architectures to override CONFIG_CMDLINE logic

[Paul Burton]

The CONFIG_CMDLINE-related logic in early_init_dt_scan_chosen() falls
back to copying CONFIG_CMDLINE into boot_command_line/data if the DT has
a /chosen node but that node has no bootargs property or a bootargs
property of length zero.

This is problematic for the MIPS architecture because we support
concatenating arguments from either the DT or the bootloader with those
from CONFIG_CMDLINE, but the behaviour of early_init_dt_scan_chosen()
gives us no way of knowing whether boot_command_line contains arguments
from DT or already contains CONFIG_CMDLINE. This can lead to us
concatenating CONFIG_CMDLINE with itself, duplicating command line
arguments which can be problematic (eg. for earlycon which will attempt
to register the same console twice & warn about it).

Move the CONFIG_CMDLINE-related logic to a weak function that
architectures can provide their own version of, such that we continue to
use the existing logic for architectures where it's suitable but also
allow MIPS to override this behaviour such that the architecture code
knows when CONFIG_CMDLINE is used.

Signed-off-by: Paul Burton <paul.burton@mips.com>
References: https://patchwork.linux-mips.org/patch/18804/
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Jaedon Shin <jaedon.shin@gmail.com>
Cc: Mathieu Malaterre <malat@debian.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: stable@vger.kernel.org # v4.16+
---
Marked for stable as a prerequisite of the following patch.

DT maintainers: if you're OK with this it'd be great to get an ack so
this can go through the mips-fixes tree.
---
 drivers/of/fdt.c       | 55 +++++++++++++++++++++++++++++-------------
 include/linux/of_fdt.h |  1 +
 2 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 800ad252cf9c..94c474315cff 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1072,6 +1072,43 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
 	return 0;
 }
 
+/**
+ * early_init_dt_fixup_cmdline_arch() - Modify a command line taken from DT
+ * @data: A pointer to the command line
+ *
+ * This function provides an opportunity to make modifications to command line
+ * arguments taken from a device tree before use, for example to concatenate
+ * them with arguments from other sources or replace them entirely.
+ *
+ * Modifications should be made directly to the string pointed at by @data,
+ * which is COMMAND_LINE_SIZE bytes in size.
+ *
+ * The default implementation supports extending or overriding the DT command
+ * line arguments using CONFIG_CMDLINE. Since other sources of command line
+ * arguments are platform-specific, architectures can provide their own
+ * implementation of this function to obtain their desired behaviour.
+ */
+void __init __weak early_init_dt_fixup_cmdline_arch(char *data)
+{
+	/*
+	 * 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 (!data[0])
+		strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
+#endif
+#endif /* CONFIG_CMDLINE */
+}
+
 int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
 				     int depth, void *data)
 {
@@ -1091,23 +1128,7 @@ 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 */
+	early_init_dt_fixup_cmdline_arch(data);
 
 	pr_debug("Command line is: %s\n", (char*)data);
 
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index b9cd9ebdf9b9..98935695f49d 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -80,6 +80,7 @@ extern void early_init_dt_add_memory_arch(u64 base, u64 size);
 extern int early_init_dt_mark_hotplug_memory_arch(u64 base, u64 size);
 extern int early_init_dt_reserve_memory_arch(phys_addr_t base, phys_addr_t size,
 					     bool no_map);
+extern void early_init_dt_fixup_cmdline_arch(char *data);
 extern u64 dt_mem_next_cell(int s, const __be32 **cellp);
 
 /* Early flat tree scan hooks */
-- 
2.18.0

[PATCH 2/2] MIPS: Fix CONFIG_CMDLINE handling

[Paul Burton]

Commit 8ce355cf2e38 ("MIPS: Setup boot_command_line before
plat_mem_setup") fixed a problem for systems which have
CONFIG_CMDLINE_BOOL=y & use a DT with a chosen node that has either no
bootargs property or an empty one. In this configuration
early_init_dt_scan_chosen() copies CONFIG_CMDLINE into
boot_command_line, but the MIPS code doesn't know this so it appends
CONFIG_CMDLINE (via builtin_cmdline) to boot_command_line again. The
result is that boot_command_line contains the arguments from
CONFIG_CMDLINE twice.

That commit took the approach of simply setting up boot_command_line
from the MIPS code before early_init_dt_scan_chosen() runs, causing it
not to copy CONFIG_CMDLINE to boot_command_line if a chosen node with no
bootargs property is found.

Unfortunately this is problematic for systems which do have a non-empty
bootargs property & CONFIG_CMDLINE_BOOL=y. There
early_init_dt_scan_chosen() will overwrite boot_command_line with the
arguments from DT, which means we lose those from CONFIG_CMDLINE
entirely. This breaks CONFIG_MIPS_CMDLINE_DTB_EXTEND. If we have
CONFIG_MIPS_CMDLINE_FROM_BOOTLOADER or
CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND selected and the DT has a bootargs
property which we should ignore, it will instead be honoured breaking
those configurations too.

Fix this by reverting commit 8ce355cf2e38 ("MIPS: Setup
boot_command_line before plat_mem_setup") to restore the former
behaviour, and fixing the CONFIG_CMDLINE duplication issue by instead
providing a no-op implementation of early_init_dt_fixup_cmdline_arch()
to prevent early_init_dt_scan_chosen() from using CONFIG_CMDLINE.

Signed-off-by: Paul Burton <paul.burton@mips.com>
Fixes: 8ce355cf2e38 ("MIPS: Setup boot_command_line before plat_mem_setup")
References: https://patchwork.linux-mips.org/patch/18804/
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Jaedon Shin <jaedon.shin@gmail.com>
Cc: Mathieu Malaterre <malat@debian.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: stable@vger.kernel.org # v4.16+
---
 arch/mips/kernel/setup.c | 47 +++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index c71d1eb7da59..7f755bc8a91c 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -841,11 +841,38 @@ static void __init request_crashkernel(struct resource *res)
 #define BUILTIN_EXTEND_WITH_PROM	\
 	IS_ENABLED(CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND)
 
+void __init early_init_dt_fixup_cmdline_arch(char *data)
+{
+	/*
+	 * Do nothing - arch_mem_init() will assemble our command line below,
+	 * for both the DT & non-DT cases.
+	 */
+}
+
 static void __init arch_mem_init(char **cmdline_p)
 {
 	struct memblock_region *reg;
 	extern void plat_mem_setup(void);
 
+	/* call board setup routine */
+	plat_mem_setup();
+
+	/*
+	 * Make sure all kernel memory is in the maps.  The "UP" and
+	 * "DOWN" are opposite for initdata since if it crosses over
+	 * into another memory section you don't want that to be
+	 * freed when the initdata is freed.
+	 */
+	arch_mem_addpart(PFN_DOWN(__pa_symbol(&_text)) << PAGE_SHIFT,
+			 PFN_UP(__pa_symbol(&_edata)) << PAGE_SHIFT,
+			 BOOT_MEM_RAM);
+	arch_mem_addpart(PFN_UP(__pa_symbol(&__init_begin)) << PAGE_SHIFT,
+			 PFN_DOWN(__pa_symbol(&__init_end)) << PAGE_SHIFT,
+			 BOOT_MEM_INIT_RAM);
+
+	pr_info("Determined physical RAM map:\n");
+	print_memory_map();
+
 #if defined(CONFIG_CMDLINE_BOOL) && defined(CONFIG_CMDLINE_OVERRIDE)
 	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
 #else
@@ -873,26 +900,6 @@ static void __init arch_mem_init(char **cmdline_p)
 	}
 #endif
 #endif
-
-	/* call board setup routine */
-	plat_mem_setup();
-
-	/*
-	 * Make sure all kernel memory is in the maps.  The "UP" and
-	 * "DOWN" are opposite for initdata since if it crosses over
-	 * into another memory section you don't want that to be
-	 * freed when the initdata is freed.
-	 */
-	arch_mem_addpart(PFN_DOWN(__pa_symbol(&_text)) << PAGE_SHIFT,
-			 PFN_UP(__pa_symbol(&_edata)) << PAGE_SHIFT,
-			 BOOT_MEM_RAM);
-	arch_mem_addpart(PFN_UP(__pa_symbol(&__init_begin)) << PAGE_SHIFT,
-			 PFN_DOWN(__pa_symbol(&__init_end)) << PAGE_SHIFT,
-			 BOOT_MEM_INIT_RAM);
-
-	pr_info("Determined physical RAM map:\n");
-	print_memory_map();
-
 	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
 
 	*cmdline_p = command_line;
-- 
2.18.0

Re: [PATCH 1/2] of/fdt: Allow architectures to override CONFIG_CMDLINE logic

[Rob Herring]

On Fri, Sep 7, 2018 at 1:55 PM Paul Burton <paul.burton@mips.com> wrote:
>
> The CONFIG_CMDLINE-related logic in early_init_dt_scan_chosen() falls
> back to copying CONFIG_CMDLINE into boot_command_line/data if the DT has
> a /chosen node but that node has no bootargs property or a bootargs
> property of length zero.

The Risc-V guys found a similar issue if chosen is missing[1]. I
started a patch[2] to address that, but then looking at the different
arches wasn't sure if I'd break something. I don't recall for sure,
but it may have been MIPS that worried me.

> This is problematic for the MIPS architecture because we support
> concatenating arguments from either the DT or the bootloader with those
> from CONFIG_CMDLINE, but the behaviour of early_init_dt_scan_chosen()
> gives us no way of knowing whether boot_command_line contains arguments
> from DT or already contains CONFIG_CMDLINE. This can lead to us
> concatenating CONFIG_CMDLINE with itself, duplicating command line
> arguments which can be problematic (eg. for earlycon which will attempt
> to register the same console twice & warn about it).

If CONFIG_CMDLINE_EXTEND is set, you know it contains CONFIG_CMDLINE.
But I guess part of the problem is MIPS using its own kconfig options.

> Move the CONFIG_CMDLINE-related logic to a weak function that
> architectures can provide their own version of, such that we continue to
> use the existing logic for architectures where it's suitable but also
> allow MIPS to override this behaviour such that the architecture code
> knows when CONFIG_CMDLINE is used.

More arch specific functions is not what I want. Really, all the
cmdline manipulating logic doesn't belong in DT code, but it shouldn't
be in the arch specific code either IMO. Really it should be some
common kernel function which calls into the DT code to retrieve the DT
bootargs and that's it. Then you can skip calling that kernel function
if you really need non-standard handling.

Perhaps you should consider filling DT bootargs with the cmdline from
bootloader. IOW, make the legacy case look like the non-legacy case
early, and then the kernel doesn't have to deal with both cases later
on.

Rob

[1] https://lkml.org/lkml/2018/3/14/701
[2] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dt/cmdline

Re: [PATCH 1/2] of/fdt: Allow architectures to override CONFIG_CMDLINE logic

[Paul Burton]

Hi Rob,

On Fri, Sep 07, 2018 at 03:29:03PM -0500, Rob Herring wrote:
> On Fri, Sep 7, 2018 at 1:55 PM Paul Burton <paul.burton@mips.com> wrote:
> > The CONFIG_CMDLINE-related logic in early_init_dt_scan_chosen() falls
> > back to copying CONFIG_CMDLINE into boot_command_line/data if the DT has
> > a /chosen node but that node has no bootargs property or a bootargs
> > property of length zero.
> 
> The Risc-V guys found a similar issue if chosen is missing[1]. I
> started a patch[2] to address that, but then looking at the different
> arches wasn't sure if I'd break something. I don't recall for sure,
> but it may have been MIPS that worried me.
> 
> > This is problematic for the MIPS architecture because we support
> > concatenating arguments from either the DT or the bootloader with those
> > from CONFIG_CMDLINE, but the behaviour of early_init_dt_scan_chosen()
> > gives us no way of knowing whether boot_command_line contains arguments
> > from DT or already contains CONFIG_CMDLINE. This can lead to us
> > concatenating CONFIG_CMDLINE with itself, duplicating command line
> > arguments which can be problematic (eg. for earlycon which will attempt
> > to register the same console twice & warn about it).
> 
> If CONFIG_CMDLINE_EXTEND is set, you know it contains CONFIG_CMDLINE.
> But I guess part of the problem is MIPS using its own kconfig options.

Yes, that's part of the problem but there's more:

  - We can also take arguments from the bootloader/prom, so it's not
    just DT or CONFIG_CMDLINE as taken into account by
    early_init_dt_scan_chosen(). MIPS has options to concatenate various
    combinations of DT, bootloader & CONFIG_CMDLINE arguments & there's
    no mapping to move all of them to just CONFIG_CMDLINE_EXTEND &
    CONFIG_CMDLINE_OVERRIDE.

  - Some MIPS systems don't use DT, so don't run
    early_init_dt_scan_chosen() at all. Thus the architecture code still
    needs to deal with the bootloader/prom & CONFIG_CMDLINE cases
    anyway. In a perfect world we'd migrate all systems to use DT but in
    this world I don't see that happening until we kill off support for
    some of the older systems, which always seems contentious. Even then
    there'd be a lot of work for some of the remaining systems. I guess
    we could enable DT for these systems & only use it for the command
    line, it just feels like overkill.

> > Move the CONFIG_CMDLINE-related logic to a weak function that
> > architectures can provide their own version of, such that we continue to
> > use the existing logic for architectures where it's suitable but also
> > allow MIPS to override this behaviour such that the architecture code
> > knows when CONFIG_CMDLINE is used.
> 
> More arch specific functions is not what I want. Really, all the
> cmdline manipulating logic doesn't belong in DT code, but it shouldn't
> be in the arch specific code either IMO. Really it should be some
> common kernel function which calls into the DT code to retrieve the DT
> bootargs and that's it. Then you can skip calling that kernel function
> if you really need non-standard handling.

That would make sense.

> Perhaps you should consider filling DT bootargs with the cmdline from
> bootloader. IOW, make the legacy case look like the non-legacy case
> early, and then the kernel doesn't have to deal with both cases later
> on.

That could work, but would force us to use DT universally & is a larger
change than this, which I was hoping to get in 4.19 to fix the
regression described in patch 2 that happened back in 4.16. But if
you're unhappy with this perhaps we just have to live with it a little
longer...

An alternative workaround to this that would contain the regression fix
within arch/mips/ would be to initialize boot_command_line to a single
space character prior to early_init_dt_scan_chosen(), which would
prevent early_init_dt_scan_chosen() from copying CONFIG_CMDLINE there.
That smells much more like a hack to me than this patch though, so I'd
rather not.

Thanks,
    Paul

Re: [PATCH 1/2] of/fdt: Allow architectures to override CONFIG_CMDLINE logic

[Rob Herring]

On Fri, Sep 7, 2018 at 4:01 PM Paul Burton <paul.burton@mips.com> wrote:
>
> Hi Rob,
>
> On Fri, Sep 07, 2018 at 03:29:03PM -0500, Rob Herring wrote:
> > On Fri, Sep 7, 2018 at 1:55 PM Paul Burton <paul.burton@mips.com> wrote:
> > > The CONFIG_CMDLINE-related logic in early_init_dt_scan_chosen() falls
> > > back to copying CONFIG_CMDLINE into boot_command_line/data if the DT has
> > > a /chosen node but that node has no bootargs property or a bootargs
> > > property of length zero.
> >
> > The Risc-V guys found a similar issue if chosen is missing[1]. I
> > started a patch[2] to address that, but then looking at the different
> > arches wasn't sure if I'd break something. I don't recall for sure,
> > but it may have been MIPS that worried me.
> >
> > > This is problematic for the MIPS architecture because we support
> > > concatenating arguments from either the DT or the bootloader with those
> > > from CONFIG_CMDLINE, but the behaviour of early_init_dt_scan_chosen()
> > > gives us no way of knowing whether boot_command_line contains arguments
> > > from DT or already contains CONFIG_CMDLINE. This can lead to us
> > > concatenating CONFIG_CMDLINE with itself, duplicating command line
> > > arguments which can be problematic (eg. for earlycon which will attempt
> > > to register the same console twice & warn about it).
> >
> > If CONFIG_CMDLINE_EXTEND is set, you know it contains CONFIG_CMDLINE.
> > But I guess part of the problem is MIPS using its own kconfig options.
>
> Yes, that's part of the problem but there's more:
>
>   - We can also take arguments from the bootloader/prom, so it's not
>     just DT or CONFIG_CMDLINE as taken into account by
>     early_init_dt_scan_chosen(). MIPS has options to concatenate various
>     combinations of DT, bootloader & CONFIG_CMDLINE arguments & there's
>     no mapping to move all of them to just CONFIG_CMDLINE_EXTEND &
>     CONFIG_CMDLINE_OVERRIDE.
>
>   - Some MIPS systems don't use DT, so don't run
>     early_init_dt_scan_chosen() at all. Thus the architecture code still
>     needs to deal with the bootloader/prom & CONFIG_CMDLINE cases
>     anyway. In a perfect world we'd migrate all systems to use DT but in
>     this world I don't see that happening until we kill off support for
>     some of the older systems, which always seems contentious. Even then
>     there'd be a lot of work for some of the remaining systems. I guess
>     we could enable DT for these systems & only use it for the command
>     line, it just feels like overkill.

We have the same thing with old systems on ARM. I think the big
difference is the code paths are well separated. We have support for
old bootloaders which populates old boot parameters (atags) into DT
(and can replace or extend DT bootargs). That's all in the zImage
decompressor wrapper. Then in the kernel, there are 2 separate paths
depending whether you have atags or DT (setup_machine_tags and
setup_machine_fdt). I think the difference is we've imposed rules such
as the bootloader can only pass parameters one of 2 ways and if DT is
used then the bootargs must come from DT.

> > > Move the CONFIG_CMDLINE-related logic to a weak function that
> > > architectures can provide their own version of, such that we continue to
> > > use the existing logic for architectures where it's suitable but also
> > > allow MIPS to override this behaviour such that the architecture code
> > > knows when CONFIG_CMDLINE is used.
> >
> > More arch specific functions is not what I want. Really, all the
> > cmdline manipulating logic doesn't belong in DT code, but it shouldn't
> > be in the arch specific code either IMO. Really it should be some
> > common kernel function which calls into the DT code to retrieve the DT
> > bootargs and that's it. Then you can skip calling that kernel function
> > if you really need non-standard handling.
>
> That would make sense.
>
> > Perhaps you should consider filling DT bootargs with the cmdline from
> > bootloader. IOW, make the legacy case look like the non-legacy case
> > early, and then the kernel doesn't have to deal with both cases later
> > on.
>
> That could work, but would force us to use DT universally & is a larger
> change than this, which I was hoping to get in 4.19 to fix the
> regression described in patch 2 that happened back in 4.16. But if
> you're unhappy with this perhaps we just have to live with it a little
> longer...

You should probably at least do just the revert as that was what
worked for some platforms. But I guess you enabled some platforms and
just reverting would break them after working for a couple of cycles.

> An alternative workaround to this that would contain the regression fix
> within arch/mips/ would be to initialize boot_command_line to a single
> space character prior to early_init_dt_scan_chosen(), which would
> prevent early_init_dt_scan_chosen() from copying CONFIG_CMDLINE there.
> That smells much more like a hack to me than this patch though, so I'd
> rather not.

That doesn't seem that bad to me. If you want to put an "if
(IS_ENABLED(CONFIG_MIPS)) return;" in the chosen scan as a short term
fix, I'd be fine with that.

Rob

Re: [PATCH 2/2] MIPS: Fix CONFIG_CMDLINE handling

[Mathieu Malaterre]

On Fri, Sep 7, 2018 at 8:55 PM Paul Burton <paul.burton@mips.com> wrote:
>
> Commit 8ce355cf2e38 ("MIPS: Setup boot_command_line before
> plat_mem_setup") fixed a problem for systems which have
> CONFIG_CMDLINE_BOOL=y & use a DT with a chosen node that has either no
> bootargs property or an empty one. In this configuration
> early_init_dt_scan_chosen() copies CONFIG_CMDLINE into
> boot_command_line, but the MIPS code doesn't know this so it appends
> CONFIG_CMDLINE (via builtin_cmdline) to boot_command_line again. The
> result is that boot_command_line contains the arguments from
> CONFIG_CMDLINE twice.
>
> That commit took the approach of simply setting up boot_command_line
> from the MIPS code before early_init_dt_scan_chosen() runs, causing it
> not to copy CONFIG_CMDLINE to boot_command_line if a chosen node with no
> bootargs property is found.
>
> Unfortunately this is problematic for systems which do have a non-empty
> bootargs property & CONFIG_CMDLINE_BOOL=y. There
> early_init_dt_scan_chosen() will overwrite boot_command_line with the
> arguments from DT, which means we lose those from CONFIG_CMDLINE
> entirely. This breaks CONFIG_MIPS_CMDLINE_DTB_EXTEND. If we have
> CONFIG_MIPS_CMDLINE_FROM_BOOTLOADER or
> CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND selected and the DT has a bootargs
> property which we should ignore, it will instead be honoured breaking
> those configurations too.
>
> Fix this by reverting commit 8ce355cf2e38 ("MIPS: Setup
> boot_command_line before plat_mem_setup") to restore the former
> behaviour, and fixing the CONFIG_CMDLINE duplication issue by instead
> providing a no-op implementation of early_init_dt_fixup_cmdline_arch()
> to prevent early_init_dt_scan_chosen() from using CONFIG_CMDLINE.

Sorry I cannot test this patch ATM. I've simply CCed Paul C. just in case.

> Signed-off-by: Paul Burton <paul.burton@mips.com>
> Fixes: 8ce355cf2e38 ("MIPS: Setup boot_command_line before plat_mem_setup")
> References: https://patchwork.linux-mips.org/patch/18804/
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: Jaedon Shin <jaedon.shin@gmail.com>
> Cc: Mathieu Malaterre <malat@debian.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: stable@vger.kernel.org # v4.16+
> ---
>  arch/mips/kernel/setup.c | 47 +++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index c71d1eb7da59..7f755bc8a91c 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -841,11 +841,38 @@ static void __init request_crashkernel(struct resource *res)
>  #define BUILTIN_EXTEND_WITH_PROM       \
>         IS_ENABLED(CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND)
>
> +void __init early_init_dt_fixup_cmdline_arch(char *data)
> +{
> +       /*
> +        * Do nothing - arch_mem_init() will assemble our command line below,
> +        * for both the DT & non-DT cases.
> +        */
> +}
> +
>  static void __init arch_mem_init(char **cmdline_p)
>  {
>         struct memblock_region *reg;
>         extern void plat_mem_setup(void);
>
> +       /* call board setup routine */
> +       plat_mem_setup();
> +
> +       /*
> +        * Make sure all kernel memory is in the maps.  The "UP" and
> +        * "DOWN" are opposite for initdata since if it crosses over
> +        * into another memory section you don't want that to be
> +        * freed when the initdata is freed.
> +        */
> +       arch_mem_addpart(PFN_DOWN(__pa_symbol(&_text)) << PAGE_SHIFT,
> +                        PFN_UP(__pa_symbol(&_edata)) << PAGE_SHIFT,
> +                        BOOT_MEM_RAM);
> +       arch_mem_addpart(PFN_UP(__pa_symbol(&__init_begin)) << PAGE_SHIFT,
> +                        PFN_DOWN(__pa_symbol(&__init_end)) << PAGE_SHIFT,
> +                        BOOT_MEM_INIT_RAM);
> +
> +       pr_info("Determined physical RAM map:\n");
> +       print_memory_map();
> +
>  #if defined(CONFIG_CMDLINE_BOOL) && defined(CONFIG_CMDLINE_OVERRIDE)
>         strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>  #else
> @@ -873,26 +900,6 @@ static void __init arch_mem_init(char **cmdline_p)
>         }
>  #endif
>  #endif
> -
> -       /* call board setup routine */
> -       plat_mem_setup();
> -
> -       /*
> -        * Make sure all kernel memory is in the maps.  The "UP" and
> -        * "DOWN" are opposite for initdata since if it crosses over
> -        * into another memory section you don't want that to be
> -        * freed when the initdata is freed.
> -        */
> -       arch_mem_addpart(PFN_DOWN(__pa_symbol(&_text)) << PAGE_SHIFT,
> -                        PFN_UP(__pa_symbol(&_edata)) << PAGE_SHIFT,
> -                        BOOT_MEM_RAM);
> -       arch_mem_addpart(PFN_UP(__pa_symbol(&__init_begin)) << PAGE_SHIFT,
> -                        PFN_DOWN(__pa_symbol(&__init_end)) << PAGE_SHIFT,
> -                        BOOT_MEM_INIT_RAM);
> -
> -       pr_info("Determined physical RAM map:\n");
> -       print_memory_map();
> -
>         strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
>
>         *cmdline_p = command_line;
> --
> 2.18.0
>

Re: [PATCH 2/2] MIPS: Fix CONFIG_CMDLINE handling

[Paul Burton]

Hi Matthieu,

On Mon, Sep 10, 2018 at 02:09:21PM +0200, Mathieu Malaterre wrote:
> On Fri, Sep 7, 2018 at 8:55 PM Paul Burton <paul.burton@mips.com> wrote:
> > Commit 8ce355cf2e38 ("MIPS: Setup boot_command_line before
> > plat_mem_setup") fixed a problem for systems which have
> > CONFIG_CMDLINE_BOOL=y & use a DT with a chosen node that has either no
> > bootargs property or an empty one. In this configuration
> > early_init_dt_scan_chosen() copies CONFIG_CMDLINE into
> > boot_command_line, but the MIPS code doesn't know this so it appends
> > CONFIG_CMDLINE (via builtin_cmdline) to boot_command_line again. The
> > result is that boot_command_line contains the arguments from
> > CONFIG_CMDLINE twice.
> >
> > That commit took the approach of simply setting up boot_command_line
> > from the MIPS code before early_init_dt_scan_chosen() runs, causing it
> > not to copy CONFIG_CMDLINE to boot_command_line if a chosen node with no
> > bootargs property is found.
> >
> > Unfortunately this is problematic for systems which do have a non-empty
> > bootargs property & CONFIG_CMDLINE_BOOL=y. There
> > early_init_dt_scan_chosen() will overwrite boot_command_line with the
> > arguments from DT, which means we lose those from CONFIG_CMDLINE
> > entirely. This breaks CONFIG_MIPS_CMDLINE_DTB_EXTEND. If we have
> > CONFIG_MIPS_CMDLINE_FROM_BOOTLOADER or
> > CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND selected and the DT has a bootargs
> > property which we should ignore, it will instead be honoured breaking
> > those configurations too.
> >
> > Fix this by reverting commit 8ce355cf2e38 ("MIPS: Setup
> > boot_command_line before plat_mem_setup") to restore the former
> > behaviour, and fixing the CONFIG_CMDLINE duplication issue by instead
> > providing a no-op implementation of early_init_dt_fixup_cmdline_arch()
> > to prevent early_init_dt_scan_chosen() from using CONFIG_CMDLINE.
> 
> Sorry I cannot test this patch ATM. I've simply CCed Paul C. just in case.

Thanks - for the record I did test on a Ci20, though I was booting using
my ci20-usb-boot tool which doesn't currently provide a command line to
the kernel. CONFIG_CMDLINE didn't get duplicated though.

I'll get back to a v2 for this & hopefully get it fixed up for 4.19, but
it might take a little while because my son was just born on Saturday :)

Thanks,
    Paul

[PATCH v2] MIPS: Fix CONFIG_CMDLINE handling

[Paul Burton]

Commit 8ce355cf2e38 ("MIPS: Setup boot_command_line before
plat_mem_setup") fixed a problem for systems which have
CONFIG_CMDLINE_BOOL=y & use a DT with a chosen node that has either no
bootargs property or an empty one. In this configuration
early_init_dt_scan_chosen() copies CONFIG_CMDLINE into
boot_command_line, but the MIPS code doesn't know this so it appends
CONFIG_CMDLINE (via builtin_cmdline) to boot_command_line again. The
result is that boot_command_line contains the arguments from
CONFIG_CMDLINE twice.

That commit took the approach of simply setting up boot_command_line
from the MIPS code before early_init_dt_scan_chosen() runs, causing it
not to copy CONFIG_CMDLINE to boot_command_line if a chosen node with no
bootargs property is found.

Unfortunately this is problematic for systems which do have a non-empty
bootargs property & CONFIG_CMDLINE_BOOL=y. There
early_init_dt_scan_chosen() will overwrite boot_command_line with the
arguments from DT, which means we lose those from CONFIG_CMDLINE
entirely. This breaks CONFIG_MIPS_CMDLINE_DTB_EXTEND. If we have
CONFIG_MIPS_CMDLINE_FROM_BOOTLOADER or
CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND selected and the DT has a bootargs
property which we should ignore, it will instead be honoured breaking
those configurations too.

Fix this by reverting commit 8ce355cf2e38 ("MIPS: Setup
boot_command_line before plat_mem_setup") to restore the former
behaviour, and fixing the CONFIG_CMDLINE duplication issue by
initializing boot_command_line to a non-empty string that
early_init_dt_scan_chosen() will not overwrite with CONFIG_CMDLINE.

This is a little ugly, but cleanup in this area is on its way. In the
meantime this is at least easy to backport & contains the ugliness
within arch/mips/.

Signed-off-by: Paul Burton <paul.burton@mips.com>
Fixes: 8ce355cf2e38 ("MIPS: Setup boot_command_line before plat_mem_setup")
References: https://patchwork.linux-mips.org/patch/18804/
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Jaedon Shin <jaedon.shin@gmail.com>
Cc: Mathieu Malaterre <malat@debian.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: stable@vger.kernel.org # v4.16+
---
This isn't pretty, but further cleanup is underway [1] and this fixes
the regression in an easily backportable manner without adding another
per-architecture function like v1.

[1] https://www.spinics.net/lists/kernel/msg2918275.html

Changes in v2:
 - Drop the arch-specific function & just initialize boot_command_line
   such that early_init_dt_scan_chosen() won't overwrite it with
   CONFIG_CMDLINE.

v1: https://marc.info/?l=linux-mips&m=153634652318477&w=2
    https://marc.info/?l=linux-mips&m=153634653718485&w=2
---
 arch/mips/kernel/setup.c | 48 +++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index c71d1eb7da59..8aaaa42f91ed 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -846,6 +846,34 @@ static void __init arch_mem_init(char **cmdline_p)
 	struct memblock_region *reg;
 	extern void plat_mem_setup(void);
 
+	/*
+	 * Initialize boot_command_line to an innocuous but non-empty string in
+	 * order to prevent early_init_dt_scan_chosen() from copying
+	 * CONFIG_CMDLINE into it without our knowledge. We handle
+	 * CONFIG_CMDLINE ourselves below & don't want to duplicate its
+	 * content because repeating arguments can be problematic.
+	 */
+	strlcpy(boot_command_line, " ", COMMAND_LINE_SIZE);
+
+	/* call board setup routine */
+	plat_mem_setup();
+
+	/*
+	 * Make sure all kernel memory is in the maps.  The "UP" and
+	 * "DOWN" are opposite for initdata since if it crosses over
+	 * into another memory section you don't want that to be
+	 * freed when the initdata is freed.
+	 */
+	arch_mem_addpart(PFN_DOWN(__pa_symbol(&_text)) << PAGE_SHIFT,
+			 PFN_UP(__pa_symbol(&_edata)) << PAGE_SHIFT,
+			 BOOT_MEM_RAM);
+	arch_mem_addpart(PFN_UP(__pa_symbol(&__init_begin)) << PAGE_SHIFT,
+			 PFN_DOWN(__pa_symbol(&__init_end)) << PAGE_SHIFT,
+			 BOOT_MEM_INIT_RAM);
+
+	pr_info("Determined physical RAM map:\n");
+	print_memory_map();
+
 #if defined(CONFIG_CMDLINE_BOOL) && defined(CONFIG_CMDLINE_OVERRIDE)
 	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
 #else
@@ -873,26 +901,6 @@ static void __init arch_mem_init(char **cmdline_p)
 	}
 #endif
 #endif
-
-	/* call board setup routine */
-	plat_mem_setup();
-
-	/*
-	 * Make sure all kernel memory is in the maps.  The "UP" and
-	 * "DOWN" are opposite for initdata since if it crosses over
-	 * into another memory section you don't want that to be
-	 * freed when the initdata is freed.
-	 */
-	arch_mem_addpart(PFN_DOWN(__pa_symbol(&_text)) << PAGE_SHIFT,
-			 PFN_UP(__pa_symbol(&_edata)) << PAGE_SHIFT,
-			 BOOT_MEM_RAM);
-	arch_mem_addpart(PFN_UP(__pa_symbol(&__init_begin)) << PAGE_SHIFT,
-			 PFN_DOWN(__pa_symbol(&__init_end)) << PAGE_SHIFT,
-			 BOOT_MEM_INIT_RAM);
-
-	pr_info("Determined physical RAM map:\n");
-	print_memory_map();
-
 	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
 
 	*cmdline_p = command_line;
-- 
2.19.0

Re: [PATCH 1/2] of/fdt: Allow architectures to override CONFIG_CMDLINE logic

[Palmer Dabbelt]

On Fri, 07 Sep 2018 13:29:03 PDT (-0700), robh+dt@kernel.org wrote:
> On Fri, Sep 7, 2018 at 1:55 PM Paul Burton <paul.burton@mips.com> wrote:
>>
>> The CONFIG_CMDLINE-related logic in early_init_dt_scan_chosen() falls
>> back to copying CONFIG_CMDLINE into boot_command_line/data if the DT has
>> a /chosen node but that node has no bootargs property or a bootargs
>> property of length zero.
>
> The Risc-V guys found a similar issue if chosen is missing[1]. I
> started a patch[2] to address that, but then looking at the different
> arches wasn't sure if I'd break something. I don't recall for sure,
> but it may have been MIPS that worried me.

IIRC we actually determined it didn't even work correctly on RISC-V, but I 
never actually got the time to figure out why and then forgot about it.  Sorry!

>
>> This is problematic for the MIPS architecture because we support
>> concatenating arguments from either the DT or the bootloader with those
>> from CONFIG_CMDLINE, but the behaviour of early_init_dt_scan_chosen()
>> gives us no way of knowing whether boot_command_line contains arguments
>> from DT or already contains CONFIG_CMDLINE. This can lead to us
>> concatenating CONFIG_CMDLINE with itself, duplicating command line
>> arguments which can be problematic (eg. for earlycon which will attempt
>> to register the same console twice & warn about it).
>
> If CONFIG_CMDLINE_EXTEND is set, you know it contains CONFIG_CMDLINE.
> But I guess part of the problem is MIPS using its own kconfig options.
>
>> Move the CONFIG_CMDLINE-related logic to a weak function that
>> architectures can provide their own version of, such that we continue to
>> use the existing logic for architectures where it's suitable but also
>> allow MIPS to override this behaviour such that the architecture code
>> knows when CONFIG_CMDLINE is used.
>
> More arch specific functions is not what I want. Really, all the
> cmdline manipulating logic doesn't belong in DT code, but it shouldn't
> be in the arch specific code either IMO. Really it should be some
> common kernel function which calls into the DT code to retrieve the DT
> bootargs and that's it. Then you can skip calling that kernel function
> if you really need non-standard handling.
>
> Perhaps you should consider filling DT bootargs with the cmdline from
> bootloader. IOW, make the legacy case look like the non-legacy case
> early, and then the kernel doesn't have to deal with both cases later
> on.
>
> Rob
>
> [1] https://lkml.org/lkml/2018/3/14/701
> [2] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dt/cmdline