[1/1] kernel: Match preferred console first.

Commit Message

Rajnesh Kanwal Feb. 13, 2023, 11:39 a.m. UTC

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 <[e-mail]>
---
 drivers/tty/hvc/hvc_console.c |  2 +-
 kernel/printk/printk.c        | 78 ++++++++++++++++++++++++-----------
 2 files changed, 56 insertions(+), 24 deletions(-)

--
2.25.1

Comments

Petr Mladek Feb. 14, 2023, 11:42 a.m. UTC | #1

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 <[e-mail]>

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

Patch

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index a683e21df19c..f05bb92d9419 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -230,7 +230,7 @@  static struct console hvc_console = {
  * -- index will remain -1
  * (2) we are first and the user specified no driver
  * -- index will be set to 0, then we will fail setup.
- * (3)  we are first and the user specified our driver
+ * (3) we are first and the user specified our driver
  * -- index will be set to user specified driver, and we will fail
  * (4) we are after driver, and this initcall will register us
  * -- if the user didn't specify a driver then the console will match
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index a5ed2e53547c..3be3ace255a9 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3148,6 +3148,49 @@  static int __init keep_bootcon_setup(char *str)

 early_param("keep_bootcon", keep_bootcon_setup);

+/* Tries to enable newcon console device matching it with passed in
+ * console_cmdline. In case if match fails the function return -EAGAIN
+ * meaning the caller should try with the next console_cmdline.
+ */
+static int __try_enable_console(struct console *newcon,
+				struct console_cmdline *c, bool user_specified,
+				bool preferred)
+{
+	int err;
+
+	if (c->user_specified != user_specified)
+		return -EAGAIN;
+
+	if (!newcon->match ||
+	    newcon->match(newcon, c->name, c->index, c->options) != 0) {
+		/* default matching */
+		BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
+		if (strcmp(c->name, newcon->name) != 0)
+			return -EAGAIN;
+
+		if (newcon->index >= 0 && newcon->index != c->index)
+			return -EAGAIN;
+
+		if (newcon->index < 0)
+			newcon->index = c->index;
+
+		if (_braille_register_console(newcon, c))
+			return 0;
+
+		if (newcon->setup &&
+		    (err = newcon->setup(newcon, c->options)) != 0)
+			return err;
+
+		newcon->flags |= CON_ENABLED;
+		if (preferred)
+			newcon->flags |= CON_CONSDEV;
+
+		return 0;
+	}
+
+	return -EAGAIN;
+}
+
 /*
  * This is called by register_console() to try to match
  * the newly registered console with any of the ones selected
@@ -3163,34 +3206,23 @@  static int try_enable_preferred_console(struct console *newcon,
 	struct console_cmdline *c;
 	int i, err;

+	/* Try to match preferred console first. */
+	if (preferred_console >= 0) {
+		c = &console_cmdline[preferred_console];
+		err = __try_enable_console(newcon, c, user_specified, true);
+		if (err != EAGAIN)
+			return err;
+	}
+
 	for (i = 0, c = console_cmdline;
 	     i < MAX_CMDLINECONSOLES && c->name[0];
 	     i++, c++) {
-		if (c->user_specified != user_specified)
+		if (i == preferred_console)
 			continue;
-		if (!newcon->match ||
-		    newcon->match(newcon, c->name, c->index, c->options) != 0) {
-			/* default matching */
-			BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
-			if (strcmp(c->name, newcon->name) != 0)
-				continue;
-			if (newcon->index >= 0 &&
-			    newcon->index != c->index)
-				continue;
-			if (newcon->index < 0)
-				newcon->index = c->index;
-
-			if (_braille_register_console(newcon, c))
-				return 0;

-			if (newcon->setup &&
-			    (err = newcon->setup(newcon, c->options)) != 0)
-				return err;
-		}
-		newcon->flags |= CON_ENABLED;
-		if (i == preferred_console)
-			newcon->flags |= CON_CONSDEV;
-		return 0;
+		err = __try_enable_console(newcon, c, user_specified, false);
+		if (err != EAGAIN)
+			return err;
 	}

 	/*