| Message ID | 20180816173902.18971-1-prarit@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2] console: Add console=auto option | expand |
On Thu 2018-08-16 13:39:01, Prarit Bhargava wrote: > ACPI may contain an SPCR table that defines a default system console. > On ARM if the table is present then the SPCR console is enabled by default. > On x86 the SPCR console is used if 'earlycon' (no parameters) is > specified as a kernel parameter and is used only as the early console. > To use the SPCR data as a console a user must boot with 'earlycon', > grep logs & specify a console= kernel parameter, and then reboot again. > > Add 'console=auto' that enables a firmware or hardware console, and on > x86 enable the SPCR console if 'console=auto' is specified. I basically like the idea. Just one or two nits below. > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index a32f2a126791..dd057224f33b 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -635,6 +635,7 @@ > > hvc<n> Use the hypervisor console device <n>. This is for > both Xen and PowerPC hypervisors. > + auto [X86] Enable ACPI SPCR console The "auto" option sounds reasonable. But earlycon does exactly this when used without no extra options. I prefer to stay consistent with the existing earlycon behavior. > If the device connected to the port is not a TTY but a braille > device, prepend "brl," before the device type, for instance > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > index 3b20607d581b..fb2616ba3b21 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -1771,3 +1771,8 @@ void __init arch_reserve_mem_area(acpi_physical_address addr, size_t size) > e820__range_add(addr, size, E820_TYPE_ACPI); > e820__update_table_print(); > } > + > +void __init arch_console_setup(void) > +{ > + acpi_parse_spcr(false, true); Just for record. I was curious that this might be called twice (also from acpi_boot_init(). But it looks safe after all. The trick is in the two bool parameters. One call allows to register/enable earlycon and ignores normal console. This other call does exactly the opposite. I do not see any unwanted side effects. Best Regards, Petr
On (08/16/18 13:39), Prarit Bhargava wrote: > > + auto [X86] Enable ACPI SPCR console ^^^^ And arm64? Any chance we can rename param to "spcr" or something more clear? To explicitly state what exactly it's going to do. `auto' sounds too general and doesn't tell me that much. I'm probably the only here who can't see a connection between "auto" and "SPCR", but still. One more thing, as far as I can tell, acpi_parse_spcr() can fail and return an error. arch_console_setup() hides all errors and returns void. Should it return error code? int arch_console_setup(void) { return acpi_parse_spcr(false, true); } Or maybe void arch_console_setup(void) { if (acpi_parse_spcr(false, true)) pr_err(.........); } There can be other consoles in the system, logging an error is not such a useless thing. -ss
On 08/17/2018 04:19 AM, Petr Mladek wrote: > On Thu 2018-08-16 13:39:01, Prarit Bhargava wrote: >> ACPI may contain an SPCR table that defines a default system console. >> On ARM if the table is present then the SPCR console is enabled by default. >> On x86 the SPCR console is used if 'earlycon' (no parameters) is >> specified as a kernel parameter and is used only as the early console. >> To use the SPCR data as a console a user must boot with 'earlycon', >> grep logs & specify a console= kernel parameter, and then reboot again. >> >> Add 'console=auto' that enables a firmware or hardware console, and on >> x86 enable the SPCR console if 'console=auto' is specified. > > I basically like the idea. Just one or two nits below. > > >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >> index a32f2a126791..dd057224f33b 100644 >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -635,6 +635,7 @@ >> >> hvc<n> Use the hypervisor console device <n>. This is for >> both Xen and PowerPC hypervisors. >> + auto [X86] Enable ACPI SPCR console > > The "auto" option sounds reasonable. But earlycon does exactly this > when used without no extra options. I prefer to stay consistent > with the existing earlycon behavior. Hi Petr, on x86 earlycon still only enables the early console but does not enable the console. > > >> If the device connected to the port is not a TTY but a braille >> device, prepend "brl," before the device type, for instance >> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c >> index 3b20607d581b..fb2616ba3b21 100644 >> --- a/arch/x86/kernel/acpi/boot.c >> +++ b/arch/x86/kernel/acpi/boot.c >> @@ -1771,3 +1771,8 @@ void __init arch_reserve_mem_area(acpi_physical_address addr, size_t size) >> e820__range_add(addr, size, E820_TYPE_ACPI); >> e820__update_table_print(); >> } >> + >> +void __init arch_console_setup(void) >> +{ >> + acpi_parse_spcr(false, true); > > Just for record. I was curious that this might be called twice > (also from acpi_boot_init(). But it looks safe after all. > Yes, the console code prevents the same console from being registered twice. > The trick is in the two bool parameters. One call allows to > register/enable earlycon and ignores normal console. This other > call does exactly the opposite. I do not see any unwanted side > effects. Thanks. I'd like to know if you (or anyone else) has strong feelings about changing the behaviour of earlycon on x86? I could make it so that specifying just earlycon would also initialize the console. P. > > Best Regards, > Petr >
On 08/17/2018 05:38 AM, Sergey Senozhatsky wrote: > On (08/16/18 13:39), Prarit Bhargava wrote: >> >> + auto [X86] Enable ACPI SPCR console > ^^^^ > And arm64? Hi Sergey, on arm64 if an SPCR is present the early console and console are initialized by default. IOW no kernel parameter is necessary to initialize the console in that case. > > > Any chance we can rename param to "spcr" or something more clear? > To explicitly state what exactly it's going to do. `auto' sounds > too general and doesn't tell me that much. I'm probably the only > here who can't see a connection between "auto" and "SPCR", but > still. I came up with "auto" because I think it is generic. I also thought about "console=fw", or just "console". If in the future another arch wants to optionally bring up a firmware or hardware defined console then they could use auto too. > > One more thing, as far as I can tell, acpi_parse_spcr() can fail > and return an error. arch_console_setup() hides all errors and > returns void. Should it return error code? > > int arch_console_setup(void) > { > return acpi_parse_spcr(false, true); > } > > Or maybe > > void arch_console_setup(void) > { > if (acpi_parse_spcr(false, true)) > pr_err(.........); > } > > There can be other consoles in the system, logging an error is not > such a useless thing. I can make the second change. The problem (IIRC) with returning an error in an setup fn is that the rest of the setup functions will not execute. I don't want to fail the setup callbacks because of an incorrect SPCR table. Like I mentioned to Petr, I'd like to know if you (or anyone else) has strong feelings about changing the behaviour of earlycon on x86? I could make it so that specifying just earlycon would also initialize the console. P. > > -ss >
Hello, Cc-ing Peter Zijlstra lkml.kernel.org/r/728a8e68-ea4b-4040-a0fc-217df4f1928d@redhat.com lkml.kernel.org/r/20180817081947.m425gok2ugt7tglp@pathway.suse.cz lkml.kernel.org/r/00c60dca-60bc-8568-eaa3-d4b0c326cab4@redhat.com On (08/17/18 06:36), Prarit Bhargava wrote: > On 08/17/2018 05:38 AM, Sergey Senozhatsky wrote: > > On (08/16/18 13:39), Prarit Bhargava wrote: > >> > >> + auto [X86] Enable ACPI SPCR console > > ^^^^ > > And arm64? > > Hi Sergey, on arm64 if an SPCR is present the early console and console are > initialized by default. IOW no kernel parameter is necessary to initialize the > console in that case. OK, thanks. > > Any chance we can rename param to "spcr" or something more clear? > > To explicitly state what exactly it's going to do. `auto' sounds > > too general and doesn't tell me that much. I'm probably the only > > here who can't see a connection between "auto" and "SPCR", but > > still. > > I came up with "auto" because I think it is generic. I also thought about > "console=fw", or just "console". If in the future another arch wants to > optionally bring up a firmware or hardware defined console then they could use > auto too. Hmm, I see your point. My [sort of a] problem with "auto" is that it tells me as much as "magic" [and "magic" tells me almost nothing]. By the way, would be fun if we had "magic" instead of "auto" all over the kernel echo "magic" > /sys/bus/usb/...../power/control > > void arch_console_setup(void) > > { > > if (acpi_parse_spcr(false, true)) > > pr_err(.........); > > } > > > > There can be other consoles in the system, logging an error is not > > such a useless thing. > > I can make the second change. The problem (IIRC) with returning an error in an > setup fn is that the rest of the setup functions will not execute. I don't want > to fail the setup callbacks because of an incorrect SPCR table. OK, fair enough. Letting users know that SPCR is incorrect also makes sense, so option #2 I guess is what we want after all. > Like I mentioned to Petr, I'd like to know if you (or anyone else) has strong > feelings about changing the behaviour of earlycon on x86? I could make it so > that specifying just earlycon would also initialize the console. x86 people and/or scheduler people might have strong opinions on this. I Cc-ed Peter Zijlstra; he represents both groups and is known to be a hardcore earlycon user. -ss
On 08/17/2018 06:50 AM, Sergey Senozhatsky wrote: > Hello, > > Cc-ing Peter Zijlstra > > lkml.kernel.org/r/728a8e68-ea4b-4040-a0fc-217df4f1928d@redhat.com > lkml.kernel.org/r/20180817081947.m425gok2ugt7tglp@pathway.suse.cz > lkml.kernel.org/r/00c60dca-60bc-8568-eaa3-d4b0c326cab4@redhat.com > > On (08/17/18 06:36), Prarit Bhargava wrote: >> On 08/17/2018 05:38 AM, Sergey Senozhatsky wrote: >>> On (08/16/18 13:39), Prarit Bhargava wrote: >>>> >>>> + auto [X86] Enable ACPI SPCR console >>> ^^^^ >>> And arm64? >> >> Hi Sergey, on arm64 if an SPCR is present the early console and console are >> initialized by default. IOW no kernel parameter is necessary to initialize the >> console in that case. > > OK, thanks. > >>> Any chance we can rename param to "spcr" or something more clear? >>> To explicitly state what exactly it's going to do. `auto' sounds >>> too general and doesn't tell me that much. I'm probably the only >>> here who can't see a connection between "auto" and "SPCR", but >>> still. >> >> I came up with "auto" because I think it is generic. I also thought about >> "console=fw", or just "console". If in the future another arch wants to >> optionally bring up a firmware or hardware defined console then they could use >> auto too. > > Hmm, I see your point. > My [sort of a] problem with "auto" is that it tells me as much as "magic" > [and "magic" tells me almost nothing]. By the way, would be fun if we had > "magic" instead of "auto" all over the kernel > > echo "magic" > /sys/bus/usb/...../power/control > >>> void arch_console_setup(void) >>> { >>> if (acpi_parse_spcr(false, true)) >>> pr_err(.........); >>> } >>> >>> There can be other consoles in the system, logging an error is not >>> such a useless thing. >> >> I can make the second change. The problem (IIRC) with returning an error in an >> setup fn is that the rest of the setup functions will not execute. I don't want >> to fail the setup callbacks because of an incorrect SPCR table. > > OK, fair enough. > Letting users know that SPCR is incorrect also makes sense, so option #2 > I guess is what we want after all. > >> Like I mentioned to Petr, I'd like to know if you (or anyone else) has strong >> feelings about changing the behaviour of earlycon on x86? I could make it so >> that specifying just earlycon would also initialize the console. > > x86 people and/or scheduler people might have strong opinions on this. > I Cc-ed Peter Zijlstra; he represents both groups and is known to be > a hardcore earlycon user. Thanks, I'm a user of earlycon too, but only moderately. peterz, to give you an overview: Currently on x86, the SPCR information is only interpreted by the early console code, and can be enabled with kernel parameter "earlycon" (no arguments). In this patch I'm proposing adding "console=auto" that would enable the console based on the SPCR data. There are two options on the table. One, we could go with this patch which would make users do "earlycon console=auto" or, I could just change the behaviour of earlycon (no arguments) to also bring up the console. In the second case the kernel parameter would just be "earlycon". There is precedent for the second option as arm64 enables both the earlycon and console by default if SPCR is present. However, on x86, I know many users do not want the console enabled by default so we should keep it on demand. tl;dr: Pick one Option 1: earlycon enables both the early console and console. Option 2: earlycon only enables the early console, and console=auto enables the console. P. > > -ss >
On Fri 2018-08-17 07:06:56, Prarit Bhargava wrote: > On 08/17/2018 06:50 AM, Sergey Senozhatsky wrote: > >> Like I mentioned to Petr, I'd like to know if you (or anyone else) has strong > >> feelings about changing the behaviour of earlycon on x86? I could make it so > >> that specifying just earlycon would also initialize the console. > > > > x86 people and/or scheduler people might have strong opinions on this. > > I Cc-ed Peter Zijlstra; he represents both groups and is known to be > > a hardcore earlycon user. > > Thanks, I'm a user of earlycon too, but only moderately. > > peterz, to give you an overview: Currently on x86, the SPCR information is only > interpreted by the early console code, and can be enabled with kernel parameter > "earlycon" (no arguments). In this patch I'm proposing adding "console=auto" > that would enable the console based on the SPCR data. > > There are two options on the table. One, we could go with this patch which > would make users do "earlycon console=auto" or, I could just change the > behaviour of earlycon (no arguments) to also bring up the console. In the > second case the kernel parameter would just be "earlycon". There is precedent > for the second option as arm64 enables both the earlycon and console by default > if SPCR is present. However, on x86, I know many users do not want the console > enabled by default so we should keep it on demand. > > tl;dr: Pick one > > Option 1: earlycon enables both the early console and console. I am afraid that this is not acceptable. Users are sensitive when a new kernel suddenly enables different consoles. For example, see: + commit dac8bbbae1d0ccba9 ("Revert "printk: fix double printing with earlycon") + commit c6c7d83b9c9e6a8b3 ("Revert "console: don't prefer first registered if DT specifies stdout-path") By other words, the new behavior must depend on a new option. > Option 2: earlycon only enables the early console, and console=auto enables the > console. I suggest: Option 3: "earlycon" enables early console defined by SPCR "console" enables console defined by SPCR I mean that "console" without extra options would enable the console defined by ACPI SPCR. It would work the same as "earlycon" without extra options for early consoles. If you would want to make it explicit than I agree with Sergey and would prefer "console=spcr" instead of "console=auto". Note kernel tries to enable some consoles automatically even when "console" parameter is not defined at all. Therefore "auto" is somehow misleading. Best Regards, Petr PS: JFYI, I have vacation the following week...
On 08/17/2018 08:57 AM, Petr Mladek wrote: > On Fri 2018-08-17 07:06:56, Prarit Bhargava wrote: >> On 08/17/2018 06:50 AM, Sergey Senozhatsky wrote: >>>> Like I mentioned to Petr, I'd like to know if you (or anyone else) has strong >>>> feelings about changing the behaviour of earlycon on x86? I could make it so >>>> that specifying just earlycon would also initialize the console. >>> >>> x86 people and/or scheduler people might have strong opinions on this. >>> I Cc-ed Peter Zijlstra; he represents both groups and is known to be >>> a hardcore earlycon user. >> >> Thanks, I'm a user of earlycon too, but only moderately. >> >> peterz, to give you an overview: Currently on x86, the SPCR information is only >> interpreted by the early console code, and can be enabled with kernel parameter >> "earlycon" (no arguments). In this patch I'm proposing adding "console=auto" >> that would enable the console based on the SPCR data. >> >> There are two options on the table. One, we could go with this patch which >> would make users do "earlycon console=auto" or, I could just change the >> behaviour of earlycon (no arguments) to also bring up the console. In the >> second case the kernel parameter would just be "earlycon". There is precedent >> for the second option as arm64 enables both the earlycon and console by default >> if SPCR is present. However, on x86, I know many users do not want the console >> enabled by default so we should keep it on demand. >> >> tl;dr: Pick one >> >> Option 1: earlycon enables both the early console and console. > > I am afraid that this is not acceptable. Users are sensitive > when a new kernel suddenly enables different consoles. For example, > see: > > + commit dac8bbbae1d0ccba9 ("Revert "printk: fix double printing with > earlycon") > > + commit c6c7d83b9c9e6a8b3 ("Revert "console: don't prefer first > registered if DT specifies stdout-path") > > > By other words, the new behavior must depend on a new option. > > >> Option 2: earlycon only enables the early console, and console=auto enables the >> console. > > I suggest: > > Option 3: "earlycon" enables early console defined by SPCR > "console" enables console defined by SPCR > > I mean that "console" without extra options would enable the console > defined by ACPI SPCR. It would work the same as "earlycon" without > extra options for early consoles. > > If you would want to make it explicit than I agree with Sergey > and would prefer "console=spcr" instead of "console=auto". I'm going with "console=spcr". FYI. P. > > Note kernel tries to enable some consoles automatically even > when "console" parameter is not defined at all. Therefore "auto" > is somehow misleading. > > Best Regards, > Petr > > PS: JFYI, I have vacation the following week... >
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index a32f2a126791..dd057224f33b 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -635,6 +635,7 @@ hvc<n> Use the hypervisor console device <n>. This is for both Xen and PowerPC hypervisors. + auto [X86] Enable ACPI SPCR console If the device connected to the port is not a TTY but a braille device, prepend "brl," before the device type, for instance diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index 3b20607d581b..fb2616ba3b21 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -1771,3 +1771,8 @@ void __init arch_reserve_mem_area(acpi_physical_address addr, size_t size) e820__range_add(addr, size, E820_TYPE_ACPI); e820__update_table_print(); } + +void __init arch_console_setup(void) +{ + acpi_parse_spcr(false, true); +} diff --git a/include/linux/console.h b/include/linux/console.h index dfd6b0e97855..c2a6dde9a799 100644 --- a/include/linux/console.h +++ b/include/linux/console.h @@ -181,6 +181,7 @@ extern int is_console_locked(void); extern int braille_register_console(struct console *, int index, char *console_options, char *braille_options); extern int braille_unregister_console(struct console *); +void arch_console_setup(void); #ifdef CONFIG_TTY extern void console_sysfs_notify(void); #else diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 247808333ba4..bdd53858664b 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -2078,6 +2078,11 @@ static int __init console_msg_format_setup(char *str) } __setup("console_msg_format=", console_msg_format_setup); +void __init __weak arch_console_setup(void) +{ + return; +} + /* * Set up a console. Called via do_early_param() in init/main.c * for each "console=" parameter in the boot command line. @@ -2088,6 +2093,11 @@ static int __init console_setup(char *str) char *s, *options, *brl_options = NULL; int idx; + if (!strcmp(str, "auto")) { + arch_console_setup(); + return 1; + } + if (_braille_console_setup(&str, &brl_options)) return 1;
ACPI may contain an SPCR table that defines a default system console. On ARM if the table is present then the SPCR console is enabled by default. On x86 the SPCR console is used if 'earlycon' (no parameters) is specified as a kernel parameter and is used only as the early console. To use the SPCR data as a console a user must boot with 'earlycon', grep logs & specify a console= kernel parameter, and then reboot again. Add 'console=auto' that enables a firmware or hardware console, and on x86 enable the SPCR console if 'console=auto' is specified. Tested on systems with and without an ACPI SPCR. The following kernel parameters were also tested: console=ttyS0,115200 works earlycon works (early console only) console=auto works (full console as expected) no console or earlycon arguments works (no output as expected) v2: Fix prototype. Signed-off-by: Prarit Bhargava <prarit@redhat.com> Cc: Mark Salter <msalter@redhat.com> Cc: Al Stone <ahs3@redhat.com> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Len Brown <len.brown@intel.com> Cc: Pavel Machek <pavel@ucw.cz> Cc: x86@kernel.org Cc: Petr Mladek <pmladek@suse.com> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Kees Cook <keescook@chromium.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-pm@vger.kernel.org --- Documentation/admin-guide/kernel-parameters.txt | 1 + arch/x86/kernel/acpi/boot.c | 5 +++++ include/linux/console.h | 1 + kernel/printk/printk.c | 10 ++++++++++ 4 files changed, 17 insertions(+)