| Message ID | 20240530015332.7305-1-liuwei09@cestc.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ACPI: Do not enable ACPI SPCR console by default on arm64 | expand |
On 5/29/24 21:53, Liu Wei wrote: > Consistency with x86 and loongarch, don't enable ACPI SPCR console > by default on arm64 > > Signed-off-by: Liu Wei <liuwei09@cestc.cn> > --- > arch/arm64/kernel/acpi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index dba8fcec7f33..1deda3e5a0d2 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -227,7 +227,8 @@ void __init acpi_boot_table_init(void) > if (earlycon_acpi_spcr_enable) > early_init_dt_scan_chosen_stdout(); > } else { > - acpi_parse_spcr(earlycon_acpi_spcr_enable, true); > + /* Do not enable ACPI SPCR console by default */ > + acpi_parse_spcr(earlycon_acpi_spcr_enable, false); > if (IS_ENABLED(CONFIG_ACPI_BGRT)) > acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt); > } It's been a while, and the status of arm hardware may have changed. IIRC the choice to force enable this is that most arm hardware is headless and this was a _required_ option for booting. I'm not sure if that's still the case as it's been a long time. Can anyone from the ARM community provide an approval here? P.
From: Prarit Bhargava <prarit@redhat.com> On 5/29/24 21:53, Liu Wei wrote: > > Consistency with x86 and loongarch, don't enable ACPI SPCR console > > by default on arm64 > > > > Signed-off-by: Liu Wei <liuwei09@cestc.cn> > > --- > > arch/arm64/kernel/acpi.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > > index dba8fcec7f33..1deda3e5a0d2 100644 > > --- a/arch/arm64/kernel/acpi.c > > +++ b/arch/arm64/kernel/acpi.c > > @@ -227,7 +227,8 @@ void __init acpi_boot_table_init(void) > > if (earlycon_acpi_spcr_enable) > > early_init_dt_scan_chosen_stdout(); > > } else { > > - acpi_parse_spcr(earlycon_acpi_spcr_enable, true); > > + /* Do not enable ACPI SPCR console by default */ > > + acpi_parse_spcr(earlycon_acpi_spcr_enable, false); > > if (IS_ENABLED(CONFIG_ACPI_BGRT)) > > acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt); > > } > > It's been a while, and the status of arm hardware may have changed. > IIRC the choice to force enable this is that most arm hardware is > headless and this was a _required_ option for booting. > > I'm not sure if that's still the case as it's been a long time. > > Can anyone from the ARM community provide an approval here? > > P. Though most ARM devices are headless, with the increasing number of ARM servers or desktops, if the SPCR table from ACPI is still used as the default console on ARM, the console behavior on ARM architecture will differ from other architecture servers. A similar effect can be achieved by adding console=tty0 in kernel cmdline, but I think it is important to ensure consistency among different architecture strategies as much as possible. Thanks for your patient explanation.
On Thu, May 30, 2024 at 09:06:17AM -0400, Prarit Bhargava wrote: > On 5/29/24 21:53, Liu Wei wrote: > > Consistency with x86 and loongarch, don't enable ACPI SPCR console > > by default on arm64 > > > > Signed-off-by: Liu Wei <liuwei09@cestc.cn> > > --- > > arch/arm64/kernel/acpi.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > > index dba8fcec7f33..1deda3e5a0d2 100644 > > --- a/arch/arm64/kernel/acpi.c > > +++ b/arch/arm64/kernel/acpi.c > > @@ -227,7 +227,8 @@ void __init acpi_boot_table_init(void) > > if (earlycon_acpi_spcr_enable) > > early_init_dt_scan_chosen_stdout(); > > } else { > > - acpi_parse_spcr(earlycon_acpi_spcr_enable, true); > > + /* Do not enable ACPI SPCR console by default */ > > + acpi_parse_spcr(earlycon_acpi_spcr_enable, false); > > if (IS_ENABLED(CONFIG_ACPI_BGRT)) > > acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt); > > } > > It's been a while, and the status of arm hardware may have changed. IIRC the > choice to force enable this is that most arm hardware is headless and this > was a _required_ option for booting. > > I'm not sure if that's still the case as it's been a long time. > > Can anyone from the ARM community provide an approval here? I don't have a strong opinion either way, but adding the Arm ACPI folks in case they care. Having said that, if the only rationale for this patch is consistency with other architectures, then I think I'd lean towards leaving the behaviour as-is so we don't give users a nasty surprise on their next kernel upgrade. Will
On Tue, Jun 18, 2024 at 04:29:24PM +0100, Will Deacon wrote: > On Thu, May 30, 2024 at 09:06:17AM -0400, Prarit Bhargava wrote: > > On 5/29/24 21:53, Liu Wei wrote: > > > Consistency with x86 and loongarch, don't enable ACPI SPCR console > > > by default on arm64 > > > > > > Signed-off-by: Liu Wei <liuwei09@cestc.cn> > > > --- > > > arch/arm64/kernel/acpi.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > > > index dba8fcec7f33..1deda3e5a0d2 100644 > > > --- a/arch/arm64/kernel/acpi.c > > > +++ b/arch/arm64/kernel/acpi.c > > > @@ -227,7 +227,8 @@ void __init acpi_boot_table_init(void) > > > if (earlycon_acpi_spcr_enable) > > > early_init_dt_scan_chosen_stdout(); > > > } else { > > > - acpi_parse_spcr(earlycon_acpi_spcr_enable, true); > > > + /* Do not enable ACPI SPCR console by default */ > > > + acpi_parse_spcr(earlycon_acpi_spcr_enable, false); > > > if (IS_ENABLED(CONFIG_ACPI_BGRT)) > > > acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt); > > > } > > > > It's been a while, and the status of arm hardware may have changed. IIRC the > > choice to force enable this is that most arm hardware is headless and this > > was a _required_ option for booting. > > > > I'm not sure if that's still the case as it's been a long time. > > > > Can anyone from the ARM community provide an approval here? > > I don't have a strong opinion either way, but adding the Arm ACPI folks > in case they care. > > Having said that, if the only rationale for this patch is consistency > with other architectures, then I think I'd lean towards leaving the > behaviour as-is so we don't give users a nasty surprise on their next > kernel upgrade. > +1, I am concerned about breaking existing behaviour on the platforms in the wild. Also many platforms running ACPI would have already used console cmdline parameter if SPCR is not their choice for the console. So I don't see the need to align with other arch default behaviour here, we can revisit if someone shouts with a real reason as why cmdline option is not viable. -- Regards, Sudeep
On 6/18/24 23:43, Liu Wei wrote: > From: Sudeep Holla <sudeep.holla@arm.com> > > On Tue, Jun 18, 2024 at 04:29:24PM +0100, Will Deacon wrote: >>> On Thu, May 30, 2024 at 09:06:17AM -0400, Prarit Bhargava wrote: >>>> On 5/29/24 21:53, Liu Wei wrote: >>>>> Consistency with x86 and loongarch, don't enable ACPI SPCR console >>>>> by default on arm64 >>>>> >>>>> Signed-off-by: Liu Wei <liuwei09@cestc.cn> >>>>> --- >>>>> arch/arm64/kernel/acpi.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >>>>> index dba8fcec7f33..1deda3e5a0d2 100644 >>>>> --- a/arch/arm64/kernel/acpi.c >>>>> +++ b/arch/arm64/kernel/acpi.c >>>>> @@ -227,7 +227,8 @@ void __init acpi_boot_table_init(void) >>>>> if (earlycon_acpi_spcr_enable) >>>>> early_init_dt_scan_chosen_stdout(); >>>>> } else { >>>>> - acpi_parse_spcr(earlycon_acpi_spcr_enable, true); >>>>> + /* Do not enable ACPI SPCR console by default */ >>>>> + acpi_parse_spcr(earlycon_acpi_spcr_enable, false); >>>>> if (IS_ENABLED(CONFIG_ACPI_BGRT)) >>>>> acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt); >>>>> } >>>> >>>> It's been a while, and the status of arm hardware may have changed. IIRC the >>>> choice to force enable this is that most arm hardware is headless and this >>>> was a _required_ option for booting. >>>> >>>> I'm not sure if that's still the case as it's been a long time. >>>> >>>> Can anyone from the ARM community provide an approval here? >>> >>> I don't have a strong opinion either way, but adding the Arm ACPI folks >>> in case they care. >>> >>> Having said that, if the only rationale for this patch is consistency >>> with other architectures, then I think I'd lean towards leaving the >>> behaviour as-is so we don't give users a nasty surprise on their next >>> kernel upgrade. >>> >> >> +1, I am concerned about breaking existing behaviour on the platforms >> in the wild. Also many platforms running ACPI would have already used >> console cmdline parameter if SPCR is not their choice for the console. >> So I don't see the need to align with other arch default behaviour here, >> we can revisit if someone shouts with a real reason as why cmdline option >> is not viable. > > For varying privacy and security reasons, sometimes we would like to > completely silence the serial console output, and only enable it through > cmdline when needed. > FWIW, I understand that concern but the feedback you're receiving is that there are many existing systems that depend on this behavior. Breaking users for your concerns is not a good idea. Perhaps a solution is to add yet-another-config-option, or add a kernel parameter to disable the SPCR console on ARM? Something like "acpi=nospcr"? P. > On ARM, it is difficult because SPCR is enabled by default. > > Thanks for your patience, > Liu Wei > >> >> -- >> Regards, >> Sudeep > > > >
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index dba8fcec7f33..1deda3e5a0d2 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -227,7 +227,8 @@ void __init acpi_boot_table_init(void) if (earlycon_acpi_spcr_enable) early_init_dt_scan_chosen_stdout(); } else { - acpi_parse_spcr(earlycon_acpi_spcr_enable, true); + /* Do not enable ACPI SPCR console by default */ + acpi_parse_spcr(earlycon_acpi_spcr_enable, false); if (IS_ENABLED(CONFIG_ACPI_BGRT)) acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt); }
Consistency with x86 and loongarch, don't enable ACPI SPCR console by default on arm64 Signed-off-by: Liu Wei <liuwei09@cestc.cn> --- arch/arm64/kernel/acpi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)