init: Don't proxy console= to earlycon

  Right now we are proxying the `console=XXX` command line args to the
param_setup_earlycon. This is done because the following are
equivalent:

    console=uart[8250],mmio,<addr>[,options]
    earlycon=uart[8250],mmio,<addr>[,options]

In addition, when `earlycon=` or just `earlycon` is specified on the
command line, we look at the SPCR table or the DT to extract the device
options.

When `console=` is specified on the command line, it's intention is to
disable the console. Right now since we are proxying the `console=`
flag to the earlycon handler, we enable the earlycon_acpi_spcr_enable
variable when an SPCR table is present. This means that we
inadvertently enable the earlycon.

This change makes it so we only proxy the console= command if it's
value is not empty. This way we can correctly handle both cases.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 init/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
  

On Fri,  7 Jul 2023 19:17:25 -0600 Raul E Rangel <rrangel@chromium.org> wrote:

> Right now we are proxying the `console=XXX` command line args to the
> param_setup_earlycon. This is done because the following are
> equivalent:
> 
>     console=uart[8250],mmio,<addr>[,options]
>     earlycon=uart[8250],mmio,<addr>[,options]
> 
> In addition, when `earlycon=` or just `earlycon` is specified on the
> command line, we look at the SPCR table or the DT to extract the device
> options.
> 
> When `console=` is specified on the command line, it's intention is to
> disable the console. Right now since we are proxying the `console=`
> flag to the earlycon handler, we enable the earlycon_acpi_spcr_enable
> variable when an SPCR table is present. This means that we
> inadvertently enable the earlycon.
> 
> This change makes it so we only proxy the console= command if it's
> value is not empty. This way we can correctly handle both cases.
> 

I hope someone understands this ;)

Please "grep -r earlycon Documentation" and check for suitable places
to update our documentation.
  

On 7/7/23 18:17, Raul E Rangel wrote:
> Right now we are proxying the `console=XXX` command line args to the
> param_setup_earlycon. This is done because the following are
> equivalent:
> 
>     console=uart[8250],mmio,<addr>[,options]
>     earlycon=uart[8250],mmio,<addr>[,options]
> 
> In addition, when `earlycon=` or just `earlycon` is specified on the
> command line, we look at the SPCR table or the DT to extract the device
> options.
> 
> When `console=` is specified on the command line, it's intention is to
> disable the console. Right now since we are proxying the `console=`

How do you figure this (its intention is to disable the console)?

> flag to the earlycon handler, we enable the earlycon_acpi_spcr_enable
> variable when an SPCR table is present. This means that we
> inadvertently enable the earlycon.
> 
> This change makes it so we only proxy the console= command if it's
> value is not empty. This way we can correctly handle both cases.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> 
>  init/main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index aa21add5f7c54..f72bf644910c1 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -738,8 +738,7 @@ static int __init do_early_param(char *param, char *val,
>  	for (p = __setup_start; p < __setup_end; p++) {
>  		if ((p->early && parameq(param, p->str)) ||
>  		    (strcmp(param, "console") == 0 &&
> -		     strcmp(p->str, "earlycon") == 0)
> -		) {
> +		     strcmp(p->str, "earlycon") == 0 && val && val[0])) {
>  			if (p->setup_func(val) != 0)
>  				pr_warn("Malformed early option '%s'\n", param);
>  		}
  

On 7/9/23 18:46, Randy Dunlap wrote:
> 
> 
> On 7/7/23 18:17, Raul E Rangel wrote:
>> Right now we are proxying the `console=XXX` command line args to the
>> param_setup_earlycon. This is done because the following are
>> equivalent:
>>
>>      console=uart[8250],mmio,<addr>[,options]
>>      earlycon=uart[8250],mmio,<addr>[,options]
>>
>> In addition, when `earlycon=` or just `earlycon` is specified on the
>> command line, we look at the SPCR table or the DT to extract the device
>> options.
>>
>> When `console=` is specified on the command line, it's intention is to
>> disable the console. Right now since we are proxying the `console=`
> 
> How do you figure this (its intention is to disable the console)?

I read that as "it's intention is to disable the default console (tty0)".

IE if I add console=ttyS0,115200,n8 to my kernel command line then I 
don't get the output on tty0 anymore.  If I want it on both then I do

console=ttyS0,115200,n8 console=tty0.

> 
>> flag to the earlycon handler, we enable the earlycon_acpi_spcr_enable
>> variable when an SPCR table is present. This means that we
>> inadvertently enable the earlycon.
>>
>> This change makes it so we only proxy the console= command if it's
>> value is not empty. This way we can correctly handle both cases.
>>
>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>> ---
>>
>>   init/main.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/init/main.c b/init/main.c
>> index aa21add5f7c54..f72bf644910c1 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -738,8 +738,7 @@ static int __init do_early_param(char *param, char *val,
>>   	for (p = __setup_start; p < __setup_end; p++) {
>>   		if ((p->early && parameq(param, p->str)) ||
>>   		    (strcmp(param, "console") == 0 &&
>> -		     strcmp(p->str, "earlycon") == 0)
>> -		) {
>> +		     strcmp(p->str, "earlycon") == 0 && val && val[0])) {
>>   			if (p->setup_func(val) != 0)
>>   				pr_warn("Malformed early option '%s'\n", param);
>>   		}
>
  

On 7/9/23 18:15, Mario Limonciello wrote:
> On 7/9/23 18:46, Randy Dunlap wrote:
>>
>>
>> On 7/7/23 18:17, Raul E Rangel wrote:
>>> Right now we are proxying the `console=XXX` command line args to the
>>> param_setup_earlycon. This is done because the following are
>>> equivalent:
>>>
>>>      console=uart[8250],mmio,<addr>[,options]
>>>      earlycon=uart[8250],mmio,<addr>[,options]
>>>
>>> In addition, when `earlycon=` or just `earlycon` is specified on the
>>> command line, we look at the SPCR table or the DT to extract the device
>>> options.
>>>
>>> When `console=` is specified on the command line, it's intention is to
>>> disable the console. Right now since we are proxying the `console=`
>>
>> How do you figure this (its intention is to disable the console)?
> 
> I read that as "it's intention is to disable the default console (tty0)".

Yes, that "default" word should be there IMO.

Does this patch affect behavior if someone uses
	console=tty0
i.e., the default?

> 
> IE if I add console=ttyS0,115200,n8 to my kernel command line then I don't get the output on tty0 anymore.  If I want it on both then I do
> 
> console=ttyS0,115200,n8 console=tty0.
> 
>>
>>> flag to the earlycon handler, we enable the earlycon_acpi_spcr_enable
>>> variable when an SPCR table is present. This means that we
>>> inadvertently enable the earlycon.
>>>
>>> This change makes it so we only proxy the console= command if it's
>>> value is not empty. This way we can correctly handle both cases.
>>>
>>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>>> ---
>>>
>>>   init/main.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/init/main.c b/init/main.c
>>> index aa21add5f7c54..f72bf644910c1 100644
>>> --- a/init/main.c
>>> +++ b/init/main.c
>>> @@ -738,8 +738,7 @@ static int __init do_early_param(char *param, char *val,
>>>       for (p = __setup_start; p < __setup_end; p++) {
>>>           if ((p->early && parameq(param, p->str)) ||
>>>               (strcmp(param, "console") == 0 &&
>>> -             strcmp(p->str, "earlycon") == 0)
>>> -        ) {
>>> +             strcmp(p->str, "earlycon") == 0 && val && val[0])) {
>>>               if (p->setup_func(val) != 0)
>>>                   pr_warn("Malformed early option '%s'\n", param);
>>>           }
>>
>
  

On Sun, Jul 9, 2023 at 8:43 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
>
>
> On 7/9/23 18:15, Mario Limonciello wrote:
> > On 7/9/23 18:46, Randy Dunlap wrote:
> >>
> >>
> >> On 7/7/23 18:17, Raul E Rangel wrote:
> >>> Right now we are proxying the `console=XXX` command line args to the
> >>> param_setup_earlycon. This is done because the following are
> >>> equivalent:
> >>>
> >>>      console=uart[8250],mmio,<addr>[,options]
> >>>      earlycon=uart[8250],mmio,<addr>[,options]
> >>>
> >>> In addition, when `earlycon=` or just `earlycon` is specified on the
> >>> command line, we look at the SPCR table or the DT to extract the device
> >>> options.
> >>>
> >>> When `console=` is specified on the command line, it's intention is to
> >>> disable the console. Right now since we are proxying the `console=`
> >>
> >> How do you figure this (its intention is to disable the console)?
> >

https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html
says the following:
console=
    { null | "" }
            Use to disable console output, i.e., to have kernel
            console messages discarded.
            This must be the only console= parameter used on the
            kernel command line.

        earlycon=       [KNL] Output early console device and options.

            When used with no options, the early console is
            determined by stdout-path property in device tree's
            chosen node or the ACPI SPCR table if supported by
            the platform.

The reason this bug showed up is that ChromeOS has set `console=` for a
very long time:
https://chromium.googlesource.com/chromiumos/platform/crosutils/+/main/build_kernel_image.sh#282
I'm not sure on the exact history, but AFAIK, we don't have the ttyX devices.

Coreboot recently added support for the ACPI SPCR table which in
combination with the
`console=` arg, we are now seeing earlycon enabled when it shouldn't be.

> > I read that as "it's intention is to disable the default console (tty0)".
>
> Yes, that "default" word should be there IMO.
>
> Does this patch affect behavior if someone uses
>         console=tty0
> i.e., the default?
>

No, it shouldn't. This change makes it so that the
param_setup_earlycon function gets
skipped if there is no value.
See https://chromium.googlesource.com/chromiumos/third_party/kernel/+/v6.1/drivers/tty/serial/earlycon.c#223

> >
> > IE if I add console=ttyS0,115200,n8 to my kernel command line then I don't get the output on tty0 anymore.  If I want it on both then I do
> >
> > console=ttyS0,115200,n8 console=tty0.
> >
> >>
> >>> flag to the earlycon handler, we enable the earlycon_acpi_spcr_enable
> >>> variable when an SPCR table is present. This means that we
> >>> inadvertently enable the earlycon.
> >>>
> >>> This change makes it so we only proxy the console= command if it's
> >>> value is not empty. This way we can correctly handle both cases.
> >>>
> >>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> >>> ---
> >>>
> >>>   init/main.c | 3 +--
> >>>   1 file changed, 1 insertion(+), 2 deletions(-)
> >>>
> >>> diff --git a/init/main.c b/init/main.c
> >>> index aa21add5f7c54..f72bf644910c1 100644
> >>> --- a/init/main.c
> >>> +++ b/init/main.c
> >>> @@ -738,8 +738,7 @@ static int __init do_early_param(char *param, char *val,
> >>>       for (p = __setup_start; p < __setup_end; p++) {
> >>>           if ((p->early && parameq(param, p->str)) ||
> >>>               (strcmp(param, "console") == 0 &&
> >>> -             strcmp(p->str, "earlycon") == 0)
> >>> -        ) {
> >>> +             strcmp(p->str, "earlycon") == 0 && val && val[0])) {
> >>>               if (p->setup_func(val) != 0)
> >>>                   pr_warn("Malformed early option '%s'\n", param);
> >>>           }
> >>
> >
>
> --
> ~Randy
  

Hi,

On 7/10/23 08:30, Raul Rangel wrote:
> On Sun, Jul 9, 2023 at 8:43 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>>
>>
>> On 7/9/23 18:15, Mario Limonciello wrote:
>>> On 7/9/23 18:46, Randy Dunlap wrote:
>>>>
>>>>
>>>> On 7/7/23 18:17, Raul E Rangel wrote:
>>>>> Right now we are proxying the `console=XXX` command line args to the
>>>>> param_setup_earlycon. This is done because the following are
>>>>> equivalent:
>>>>>
>>>>>      console=uart[8250],mmio,<addr>[,options]
>>>>>      earlycon=uart[8250],mmio,<addr>[,options]
>>>>>
>>>>> In addition, when `earlycon=` or just `earlycon` is specified on the
>>>>> command line, we look at the SPCR table or the DT to extract the device
>>>>> options.
>>>>>
>>>>> When `console=` is specified on the command line, it's intention is to
>>>>> disable the console. Right now since we are proxying the `console=`
>>>>
>>>> How do you figure this (its intention is to disable the console)?
>>>
> 
> https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html
> says the following:

> console=

[many lines deleted here in kernel-parameters.txt]

>     { null | "" }
>             Use to disable console output, i.e., to have kernel
>             console messages discarded.
>             This must be the only console= parameter used on the
>             kernel command line.
> 

OK, I see what you are referring to: "console=" without any following parameters,
not the general "console=" command line parameter.

Thanks for the clarification.

>         earlycon=       [KNL] Output early console device and options.
> 
>             When used with no options, the early console is
>             determined by stdout-path property in device tree's
>             chosen node or the ACPI SPCR table if supported by
>             the platform.
> 
> The reason this bug showed up is that ChromeOS has set `console=` for a
> very long time:
> https://chromium.googlesource.com/chromiumos/platform/crosutils/+/main/build_kernel_image.sh#282
> I'm not sure on the exact history, but AFAIK, we don't have the ttyX devices.
> 
> Coreboot recently added support for the ACPI SPCR table which in
> combination with the
> `console=` arg, we are now seeing earlycon enabled when it shouldn't be.
> 
>>> I read that as "it's intention is to disable the default console (tty0)".
>>
>> Yes, that "default" word should be there IMO.
>>
>> Does this patch affect behavior if someone uses
>>         console=tty0
>> i.e., the default?
>>
> 
> No, it shouldn't. This change makes it so that the
> param_setup_earlycon function gets
> skipped if there is no value.
> See https://chromium.googlesource.com/chromiumos/third_party/kernel/+/v6.1/drivers/tty/serial/earlycon.c#223
  

On Sat, Jul 8, 2023 at 6:48 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri,  7 Jul 2023 19:17:25 -0600 Raul E Rangel <rrangel@chromium.org> wrote:
>
> > Right now we are proxying the `console=XXX` command line args to the
> > param_setup_earlycon. This is done because the following are
> > equivalent:
> >
> >     console=uart[8250],mmio,<addr>[,options]
> >     earlycon=uart[8250],mmio,<addr>[,options]
> >
> > In addition, when `earlycon=` or just `earlycon` is specified on the
> > command line, we look at the SPCR table or the DT to extract the device
> > options.
> >
> > When `console=` is specified on the command line, it's intention is to
> > disable the console. Right now since we are proxying the `console=`
> > flag to the earlycon handler, we enable the earlycon_acpi_spcr_enable
> > variable when an SPCR table is present. This means that we
> > inadvertently enable the earlycon.
> >
> > This change makes it so we only proxy the console= command if it's
> > value is not empty. This way we can correctly handle both cases.
> >
>
> I hope someone understands this ;)

Is there a more specific list I should send this patch too?

>
> Please "grep -r earlycon Documentation" and check for suitable places
> to update our documentation.
>

The documentation is correct, this just makes the code match the
documentation :)
  

On Mon 2023-07-10 09:30:19, Raul Rangel wrote:
> On Sun, Jul 9, 2023 at 8:43 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> >
> >
> >
> > On 7/9/23 18:15, Mario Limonciello wrote:
> > > On 7/9/23 18:46, Randy Dunlap wrote:
> > >>
> > >>
> > >> On 7/7/23 18:17, Raul E Rangel wrote:
> > >>> Right now we are proxying the `console=XXX` command line args to the
> > >>> param_setup_earlycon. This is done because the following are
> > >>> equivalent:
> > >>>
> > >>>      console=uart[8250],mmio,<addr>[,options]
> > >>>      earlycon=uart[8250],mmio,<addr>[,options]
> > >>>
> > >>> In addition, when `earlycon=` or just `earlycon` is specified on the
> > >>> command line, we look at the SPCR table or the DT to extract the device
> > >>> options.
> > >>>
> > >>> When `console=` is specified on the command line, it's intention is to
> > >>> disable the console. Right now since we are proxying the `console=`
> > >>
> > >> How do you figure this (its intention is to disable the console)?
> > >
> 
> https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html
> says the following:
> console=
>     { null | "" }
>             Use to disable console output, i.e., to have kernel
>             console messages discarded.
>             This must be the only console= parameter used on the
>             kernel command line.
> 
>         earlycon=       [KNL] Output early console device and options.
> 
>             When used with no options, the early console is
>             determined by stdout-path property in device tree's
>             chosen node or the ACPI SPCR table if supported by
>             the platform.

Sigh, I wasn't aware of this when we discussed the console= handling.

> The reason this bug showed up is that ChromeOS has set `console=` for a
> very long time:
> https://chromium.googlesource.com/chromiumos/platform/crosutils/+/main/build_kernel_image.sh#282
> I'm not sure on the exact history, but AFAIK, we don't have the ttyX devices.
>
> Coreboot recently added support for the ACPI SPCR table which in
> combination with the
> `console=` arg, we are now seeing earlycon enabled when it shouldn't be.

But this happens only when both "earlycon" and "console=" parameters
are used together. Do I get it correctly?

This combination is ambiguous on its own. Why would anyone add
"earlycon" parameter and wanted to keep it disabled?

> > >>> diff --git a/init/main.c b/init/main.c
> > >>> index aa21add5f7c54..f72bf644910c1 100644
> > >>> --- a/init/main.c
> > >>> +++ b/init/main.c
> > >>> @@ -738,8 +738,7 @@ static int __init do_early_param(char *param, char *val,
> > >>>       for (p = __setup_start; p < __setup_end; p++) {
> > >>>           if ((p->early && parameq(param, p->str)) ||
> > >>>               (strcmp(param, "console") == 0 &&
> > >>> -             strcmp(p->str, "earlycon") == 0)
> > >>> -        ) {
> > >>> +             strcmp(p->str, "earlycon") == 0 && val && val[0])) {
> > >>>               if (p->setup_func(val) != 0)
> > >>>                   pr_warn("Malformed early option '%s'\n", param);
> > >>>           }

Huh, this is getting more and more complicated.

You know, it is already bad that:

    + "console" enables the default console which might be overridden
      by ACPI SPCR and devicetree

    + "console=" causes that "ttynull" console is preferred,
	  it might cause that no console is registered at all

    + both "earlyconsole" and "earlyconsole=" cause that
          
     consoles is enabled 
   

It is already bad that "earlycon" and "console" handle the empty value
a different way. But this makes it even worse. The behaviour
would depend on a subtle difference whether "console" or
"console=" is used.



> > >>
> > >
> >
> > --
> > ~Randy
  

On Fri, Jul 14, 2023 at 10:38 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Mon 2023-07-10 09:30:19, Raul Rangel wrote:
> > On Sun, Jul 9, 2023 at 8:43 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> > >
> > >
> > >
> > > On 7/9/23 18:15, Mario Limonciello wrote:
> > > > On 7/9/23 18:46, Randy Dunlap wrote:
> > > >>
> > > >>
> > > >> On 7/7/23 18:17, Raul E Rangel wrote:
> > > >>> Right now we are proxying the `console=XXX` command line args to the
> > > >>> param_setup_earlycon. This is done because the following are
> > > >>> equivalent:
> > > >>>
> > > >>>      console=uart[8250],mmio,<addr>[,options]
> > > >>>      earlycon=uart[8250],mmio,<addr>[,options]
> > > >>>
> > > >>> In addition, when `earlycon=` or just `earlycon` is specified on the
> > > >>> command line, we look at the SPCR table or the DT to extract the device
> > > >>> options.
> > > >>>
> > > >>> When `console=` is specified on the command line, it's intention is to
> > > >>> disable the console. Right now since we are proxying the `console=`
> > > >>
> > > >> How do you figure this (its intention is to disable the console)?
> > > >
> >
> > https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html
> > says the following:
> > console=
> >     { null | "" }
> >             Use to disable console output, i.e., to have kernel
> >             console messages discarded.
> >             This must be the only console= parameter used on the
> >             kernel command line.
> >
> >         earlycon=       [KNL] Output early console device and options.
> >
> >             When used with no options, the early console is
> >             determined by stdout-path property in device tree's
> >             chosen node or the ACPI SPCR table if supported by
> >             the platform.
>
> Sigh, I wasn't aware of this when we discussed the console= handling.

It took a bit of digging to figure out what the actual intention was :)

>
> > The reason this bug showed up is that ChromeOS has set `console=` for a
> > very long time:
> > https://chromium.googlesource.com/chromiumos/platform/crosutils/+/main/build_kernel_image.sh#282
> > I'm not sure on the exact history, but AFAIK, we don't have the ttyX devices.
> >
> > Coreboot recently added support for the ACPI SPCR table which in
> > combination with the
> > `console=` arg, we are now seeing earlycon enabled when it shouldn't be.
>
> But this happens only when both "earlycon" and "console=" parameters
> are used together. Do I get it correctly?

The bug shows up when an SPCR table is present and the `console=`
parameter is set. No need to specify `earlycon` on the command line.

>
> This combination is ambiguous on its own. Why would anyone add
> "earlycon" parameter and wanted to keep it disabled?

This is not the case I'm hitting. I'm honestly not sure what the
behavior should be in the `earlycon console=` case?

>
> > > >>> diff --git a/init/main.c b/init/main.c
> > > >>> index aa21add5f7c54..f72bf644910c1 100644
> > > >>> --- a/init/main.c
> > > >>> +++ b/init/main.c
> > > >>> @@ -738,8 +738,7 @@ static int __init do_early_param(char *param, char *val,
> > > >>>       for (p = __setup_start; p < __setup_end; p++) {
> > > >>>           if ((p->early && parameq(param, p->str)) ||
> > > >>>               (strcmp(param, "console") == 0 &&
> > > >>> -             strcmp(p->str, "earlycon") == 0)
> > > >>> -        ) {
> > > >>> +             strcmp(p->str, "earlycon") == 0 && val && val[0])) {
> > > >>>               if (p->setup_func(val) != 0)
> > > >>>                   pr_warn("Malformed early option '%s'\n", param);
> > > >>>           }
>
> Huh, this is getting more and more complicated.
>
> You know, it is already bad that:
>
>     + "console" enables the default console which might be overridden
>       by ACPI SPCR and devicetree

That's what this patch fixes. You need to specify `earlycon` in order
to get the ACPI SPCR or DT console.

>
>     + "console=" causes that "ttynull" console is preferred,
>           it might cause that no console is registered at all

This patch also makes it so `console=` no longer enables the earlycon.

>
>     + both "earlyconsole" and "earlyconsole=" cause that
>
>      consoles is enabled
>
>
> It is already bad that "earlycon" and "console" handle the empty value
> a different way. But this makes it even worse. The behaviour
> would depend on a subtle difference whether "console" or
> "console=" is used.

This patch makes it so that `console` or `console=` don't enable
earlycon. Earlycon can only be enabled via `earlycon` or `earlycon=`.

I don't see the `console` (without the =) documented:
https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html.
I'm guessing this is an undocumented "feature" that snuck in while the
`earlycon` stuff was being added.

>
>
>
> > > >>
> > > >
> > >
> > > --
> > > ~Randy

Thanks for reviewing,
Raul
  

On Fri, Jul 14, 2023 at 11:21 AM Raul Rangel <rrangel@chromium.org> wrote:
>
> On Fri, Jul 14, 2023 at 10:38 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Mon 2023-07-10 09:30:19, Raul Rangel wrote:
> > > On Sun, Jul 9, 2023 at 8:43 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> > > >
> > > >
> > > >
> > > > On 7/9/23 18:15, Mario Limonciello wrote:
> > > > > On 7/9/23 18:46, Randy Dunlap wrote:
> > > > >>
> > > > >>
> > > > >> On 7/7/23 18:17, Raul E Rangel wrote:
> > > > >>> Right now we are proxying the `console=XXX` command line args to the
> > > > >>> param_setup_earlycon. This is done because the following are
> > > > >>> equivalent:
> > > > >>>
> > > > >>>      console=uart[8250],mmio,<addr>[,options]
> > > > >>>      earlycon=uart[8250],mmio,<addr>[,options]
> > > > >>>
> > > > >>> In addition, when `earlycon=` or just `earlycon` is specified on the
> > > > >>> command line, we look at the SPCR table or the DT to extract the device
> > > > >>> options.
> > > > >>>
> > > > >>> When `console=` is specified on the command line, it's intention is to
> > > > >>> disable the console. Right now since we are proxying the `console=`
> > > > >>
> > > > >> How do you figure this (its intention is to disable the console)?
> > > > >
> > >
> > > https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html
> > > says the following:
> > > console=
> > >     { null | "" }
> > >             Use to disable console output, i.e., to have kernel
> > >             console messages discarded.
> > >             This must be the only console= parameter used on the
> > >             kernel command line.
> > >
> > >         earlycon=       [KNL] Output early console device and options.
> > >
> > >             When used with no options, the early console is
> > >             determined by stdout-path property in device tree's
> > >             chosen node or the ACPI SPCR table if supported by
> > >             the platform.
> >
> > Sigh, I wasn't aware of this when we discussed the console= handling.
>
> It took a bit of digging to figure out what the actual intention was :)
>
> >
> > > The reason this bug showed up is that ChromeOS has set `console=` for a
> > > very long time:
> > > https://chromium.googlesource.com/chromiumos/platform/crosutils/+/main/build_kernel_image.sh#282
> > > I'm not sure on the exact history, but AFAIK, we don't have the ttyX devices.
> > >
> > > Coreboot recently added support for the ACPI SPCR table which in
> > > combination with the
> > > `console=` arg, we are now seeing earlycon enabled when it shouldn't be.
> >
> > But this happens only when both "earlycon" and "console=" parameters
> > are used together. Do I get it correctly?
>
> The bug shows up when an SPCR table is present and the `console=`
> parameter is set. No need to specify `earlycon` on the command line.
>
> >
> > This combination is ambiguous on its own. Why would anyone add
> > "earlycon" parameter and wanted to keep it disabled?
>
> This is not the case I'm hitting. I'm honestly not sure what the
> behavior should be in the `earlycon console=` case?
>
> >
> > > > >>> diff --git a/init/main.c b/init/main.c
> > > > >>> index aa21add5f7c54..f72bf644910c1 100644
> > > > >>> --- a/init/main.c
> > > > >>> +++ b/init/main.c
> > > > >>> @@ -738,8 +738,7 @@ static int __init do_early_param(char *param, char *val,
> > > > >>>       for (p = __setup_start; p < __setup_end; p++) {
> > > > >>>           if ((p->early && parameq(param, p->str)) ||
> > > > >>>               (strcmp(param, "console") == 0 &&
> > > > >>> -             strcmp(p->str, "earlycon") == 0)
> > > > >>> -        ) {
> > > > >>> +             strcmp(p->str, "earlycon") == 0 && val && val[0])) {
> > > > >>>               if (p->setup_func(val) != 0)
> > > > >>>                   pr_warn("Malformed early option '%s'\n", param);
> > > > >>>           }
> >
> > Huh, this is getting more and more complicated.
> >
> > You know, it is already bad that:
> >
> >     + "console" enables the default console which might be overridden
> >       by ACPI SPCR and devicetree
>
> That's what this patch fixes. You need to specify `earlycon` in order
> to get the ACPI SPCR or DT console.
>
> >
> >     + "console=" causes that "ttynull" console is preferred,
> >           it might cause that no console is registered at all
>
> This patch also makes it so `console=` no longer enables the earlycon.
>
> >
> >     + both "earlyconsole" and "earlyconsole=" cause that
> >
> >      consoles is enabled
> >
> >
> > It is already bad that "earlycon" and "console" handle the empty value
> > a different way. But this makes it even worse. The behaviour
> > would depend on a subtle difference whether "console" or
> > "console=" is used.
>
> This patch makes it so that `console` or `console=` don't enable
> earlycon. Earlycon can only be enabled via `earlycon` or `earlycon=`.

Sorry, I misspoke. Earlycon that is configured using the SPCR/DT can
only be enabled via `earlycon` or `earlycon=`. You can still enable
`earlycon` via `earlycon=uart,mmio32,0xFFFFFFFF` or
`console=uart,mmio32,0xFFFFFFFF`.

>
> I don't see the `console` (without the =) documented:
> https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html.
> I'm guessing this is an undocumented "feature" that snuck in while the
> `earlycon` stuff was being added.
>
> >
> >
> >
> > > > >>
> > > > >
> > > >
> > > > --
> > > > ~Randy
>
> Thanks for reviewing,
> Raul
  

First, I am sorry I sent the first mail too early by mistake.
(Friday evening effect).

On Fri 2023-07-14 11:21:09, Raul Rangel wrote:
> On Fri, Jul 14, 2023 at 10:38 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Mon 2023-07-10 09:30:19, Raul Rangel wrote:
> > > On Sun, Jul 9, 2023 at 8:43 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> > > >
> > > >
> > > >
> > > > On 7/9/23 18:15, Mario Limonciello wrote:
> > > > > On 7/9/23 18:46, Randy Dunlap wrote:
> > > > >>
> > > > >>
> > > > >> On 7/7/23 18:17, Raul E Rangel wrote:
> > > > >>> Right now we are proxying the `console=XXX` command line args to the
> > > > >>> param_setup_earlycon. This is done because the following are
> > > > >>> equivalent:
> > > > >>>
> > > > >>>      console=uart[8250],mmio,<addr>[,options]
> > > > >>>      earlycon=uart[8250],mmio,<addr>[,options]
> > > > >>>
> > > > >>> In addition, when `earlycon=` or just `earlycon` is specified on the
> > > > >>> command line, we look at the SPCR table or the DT to extract the device
> > > > >>> options.
> > > > >>>
> > > > >>> When `console=` is specified on the command line, it's intention is to
> > > > >>> disable the console. Right now since we are proxying the `console=`
> > > > >>
> > > > >> How do you figure this (its intention is to disable the console)?
> > > > >
> > >
> > > https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html
> > > says the following:
> > > console=
> > >     { null | "" }
> > >             Use to disable console output, i.e., to have kernel
> > >             console messages discarded.
> > >             This must be the only console= parameter used on the
> > >             kernel command line.
> > >
> > >         earlycon=       [KNL] Output early console device and options.
> > >
> > >             When used with no options, the early console is
> > >             determined by stdout-path property in device tree's
> > >             chosen node or the ACPI SPCR table if supported by
> > >             the platform.
> >
> > Sigh, I wasn't aware of this when we discussed the console= handling.
> 
> It took a bit of digging to figure out what the actual intention was :)
> 
> >
> > > The reason this bug showed up is that ChromeOS has set `console=` for a
> > > very long time:
> > > https://chromium.googlesource.com/chromiumos/platform/crosutils/+/main/build_kernel_image.sh#282
> > > I'm not sure on the exact history, but AFAIK, we don't have the ttyX devices.
> > >
> > > Coreboot recently added support for the ACPI SPCR table which in
> > > combination with the
> > > `console=` arg, we are now seeing earlycon enabled when it shouldn't be.
> >
> > But this happens only when both "earlycon" and "console=" parameters
> > are used together. Do I get it correctly?
> 
> The bug shows up when an SPCR table is present and the `console=`
> parameter is set. No need to specify `earlycon` on the command line.

Strange, see below.

> > This combination is ambiguous on its own. Why would anyone add
> > "earlycon" parameter and wanted to keep it disabled?
> 
> This is not the case I'm hitting. I'm honestly not sure what the
> behavior should be in the `earlycon console=` case?
> 
> >
> > > > >>> diff --git a/init/main.c b/init/main.c
> > > > >>> index aa21add5f7c54..f72bf644910c1 100644
> > > > >>> --- a/init/main.c
> > > > >>> +++ b/init/main.c
> > > > >>> @@ -738,8 +738,7 @@ static int __init do_early_param(char *param, char *val,
> > > > >>>       for (p = __setup_start; p < __setup_end; p++) {
> > > > >>>           if ((p->early && parameq(param, p->str)) ||
> > > > >>>               (strcmp(param, "console") == 0 &&
> > > > >>> -             strcmp(p->str, "earlycon") == 0)
> > > > >>> -        ) {
> > > > >>> +             strcmp(p->str, "earlycon") == 0 && val && val[0])) {
> > > > >>>               if (p->setup_func(val) != 0)
> > > > >>>                   pr_warn("Malformed early option '%s'\n", param);
> > > > >>>           }

My understanding is that this code in do_early_param() allows to call
param_setup_earlycon() with the @val defined via console=val.
It reduces cut&paste on the kernel command line.

It should never enable an early console when "earlycon" is not defined
on the command line. Otherwise, console=uart[8250],mmio,<addr>[,options]
would always enable earlycon as well.

If the "earlycon" is not defined on the command line then
we should never call param_setup_earlycon() in the first place.

Or the behavior is even more crazy than I thought.

> >
> >     + "console" enables the default console which might be overridden
> >       by ACPI SPCR and devicetree
> 
> That's what this patch fixes. You need to specify `earlycon` in order
> to get the ACPI SPCR or DT console.

It sounds strange. earlycon is needed only for debugging. While
ACPI SPRC or DT should define the preferred console by the platform.

There are three levels of preference:

   + console= parameter defines the user preferred. It overrides
     everything.

   + ACPI SPCR or DT should define the preferred console by
     platform. It will be used when there is no user preference.

   + Kernel registers the first initialized console with tty driver
     when the is no preferred console by the user, ACPI SPCR, or DT.

As I said, I would expect that early console is enabled only when
earlycon parameter is defined on the command line.

In each case, it seems that acpi_parse_spcr() and of_console_check()
call add_preferred_console() even when earlycon is not defined
on the commandline.

> I don't see the `console` (without the =) documented:
> https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html.
> I'm guessing this is an undocumented "feature" that snuck in while the
> `earlycon` stuff was being added.

Honestly, I do not see where the "console" without '=' is handled.
console_setup() does not check if the @str parameter is NULL.


Anyway, the behavior already is complicated. But it might still
make some sense when:

   + "earlycon" parameter would try to call param_setup_earlycon()
     with @val from "console=val" parameter. It reduces cut&paste.

   + "console=" causes that "ttynull" driver gets preferred. Which might
       cause that no console driver gets registered at all. [*]

But seems to be yet another level of craziness when "console" or
"console=" would affect whether the early console will try
to be defined via ACPI SPCR or not.

I believe that this patch solves the problem. But it looks
like a workaround which makes the logic even more tricky/hacky.


IMHO, the right fix is to make sure that param_setup_earlycon()
should get called only when "earlycon" is defined on the commandline.

Best Regards,
Petr
  

On Fri, Jul 14, 2023 at 12:35 PM Petr Mladek <pmladek@suse.com> wrote:
>
> First, I am sorry I sent the first mail too early by mistake.
> (Friday evening effect).

No worries.

>
> On Fri 2023-07-14 11:21:09, Raul Rangel wrote:
> > On Fri, Jul 14, 2023 at 10:38 AM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > On Mon 2023-07-10 09:30:19, Raul Rangel wrote:
> > > > On Sun, Jul 9, 2023 at 8:43 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 7/9/23 18:15, Mario Limonciello wrote:
> > > > > > On 7/9/23 18:46, Randy Dunlap wrote:
> > > > > >>
> > > > > >>
> > > > > >> On 7/7/23 18:17, Raul E Rangel wrote:
> > > > > >>> Right now we are proxying the `console=XXX` command line args to the
> > > > > >>> param_setup_earlycon. This is done because the following are
> > > > > >>> equivalent:
> > > > > >>>
> > > > > >>>      console=uart[8250],mmio,<addr>[,options]
> > > > > >>>      earlycon=uart[8250],mmio,<addr>[,options]
> > > > > >>>
> > > > > >>> In addition, when `earlycon=` or just `earlycon` is specified on the
> > > > > >>> command line, we look at the SPCR table or the DT to extract the device
> > > > > >>> options.
> > > > > >>>
> > > > > >>> When `console=` is specified on the command line, it's intention is to
> > > > > >>> disable the console. Right now since we are proxying the `console=`
> > > > > >>
> > > > > >> How do you figure this (its intention is to disable the console)?
> > > > > >
> > > >
> > > > https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html
> > > > says the following:
> > > > console=
> > > >     { null | "" }
> > > >             Use to disable console output, i.e., to have kernel
> > > >             console messages discarded.
> > > >             This must be the only console= parameter used on the
> > > >             kernel command line.
> > > >
> > > >         earlycon=       [KNL] Output early console device and options.
> > > >
> > > >             When used with no options, the early console is
> > > >             determined by stdout-path property in device tree's
> > > >             chosen node or the ACPI SPCR table if supported by
> > > >             the platform.
> > >
> > > Sigh, I wasn't aware of this when we discussed the console= handling.
> >
> > It took a bit of digging to figure out what the actual intention was :)
> >
> > >
> > > > The reason this bug showed up is that ChromeOS has set `console=` for a
> > > > very long time:
> > > > https://chromium.googlesource.com/chromiumos/platform/crosutils/+/main/build_kernel_image.sh#282
> > > > I'm not sure on the exact history, but AFAIK, we don't have the ttyX devices.
> > > >
> > > > Coreboot recently added support for the ACPI SPCR table which in
> > > > combination with the
> > > > `console=` arg, we are now seeing earlycon enabled when it shouldn't be.
> > >
> > > But this happens only when both "earlycon" and "console=" parameters
> > > are used together. Do I get it correctly?
> >
> > The bug shows up when an SPCR table is present and the `console=`
> > parameter is set. No need to specify `earlycon` on the command line.
>
> Strange, see below.
>
> > > This combination is ambiguous on its own. Why would anyone add
> > > "earlycon" parameter and wanted to keep it disabled?
> >
> > This is not the case I'm hitting. I'm honestly not sure what the
> > behavior should be in the `earlycon console=` case?
> >
> > >
> > > > > >>> diff --git a/init/main.c b/init/main.c
> > > > > >>> index aa21add5f7c54..f72bf644910c1 100644
> > > > > >>> --- a/init/main.c
> > > > > >>> +++ b/init/main.c
> > > > > >>> @@ -738,8 +738,7 @@ static int __init do_early_param(char *param, char *val,
> > > > > >>>       for (p = __setup_start; p < __setup_end; p++) {
> > > > > >>>           if ((p->early && parameq(param, p->str)) ||
> > > > > >>>               (strcmp(param, "console") == 0 &&
> > > > > >>> -             strcmp(p->str, "earlycon") == 0)
> > > > > >>> -        ) {
> > > > > >>> +             strcmp(p->str, "earlycon") == 0 && val && val[0])) {
> > > > > >>>               if (p->setup_func(val) != 0)
> > > > > >>>                   pr_warn("Malformed early option '%s'\n", param);
> > > > > >>>           }
>
> My understanding is that this code in do_early_param() allows to call
> param_setup_earlycon() with the @val defined via console=val.
> It reduces cut&paste on the kernel command line.

Exactly

>
> It should never enable an early console when "earlycon" is not defined
> on the command line. Otherwise, console=uart[8250],mmio,<addr>[,options]
> would always enable earlycon as well.
>

Yep, this is what my patch fixes.

> If the "earlycon" is not defined on the command line then
> we should never call param_setup_earlycon() in the first place.
>
> Or the behavior is even more crazy than I thought.

This contradicts your first point. We need to call
`param_setup_earlycon` so it can handle `console=uart,mmio,XXXX`.
That's why this block of code is here. IMO it's very confusing
behavior that `earlycon=uart,mmio,XXXX` and `console=uart,mmio,XXXX`
are the same thing.

The reason my patch checks for a NULL or empty val is because
`param_setup_earlycon` has a special case to handle the
`earlycon`/`earlycon=` case:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-6.1/drivers/tty/serial/earlycon.c#223

```
/* Just 'earlycon' is a valid param for devicetree and ACPI SPCR. */
if (!buf || !buf[0]) {
  if (IS_ENABLED(CONFIG_ACPI_SPCR_TABLE)) {
    earlycon_acpi_spcr_enable = true;
    return 0;
  } else if (!buf) {
    return early_init_dt_scan_chosen_stdout();
  }
}
```

Before my patch `console=` would call `param_setup_earlycon` which
would toggle the `earlycon_acpi_spcr_enable` flag true. This is
probably also the code that handled the naked `console` command too.

> > >
> > >     + "console" enables the default console which might be overridden
> > >       by ACPI SPCR and devicetree
> >
> > That's what this patch fixes. You need to specify `earlycon` in order
> > to get the ACPI SPCR or DT console.
>
> It sounds strange. earlycon is needed only for debugging. While
> ACPI SPRC or DT should define the preferred console by the platform.
>
> There are three levels of preference:
>
>    + console= parameter defines the user preferred. It overrides
>      everything.
>
>    + ACPI SPCR or DT should define the preferred console by
>      platform. It will be used when there is no user preference.
>
>    + Kernel registers the first initialized console with tty driver
>      when the is no preferred console by the user, ACPI SPCR, or DT.
>
> As I said, I would expect that early console is enabled only when
> earlycon parameter is defined on the command line.
>
> In each case, it seems that acpi_parse_spcr() and of_console_check()
> call add_preferred_console() even when earlycon is not defined
> on the commandline.
>

Currently the policy if SPCR is used for the default console is
determined by architectural policy. ARM64 enabled it, while x86 keeps
it disabled:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-6.1/arch/arm64/kernel/acpi.c#229
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-6.1/arch/x86/kernel/acpi/boot.c#1748

I'm honestly not a fan of enabling the SPCR console by default. It
will slow down booting since things now need to get written to the
UART. It's useful for debugging problems before the real console
driver can register (ttyS0, etc). It would honestly be nice if kernel
OOPS/panics got written to the SPCR UART. The earlycon also has
problems in that the ACPI power resources can't be specified so we can
run into problems when the ttyS0 ACPI power resources get enumerated
and turned off because there isn't a driver registered yet. This can
cause the earlycon to die/hang since it just got powerd off. But I
digress :)

> > I don't see the `console` (without the =) documented:
> > https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html.
> > I'm guessing this is an undocumented "feature" that snuck in while the
> > `earlycon` stuff was being added.
>
> Honestly, I do not see where the "console" without '=' is handled.
> console_setup() does not check if the @str parameter is NULL.
>
>
> Anyway, the behavior already is complicated. But it might still
> make some sense when:
>
>    + "earlycon" parameter would try to call param_setup_earlycon()
>      with @val from "console=val" parameter. It reduces cut&paste.
>
>    + "console=" causes that "ttynull" driver gets preferred. Which might
>        cause that no console driver gets registered at all. [*]
>
> But seems to be yet another level of craziness when "console" or
> "console=" would affect whether the early console will try
> to be defined via ACPI SPCR or not.
>
> I believe that this patch solves the problem. But it looks
> like a workaround which makes the logic even more tricky/hacky.
>
>
> IMHO, the right fix is to make sure that param_setup_earlycon()
> should get called only when "earlycon" is defined on the commandline.

We can do that, but it will remove the `console=uart,mmio,XXXX`
handling. IMO that's the correct thing to do, but I suspect there are
a lot of people that depend on it.

>
> Best Regards,
> Petr
  

On Fri 2023-07-14 15:42:36, Raul Rangel wrote:
> On Fri, Jul 14, 2023 at 12:35 PM Petr Mladek <pmladek@suse.com> wrote:
> > On Fri 2023-07-14 11:21:09, Raul Rangel wrote:
> > > On Fri, Jul 14, 2023 at 10:38 AM Petr Mladek <pmladek@suse.com> wrote:
> > > > On Mon 2023-07-10 09:30:19, Raul Rangel wrote:
> > > > > On Sun, Jul 9, 2023 at 8:43 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> > > > > > On 7/9/23 18:15, Mario Limonciello wrote:
> > > > > > > On 7/9/23 18:46, Randy Dunlap wrote:
> > > > > > >> On 7/7/23 18:17, Raul E Rangel wrote:
> > > > > > >>> Right now we are proxying the `console=XXX` command line args to the
> > > > > > >>> param_setup_earlycon. This is done because the following are
> > > > > > >>> equivalent:
> > > > > > >>>
> > > > > > >>>      console=uart[8250],mmio,<addr>[,options]
> > > > > > >>>      earlycon=uart[8250],mmio,<addr>[,options]

I have finally got the meaning of the above paragraph. I thought that
it was talking about that the format was equivalent. But it was about
that also the effect was equivalent.

> > > > > > >>> In addition, when `earlycon=` or just `earlycon` is specified on the
> > > > > > >>> command line, we look at the SPCR table or the DT to extract the device
> > > > > > >>> options.
> > > > > > >>>
> > > > > > >>> When `console=` is specified on the command line, it's intention is to
> > > > > > >>> disable the console. Right now since we are proxying the `console=`
> > > > > > >>
> > > > > > >> How do you figure this (its intention is to disable the console)?
> > > > > > >
> > > > >
> > > > > https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html
> > > > > says the following:
> > > > > console=
> > > > >     { null | "" }
> > > > >             Use to disable console output, i.e., to have kernel
> > > > >             console messages discarded.
> > > > >             This must be the only console= parameter used on the
> > > > >             kernel command line.
> > > > >
> > > > >         earlycon=       [KNL] Output early console device and options.
> > > > >
> > > > >             When used with no options, the early console is
> > > > >             determined by stdout-path property in device tree's
> > > > >             chosen node or the ACPI SPCR table if supported by
> > > > >             the platform.
> > > >
> > > > Sigh, I wasn't aware of this when we discussed the console= handling.
> > >
> > > It took a bit of digging to figure out what the actual intention was :)
> > >
> > > >
> > > > > The reason this bug showed up is that ChromeOS has set `console=` for a
> > > > > very long time:
> > > > > https://chromium.googlesource.com/chromiumos/platform/crosutils/+/main/build_kernel_image.sh#282
> > > > > I'm not sure on the exact history, but AFAIK, we don't have the ttyX devices.
> > > > >
> > > > > Coreboot recently added support for the ACPI SPCR table which in
> > > > > combination with the
> > > > > `console=` arg, we are now seeing earlycon enabled when it shouldn't be.
> > > >
> > > > But this happens only when both "earlycon" and "console=" parameters
> > > > are used together. Do I get it correctly?
> > >
> > > The bug shows up when an SPCR table is present and the `console=`
> > > parameter is set. No need to specify `earlycon` on the command line.
> >
> > Strange, see below.
> >
> > > > This combination is ambiguous on its own. Why would anyone add
> > > > "earlycon" parameter and wanted to keep it disabled?
> > >
> > > This is not the case I'm hitting. I'm honestly not sure what the
> > > behavior should be in the `earlycon console=` case?
> > >
> > > >
> > > > > > >>> diff --git a/init/main.c b/init/main.c
> > > > > > >>> index aa21add5f7c54..f72bf644910c1 100644
> > > > > > >>> --- a/init/main.c
> > > > > > >>> +++ b/init/main.c
> > > > > > >>> @@ -738,8 +738,7 @@ static int __init do_early_param(char *param, char *val,
> > > > > > >>>       for (p = __setup_start; p < __setup_end; p++) {
> > > > > > >>>           if ((p->early && parameq(param, p->str)) ||
> > > > > > >>>               (strcmp(param, "console") == 0 &&
> > > > > > >>> -             strcmp(p->str, "earlycon") == 0)
> > > > > > >>> -        ) {
> > > > > > >>> +             strcmp(p->str, "earlycon") == 0 && val && val[0])) {
> > > > > > >>>               if (p->setup_func(val) != 0)
> > > > > > >>>                   pr_warn("Malformed early option '%s'\n", param);
> > > > > > >>>           }
> >
> This contradicts your first point. We need to call
> `param_setup_earlycon` so it can handle `console=uart,mmio,XXXX`.
> That's why this block of code is here. IMO it's very confusing
> behavior that `earlycon=uart,mmio,XXXX` and `console=uart,mmio,XXXX`
> are the same thing.

Urgh, I didn't know that "console=uart*" started early console.
I always thought that it was just another way how to define
the normal console.

(I feel shame as a printk maintainer. But I have never used it.
And this has been the first patch in the related code since
I started watching printk 10 years ago.)

Looking into the history, "earlycon" parameter was added by
the commit 18a8bd949d6adb311 ("serial: convert early_uart to
earlycon for 8250") in 2007. The parameter "console=uart,mmio,XXX"
started the console earlier even before.

The "earlycon" parameter allowed to define the early console
an explicit way on the command line. But it was not strictly
necessary. The same effect could have been achieved by:

static int __init do_early_param(char *param, char *val,
				 const char *unused, void *arg)
{
	struct obs_kernel_param *p;

	for (p = __setup_start; p < __setup_end; p++) {
		if (p->early && strcmp(param, p->str) == 0) {
			if (p->setup_func(val) != 0)
				printk(KERN_WARNING
				       "Malformed early option '%s'\n", param);
		}

		if (strcmp(param, "console") == 0) {
			if (setup_early_serial8250_console(val) != 0)
				printk(KERN_WARNING, "Failed to setup early console");
	}
}

> The reason my patch checks for a NULL or empty val is because
> `param_setup_earlycon` has a special case to handle the
> `earlycon`/`earlycon=` case:
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-6.1/drivers/tty/serial/earlycon.c#223
> 
> ```
> /* Just 'earlycon' is a valid param for devicetree and ACPI SPCR. */
> if (!buf || !buf[0]) {
>   if (IS_ENABLED(CONFIG_ACPI_SPCR_TABLE)) {
>     earlycon_acpi_spcr_enable = true;
>     return 0;
>   } else if (!buf) {
>     return early_init_dt_scan_chosen_stdout();
>   }
> }
> ```

I see. It all makes sense now.

Your patch is good then. Well, would you mind to add a comment into
the code and make the commit message more clear even for dummies like
me?

Something like the patch below. It would be better to split it into
two:

   + 1st shuffling the check and adding the first part of the comment
   + 2nd fixing the case with empty console= options.

I could prepare the patchset. I would keep your SOB for the 2nd patch
if you agreed.

Here is the proposal:

From d2fb7c54bed4c67df229c56fcc1b0af83ada5227 Mon Sep 17 00:00:00 2001
From: Raul E Rangel <rrangel@chromium.org>
Date: Fri, 7 Jul 2023 19:17:25 -0600
Subject: [PATCH] init: Don't proxy the empty console= options to earlycon

Right now we are proxying the `console=XXX` command line args to the
param_setup_earlycon. This is done because the early consoles used to
be enabled via the `console` parameter.

The `earlycon` parameter was added later by the commit 18a8bd949d6adb311
("serial: convert early_uart to earlycon for 8250"). It allowed to
setup the early consoles using another callback. And it was indeed
more self-explanatory and cleaner approach.

As a result, for example, the following parameters have the same effect:

    console=uart[8250],mmio,<addr>[,options]
    earlycon=uart[8250],mmio,<addr>[,options]

Nevertheless, `console` and `earlycon` parameters behave different when
the options do not match an early console. setup_earlycon() accepts only
console names in __earlycon_table. Also the empty options are handled
differently:

  + When `earlycon=` or just `earlycon` is specified on the command line,
    param_setup_earlycon() tries to enable an early console via the SPCR
    table or the device tree.

  + When `console=` is specified on the command line, it's intention is to
    disable the console.

Here comes the problem. The current code calls param_setup_earlycon()
even when `console=` is used with no options. As a result, it might
enable the early console when it is defined in the SPCR table or
a device tree. This is obviously not what users want.

The early console should be enabled via SPCR or DT only when `earlycon`
is used explicitly with no options.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
[pmladek@suse.com: Add comments and more details into the commit message]
---
 init/main.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/init/main.c b/init/main.c
index ad920fac325c..3b059e316e62 100644
--- a/init/main.c
+++ b/init/main.c
@@ -733,13 +733,25 @@ static int __init do_early_param(char *param, char *val,
 	const struct obs_kernel_param *p;
 
 	for (p = __setup_start; p < __setup_end; p++) {
-		if ((p->early && parameq(param, p->str)) ||
-		    (strcmp(param, "console") == 0 &&
-		     strcmp(p->str, "earlycon") == 0)
-		) {
+		if (p->early && parameq(param, p->str)) {
 			if (p->setup_func(val) != 0)
 				pr_warn("Malformed early option '%s'\n", param);
 		}
+
+		/*
+		 * Early consoles can be historically enabled also when earlycon
+		 * specific options are passed via console=[earlycon options]
+		 * parameter.
+		 *
+		 * Do not try it with an empty value. "console=" is used to
+		 * disable real normal consoles. While "earlycon" is used to
+		 * enable an early console via SPCR or DT.
+		 */
+		if (strcmp(param, "console") == 0 && val && val[0] &&
+		    strcmp(p->str, "earlycon") == 0) {
+			if (p->setup_func(val) != 0)
+				pr_warn("Failed to setup earlycon via console=%s\n", val);
+		}
 	}
 	/* We accept everything at this stage. */
 	return 0;
  

Sorry, sending again since my original reply wasn't in plain text mode
and got bounced from the mailing list.

On Tue, Jul 18, 2023 at 3:50 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Fri 2023-07-14 15:42:36, Raul Rangel wrote:
> > On Fri, Jul 14, 2023 at 12:35 PM Petr Mladek <pmladek@suse.com> wrote:
> > > On Fri 2023-07-14 11:21:09, Raul Rangel wrote:
> > > > On Fri, Jul 14, 2023 at 10:38 AM Petr Mladek <pmladek@suse.com> wrote:
> > > > > On Mon 2023-07-10 09:30:19, Raul Rangel wrote:
> > > > > > On Sun, Jul 9, 2023 at 8:43 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> > > > > > > On 7/9/23 18:15, Mario Limonciello wrote:
> > > > > > > > On 7/9/23 18:46, Randy Dunlap wrote:
> > > > > > > >> On 7/7/23 18:17, Raul E Rangel wrote:
> > > > > > > >>> Right now we are proxying the `console=XXX` command line args to the
> > > > > > > >>> param_setup_earlycon. This is done because the following are
> > > > > > > >>> equivalent:
> > > > > > > >>>
> > > > > > > >>>      console=uart[8250],mmio,<addr>[,options]
> > > > > > > >>>      earlycon=uart[8250],mmio,<addr>[,options]
>
> I have finally got the meaning of the above paragraph. I thought that
> it was talking about that the format was equivalent. But it was about
> that also the effect was equivalent.
>
> > > > > > > >>> In addition, when `earlycon=` or just `earlycon` is specified on the
> > > > > > > >>> command line, we look at the SPCR table or the DT to extract the device
> > > > > > > >>> options.
> > > > > > > >>>
> > > > > > > >>> When `console=` is specified on the command line, it's intention is to
> > > > > > > >>> disable the console. Right now since we are proxying the `console=`
> > > > > > > >>
> > > > > > > >> How do you figure this (its intention is to disable the console)?
> > > > > > > >
> > > > > >
> > > > > > https://www.kernel.org/doc/html/v6.1/admin-guide/kernel-parameters.html
> > > > > > says the following:
> > > > > > console=
> > > > > >     { null | "" }
> > > > > >             Use to disable console output, i.e., to have kernel
> > > > > >             console messages discarded.
> > > > > >             This must be the only console= parameter used on the
> > > > > >             kernel command line.
> > > > > >
> > > > > >         earlycon=       [KNL] Output early console device and options.
> > > > > >
> > > > > >             When used with no options, the early console is
> > > > > >             determined by stdout-path property in device tree's
> > > > > >             chosen node or the ACPI SPCR table if supported by
> > > > > >             the platform.
> > > > >
> > > > > Sigh, I wasn't aware of this when we discussed the console= handling.
> > > >
> > > > It took a bit of digging to figure out what the actual intention was :)
> > > >
> > > > >
> > > > > > The reason this bug showed up is that ChromeOS has set `console=` for a
> > > > > > very long time:
> > > > > > https://chromium.googlesource.com/chromiumos/platform/crosutils/+/main/build_kernel_image.sh#282
> > > > > > I'm not sure on the exact history, but AFAIK, we don't have the ttyX devices.
> > > > > >
> > > > > > Coreboot recently added support for the ACPI SPCR table which in
> > > > > > combination with the
> > > > > > `console=` arg, we are now seeing earlycon enabled when it shouldn't be.
> > > > >
> > > > > But this happens only when both "earlycon" and "console=" parameters
> > > > > are used together. Do I get it correctly?
> > > >
> > > > The bug shows up when an SPCR table is present and the `console=`
> > > > parameter is set. No need to specify `earlycon` on the command line.
> > >
> > > Strange, see below.
> > >
> > > > > This combination is ambiguous on its own. Why would anyone add
> > > > > "earlycon" parameter and wanted to keep it disabled?
> > > >
> > > > This is not the case I'm hitting. I'm honestly not sure what the
> > > > behavior should be in the `earlycon console=` case?
> > > >
> > > > >
> > > > > > > >>> diff --git a/init/main.c b/init/main.c
> > > > > > > >>> index aa21add5f7c54..f72bf644910c1 100644
> > > > > > > >>> --- a/init/main.c
> > > > > > > >>> +++ b/init/main.c
> > > > > > > >>> @@ -738,8 +738,7 @@ static int __init do_early_param(char *param, char *val,
> > > > > > > >>>       for (p = __setup_start; p < __setup_end; p++) {
> > > > > > > >>>           if ((p->early && parameq(param, p->str)) ||
> > > > > > > >>>               (strcmp(param, "console") == 0 &&
> > > > > > > >>> -             strcmp(p->str, "earlycon") == 0)
> > > > > > > >>> -        ) {
> > > > > > > >>> +             strcmp(p->str, "earlycon") == 0 && val && val[0])) {
> > > > > > > >>>               if (p->setup_func(val) != 0)
> > > > > > > >>>                   pr_warn("Malformed early option '%s'\n", param);
> > > > > > > >>>           }
> > >
> > This contradicts your first point. We need to call
> > `param_setup_earlycon` so it can handle `console=uart,mmio,XXXX`.
> > That's why this block of code is here. IMO it's very confusing
> > behavior that `earlycon=uart,mmio,XXXX` and `console=uart,mmio,XXXX`
> > are the same thing.
>
> Urgh, I didn't know that "console=uart*" started early console.
> I always thought that it was just another way how to define
> the normal console.

Yeah, it's all very confusing. We should probably remove the
`console=uart*` from the kernel documentation so people use
`earlycon=uart*` instead.

>
> (I feel shame as a printk maintainer. But I have never used it.
> And this has been the first patch in the related code since
> I started watching printk 10 years ago.)

lol, please don't shame yourself. There are plenty of odd quirks to be
found in maintaining such a large project.

>
> Looking into the history, "earlycon" parameter was added by
> the commit 18a8bd949d6adb311 ("serial: convert early_uart to
> earlycon for 8250") in 2007. The parameter "console=uart,mmio,XXX"
> started the console earlier even before.
>
> The "earlycon" parameter allowed to define the early console
> an explicit way on the command line. But it was not strictly
> necessary. The same effect could have been achieved by:
>
> static int __init do_early_param(char *param, char *val,
>                                  const char *unused, void *arg)
> {
>         struct obs_kernel_param *p;
>
>         for (p = __setup_start; p < __setup_end; p++) {
>                 if (p->early && strcmp(param, p->str) == 0) {
>                         if (p->setup_func(val) != 0)
>                                 printk(KERN_WARNING
>                                        "Malformed early option '%s'\n", param);
>                 }
>
>                 if (strcmp(param, "console") == 0) {
>                         if (setup_early_serial8250_console(val) != 0)
>                                 printk(KERN_WARNING, "Failed to setup early console");
>         }
> }
>
> > The reason my patch checks for a NULL or empty val is because
> > `param_setup_earlycon` has a special case to handle the
> > `earlycon`/`earlycon=` case:
> > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-6.1/drivers/tty/serial/earlycon.c#223
> >
> > ```
> > /* Just 'earlycon' is a valid param for devicetree and ACPI SPCR. */
> > if (!buf || !buf[0]) {
> >   if (IS_ENABLED(CONFIG_ACPI_SPCR_TABLE)) {
> >     earlycon_acpi_spcr_enable = true;
> >     return 0;
> >   } else if (!buf) {
> >     return early_init_dt_scan_chosen_stdout();
> >   }
> > }
> > ```
>
> I see. It all makes sense now.

Great :)

>
> Your patch is good then. Well, would you mind to add a comment into
> the code and make the commit message more clear even for dummies like
> me?
>
> Something like the patch below. It would be better to split it into
> two:
>
>    + 1st shuffling the check and adding the first part of the comment
>    + 2nd fixing the case with empty console= options.
>
> I could prepare the patchset. I would keep your SOB for the 2nd patch
> if you agreed.

Yes, feel free. Thanks!

>
> Here is the proposal:
>
> From d2fb7c54bed4c67df229c56fcc1b0af83ada5227 Mon Sep 17 00:00:00 2001
> From: Raul E Rangel <rrangel@chromium.org>
> Date: Fri, 7 Jul 2023 19:17:25 -0600
> Subject: [PATCH] init: Don't proxy the empty console= options to earlycon
>
> Right now we are proxying the `console=XXX` command line args to the
> param_setup_earlycon. This is done because the early consoles used to
> be enabled via the `console` parameter.
>
> The `earlycon` parameter was added later by the commit 18a8bd949d6adb311
> ("serial: convert early_uart to earlycon for 8250"). It allowed to
> setup the early consoles using another callback. And it was indeed
> more self-explanatory and cleaner approach.
>
> As a result, for example, the following parameters have the same effect:
>
>     console=uart[8250],mmio,<addr>[,options]
>     earlycon=uart[8250],mmio,<addr>[,options]
>
> Nevertheless, `console` and `earlycon` parameters behave different when
> the options do not match an early console. setup_earlycon() accepts only
> console names in __earlycon_table. Also the empty options are handled
> differently:
>
>   + When `earlycon=` or just `earlycon` is specified on the command line,
>     param_setup_earlycon() tries to enable an early console via the SPCR
>     table or the device tree.
>
>   + When `console=` is specified on the command line, it's intention is to
>     disable the console.
>
> Here comes the problem. The current code calls param_setup_earlycon()
> even when `console=` is used with no options. As a result, it might
> enable the early console when it is defined in the SPCR table or
> a device tree. This is obviously not what users want.
>
> The early console should be enabled via SPCR or DT only when `earlycon`
> is used explicitly with no options.
>
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> [pmladek@suse.com: Add comments and more details into the commit message]
> ---
>  init/main.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/init/main.c b/init/main.c
> index ad920fac325c..3b059e316e62 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -733,13 +733,25 @@ static int __init do_early_param(char *param, char *val,
>         const struct obs_kernel_param *p;
>
>         for (p = __setup_start; p < __setup_end; p++) {
> -               if ((p->early && parameq(param, p->str)) ||
> -                   (strcmp(param, "console") == 0 &&
> -                    strcmp(p->str, "earlycon") == 0)
> -               ) {
> +               if (p->early && parameq(param, p->str)) {
>                         if (p->setup_func(val) != 0)
>                                 pr_warn("Malformed early option '%s'\n", param);
>                 }
> +
> +               /*
> +                * Early consoles can be historically enabled also when earlycon
> +                * specific options are passed via console=[earlycon options]
> +                * parameter.
> +                *
> +                * Do not try it with an empty value. "console=" is used to
> +                * disable real normal consoles. While "earlycon" is used to
> +                * enable an early console via SPCR or DT.
> +                */
> +               if (strcmp(param, "console") == 0 && val && val[0] &&
> +                   strcmp(p->str, "earlycon") == 0) {
> +                       if (p->setup_func(val) != 0)
> +                               pr_warn("Failed to setup earlycon via console=%s\n", val);
> +               }
>         }
>         /* We accept everything at this stage. */
>         return 0;
> --
> 2.35.3
>

Signed-off-by: Raul E Rangel <rrangel@chromium.org>