Re: [PATCH 1/4] MIPS: cmdline: Remove CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND

[Paul Burton <paul.burton@mips.com>]

Hi Maciej,

On Wed, Oct 09, 2019 at 03:13:09PM +0100, Maciej W. Rozycki wrote:
> On Wed, 9 Oct 2019, Maciej W. Rozycki wrote:
> 
> > > CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND is not selected by any of our
> > > defconfigs, so remove it to simplify the messy command line logic in
> > > arch_mem_init() a little.
> > 
> >  That sounds like a poor argument for a functional regression to me.  I 
> > have the option enabled in several configs I have been using just to be 
> > able to temporarily override any built-in parameters with ones typed from 
> > the console monitor's prompt.  Is it my mistake that I haven't put it in a 
> > defconfig?

For a "functional regression" sure. For removal of an unused code path
I'd say not, and presence in defconfigs is generally a reasonable
indication of use. The knowledge that you do actually use this is useful
information.

I'll note that I dislike the idea of overriding options built into the
kernel from the bootloader because:

 1) Some options just don't work that way & specifying them twice has
    unexpected effects, so this option gives users another way to screw
    up. But sure, a user will always be able to screw up if they try
    hard enough etc etc.

 2) As you say yourself a defconfig generally won't (or at least
    shouldn't) have much of anything in CONFIG_CMDLINE anyway. That
    means this use case requires someone to build their own kernel
    whilst explicitly specifying built-in arguments they don't actually
    want, which I'd consider somewhat niche.

Having said that if you find it useful I'm not against keeping that
ability, but I think we should do it a little differently by noting that
the option is essentially the same as MIPS_CMDLINE_FROM_BOOTLOADER apart
from the order in which bootloader & built-in arguments are
concatenated.

So I'd prefer that we handle this by either providing an option to
specify whether CONFIG_CMDLINE is prepended or appended to the
bootloader/DTB arguments, or perhaps by adding a separate
CONFIG_CMDLINE_PREPEND string. Either one of these should still allow
for your use case whilst also allowing the code that has to deal with it
to be less of an eyesore.

>  Elaborating, there's IMO little sense to set MIPS_CMDLINE_BUILTIN_EXTEND 
> in a defconfig, because there's usually no default command line to set 
> there in the first place, as this will be installation-specific.  Ergo I 
> highly doubt the absence of the setting across the board is due to nobody 
> (except for myself) using it.
> 
>  Therefore:
> 
> Nacked-by: Maciej W. Rozycki <macro@linux-mips.org>
> 
>  NB DECstation systems use a DS1287 or a similar RTC/NVRAM chip to hold 
> configuration and space is limited there.  Up to 37 of kernel command line 
> characters can be permanently stored, fewer on some systems, and used for 
> automatic boot.  Conversely when typed at the console monitor prompt there 
> is no (known to me) limitation as to the length of the kernel command line 
> requested.
> 
>  Therefore a kernel configured for those systems will normally have 
> several parameters embedded within itself while letting the non-volatile 
> storage or user input extend and/or selectively override them, e.g. for a 
> different root device or whatever.

That's a pretty awful limit.

My intent isn't to actively break these 30 year old machines, but the
cruft that has built up to deal with their limitations over the years
certainly needs some cleanup if it is to remain. Said limitations ought
to be documented within the code so that developers have a chance of
seeing why options like this exist - otherwise every bit of progress
will result in pushback from people using these ancient machines, and I
won't allow these antiques to be an anchor tied to the ankles of
developers making forward progress on machines that actually have users.
There are almost certainly some unmaintained ones whose time is well
past up.

Thanks,
    Paul