| Message ID | 20161208232100.5747-1-hauke@hauke-m.de |
|---|---|
| State | Changes Requested |
| Headers | show |
On 09/12/2016 00:21, Hauke Mehrtens wrote: > This was introduced in kernel 4.4, but broken there and fixed in 4.5. > I would like to activate it, but I am scared about the boot loader > around giving us all sorts of command lines. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> we had previously used a syntax of prepending the cmdline with +/- to indicate whether this should replace/extend the bootloader cmdline. the biggest problem is that the bootloaders will pass console= and root= and thus breaking boot. merging/activating this as is will most likely lead to 50% of lantiq boards not booting anymore. imho this feature should be an opt-in based on the specific board John > --- > .../patches-4.4/094-MIPS-Fix-macro-typo.patch | 33 ++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > create mode 100644 target/linux/generic/patches-4.4/094-MIPS-Fix-macro-typo.patch > > diff --git a/target/linux/generic/patches-4.4/094-MIPS-Fix-macro-typo.patch b/target/linux/generic/patches-4.4/094-MIPS-Fix-macro-typo.patch > new file mode 100644 > index 0000000..783e1cf > --- /dev/null > +++ b/target/linux/generic/patches-4.4/094-MIPS-Fix-macro-typo.patch > @@ -0,0 +1,33 @@ > +From 2549cc967ebb4043f3507b55e3dc579f44d3b516 Mon Sep 17 00:00:00 2001 > +From: Jaedon Shin <jaedon.shin@gmail.com> > +Date: Mon, 21 Dec 2015 12:47:35 +0900 > +Subject: [PATCH] MIPS: Fix macro typo > + > +Change the CONFIG_MIPS_CMDLINE_EXTEND to CONFIG_MIPS_CMDLINE_DTB_EXTEND > +to resolve the EXTEND_WITH_PROM macro. > + > +Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com> > +Fixes: 2024972ef533 ("MIPS: Make the kernel arguments from dtb available") > +Reviewed-by: Alexander Sverdlin <alexander.svedlin@gmail.com> > +Cc: Jonas Gorski <jogo@openwrt.org> > +Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > +Cc: Paul Burton <paul.burton@imgtec.com> > +Cc: Aaro Koskinen <aaro.koskinen@nokia.com> > +Cc: linux-mips@linux-mips.org > +Patchwork: https://patchwork.linux-mips.org/patch/11909/ > +Signed-off-by: Ralf Baechle <ralf@linux-mips.org> > +--- > + arch/mips/kernel/setup.c | 2 +- > + 1 file changed, 1 insertion(+), 1 deletion(-) > + > +--- a/arch/mips/kernel/setup.c > ++++ b/arch/mips/kernel/setup.c > +@@ -623,7 +623,7 @@ static void __init request_crashkernel(s > + > + #define USE_PROM_CMDLINE IS_ENABLED(CONFIG_MIPS_CMDLINE_FROM_BOOTLOADER) > + #define USE_DTB_CMDLINE IS_ENABLED(CONFIG_MIPS_CMDLINE_FROM_DTB) > +-#define EXTEND_WITH_PROM IS_ENABLED(CONFIG_MIPS_CMDLINE_EXTEND) > ++#define EXTEND_WITH_PROM IS_ENABLED(CONFIG_MIPS_CMDLINE_DTB_EXTEND) > + > + static void __init arch_mem_init(char **cmdline_p) > + { >
On 12/09/2016 08:08 AM, John Crispin wrote: > > > On 09/12/2016 00:21, Hauke Mehrtens wrote: >> This was introduced in kernel 4.4, but broken there and fixed in 4.5. >> I would like to activate it, but I am scared about the boot loader >> around giving us all sorts of command lines. >> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > > we had previously used a syntax of prepending the cmdline with +/- to > indicate whether this should replace/extend the bootloader cmdline. the > biggest problem is that the bootloaders will pass console= and root= and > thus breaking boot. merging/activating this as is will most likely lead > to 50% of lantiq boards not booting anymore. imho this feature should be > an opt-in based on the specific board > > John Yes when you do not have control over the boot loader this is a big problem, like I said in the commit comment. We had this "bootargs-append" option in our kernel for a long time, but this never got upstream. Are there any more upstream ways to extend the boot loader command line in a device tree file, but not replacing it completely? I was thinking about adding a Kconfig option to append the device tree version to the boot loader version, but I would prefer to do this in the device tree for each board and not globally. Currently I would remove the dtb bootargs line for every board were we need the boot loader command line. Sometimes the Mac address is given by the boot loader in the boot args. Internally at Intel we are booting some SoC with a root file system mounted from a NFS server that makes it pretty easy to modify files without reboot. For this use case we provide the settings for the NFS server in the boot command line. This works with just not defining any bootargs line in the device tree at all. Hauke
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software. Hi John, Hi Hauke, On Fri, Dec 9, 2016 at 8:08 AM, John Crispin <john@phrozen.org> wrote: > > > On 09/12/2016 00:21, Hauke Mehrtens wrote: >> This was introduced in kernel 4.4, but broken there and fixed in 4.5. >> I would like to activate it, but I am scared about the boot loader >> around giving us all sorts of command lines. >> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > > we had previously used a syntax of prepending the cmdline with +/- to > indicate whether this should replace/extend the bootloader cmdline. the > biggest problem is that the bootloaders will pass console= and root= and > thus breaking boot. merging/activating this as is will most likely lead > to 50% of lantiq boards not booting anymore. imho this feature should be > an opt-in based on the specific board Mathias just pointed me at a bug report [0] where I caused the exact problem you described (with [1]). When /chosen/bootargs is not defined the kernel falls back to using the cmdline passed from the bootloader - breaking at least the console in that specific case. maybe we should consider the following use-cases: 1) bootloader passed bogus root=/console= parameters 2) Hauke wants to specify root= himself in the bootloader (where Hauke stands for "developer who wants to use NFS rootfs) 3) bootloader passes some valid parameters (for example ethaddr) - could be the same bootloader as #1 maybe we can brainstorm potential solutions which work in all cases: - if we simply revert my patch we're fixing #1 but that would still leave #2 and #3 unsolved - checking with upstream why the bootloader cmdline is used (CONFIG_MIPS_CMDLINE_FROM_BOOTLOADER is not set in our kernel config), but that would only fix #1 - not reverting my patch and enabling CONFIG_MIPS_CMDLINE_DTB_EXTEND would work for #2 and #3 but leave #1 broken - reverting my patch and enabling CONFIG_MIPS_CMDLINE_DTB_EXTEND would fix #2 and #3 while #1 is broken again - not reverting my patch, enabling CONFIG_MIPS_CMDLINE_DTB_EXTEND and adding a "black- or white-listed bootargs from bootloader" per device might work for all 3 (John, I guess this is what you meant with this opt-in specific behavior?) I'm open for more ideas. I can also have a look at the implementation of a patch once we decided on a solution. Regards, Martin [0] https://bugs.lede-project.org/index.php?do=details&task_id=350 [1] https://git.lede-project.org/?p=source.git;a=commitdiff;h=6b94234a6598b855573a6516494de8e7d755e944
On 12/25/2016 07:12 PM, Martin Blumenstingl wrote: > Hi John, Hi Hauke, > > On Fri, Dec 9, 2016 at 8:08 AM, John Crispin <john@phrozen.org> wrote: >> >> >> On 09/12/2016 00:21, Hauke Mehrtens wrote: >>> This was introduced in kernel 4.4, but broken there and fixed in 4.5. >>> I would like to activate it, but I am scared about the boot loader >>> around giving us all sorts of command lines. >>> >>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >> >> we had previously used a syntax of prepending the cmdline with +/- to >> indicate whether this should replace/extend the bootloader cmdline. the >> biggest problem is that the bootloaders will pass console= and root= and >> thus breaking boot. merging/activating this as is will most likely lead >> to 50% of lantiq boards not booting anymore. imho this feature should be >> an opt-in based on the specific board > Mathias just pointed me at a bug report [0] where I caused the exact > problem you described (with [1]). > When /chosen/bootargs is not defined the kernel falls back to using > the cmdline passed from the bootloader - breaking at least the console > in that specific case. > > maybe we should consider the following use-cases: > 1) bootloader passed bogus root=/console= parameters > 2) Hauke wants to specify root= himself in the bootloader (where Hauke > stands for "developer who wants to use NFS rootfs) It would be sufficient if this would be possible for some boards where I have also control over the boot loader. This way I can make sure they will not add wrong data. > 3) bootloader passes some valid parameters (for example ethaddr) - > could be the same bootloader as #1 For Voice we also get some parameters to reserve memory for the firmware, but we can also handle this reservation in a different way. This would also not get upstream because upstream already have a better mechanism to do this, but it needs a modified firmware which works on virtual memory. This is also not board specific and can also be added statically into the device tree. > maybe we can brainstorm potential solutions which work in all cases: > - if we simply revert my patch we're fixing #1 but that would still > leave #2 and #3 unsolved I like your patch in general. ;-) > - checking with upstream why the bootloader cmdline is used > (CONFIG_MIPS_CMDLINE_FROM_BOOTLOADER is not set in our kernel config), > but that would only fix #1 > - not reverting my patch and enabling CONFIG_MIPS_CMDLINE_DTB_EXTEND > would work for #2 and #3 but leave #1 broken We could do CONFIG_MIPS_CMDLINE_DTB_EXTEND the other way around. let the dtb overwrite everything is gets from the boot loader. > - reverting my patch and enabling CONFIG_MIPS_CMDLINE_DTB_EXTEND would > fix #2 and #3 while #1 is broken again > - not reverting my patch, enabling CONFIG_MIPS_CMDLINE_DTB_EXTEND and > adding a "black- or white-listed bootargs from bootloader" per device > might work for all 3 (John, I guess this is what you meant with this > opt-in specific behavior?) I would like this approach, would it be possible to get this into mainline? > I'm open for more ideas. I can also have a look at the implementation > of a patch once we decided on a solution. > > > Regards, > Martin > > > [0] https://bugs.lede-project.org/index.php?do=details&task_id=350 > [1] https://git.lede-project.org/?p=source.git;a=commitdiff;h=6b94234a6598b855573a6516494de8e7d755e944 >
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software. On Tue, Dec 27, 2016 at 2:01 PM, Hauke Mehrtens <hauke@hauke-m.de> wrote: > > > On 12/25/2016 07:12 PM, Martin Blumenstingl wrote: >> Hi John, Hi Hauke, >> >> On Fri, Dec 9, 2016 at 8:08 AM, John Crispin <john@phrozen.org> wrote: >>> >>> >>> On 09/12/2016 00:21, Hauke Mehrtens wrote: >>>> This was introduced in kernel 4.4, but broken there and fixed in 4.5. >>>> I would like to activate it, but I am scared about the boot loader >>>> around giving us all sorts of command lines. >>>> >>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >>> >>> we had previously used a syntax of prepending the cmdline with +/- to >>> indicate whether this should replace/extend the bootloader cmdline. the >>> biggest problem is that the bootloaders will pass console= and root= and >>> thus breaking boot. merging/activating this as is will most likely lead >>> to 50% of lantiq boards not booting anymore. imho this feature should be >>> an opt-in based on the specific board >> Mathias just pointed me at a bug report [0] where I caused the exact >> problem you described (with [1]). >> When /chosen/bootargs is not defined the kernel falls back to using >> the cmdline passed from the bootloader - breaking at least the console >> in that specific case. >> >> maybe we should consider the following use-cases: >> 1) bootloader passed bogus root=/console= parameters >> 2) Hauke wants to specify root= himself in the bootloader (where Hauke >> stands for "developer who wants to use NFS rootfs) > > It would be sufficient if this would be possible for some boards where > I have also control over the boot loader. This way I can make sure they > will not add wrong data. I guess there may be other use-cases than just NFS rootfs - but I kept it because IMHO it's a good example >> 3) bootloader passes some valid parameters (for example ethaddr) - >> could be the same bootloader as #1 > > For Voice we also get some parameters to reserve memory for the > firmware, but we can also handle this reservation in a different way. > This would also not get upstream because upstream already have a better > mechanism to do this, but it needs a modified firmware which works on > virtual memory. This is also not board specific and can also be added > statically into the device tree. do we typically get these parameters from the bootloader and if we do: is this the desired state? I'm not an expert on this, but it seems to me that describing this information using devicetree and reserved-memory seems to be the "upstream way" >> maybe we can brainstorm potential solutions which work in all cases: >> - if we simply revert my patch we're fixing #1 but that would still >> leave #2 and #3 unsolved > > I like your patch in general. ;-) so do I, that's why I sent it. unfortunately I had a typo during my tests and failed to see that I could actually use the bootloader to pass the cmdline :( >> - checking with upstream why the bootloader cmdline is used >> (CONFIG_MIPS_CMDLINE_FROM_BOOTLOADER is not set in our kernel config), >> but that would only fix #1 >> - not reverting my patch and enabling CONFIG_MIPS_CMDLINE_DTB_EXTEND >> would work for #2 and #3 but leave #1 broken > > We could do CONFIG_MIPS_CMDLINE_DTB_EXTEND the other way around. let the > dtb overwrite everything is gets from the boot loader. but wouldn't that mean that we still had to duplicate the values (= virtually reverting my patch) which we want to override in .dts (for example if we don't want anyone to pass init, then we'd have to specify init=/etc/preinit in our .dts, instead of just saying "I don't want bootloader's init arg")? >> - reverting my patch and enabling CONFIG_MIPS_CMDLINE_DTB_EXTEND would >> fix #2 and #3 while #1 is broken again >> - not reverting my patch, enabling CONFIG_MIPS_CMDLINE_DTB_EXTEND and >> adding a "black- or white-listed bootargs from bootloader" per device >> might work for all 3 (John, I guess this is what you meant with this >> opt-in specific behavior?) > > I would like this approach, would it be possible to get this into mainline? should we discuss this with the linux-mips or devicetree list (or even both)? Regards, Martin
diff --git a/target/linux/generic/patches-4.4/094-MIPS-Fix-macro-typo.patch b/target/linux/generic/patches-4.4/094-MIPS-Fix-macro-typo.patch new file mode 100644 index 0000000..783e1cf --- /dev/null +++ b/target/linux/generic/patches-4.4/094-MIPS-Fix-macro-typo.patch @@ -0,0 +1,33 @@ +From 2549cc967ebb4043f3507b55e3dc579f44d3b516 Mon Sep 17 00:00:00 2001 +From: Jaedon Shin <jaedon.shin@gmail.com> +Date: Mon, 21 Dec 2015 12:47:35 +0900 +Subject: [PATCH] MIPS: Fix macro typo + +Change the CONFIG_MIPS_CMDLINE_EXTEND to CONFIG_MIPS_CMDLINE_DTB_EXTEND +to resolve the EXTEND_WITH_PROM macro. + +Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com> +Fixes: 2024972ef533 ("MIPS: Make the kernel arguments from dtb available") +Reviewed-by: Alexander Sverdlin <alexander.svedlin@gmail.com> +Cc: Jonas Gorski <jogo@openwrt.org> +Cc: Masahiro Yamada <yamada.masahiro@socionext.com> +Cc: Paul Burton <paul.burton@imgtec.com> +Cc: Aaro Koskinen <aaro.koskinen@nokia.com> +Cc: linux-mips@linux-mips.org +Patchwork: https://patchwork.linux-mips.org/patch/11909/ +Signed-off-by: Ralf Baechle <ralf@linux-mips.org> +--- + arch/mips/kernel/setup.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +--- a/arch/mips/kernel/setup.c ++++ b/arch/mips/kernel/setup.c +@@ -623,7 +623,7 @@ static void __init request_crashkernel(s + + #define USE_PROM_CMDLINE IS_ENABLED(CONFIG_MIPS_CMDLINE_FROM_BOOTLOADER) + #define USE_DTB_CMDLINE IS_ENABLED(CONFIG_MIPS_CMDLINE_FROM_DTB) +-#define EXTEND_WITH_PROM IS_ENABLED(CONFIG_MIPS_CMDLINE_EXTEND) ++#define EXTEND_WITH_PROM IS_ENABLED(CONFIG_MIPS_CMDLINE_DTB_EXTEND) + + static void __init arch_mem_init(char **cmdline_p) + {
This was introduced in kernel 4.4, but broken there and fixed in 4.5. I would like to activate it, but I am scared about the boot loader around giving us all sorts of command lines. Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> --- .../patches-4.4/094-MIPS-Fix-macro-typo.patch | 33 ++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 target/linux/generic/patches-4.4/094-MIPS-Fix-macro-typo.patch