Re: [PATCH] init: Don't proxy console= to earlycon

[Petr Mladek]

On Fri 2023-07-14 15:42:36, Raul Rangel wrote:
> On Fri, Jul 14, 2023 at 12:35 PM Petr Mladek <pmladek@xxxxxxxx> wrote:
> > On Fri 2023-07-14 11:21:09, Raul Rangel wrote:
> > > On Fri, Jul 14, 2023 at 10:38 AM Petr Mladek <pmladek@xxxxxxxx> wrote:
> > > > On Mon 2023-07-10 09:30:19, Raul Rangel wrote:
> > > > > On Sun, Jul 9, 2023 at 8:43 PM Randy Dunlap <rdunlap@xxxxxxxxxxxxx> 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: