[LEDE-DEV] [PATCH] kernel: make fix extending dtb cmd with bootloader

[Martin Blumenstingl]

On Tue, Dec 27, 2016 at 2:01 PM, Hauke Mehrtens <hauke at 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 at 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 at 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