Re: [PATCH v4] console: Add console=spcr option

[Petr Mladek]

On Thu 2018-08-30 08:38:49, 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=spcr' that enables a firmware or hardware console, and on
> x86 enable the SPCR console if 'console=spcr' is specified.

> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 3b20607d581b..a43a34734f02 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -1771,3 +1771,17 @@ 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();
>  }
> +
> +int __init arch_console_setup(char *str)
> +{
> +	int ret;
> +
> +	if (strcmp("spcr", str))
> +		return 1;
> +
> +	ret = acpi_parse_spcr(false, true);
> +	if (ret)
> +		pr_err(PREFIX "ERROR: SPCR console is not enabled (%d)\n", ret);
> +
> +	return 0;
> +}
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 924e37fb1620..ceee021a37ec 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2107,6 +2112,9 @@ static int __init console_setup(char *str)
>  	char *s, *options, *brl_options = NULL;
>  	int idx;
>  
> +	if (!arch_console_setup(str))
> +		return 1;
> +
>  	if (_braille_console_setup(&str, &brl_options))
>  		return 1;

Sigh, I am still a bit confused by the error handling. Especially
I am not sure why console_setup() always returns 1.

It looks like it means an error when called from do_early_param().
But it means that the option was proceed when called from
obsolete_checksetup(). Do I get this correctly, please?

If this is true, we should change the logic in arch_console_setup().
It should return 1 when "spcr" is handled and it should return 0
otherwise. Also it should be commented.

It looks like a historic mess. This is why I do not ask you to do
any bigger clean up. But the new patch should not make it even
more confusing by an inverted 0/1 logic.

Best Regards,
Petr