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
