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 */
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;
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
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
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
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
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
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;
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
linux-stable-mirror@lists.linaro.org
-
Mathieu Malaterre -
Palmer Dabbelt -
Paul Burton -
Rob Herring