All of lore.kernel.org
 help / color / mirror / Atom feed
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.
Date: Tue, 14 Feb 2023 12:42:00 +0100	[thread overview]
Message-ID: <Y+tziG0Uo5ey+Ocy@alley> (raw)
In-Reply-To: <20230213113912.1237943-1-rkanwal@rivosinc.com>

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

WARNING: multiple messages have this Message-ID (diff)
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.
Date: Tue, 14 Feb 2023 12:42:00 +0100	[thread overview]
Message-ID: <Y+tziG0Uo5ey+Ocy@alley> (raw)
In-Reply-To: <20230213113912.1237943-1-rkanwal@rivosinc.com>

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2023-02-14 11:42 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-13 11:39 [PATCH 1/1] kernel: Match preferred console first Rajnesh Kanwal
2023-02-13 11:39 ` Rajnesh Kanwal
2023-02-14 11:42 ` Petr Mladek [this message]
2023-02-14 11:42   ` Petr Mladek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y+tziG0Uo5ey+Ocy@alley \
    --to=pmladek@suse.com \
    --cc=jirislaby@kernel.org \
    --cc=john.ogness@linutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=rkanwal@rivosinc.com \
    --cc=rostedt@goodmis.org \
    --cc=senozhatsky@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.