| Message ID | 20191007221951.1889661-1-paul.burton@mips.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/4] MIPS: cmdline: Remove CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND | expand |
On 10/8/19 12:20 AM, Paul Burton 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. > > Signed-off-by: Paul Burton <paul.burton@mips.com> > --- > > arch/mips/Kconfig | 4 ---- > arch/mips/kernel/setup.c | 8 -------- > 2 files changed, 12 deletions(-) > > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig > index a0bd9bdb5f83..ec922e6ff40b 100644 > --- a/arch/mips/Kconfig > +++ b/arch/mips/Kconfig > @@ -3034,10 +3034,6 @@ choice > > config MIPS_CMDLINE_FROM_BOOTLOADER > bool "Bootloader kernel arguments if available" > - > - config MIPS_CMDLINE_BUILTIN_EXTEND > - depends on CMDLINE_BOOL > - bool "Extend builtin kernel arguments with bootloader arguments" > endchoice > > endmenu > diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c > index 5eec13b8d222..c2a09f082d88 100644 > --- a/arch/mips/kernel/setup.c > +++ b/arch/mips/kernel/setup.c > @@ -541,8 +541,6 @@ static void __init check_kernel_sections_mem(void) > #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_DTB_EXTEND) > -#define BUILTIN_EXTEND_WITH_PROM \ > - IS_ENABLED(CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND) > > /* > * arch_mem_init - initialize memory management subsystem > @@ -602,12 +600,6 @@ static void __init arch_mem_init(char **cmdline_p) > strlcat(boot_command_line, " ", COMMAND_LINE_SIZE); > strlcat(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); > } > - > - if (BUILTIN_EXTEND_WITH_PROM && arcs_cmdline[0]) { > - if (boot_command_line[0]) > - strlcat(boot_command_line, " ", COMMAND_LINE_SIZE); > - strlcat(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE); > - } > #endif > #endif > strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE); > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On Mon, 7 Oct 2019, Paul Burton 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? Maciej
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? 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. However, as I noted above, there's no reasonable universal default command line to use with a defconfig, just as you could not come up with one for say an x86 PC. I hope this clarifies the matter. Maciej
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
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig index a0bd9bdb5f83..ec922e6ff40b 100644 --- a/arch/mips/Kconfig +++ b/arch/mips/Kconfig @@ -3034,10 +3034,6 @@ choice config MIPS_CMDLINE_FROM_BOOTLOADER bool "Bootloader kernel arguments if available" - - config MIPS_CMDLINE_BUILTIN_EXTEND - depends on CMDLINE_BOOL - bool "Extend builtin kernel arguments with bootloader arguments" endchoice endmenu diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c index 5eec13b8d222..c2a09f082d88 100644 --- a/arch/mips/kernel/setup.c +++ b/arch/mips/kernel/setup.c @@ -541,8 +541,6 @@ static void __init check_kernel_sections_mem(void) #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_DTB_EXTEND) -#define BUILTIN_EXTEND_WITH_PROM \ - IS_ENABLED(CONFIG_MIPS_CMDLINE_BUILTIN_EXTEND) /* * arch_mem_init - initialize memory management subsystem @@ -602,12 +600,6 @@ static void __init arch_mem_init(char **cmdline_p) strlcat(boot_command_line, " ", COMMAND_LINE_SIZE); strlcat(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE); } - - if (BUILTIN_EXTEND_WITH_PROM && arcs_cmdline[0]) { - if (boot_command_line[0]) - strlcat(boot_command_line, " ", COMMAND_LINE_SIZE); - strlcat(boot_command_line, arcs_cmdline, COMMAND_LINE_SIZE); - } #endif #endif strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
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. Signed-off-by: Paul Burton <paul.burton@mips.com> --- arch/mips/Kconfig | 4 ---- arch/mips/kernel/setup.c | 8 -------- 2 files changed, 12 deletions(-)