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

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

[Raul E Rangel]

Today we are proxying the `console=` command line args to the
`param_setup_earlycon()` handler. This is done because the following are
equivalent:

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

Both invocations enable an early `bootconsole`. `console=uartXXXX` is
just an alias for `earlycon=uartXXXX`.

In addition, when `earlycon=` (empty value) or just `earlycon`
(no value) is specified on the command line, we enable the earlycon
`bootconsole` specified by the SPCR table or the DT.

The problem arises when `console=` (empty value) is specified on the
command line. It's intention is to disable the `console`, but what
happens instead is that the SPRC/DT console gets enabled.

This happens because we are proxying the `console=` (empty value)
parameter to the `earlycon` handler. The `earlycon` handler then sees
that the parameter value is empty, so it enables the SPCR/DT
`bootconsole`.

This change makes it so that the `console` or `console=` parameters no
longer enable the SPCR/DT `bootconsole`. I also cleans up the hack in
`main.c` that would forward the `console` parameter to the `earlycon`
handler.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
I tested this patch with the following permutations:
* console=
* console
* console=ttyS0,115200
* earlycon
* earlycon=uart,mmio32,0xfedc9000
* earlycon=
* console=uart,mmio32,0xfedc9000,115200n8

One unfortunate thing (unrelated to this patch) is that the
`univ8250_console` registers as a real console (regardless if the legacy
0x3F8 UARTs are present) early on. This causes the `bootconsole` to get
disabled and replaced with the non functional `univ8250_console`.

[    0.000000] earlycon: uart0 at MMIO32 0x00000000fedc9000 (options '')
[    0.000000] printk: legacy bootconsole [uart0] enabled
...
[    1.141835] printk: legacy console [ttyS0] enabled <-- Booo!
[    1.156500] printk: legacy bootconsole [uart0] disabled

Changes in v2:
- Switched to defining an early console parameter
- Removed hack in main.c

 drivers/tty/serial/earlycon.c | 23 +++++++++++++++++++++++
 init/main.c                   |  5 +----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index a5fbb6ed38aed6..ab9af37f6cda35 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -248,6 +248,29 @@ static int __init param_setup_earlycon(char *buf)
 }
 early_param("earlycon", param_setup_earlycon);
 
+/*
+ * The `console` parameter is overloaded. It's handled here as an early param
+ * and in `printk.c` as a late param. It's possible to specify an early
+ * `bootconsole` using `earlycon=uartXXXX` (handled above), or via
+ * the `console=uartXXX` alias. See the comment in `8250_early.c`.
+ */
+static int __init param_setup_earlycon_console_alias(char *buf)
+{
+	/*
+	 * A plain `console` parameter must not enable the SPCR `bootconsole`
+	 * like a plain `earlycon` does.
+	 *
+	 * A `console=` parameter that specifies an empty value is used to
+	 * disable the `console`, not the `earlycon` `bootconsole`. The
+	 * disabling of the `console` is handled by `printk.c`.
+	 */
+	if (!buf || !buf[0])
+		return 0;
+
+	return param_setup_earlycon(buf);
+}
+early_param("console", param_setup_earlycon_console_alias);
+
 #ifdef CONFIG_OF_EARLY_FLATTREE
 
 int __init of_setup_earlycon(const struct earlycon_id *match,
diff --git a/init/main.c b/init/main.c
index 206acdde51f5a9..e668dda1c68f3f 100644
--- a/init/main.c
+++ b/init/main.c
@@ -754,10 +754,7 @@ 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);
 		}
-- 
2.46.0.598.g6f2099f65c-goog

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

[Petr Mladek]

On Wed 2024-09-11 12:35:14, Raul E Rangel wrote:
> Today we are proxying the `console=` command line args to the
> `param_setup_earlycon()` handler. This is done because the following are
> equivalent:
> 
>     console=uart[8250],mmio,<addr>[,options]
>     earlycon=uart[8250],mmio,<addr>[,options]
> 
> Both invocations enable an early `bootconsole`. `console=uartXXXX` is
> just an alias for `earlycon=uartXXXX`.
> 
> In addition, when `earlycon=` (empty value) or just `earlycon`
> (no value) is specified on the command line, we enable the earlycon
> `bootconsole` specified by the SPCR table or the DT.
> 
> The problem arises when `console=` (empty value) is specified on the
> command line. It's intention is to disable the `console`, but what
> happens instead is that the SPRC/DT console gets enabled.
> 
> This happens because we are proxying the `console=` (empty value)
> parameter to the `earlycon` handler. The `earlycon` handler then sees
> that the parameter value is empty, so it enables the SPCR/DT
> `bootconsole`.
> 
> This change makes it so that the `console` or `console=` parameters no
> longer enable the SPCR/DT `bootconsole`. I also cleans up the hack in
> `main.c` that would forward the `console` parameter to the `earlycon`
> handler.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>

It like this approach. It works well:

Reviewed-by: Petr Mladek <pmladek@suse.com>
Tested-by: Petr Mladek <pmladek@suse.com>

I could take it via the printk tree for 6.13. From my POV, it is too
late for 6.12. I am sorry I have been busy with the printk rework :-(

I am going to wait few days for another eventual review or taker.

Best Regards,
Petr

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

[Petr Mladek]

On Tue 2024-09-24 10:05:08, Raul Rangel wrote:
> On Thu, Sep 19, 2024 at 6:57 AM Petr Mladek <pmladek@suse.com> wrote:
> 
> > On Wed 2024-09-11 12:35:14, Raul E Rangel wrote:
> > > Today we are proxying the `console=` command line args to the
> > > `param_setup_earlycon()` handler. This is done because the following are
> > > equivalent:
> > >
> > >     console=uart[8250],mmio,<addr>[,options]
> > >     earlycon=uart[8250],mmio,<addr>[,options]
> > >
> > > Both invocations enable an early `bootconsole`. `console=uartXXXX` is
> > > just an alias for `earlycon=uartXXXX`.
> > >
> > > In addition, when `earlycon=` (empty value) or just `earlycon`
> > > (no value) is specified on the command line, we enable the earlycon
> > > `bootconsole` specified by the SPCR table or the DT.
> > >
> > > The problem arises when `console=` (empty value) is specified on the
> > > command line. It's intention is to disable the `console`, but what
> > > happens instead is that the SPRC/DT console gets enabled.
> > >
> > > This happens because we are proxying the `console=` (empty value)
> > > parameter to the `earlycon` handler. The `earlycon` handler then sees
> > > that the parameter value is empty, so it enables the SPCR/DT
> > > `bootconsole`.
> > >
> > > This change makes it so that the `console` or `console=` parameters no
> > > longer enable the SPCR/DT `bootconsole`. I also cleans up the hack in
> > > `main.c` that would forward the `console` parameter to the `earlycon`
> > > handler.
> > >
> > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> >
> > It like this approach. It works well:
> >
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > Tested-by: Petr Mladek <pmladek@suse.com>
> >
> 
> Thanks for reviewing and testing! I know it takes a significant amount of
> time, so thank you.
> 
> >
> > I could take it via the printk tree for 6.13. From my POV, it is too
> > late for 6.12. I am sorry I have been busy with the printk rework :-(
> >
> 
> 6.13 is fine. As long as it lands upstream I can cherry pick the patch into
> our forks without any pushback.

JFYI, the patch has been committed into printk/linux.git,
branch for-6.13.

Best Regards,
Petr

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

[Raul Rangel]

On Tue, Oct 1, 2024 at 9:09 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Tue 2024-09-24 10:05:08, Raul Rangel wrote:
> > On Thu, Sep 19, 2024 at 6:57 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > > On Wed 2024-09-11 12:35:14, Raul E Rangel wrote:
> > > > Today we are proxying the `console=` command line args to the
> > > > `param_setup_earlycon()` handler. This is done because the following are
> > > > equivalent:
> > > >
> > > >     console=uart[8250],mmio,<addr>[,options]
> > > >     earlycon=uart[8250],mmio,<addr>[,options]
> > > >
> > > > Both invocations enable an early `bootconsole`. `console=uartXXXX` is
> > > > just an alias for `earlycon=uartXXXX`.
> > > >
> > > > In addition, when `earlycon=` (empty value) or just `earlycon`
> > > > (no value) is specified on the command line, we enable the earlycon
> > > > `bootconsole` specified by the SPCR table or the DT.
> > > >
> > > > The problem arises when `console=` (empty value) is specified on the
> > > > command line. It's intention is to disable the `console`, but what
> > > > happens instead is that the SPRC/DT console gets enabled.
> > > >
> > > > This happens because we are proxying the `console=` (empty value)
> > > > parameter to the `earlycon` handler. The `earlycon` handler then sees
> > > > that the parameter value is empty, so it enables the SPCR/DT
> > > > `bootconsole`.
> > > >
> > > > This change makes it so that the `console` or `console=` parameters no
> > > > longer enable the SPCR/DT `bootconsole`. I also cleans up the hack in
> > > > `main.c` that would forward the `console` parameter to the `earlycon`
> > > > handler.
> > > >
> > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > >
> > > It like this approach. It works well:
> > >
> > > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > > Tested-by: Petr Mladek <pmladek@suse.com>
> > >
> >
> > Thanks for reviewing and testing! I know it takes a significant amount of
> > time, so thank you.
> >
> > >
> > > I could take it via the printk tree for 6.13. From my POV, it is too
> > > late for 6.12. I am sorry I have been busy with the printk rework :-(
> > >
> >
> > 6.13 is fine. As long as it lands upstream I can cherry pick the patch into
> > our forks without any pushback.
>
> JFYI, the patch has been committed into printk/linux.git,
> branch for-6.13.

Thank you!
>
> Best Regards,
> Petr