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@xxxxxxxx> 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