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);
 	}
-- 
2.42.1

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.

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
> 
> 
> 
>

For varying privacy and security reasons, sometimes we would like to
completely silence the _serial_ console, and only enable it when needed.

But there are many existing systems that depend on this _serial_ console,
so add acpi=nospcr to disable console in ACPI SPCR table as default
_serial_ console.

Signed-off-by: Liu Wei <liuwei09@cestc.cn>
Suggested-by: Prarit Bhargava <prarit@redhat.com>
Suggested-by: Will Deacon <will@kernel.org>
Suggested-by: Andrew Lunn <andrew@lunn.ch>
---

v2: Add a config option suggested by Prarit

v3: Use cmdline acpi=nospcr instead of config

v4: Some description in comment or document
---
 .../admin-guide/kernel-parameters.txt          | 10 +++++++---
 arch/arm64/kernel/acpi.c                       | 18 +++++++++++++++++-
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 11e57ba2985c..6814ff7ae446 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -12,7 +12,7 @@
 	acpi=		[HW,ACPI,X86,ARM64,RISCV64,EARLY]
 			Advanced Configuration and Power Interface
 			Format: { force | on | off | strict | noirq | rsdt |
-				  copy_dsdt }
+				  copy_dsdt | nospcr }
 			force -- enable ACPI if default was off
 			on -- enable ACPI but allow fallback to DT [arm64,riscv64]
 			off -- disable ACPI if default was on
@@ -21,8 +21,12 @@
 				strictly ACPI specification compliant.
 			rsdt -- prefer RSDT over (default) XSDT
 			copy_dsdt -- copy DSDT to memory
-			For ARM64 and RISCV64, ONLY "acpi=off", "acpi=on" or
-			"acpi=force" are available
+			nospcr -- disable console in ACPI SPCR table as
+				default _serial_ console on ARM64
+			For ARM64, ONLY "acpi=off", "acpi=on", "acpi=force" or
+			"acpi=nospcr" are available
+			For RISCV64, ONLY "acpi=off", "acpi=on" or "acpi=force"
+			are available
 
 			See also Documentation/power/runtime_pm.rst, pci=noacpi
 
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index e0e7b93c16cc..55757d8884d4 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -45,6 +45,7 @@ EXPORT_SYMBOL(acpi_pci_disabled);
 static bool param_acpi_off __initdata;
 static bool param_acpi_on __initdata;
 static bool param_acpi_force __initdata;
+static bool param_acpi_nospcr __initdata;
 
 static int __init parse_acpi(char *arg)
 {
@@ -58,6 +59,8 @@ static int __init parse_acpi(char *arg)
 		param_acpi_on = true;
 	else if (strcmp(arg, "force") == 0) /* force ACPI to be enabled */
 		param_acpi_force = true;
+	else if (strcmp(arg, "nospcr") == 0) /* disable SPCR as default console */
+		param_acpi_nospcr = true;
 	else
 		return -EINVAL;	/* Core will print when we return error */
 
@@ -237,7 +240,20 @@ void __init acpi_boot_table_init(void)
 			acpi_put_table(facs);
 		}
 #endif
-		acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
+
+		/*
+		 * For varying privacy and security reasons, sometimes need
+		 * to completely silence the serial console output, and only
+		 * enable it when needed.
+		 * But there are many existing systems that depend on this
+		 * behavior, use acpi=nospcr to disable console in ACPI SPCR
+		 * table as default serial console.
+		 */
+		acpi_parse_spcr(earlycon_acpi_spcr_enable,
+			!param_acpi_nospcr);
+		pr_info("Use ACPI SPCR as default console: %s\n",
+				param_acpi_nospcr ? "No" : "Yes");
+
 		if (IS_ENABLED(CONFIG_ACPI_BGRT))
 			acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
 	}
-- 
2.42.1

On Tue, 25 Jun 2024 11:05:04 +0800, Liu Wei wrote:
> For varying privacy and security reasons, sometimes we would like to
> completely silence the _serial_ console, and only enable it when needed.
> 
> But there are many existing systems that depend on this _serial_ console,
> so add acpi=nospcr to disable console in ACPI SPCR table as default
> _serial_ console.
> 
> [...]

Applied to arm64 (for-next/acpi), thanks!

[1/1] ACPI: Add acpi=nospcr to disable ACPI SPCR as default console on ARM64
      https://git.kernel.org/arm64/c/f5a4af3c7527

-- 
Catalin

On 2024/6/25 11:05, Liu Wei wrote:
> For varying privacy and security reasons, sometimes we would like to
> completely silence the _serial_ console, and only enable it when needed.
> 
> But there are many existing systems that depend on this _serial_ console,
> so add acpi=nospcr to disable console in ACPI SPCR table as default
> _serial_ console.

I think this is reasonable, with ACPI SPCR on in default is compatible
with old systems.

> 
> Signed-off-by: Liu Wei <liuwei09@cestc.cn>
> Suggested-by: Prarit Bhargava <prarit@redhat.com>
> Suggested-by: Will Deacon <will@kernel.org>
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> ---
> 
> v2: Add a config option suggested by Prarit
> 
> v3: Use cmdline acpi=nospcr instead of config
> 
> v4: Some description in comment or document
> ---
>   .../admin-guide/kernel-parameters.txt          | 10 +++++++---
>   arch/arm64/kernel/acpi.c                       | 18 +++++++++++++++++-
>   2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 11e57ba2985c..6814ff7ae446 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -12,7 +12,7 @@
>   	acpi=		[HW,ACPI,X86,ARM64,RISCV64,EARLY]
>   			Advanced Configuration and Power Interface
>   			Format: { force | on | off | strict | noirq | rsdt |
> -				  copy_dsdt }
> +				  copy_dsdt | nospcr }
>   			force -- enable ACPI if default was off
>   			on -- enable ACPI but allow fallback to DT [arm64,riscv64]
>   			off -- disable ACPI if default was on
> @@ -21,8 +21,12 @@
>   				strictly ACPI specification compliant.
>   			rsdt -- prefer RSDT over (default) XSDT
>   			copy_dsdt -- copy DSDT to memory
> -			For ARM64 and RISCV64, ONLY "acpi=off", "acpi=on" or
> -			"acpi=force" are available
> +			nospcr -- disable console in ACPI SPCR table as
> +				default _serial_ console on ARM64
> +			For ARM64, ONLY "acpi=off", "acpi=on", "acpi=force" or
> +			"acpi=nospcr" are available
> +			For RISCV64, ONLY "acpi=off", "acpi=on" or "acpi=force"
> +			are available
>   
>   			See also Documentation/power/runtime_pm.rst, pci=noacpi
>   
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index e0e7b93c16cc..55757d8884d4 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -45,6 +45,7 @@ EXPORT_SYMBOL(acpi_pci_disabled);
>   static bool param_acpi_off __initdata;
>   static bool param_acpi_on __initdata;
>   static bool param_acpi_force __initdata;
> +static bool param_acpi_nospcr __initdata;
>   
>   static int __init parse_acpi(char *arg)
>   {
> @@ -58,6 +59,8 @@ static int __init parse_acpi(char *arg)
>   		param_acpi_on = true;
>   	else if (strcmp(arg, "force") == 0) /* force ACPI to be enabled */
>   		param_acpi_force = true;
> +	else if (strcmp(arg, "nospcr") == 0) /* disable SPCR as default console */
> +		param_acpi_nospcr = true;
>   	else
>   		return -EINVAL;	/* Core will print when we return error */
>   
> @@ -237,7 +240,20 @@ void __init acpi_boot_table_init(void)
>   			acpi_put_table(facs);
>   		}
>   #endif
> -		acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
> +
> +		/*
> +		 * For varying privacy and security reasons, sometimes need
> +		 * to completely silence the serial console output, and only
> +		 * enable it when needed.
> +		 * But there are many existing systems that depend on this
> +		 * behavior, use acpi=nospcr to disable console in ACPI SPCR

Nit: s/behavior/behaviour

> +		 * table as default serial console.
> +		 */
> +		acpi_parse_spcr(earlycon_acpi_spcr_enable,
> +			!param_acpi_nospcr);
> +		pr_info("Use ACPI SPCR as default console: %s\n",
> +				param_acpi_nospcr ? "No" : "Yes");
> +
>   		if (IS_ENABLED(CONFIG_ACPI_BGRT))
>   			acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
>   	}

Reviewed-by: Hanjun Guo <guohanjun@huawei.com>

Thanks
Hanjun

On 6/24/24 23:05, Liu Wei wrote:
> For varying privacy and security reasons, sometimes we would like to
> completely silence the _serial_ console, and only enable it when needed.
> 
> But there are many existing systems that depend on this _serial_ console,
> so add acpi=nospcr to disable console in ACPI SPCR table as default
> _serial_ console.
> 
> Signed-off-by: Liu Wei <liuwei09@cestc.cn>
> Suggested-by: Prarit Bhargava <prarit@redhat.com>
> Suggested-by: Will Deacon <will@kernel.org>
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> ---
> 
> v2: Add a config option suggested by Prarit
> 
> v3: Use cmdline acpi=nospcr instead of config
> 
> v4: Some description in comment or document
> ---
>   .../admin-guide/kernel-parameters.txt          | 10 +++++++---
>   arch/arm64/kernel/acpi.c                       | 18 +++++++++++++++++-
>   2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 11e57ba2985c..6814ff7ae446 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -12,7 +12,7 @@
>   	acpi=		[HW,ACPI,X86,ARM64,RISCV64,EARLY]
>   			Advanced Configuration and Power Interface
>   			Format: { force | on | off | strict | noirq | rsdt |
> -				  copy_dsdt }
> +				  copy_dsdt | nospcr }
>   			force -- enable ACPI if default was off
>   			on -- enable ACPI but allow fallback to DT [arm64,riscv64]
>   			off -- disable ACPI if default was on
> @@ -21,8 +21,12 @@
>   				strictly ACPI specification compliant.
>   			rsdt -- prefer RSDT over (default) XSDT
>   			copy_dsdt -- copy DSDT to memory
> -			For ARM64 and RISCV64, ONLY "acpi=off", "acpi=on" or
> -			"acpi=force" are available
> +			nospcr -- disable console in ACPI SPCR table as
> +				default _serial_ console on ARM64
> +			For ARM64, ONLY "acpi=off", "acpi=on", "acpi=force" or
> +			"acpi=nospcr" are available
> +			For RISCV64, ONLY "acpi=off", "acpi=on" or "acpi=force"
> +			are available
>   
>   			See also Documentation/power/runtime_pm.rst, pci=noacpi
>   
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index e0e7b93c16cc..55757d8884d4 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -45,6 +45,7 @@ EXPORT_SYMBOL(acpi_pci_disabled);
>   static bool param_acpi_off __initdata;
>   static bool param_acpi_on __initdata;
>   static bool param_acpi_force __initdata;
> +static bool param_acpi_nospcr __initdata;
>   
>   static int __init parse_acpi(char *arg)
>   {
> @@ -58,6 +59,8 @@ static int __init parse_acpi(char *arg)
>   		param_acpi_on = true;
>   	else if (strcmp(arg, "force") == 0) /* force ACPI to be enabled */
>   		param_acpi_force = true;
> +	else if (strcmp(arg, "nospcr") == 0) /* disable SPCR as default console */
> +		param_acpi_nospcr = true;
>   	else
>   		return -EINVAL;	/* Core will print when we return error */
>   
> @@ -237,7 +240,20 @@ void __init acpi_boot_table_init(void)
>   			acpi_put_table(facs);
>   		}
>   #endif
> -		acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
> +
> +		/*
> +		 * For varying privacy and security reasons, sometimes need
> +		 * to completely silence the serial console output, and only
> +		 * enable it when needed.
> +		 * But there are many existing systems that depend on this
> +		 * behavior, use acpi=nospcr to disable console in ACPI SPCR
> +		 * table as default serial console.
> +		 */
> +		acpi_parse_spcr(earlycon_acpi_spcr_enable,
> +			!param_acpi_nospcr);
> +		pr_info("Use ACPI SPCR as default console: %s\n",
> +				param_acpi_nospcr ? "No" : "Yes");
> +
>   		if (IS_ENABLED(CONFIG_ACPI_BGRT))
>   			acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
>   	}

This looks good to me.  Sorry that this took so long Liu.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>

P.

From: Prarit Bhargava <prarit@redhat.com>

> On 6/24/24 23:05, Liu Wei wrote:
> > For varying privacy and security reasons, sometimes we would like to
> > completely silence the _serial_ console, and only enable it when needed.
> > 
> > But there are many existing systems that depend on this _serial_ console,
> > so add acpi=nospcr to disable console in ACPI SPCR table as default
> > _serial_ console.
> > 
> > Signed-off-by: Liu Wei <liuwei09@cestc.cn>
> > Suggested-by: Prarit Bhargava <prarit@redhat.com>
> > Suggested-by: Will Deacon <will@kernel.org>
> > Suggested-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> > 
> > v2: Add a config option suggested by Prarit
> > 
> > v3: Use cmdline acpi=nospcr instead of config
> > 
> > v4: Some description in comment or document
> > ---
> >   .../admin-guide/kernel-parameters.txt          | 10 +++++++---
> >   arch/arm64/kernel/acpi.c                       | 18 +++++++++++++++++-
> >   2 files changed, 24 insertions(+), 4 deletions(-)
> > 
> >  ... 
> > 
> 
> This looks good to me.  Sorry that this took so long Liu.
> 
> Reviewed-by: Prarit Bhargava <prarit@redhat.com>
> 

Thanks, my pleasure.

Liu Wei
 
> 
> P.

For varying privacy and security reasons, sometimes we would like to
completely silence the serial console, and only enable it when needed.

But there are many existing systems that depend on this console,
so add acpi=nospcr for this situation.

Signed-off-by: Liu Wei <liuwei09@cestc.cn>
Suggested-by: Prarit Bhargava <prarit@redhat.com>
---

v2: Add a config option suggested by Prarit

v3: Use cmdline acpi=nospcr instead of config
---
 Documentation/admin-guide/kernel-parameters.txt |  9 ++++++---
 arch/arm64/kernel/acpi.c                        | 17 ++++++++++++++++-
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 11e57ba2985c..4c331cfb28f5 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -12,7 +12,7 @@
 	acpi=		[HW,ACPI,X86,ARM64,RISCV64,EARLY]
 			Advanced Configuration and Power Interface
 			Format: { force | on | off | strict | noirq | rsdt |
-				  copy_dsdt }
+				  copy_dsdt | nospcr }
 			force -- enable ACPI if default was off
 			on -- enable ACPI but allow fallback to DT [arm64,riscv64]
 			off -- disable ACPI if default was on
@@ -21,8 +21,11 @@
 				strictly ACPI specification compliant.
 			rsdt -- prefer RSDT over (default) XSDT
 			copy_dsdt -- copy DSDT to memory
-			For ARM64 and RISCV64, ONLY "acpi=off", "acpi=on" or
-			"acpi=force" are available
+			nospcr -- disable ACPI SPCR as default console on ARM64
+			For ARM64, ONLY "acpi=off", "acpi=on", "acpi=force" or
+			"acpi=nospcr" are available
+			For RISCV64, ONLY "acpi=off", "acpi=on" or "acpi=force"
+			are available
 
 			See also Documentation/power/runtime_pm.rst, pci=noacpi
 
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index e0e7b93c16cc..35cb12f3b9bd 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -45,6 +45,7 @@ EXPORT_SYMBOL(acpi_pci_disabled);
 static bool param_acpi_off __initdata;
 static bool param_acpi_on __initdata;
 static bool param_acpi_force __initdata;
+static bool param_acpi_nospcr __initdata;
 
 static int __init parse_acpi(char *arg)
 {
@@ -58,6 +59,8 @@ static int __init parse_acpi(char *arg)
 		param_acpi_on = true;
 	else if (strcmp(arg, "force") == 0) /* force ACPI to be enabled */
 		param_acpi_force = true;
+	else if (strcmp(arg, "nospcr") == 0) /* disable ACPI SPCR as default console */
+		param_acpi_nospcr = true;
 	else
 		return -EINVAL;	/* Core will print when we return error */
 
@@ -237,7 +240,19 @@ void __init acpi_boot_table_init(void)
 			acpi_put_table(facs);
 		}
 #endif
-		acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
+
+		/*
+		 * For varying privacy and security reasons, sometimes need
+		 * to completely silence the serial console output, and only
+		 * enable it when needed.
+		 * But there are many existing systems that depend on this
+		 * behavior, use acpi=nospcr for this.
+		 */
+		acpi_parse_spcr(earlycon_acpi_spcr_enable,
+			!param_acpi_nospcr);
+		pr_info("Use ACPI SPCR as default console: %s\n",
+				param_acpi_nospcr ? "No" : "Yes");
+
 		if (IS_ENABLED(CONFIG_ACPI_BGRT))
 			acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
 	}
-- 
2.42.1

On Sat, Jun 22, 2024 at 05:35:21PM +0800, Liu Wei wrote:
> For varying privacy and security reasons, sometimes we would like to
> completely silence the serial console, and only enable it when needed.
> 
> But there are many existing systems that depend on this console,
> so add acpi=nospcr for this situation.

Maybe it is just me, but i see nospcr and my brain expands it to "no
speaker". Adding to that, your commit message says "completely
silence"...

> +			nospcr -- disable ACPI SPCR as default console on ARM64
> +			For ARM64, ONLY "acpi=off", "acpi=on", "acpi=force" or
> +			"acpi=nospcr" are available
> +			For RISCV64, ONLY "acpi=off", "acpi=on" or "acpi=force"
> +			are available

How about putting the word 'serial' in here somewhere, just to give
users an additional clue you are not talking about a speaker, CTRL-G
etc.

	Andrew

From: Andrew Lunn <andrew@lunn.ch>

> On Sat, Jun 22, 2024 at 05:35:21PM +0800, Liu Wei wrote:
> > For varying privacy and security reasons, sometimes we would like to
> > completely silence the serial console, and only enable it when needed.
> > 
> > But there are many existing systems that depend on this console,
> > so add acpi=nospcr for this situation.
> 
> Maybe it is just me, but i see nospcr and my brain expands it to "no
> speaker". Adding to that, your commit message says "completely
> silence"...
> 
> > +			nospcr -- disable ACPI SPCR as default console on ARM64
> > +			For ARM64, ONLY "acpi=off", "acpi=on", "acpi=force" or
> > +			"acpi=nospcr" are available
> > +			For RISCV64, ONLY "acpi=off", "acpi=on" or "acpi=force"
> > +			are available
> 
> How about putting the word 'serial' in here somewhere, just to give
> users an additional clue you are not talking about a speaker, CTRL-G
> etc.

Thank you for your suggestion. 

You mean acpi=nospcr_serial or acpi=no_spcrserial? However, it appears 
somewhat unconventional compared to the original acpi=* parameter.

How about introducing a new one, such as acpi_no_spcr_serial or 
acpi_no_spcr_console?

Best wishes,
Liu Wei

> 	Andrew

On Mon, Jun 24, 2024 at 01:04:04PM +0800, Liu Wei wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> > On Sat, Jun 22, 2024 at 05:35:21PM +0800, Liu Wei wrote:
> > > For varying privacy and security reasons, sometimes we would like to
> > > completely silence the serial console, and only enable it when needed.
> > > 
> > > But there are many existing systems that depend on this console,
> > > so add acpi=nospcr for this situation.
> > 
> > Maybe it is just me, but i see nospcr and my brain expands it to "no
> > speaker". Adding to that, your commit message says "completely
> > silence"...
> > 
> > > +			nospcr -- disable ACPI SPCR as default console on ARM64
> > > +			For ARM64, ONLY "acpi=off", "acpi=on", "acpi=force" or
> > > +			"acpi=nospcr" are available
> > > +			For RISCV64, ONLY "acpi=off", "acpi=on" or "acpi=force"
> > > +			are available
> > 
> > How about putting the word 'serial' in here somewhere, just to give
> > users an additional clue you are not talking about a speaker, CTRL-G
> > etc.
> 
> Thank you for your suggestion. 
> 
> You mean acpi=nospcr_serial or acpi=no_spcrserial? However, it appears 
> somewhat unconventional compared to the original acpi=* parameter.

How about:

nospcr -- disable ACPI SPCR as default _serial_ console on ARM64

Please as Will suggested, add a definition somewhere.

       Andrew

From: Andrew Lunn <andrew@lunn.ch>

> On Mon, Jun 24, 2024 at 01:04:04PM +0800, Liu Wei wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > 
> > > On Sat, Jun 22, 2024 at 05:35:21PM +0800, Liu Wei wrote:
> > > > For varying privacy and security reasons, sometimes we would like to
> > > > completely silence the serial console, and only enable it when needed.
> > > > 
> > > > But there are many existing systems that depend on this console,
> > > > so add acpi=nospcr for this situation.
> > > 
> > > Maybe it is just me, but i see nospcr and my brain expands it to "no
> > > speaker". Adding to that, your commit message says "completely
> > > silence"...
> > > 
> > > > +			nospcr -- disable ACPI SPCR as default console on ARM64
> > > > +			For ARM64, ONLY "acpi=off", "acpi=on", "acpi=force" or
> > > > +			"acpi=nospcr" are available
> > > > +			For RISCV64, ONLY "acpi=off", "acpi=on" or "acpi=force"
> > > > +			are available
> > > 
> > > How about putting the word 'serial' in here somewhere, just to give
> > > users an additional clue you are not talking about a speaker, CTRL-G
> > > etc.
> > 
> > Thank you for your suggestion. 
> > 
> > You mean acpi=nospcr_serial or acpi=no_spcrserial? However, it appears 
> > somewhat unconventional compared to the original acpi=* parameter.
> 
> How about:
> 
> nospcr -- disable ACPI SPCR as default _serial_ console on ARM64
> 
> Please as Will suggested, add a definition somewhere.
>

Ok, I will send the v4 version later.

Thanks for all suggestion.

Liu Wei
 
>        Andrew

On Mon, Jun 24, 2024 at 01:04:04PM +0800, Liu Wei wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> > On Sat, Jun 22, 2024 at 05:35:21PM +0800, Liu Wei wrote:
> > > For varying privacy and security reasons, sometimes we would like to
> > > completely silence the serial console, and only enable it when needed.
> > > 
> > > But there are many existing systems that depend on this console,
> > > so add acpi=nospcr for this situation.
> > 
> > Maybe it is just me, but i see nospcr and my brain expands it to "no
> > speaker". Adding to that, your commit message says "completely
> > silence"...
> > 
> > > +			nospcr -- disable ACPI SPCR as default console on ARM64
> > > +			For ARM64, ONLY "acpi=off", "acpi=on", "acpi=force" or
> > > +			"acpi=nospcr" are available
> > > +			For RISCV64, ONLY "acpi=off", "acpi=on" or "acpi=force"
> > > +			are available
> > 
> > How about putting the word 'serial' in here somewhere, just to give
> > users an additional clue you are not talking about a speaker, CTRL-G
> > etc.
> 
> Thank you for your suggestion. 
> 
> You mean acpi=nospcr_serial or acpi=no_spcrserial? However, it appears 
> somewhat unconventional compared to the original acpi=* parameter.
> 
> How about introducing a new one, such as acpi_no_spcr_serial or 
> acpi_no_spcr_console?

I think acpi=nospcr is fine like it is. Just expand the acronym in the
documentation so that it's obviously not talking about a speaker.

Will

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.

But there are many existing systems that depend on this console,
so add CONFIG_ARM_DISABLE_ACPI_SPCR_CONSOLE for this situation.

Signed-off-by: Liu Wei <liuwei09@cestc.cn>
Suggested-by: Prarit Bhargava <prarit@redhat.com>
---

v2: Add a config option suggested by Prarit
---
 arch/arm64/kernel/acpi.c   | 12 ++++++++++++
 drivers/acpi/arm64/Kconfig | 11 +++++++++++
 2 files changed, 23 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index dba8fcec7f33..3365fabb5cf8 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -227,7 +227,19 @@ void __init acpi_boot_table_init(void)
 		if (earlycon_acpi_spcr_enable)
 			early_init_dt_scan_chosen_stdout();
 	} else {
+		/*
+		 * For varying privacy and security reasons, sometimes need
+		 * to completely silence the serial console output, and only 
+		 * enable it by cmdline when needed.
+		 * But there are many existing systems that depend on this
+		 * behavior, so use CONFIG_ARM_DISABLE_ACPI_SPCR_CONSOLE.
+		 */
+#ifdef CONFIG_ARM_DISABLE_ACPI_SPCR_CONSOLE
+		acpi_parse_spcr(earlycon_acpi_spcr_enable, false);
+#else
 		acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
+#endif
+
 		if (IS_ENABLED(CONFIG_ACPI_BGRT))
 			acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
 	}
diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
index b3ed6212244c..7e4d860d7089 100644
--- a/drivers/acpi/arm64/Kconfig
+++ b/drivers/acpi/arm64/Kconfig
@@ -21,3 +21,14 @@ config ACPI_AGDI
 
 config ACPI_APMT
 	bool
+
+config ARM_DISABLE_ACPI_SPCR_CONSOLE
+	bool "Disable ACPI SPCR Console by Default on Arm64"
+	depends on ARM64 && ACPI_SPCR_TABLE
+	default n
+	help
+		For varying privacy and security reasons, sometimes need to
+		completely silence the serial console output, and only enable
+		it by kernel cmdline when needed.
+
+		Say Y to disable ACPI SPCR console by default.
-- 
2.42.1

On 6/21/24 00:47, Liu Wei wrote:
> 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.
> 
> But there are many existing systems that depend on this console,
> so add CONFIG_ARM_DISABLE_ACPI_SPCR_CONSOLE for this situation.
> 
> Signed-off-by: Liu Wei <liuwei09@cestc.cn>
> Suggested-by: Prarit Bhargava <prarit@redhat.com>
> ---
> 
> v2: Add a config option suggested by Prarit
> ---
>   arch/arm64/kernel/acpi.c   | 12 ++++++++++++
>   drivers/acpi/arm64/Kconfig | 11 +++++++++++
>   2 files changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index dba8fcec7f33..3365fabb5cf8 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -227,7 +227,19 @@ void __init acpi_boot_table_init(void)
>   		if (earlycon_acpi_spcr_enable)
>   			early_init_dt_scan_chosen_stdout();
>   	} else {
> +		/*
> +		 * For varying privacy and security reasons, sometimes need
> +		 * to completely silence the serial console output, and only
> +		 * enable it by cmdline when needed.
> +		 * But there are many existing systems that depend on this
> +		 * behavior, so use CONFIG_ARM_DISABLE_ACPI_SPCR_CONSOLE.
> +		 */
> +#ifdef CONFIG_ARM_DISABLE_ACPI_SPCR_CONSOLE
> +		acpi_parse_spcr(earlycon_acpi_spcr_enable, false);
> +#else
>   		acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
> +#endif
> +

I don't think you want a config option here after all.  See my previous 
comment about "acpi=nospcr".  I realized that if you do use a config 
then distros will not have the ability to default 'on', and advise users 
to disable it for their use cases.

Try the 'acpi=nospcr' option.  That should keep everyone happy.

P.

>   		if (IS_ENABLED(CONFIG_ACPI_BGRT))
>   			acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
>   	}
> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
> index b3ed6212244c..7e4d860d7089 100644
> --- a/drivers/acpi/arm64/Kconfig
> +++ b/drivers/acpi/arm64/Kconfig
> @@ -21,3 +21,14 @@ config ACPI_AGDI
>   
>   config ACPI_APMT
>   	bool
> +
> +config ARM_DISABLE_ACPI_SPCR_CONSOLE
> +	bool "Disable ACPI SPCR Console by Default on Arm64"
> +	depends on ARM64 && ACPI_SPCR_TABLE
> +	default n
> +	help
> +		For varying privacy and security reasons, sometimes need to
> +		completely silence the serial console output, and only enable
> +		it by kernel cmdline when needed.
> +
> +		Say Y to disable ACPI SPCR console by default.