Date:   Tue, 14 Feb 2023 12:42:00 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     Rajnesh Kanwal <rkanwal@rivosinc.com>
Cc:     jirislaby@kernel.org, senozhatsky@chromium.org,
        rostedt@goodmis.org, john.ogness@linutronix.de,
        linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org
Subject: Re: [PATCH 1/1] kernel: Match preferred console first.
Message-ID: <Y+tziG0Uo5ey+Ocy@alley>
References: <20230213113912.1237943-1-rkanwal@rivosinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230213113912.1237943-1-rkanwal@rivosinc.com>
Precedence: bulk

On Mon 2023-02-13 11:39:12, Rajnesh Kanwal wrote:
> For cases where the driver registers the console with the index set to -1
> the `try_enable_preferred_console()` doesn't work as expected for multiple
> hvc consoles.
> 
> For example with `CONFIG_HVC_RISCV_SBI` and `CONFIG_VIRTIO_CONSOLE` enabled
> and cmdline set to "console=hvc0 console=hvc1" where hvc0 being RISCV_SBI
> and hvc1 being VIRTIO_CONSOLE (the preferred one as per the preferred
> selection logic in `__add_preferred_console`). hvc1 never gets enabled as hvc0
> matches first with the driver name "hvc" and index set to -1.
> 
> Here is the hvc console structure from hvc_console.c L217.
> static struct console hvc_console = {
>         .name           = "hvc",
>         .write          = hvc_console_print,
>         .device         = hvc_console_device,
>         .setup          = hvc_console_setup,
>         .flags          = CON_PRINTBUFFER,
>         .index          = -1,
> };
> 
> The expected flow is that hvc1 should take precedence but given hvc console has
> the index set to -1, the current logic will try to assign the first console that
> matches, which is hvc0 and return. Never touching other possible consoles.
> 
> This change makes sure the preferred console is checked first and then iterate
> the remaining list.
> 
> Signed-off-by: Rajnesh Kanwal <rkanwal@rivosinc.com>

Unfortunately, we could not do this. We have already been there, see
the commit dac8bbbae1d0ccba96402 ("Revert "printk: fix double printing
with earlycon") for more details.

It is sad. The patch would make perfect sense. But we could not take
it because people would report regressions.

I am afraid that we could only better document the existing behavior.

Proposed patch:

--- a/Documentation/admin-guide/serial-console.rst
+++ b/Documentation/admin-guide/serial-console.rst
@@ -33,8 +33,11 @@ The format of this option is::
 			9600n8. The maximum baudrate is 115200.
 
 You can specify multiple console= options on the kernel command line.
-Output will appear on all of them. The last device will be used when
-you open ``/dev/console``. So, for example::
+
+The behavior is well defined when each device type is mentioned
+only once. In this case, the output will appear on all requested
+consoles. And the last device will be used when you open
+``/dev/console``. So, for example::
 
 	console=ttyS1,9600 console=tty0
 
@@ -42,7 +45,29 @@ defines that opening ``/dev/console`` will get you the current foreground
 virtual console, and kernel messages will appear on both the VGA
 console and the 2nd serial port (ttyS1 or COM2) at 9600 baud.
 
-Note that you can only define one console per device type (serial, video).
+The behavior is more complicated when a device type is defined more
+times. In this case, there are the following two rules:
+
+1. The output will appear only on the first device of each defined type.
+
+2. ``/dev/console`` will be associated with the first registered device
+   when the last defined device is ignored because of the 1st rule.
+
+The result might be surprising. For example, the following two command
+lines have the same result:
+
+	console=ttyS1,9600 console=tty0 console=tty1
+	console=tty0 console=ttyS1,9600 console=tty1
+
+The kernel messages are printed only on ``tty0`` and ``ttyS1``. And
+``/dev/console`` gets associated with ``tty0``. It is because kernel
+tries to register graphical consoles before serial ones. It does it
+because of the default behavior when no console device is specified,
+see below.
+
+Note that the last ``console=tty1`` parameter still makes a difference.
+The kernel command line is used also by systemd. It would use the last
+defined ``tty1`` as the login console.
 
 If no console device is specified, the first device found capable of
 acting as a system console will be used. At this time, the system


Best Regards,
Petr