[RFC/PATCH] printk: Fix preferred console selection with multiple matches

[RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Benjamin Herrenschmidt]

In the following circumstances, the rule of selecting the console
corresponding to the last "console=" entry on the command line as
the preferred console (CON_CONSDEV, ie, /dev/console) fails. This
is a specific example, but it could happen with different consoles
that have a similar name aliasing mechanism.

 - The kernel command line has both console=tty0 and console=ttyS0
in that order (the latter with speed etc... arguments). This is common
with some cloud setups such as Amazon Linux.

 - add_preferred_console is called early to register "uart0". In
our case that happens from acpi_parse_spcr() on arm64 since the
"enable_console" argument is true on that architecture. This causes
"uart0" to become entry 0 of the console_cmdline array.

Now, because of the above, what happens is:

 - add_preferred_console is called by the cmdline parsing for tty0
and ttyS0 respectively, thus occupying entries 1 and 2 of the
console_cmdline array (since this happens after ACPI SPCR parsing).
At that point preferred_console is set to 2 as expected.

 - When the tty layer kicks in, it will call register_console for tty0.
This will match entry 1 in console_cmdline array. It isn't our preferred
console but because it's our only console at this point, it will end up
"first" in the consoles list.

 - When 8250 probes the actual serial port later on, it calls
register_console for ttyS0. At that point the loop in register_console
tries to match it with the entries in the console_cmdline array. Ideally
this should match ttyS0 in entry 2, which is preferred, causing it to
be inserted first and to replace tty0 as CONSDEV. However, 8250 provides
a "match" hook in its struct console, and that hook will match "uart"
as an alias to "ttyS". So we match uart0 at entry 0 in the array which
is not the preferred console and will not match entry 2 which is since
we break out of the loop on the first match. As a result, we don't set
CONSDEV and don't insert it first, but second in the console list.

As a result, we end up with tty0 remaining first in the array, and thus
/dev/console going there instead of the last user specified one which
is ttyS0.

This tentative fix changes the loop in register_console to continue
matching with the array until the match is actually the preferred
console.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 kernel/printk/printk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 5aa96098c64d..d36b9901c0e0 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2646,8 +2646,8 @@ void register_console(struct console *newcon)
 		if (i == preferred_console) {
 			newcon->flags |= CON_CONSDEV;
 			has_preferred = true;
+			break;
 		}
-		break;
 	}
 
 	if (!(newcon->flags & CON_ENABLED))
-- 
2.17.1

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Sergey Senozhatsky]

On (19/12/10 11:57), Benjamin Herrenschmidt wrote:
[..]
>  - add_preferred_console is called early to register "uart0". In
> our case that happens from acpi_parse_spcr() on arm64 since the
> "enable_console" argument is true on that architecture. This causes
> "uart0" to become entry 0 of the console_cmdline array.

Hmm, two independent console list configuration sources.

[..]
> +++ b/kernel/printk/printk.c
> @@ -2646,8 +2646,8 @@ void register_console(struct console *newcon)
>  		if (i == preferred_console) {
>  			newcon->flags |= CON_CONSDEV;
>  			has_preferred = true;
> +			break;
>  		}
> -		break;
>  	}
>  
>  	if (!(newcon->flags & CON_ENABLED))

Wouldn't this, basically, mean that we want to match only consoles,
which were in the kernel's console= cmdline? IOW, ignore consoles
that were placed into consoles list via alternative path - ACPI.

Hmm.

The patch may affect setups where alias matching is expected to
happen. E.g.:

	console=uartFOO,BAR

Is 8250 the only console that does alias matching?

	-ss

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Petr Mladek]

On Tue 2019-12-10 11:57:26, Benjamin Herrenschmidt wrote:
> In the following circumstances, the rule of selecting the console
> corresponding to the last "console=" entry on the command line as
> the preferred console (CON_CONSDEV, ie, /dev/console) fails. This
> is a specific example, but it could happen with different consoles
> that have a similar name aliasing mechanism.
> 
> This tentative fix changes the loop in register_console to continue
> matching with the array until the match is actually the preferred
> console.

One problem with this is that con->match() callbacks might have
side effects. If the console matches, the callback sometimes
do some changes in the console driver because it expects
that the console is going to be registered and used.

I have solved the same problem some time ago and we use the following
patch in SUSE kernels.

Sigh, I have never send it upstream because it looked too complicated
to me. I wanted to clean up the console registration code a bit first,
see https://lkml.kernel.org/r/1497358444-30736-1-git-send-email-pmladek@suse.com
But it was pretty complicated and it has somehow fallen into cracks.


Anyway, here is the patch that we use. Could you please check if it
works for you as well? Does it make sense, please?

From: Petr Mladek <pmladek@suse.com>
Date: Tue, 20 Jun 2017 14:40:34 +0200
Subject: printk/console: Correctly mark console that is used when opening /dev/console
Patch-mainline: Never, an extensive clean up is being prepared for upstream
References: bsc#1040020

showconsole tool shows the real name of tty device associated with
/dev/console. It expects that the related console driver has set
CON_CONSDEV flag.

On the other hand, kernel ignores CON_CONSDEV flag when it looks
for the right driver. Instead, it takes the first driver that
has the tty binding (console->device). See tty_lookup_driver()
and console_device().

All this works most of the time because kernel puts the driver
with CON_CONSDEV flag first into the list. There is almost always
registered a real (non-boot) console with this flag set. The real
consoles mostly (always?) have tty binding. Boot consoles that
might miss the tty binding are always removed unless keep_bootcon
command line parameter is used.

The problem is when some consoles are defined on the command line
and the preferred one (last one) is not registered from some reason.
Note that the consoles might be added to the command line also
using ACPI SPCR or device tree. It might happen that, for example,
SPCR code and user add the same console using two aliases.
Then the first alias matches and we might miss that it matched
also with the preferred console.

There was one attempt to fix this by searching the command line
from the end and match the preferred alias first. But it caused
regressions. For example, ttyS* are taken as aliases as well
and kernel messages can appear only on one serial port.
The reversed matching caused that the logs suddenly appeared
on another serial port.

The right solution is to set CON_CONSDEV flag for the driver
used by tty_lookup_driver() even when the preferred console
is not registered.

It is a bit complicated because register_console() code is tricky.
It expects that only the preferred driver will have CON_CONSDEV
flag set. Also it expects that a boot console will stay first
in the list until the preferred console is registered. These
information are used to make various decisions:

    + Use a fallback code when none console is configured on
      the command line. This code tries to enable any console
      until a real one is enabled.

    + Unregister all boot consoles when the real preferred one
      is registered. And do not reply the log on the real console
      to avoid duplicates.

A rather invasive clean up is being prepared for upstream. This
patch tries to be as minimalist and do not change the order
of consoles as possible.

It keeps the logic about having a boot console first until
the real preferred console is registered. But it makes sure
that the first console with tty binding (console->device) will
have CON_CONSDEV flag set. Let's look at this in more details.

The fallback code in console_register() already works as
expected. It sets CON_CONSDEV flag for any console with
tty binding.

The code matching all consoles from the command line newly sets
CON_CONSDEV flag also for the fist console with the tty binding.
But it sets "consdev_fallback" to avoid putting this console
first into the list. Remember that we want to keep the boot
console first until the preferred is registered. The information
about the fallback is used also to avoid doing other actions
that need to wait for the preferred console.

The code adding the console into the list of drivers must
put non-preferred drivers with tty binding next to the
console with CON_CONSDEV set. This is the only change that
might change the order of console drivers in the list
and eventually cause regressions. But it has an effect only
when there are at least three drivers mentioned on the command
line, a boot console is registered and the preferred driver
is not registered. This should be a corner case.

Finally, unregister_console() sets CON_CONSDEV to first console
with tty binding instead of the first one in the list.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 59 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 9 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 94ec1aacea64..b6bb4d362b22 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2662,16 +2662,23 @@ void register_console(struct console *newcon)
 	int i;
 	unsigned long flags;
 	struct console *bcon = NULL;
+	struct console *con_consdev = NULL;
 	struct console_cmdline *c;
 	static bool has_preferred;
+	bool consdev_fallback = false;
 
-	if (console_drivers)
-		for_each_console(bcon)
+	if (console_drivers) {
+		for_each_console(bcon) {
 			if (WARN(bcon == newcon,
 					"console '%s%d' already registered\n",
 					bcon->name, bcon->index))
 				return;
 
+			if (bcon->flags & CON_CONSDEV && !con_consdev)
+				con_consdev = bcon;
+		}
+	}
+
 	/*
 	 * before we register a new CON_BOOT console, make sure we don't
 	 * already have a valid console
@@ -2739,8 +2746,17 @@ void register_console(struct console *newcon)
 
 		newcon->flags |= CON_ENABLED;
 		if (i == preferred_console) {
+			/* This is the last console on the command line. */
 			newcon->flags |= CON_CONSDEV;
 			has_preferred = true;
+		} else if (newcon->device && !con_consdev) {
+			/*
+			 * This is the first console with tty binding. It will
+			 * be used for /dev/console when the preferred one
+			 * will not get registered for some reason.
+			 */
+			newcon->flags |= CON_CONSDEV;
+			consdev_fallback = true;
 		}
 		break;
 	}
@@ -2754,7 +2770,9 @@ void register_console(struct console *newcon)
 	 * the real console are the same physical device, it's annoying to
 	 * see the beginning boot messages twice
 	 */
-	if (bcon && ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV))
+	if (bcon &&
+	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
+	    !consdev_fallback)
 		newcon->flags &= ~CON_PRINTBUFFER;
 
 	/*
@@ -2762,12 +2780,28 @@ void register_console(struct console *newcon)
 	 *	preferred driver at the head of the list.
 	 */
 	console_lock();
-	if ((newcon->flags & CON_CONSDEV) || console_drivers == NULL) {
+	if ((newcon->flags & CON_CONSDEV && !consdev_fallback) ||
+	     console_drivers == NULL) {
+		/* Put the preferred or the first console at the head. */
 		newcon->next = console_drivers;
 		console_drivers = newcon;
-		if (newcon->next)
-			newcon->next->flags &= ~CON_CONSDEV;
+		/* Only one console can have CON_CONSDEV flag set */
+		if (con_consdev)
+			con_consdev->flags &= ~CON_CONSDEV;
+	} else if (newcon->device && con_consdev) {
+		/*
+		 * Keep the driver associated with /dev/console.
+		 * We are here only when the console was enabled by the cycle
+		 * checking console_cmdline and this is neither preferred
+		 * console nor the consdev fallback.
+		 */
+		newcon->next = con_consdev->next;
+		con_consdev->next = newcon;
 	} else {
+		/*
+		 * Keep a boot console first until the preferred real one
+		 * is registered.
+		 */
 		newcon->next = console_drivers->next;
 		console_drivers->next = newcon;
 	}
@@ -2808,6 +2842,7 @@ void register_console(struct console *newcon)
 		newcon->name, newcon->index);
 	if (bcon &&
 	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
+	    !consdev_fallback &&
 	    !keep_bootcon) {
 		/* We need to iterate through all boot consoles, to make
 		 * sure we print everything out, before we unregister them.
@@ -2853,10 +2888,16 @@ int unregister_console(struct console *console)
 
 	/*
 	 * If this isn't the last console and it has CON_CONSDEV set, we
-	 * need to set it on the next preferred console.
+	 * need to set it on the first console with tty binding.
 	 */
-	if (console_drivers != NULL && console->flags & CON_CONSDEV)
-		console_drivers->flags |= CON_CONSDEV;
+	if (console_drivers != NULL && console->flags & CON_CONSDEV) {
+		for_each_console(a) {
+			if (a->device) {
+				a->flags |= CON_CONSDEV;
+				break;
+			}
+		}
+	}
 
 	console->flags &= ~CON_ENABLED;
 	console_unlock();
-- 
1.8.5.6

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Benjamin Herrenschmidt]

On Tue, 2019-12-10 at 17:01 +0900, Sergey Senozhatsky wrote:
> On (19/12/10 11:57), Benjamin Herrenschmidt wrote:
> [..]
> >  - add_preferred_console is called early to register "uart0". In
> > our case that happens from acpi_parse_spcr() on arm64 since the
> > "enable_console" argument is true on that architecture. This causes
> > "uart0" to become entry 0 of the console_cmdline array.
> 
> Hmm, two independent console list configuration sources.

Yes, we've had that for a while. So far it "worked" because the
explicit calls to add_preferred_console() tends to happen before the
parsing of the command line, and thus are "overriden" by the latter if
any.

> [..]
> > +++ b/kernel/printk/printk.c
> > @@ -2646,8 +2646,8 @@ void register_console(struct console *newcon)
> >  		if (i == preferred_console) {
> >  			newcon->flags |= CON_CONSDEV;
> >  			has_preferred = true;
> > +			break;
> >  		}
> > -		break;
> >  	}
> >  
> >  	if (!(newcon->flags & CON_ENABLED))
> 
> Wouldn't this, basically, mean that we want to match only consoles,
> which were in the kernel's console= cmdline? IOW, ignore consoles
> that were placed into consoles list via alternative path - ACPI.

No not exactly. Architectures/platforms use add_preferred_console()
(such as arm64 with ACPI but powerpc at least does it too) based on
various factors to select a reasonable "default" for that specific
platform. Without that the kernel will basically default to the first
one to register which may not be what you want.

The command line ones however want to override the defaults (provided
they exist, ie, it's possible that whever is specified on the command
line doesn't actually exist, and thus shall be ignored. That typically
happens when there is either no match or ->setup fails).

> Hmm.
> 
> The patch may affect setups where alias matching is expected to
> happen. E.g.:
> 
> 	console=uartFOO,BAR
> 
> Is 8250 the only console that does alias matching?

Why would the patch affect this negatively ? Today we stop on the first
match, mark the driver enabled, and make it preferred if the match
index matches preferred_console.

My patch makes us continue searching if it wasnt' the preferred console
(but we still mark it enabled). Which means either of those two things
happen:

 - No more match or another match that isn't the preferred_console,
this won't change the existing behaviour.

 - Another match that is marked preferred_console, in which case in
addition to being enabled, the newly registered console will also be
made the default console (ie, first in the list with CONSDEV set). This
is actually what we want ! IE. The console matches the last specified
one on the command line.

Cheers,
Ben.

> 	-ss

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Benjamin Herrenschmidt]

On Tue, 2019-12-10 at 10:15 +0100, Petr Mladek wrote:
> On Tue 2019-12-10 11:57:26, Benjamin Herrenschmidt wrote:
> > In the following circumstances, the rule of selecting the console
> > corresponding to the last "console=" entry on the command line as
> > the preferred console (CON_CONSDEV, ie, /dev/console) fails. This
> > is a specific example, but it could happen with different consoles
> > that have a similar name aliasing mechanism.
> > 
> > This tentative fix changes the loop in register_console to continue
> > matching with the array until the match is actually the preferred
> > console.
> 
> One problem with this is that con->match() callbacks might have
> side effects. If the console matches, the callback sometimes
> do some changes in the console driver because it expects
> that the console is going to be registered and used.

It will still be enabled. I am not changing that. The main issue would
be if the match callback chokes on being called multiple times.

IE. The only change in behaviour I think with my patch is who gets to
be the default, ie who gets to be first in the list with CONSDEV set.
There should be no change in who gets enabled.

> I have solved the same problem some time ago and we use the following
> patch in SUSE kernels.
> 
> Sigh, I have never send it upstream because it looked too complicated
> to me. I wanted to clean up the console registration code a bit first,
> see https://lkml.kernel.org/r/1497358444-30736-1-git-send-email-pmladek@suse.com
> But it was pretty complicated and it has somehow fallen into cracks.

This looks indeed a lot more invasive. Is there any reason why what I
propose wouldn't work as a first patch that can easily be backported ?

I don't see how it would break anything but I haven't necessarily fully
understood everything the driver match callbacks might be doing...

We can continue cleaning up from there of course, but I'd be keen on
having a minimal fix that can go back to stable first.

> Anyway, here is the patch that we use. Could you please check if it
> works for you as well? Does it make sense, please?

I'll give it a spin. However I don't fully grasp why it's necessarily
so complicated. Correct me if I'm wrong here but you are trying to
address two issues in that patch:

 - The one I'm trying to address which is that we might "miss" the
preferred console in the case of multiple matches.

 - The fact that when the preferred console isn't found, the one we
default to (which ends up first in the list) is missing CONSDEV.

Or am I missing something here ?

Now couldnt we just use a combination of my patch and one that sets
CONSDEV on the first enabled console if not set at the end of
register_console ?

If later on the preferred one comes in, it will be inserted first with
CONSDEV and the flag will be removed from the previous first unless I
misread the code.

Cheers,
Ben.

> From: Petr Mladek <pmladek@suse.com>
> Date: Tue, 20 Jun 2017 14:40:34 +0200
> Subject: printk/console: Correctly mark console that is used when opening /dev/console
> Patch-mainline: Never, an extensive clean up is being prepared for upstream
> References: bsc#1040020
> 
> showconsole tool shows the real name of tty device associated with
> /dev/console. It expects that the related console driver has set
> CON_CONSDEV flag.
> 
> On the other hand, kernel ignores CON_CONSDEV flag when it looks
> for the right driver. Instead, it takes the first driver that
> has the tty binding (console->device). See tty_lookup_driver()
> and console_device().
> 
> All this works most of the time because kernel puts the driver
> with CON_CONSDEV flag first into the list. There is almost always
> registered a real (non-boot) console with this flag set. The real
> consoles mostly (always?) have tty binding. Boot consoles that
> might miss the tty binding are always removed unless keep_bootcon
> command line parameter is used.
> 
> The problem is when some consoles are defined on the command line
> and the preferred one (last one) is not registered from some reason.
> Note that the consoles might be added to the command line also
> using ACPI SPCR or device tree. It might happen that, for example,
> SPCR code and user add the same console using two aliases.
> Then the first alias matches and we might miss that it matched
> also with the preferred console.
> 
> There was one attempt to fix this by searching the command line
> from the end and match the preferred alias first. But it caused
> regressions. For example, ttyS* are taken as aliases as wellhttps://lkml.kernel.org/r/1497358444-30736-1-git-send-email-pmladek@suse.com
> and kernel messages can appear only on one serial port.
> The reversed matching caused that the logs suddenly appeared
> on another serial port.
> 
> The right solution is to set CON_CONSDEV flag for the driver
> used by tty_lookup_driver() even when the preferred console
> is not registered.
> 
> It is a bit complicated because register_console() code is tricky.
> It expects that only the preferred driver will have CON_CONSDEV
> flag set. Also it expects that a boot console will stay firsthttps://lkml.kernel.org/r/1497358444-30736-1-git-send-email-pmladek@suse.com
> in the list until the preferred console is registered. These
> information are used to make various decisions:
> 
>     + Use a fallback code when none console is configured on
>       the command line. This code tries to enable any console
>       until a real one is enabled.
> 
>     + Unregister all boot consoles when the real preferred one
>       is registered. And do not reply the log on the real console
>       to avoid duplicates.
> 
> A rather invasive clean up is being prepared for upstream. This
> patch tries to be as minimalist and do not change the order
> of consoles as possible.
> 
> It keeps the logic about having a boot console first until
> the real preferred console is registered. But it makes sure
> that the first console with tty binding (console->device) will
> have CON_CONSDEV flag set. Let's look at this in more details.
> 
> The fallback code in console_register() already works as
> expected. It sets CON_CONSDEV flag for any console with
> tty binding.
> 
> The code matching all consoles from the command line newly sets
> CON_CONSDEV flag also for the fist console with the tty binding.
> But it sets "consdev_fallback" to avoid putting this console
> first into the list. Remember that we want to keep the boot
> console first until the preferred is registered. The information
> about the fallback is used also to avoid doing other actions
> that need to wait for the preferred console.
> 
> The code adding the console into the list of drivers must
> put non-preferred drivers with tty binding next to the
> console with CON_CONSDEV set. This is the only change that
> might change the order of console drivers in the list
> and eventually cause regressions. But it has an effect only
> when there are at least three drivers mentioned on the command
> line, a boot console is registered and the preferred driver
> is not registered. This should be a corner case.
> 
> Finally, unregister_console() sets CON_CONSDEV to first console
> with tty binding instead of the first one in the list.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/printk/printk.c | 59 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 94ec1aacea64..b6bb4d362b22 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2662,16 +2662,23 @@ void register_console(struct console *newcon)
>  	int i;
>  	unsigned long flags;
>  	struct console *bcon = NULL;
> +	struct console *con_consdev = NULL;
>  	struct console_cmdline *c;
>  	static bool has_preferred;
> +	bool consdev_fallback = false;
>  
> -	if (console_drivers)
> -		for_each_console(bcon)
> +	if (console_drivers) {
> +		for_each_console(bcon) {
>  			if (WARN(bcon == newcon,
>  					"console '%s%d' already registered\n",
>  					bcon->name, bcon->index))
>  				return;
>  
> +			if (bcon->flags & CON_CONSDEV && !con_consdev)
> +				con_consdev = bcon;
> +		}
> +	}
> +
>  	/*
>  	 * before we register a new CON_BOOT console, make sure we don't
>  	 * already have a valid console
> @@ -2739,8 +2746,17 @@ void register_console(struct console *newcon)
>  
>  		newcon->flags |= CON_ENABLED;
>  		if (i == preferred_console) {
> +			/* This is the last console on the command line. */
>  			newcon->flags |= CON_CONSDEV;
>  			has_preferred = true;
> +		} else if (newcon->device && !con_consdev) {
> +			/*
> +			 * This is the first console with tty binding. It will
> +			 * be used for /dev/console when the preferred one
> +			 * will not get registered for some reason.
> +			 */
> +			newcon->flags |= CON_CONSDEV;
> +			consdev_fallback = true;
>  		}
>  		break;
>  	}
> @@ -2754,7 +2770,9 @@ void register_console(struct console *newcon)
>  	 * the real console are the same physical device, it's annoying to
>  	 * see the beginning boot messages twice
>  	 */
> -	if (bcon && ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV))
> +	if (bcon &&
> +	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
> +	    !consdev_fallback)
>  		newcon->flags &= ~CON_PRINTBUFFER;
>  
>  	/*
> @@ -2762,12 +2780,28 @@ void register_console(struct console *newcon)
>  	 *	preferred driver at the head of the list.
>  	 */
>  	console_lock();
> -	if ((newcon->flags & CON_CONSDEV) || console_drivers == NULL) {
> +	if ((newcon->flags & CON_CONSDEV && !consdev_fallback) ||
> +	     console_drivers == NULL) {
> +		/* Put the preferred or the first console at the head. */
>  		newcon->next = console_drivers;
>  		console_drivers = newcon;
> -		if (newcon->next)
> see 
> https://lkml.kernel.org/r/1497358444-30736-1-git-send-email-pmladek@suse.com>
> ; But it was pretty complicated and it has somehow fallen into
> cracks.
> This looks indeed a lot more invasive. Is there any reason why what I
> propose wouldn't work as a first patch that can easily be backported
> ?
> I don't see how it would break anything but I haven't necessarily
> fully understood everything the driver match callbacks might be
> doing...
> We can continue cleaning up from there of course, but I'd be keen on
> having a minimal fix that can go back to stable first.
> > Anyway, here is the patch that we use. Could you please check if
> it> works for you as well? Does it make sense, please?
> I'll give it a spin. Some comments on it at first look though:> 
> 
> -			newcon->next->flags &= ~CON_CONSDEV;
> +		/* Only one console can have CON_CONSDEV flag set */
> +		if (con_consdev)
> +			con_consdev->flags &= ~CON_CONSDEV;
> +	} else if (newcon->device && con_consdev) {
> +		/*
> +		 * Keep the driver associated with /dev/console.
> +		 * We are here only when the console was enabled by the cycle
> +		 * checking console_cmdline and this is neither preferred
> +		 * console nor the consdev fallback.
> +		 */
> +		newcon->next = con_consdev->next;
> +		con_consdev->next = newcon;
>  	} else {
> +		/*
> +		 * Keep a boot console first until the preferred real one
> +		 * is registered.
> +		 */
>  		newcon->next = console_drivers->next;
>  		console_drivers->next = newcon;
>  	}
> @@ -2808,6 +2842,7 @@ void register_console(struct console *newcon)
>  		newcon->name, newcon->index);
>  	if (bcon &&
>  	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
> +	    !consdev_fallback &&
>  	    !keep_bootcon) {
>  		/* We need to iterate through all boot consoles, to make
>  		 * sure we print everything out, before we unregister them.
> @@ -2853,10 +2888,16 @@ int unregister_console(struct console *console)
>  
>  	/*
>  	 * If this isn't the last console and it has CON_CONSDEV set, we
> -	 * need to set it on the next preferred console.
> +	 * need to set it on the first console with tty binding.
>  	 */
> -	if (console_drivers != NULL && console->flags & CON_CONSDEV)
> -		console_drivers->flags |= CON_CONSDEV;
> +	if (console_drivers != NULL && console->flags & CON_CONSDEV) {
> +		for_each_console(a) {
> +			if (a->device) {
> +				a->flags |= CON_CONSDEV;
> +				break;
> +			}
> +		}
> +	}
>  
>  	console->flags &= ~CON_ENABLED;
>  	console_unlock();

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Sergey Senozhatsky]

On (19/12/11 09:26), Benjamin Herrenschmidt wrote:
[..]
> No not exactly. Architectures/platforms use add_preferred_console()
> (such as arm64 with ACPI but powerpc at least does it too) based on
> various factors to select a reasonable "default" for that specific
> platform. Without that the kernel will basically default to the first
> one to register which may not be what you want.
> 
> The command line ones however want to override the defaults (provided
> they exist, ie, it's possible that whever is specified on the command
> line doesn't actually exist, and thus shall be ignored. That typically
> happens when there is either no match or ->setup fails).
> 
> > Hmm.
> > 
> > The patch may affect setups where alias matching is expected to
> > happen. E.g.:
> > 
> > 	console=uartFOO,BAR
> > 
> > Is 8250 the only console that does alias matching?
> 
> Why would the patch affect this negatively ? Today we stop on the first
> match, mark the driver enabled, and make it preferred if the match
> index matches preferred_console.

As far as I know, ->match() does not only match but also does ->setup().
If we have two console list entries that match (one via aliasing and one
via exact match) then the console driver is setup twice. Do all console
drivers handle it? [double setup]

If we could perform simple alias matching, without ->setup() call, and
exact matching (strcmp()), and then, if newcon would match two entries,
we would pick up the last matching entry and configure newcon only once.

This changes the order, tho.

[..]
>  - Another match that is marked preferred_console, in which case in
> addition to being enabled, the newly registered console will also be
> made the default console (ie, first in the list with CONSDEV set). This
> is actually what we want ! IE. The console matches the last specified
> one on the command line.

Well, it still looks to me that what you want is to "ignore alias
match and prefer exact match".

	-ss

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Benjamin Herrenschmidt]

On Wed, 2019-12-11 at 11:01 +0900, Sergey Senozhatsky wrote:
> On (19/12/11 09:26), Benjamin Herrenschmidt wrote:
> 

> As far as I know, ->match() does not only match but also does ->setup().
> If we have two console list entries that match (one via aliasing and one
> via exact match) then the console driver is setup twice. Do all console
> drivers handle it? [double setup]

I don't think it's an issue but I may be wrong. I had a quick look
at some of the drivers and I don't really see why they would break but
I couldn't look at them all and I might be mistaken.

We could skip setup if the console is already enabled but I would
advise against that since the two calls might have different options
(the firmware baud rate could be different from the command line one
for example) and we want the options for the last one.

> If we could perform simple alias matching, without ->setup() call, and
> exact matching (strcmp()), and then, if newcon would match two entries,
> we would pick up the last matching entry and configure newcon only once.
> 
> This changes the order, tho.

Walking the array backwards might just be what we want actually for the
case at hand, but of course if some platforms or driver call
add_preferred_console *after* the command line parsing, then it will
break those.

Simple alias matching would require re-working all the match()
callbacks. That said I think it was a mistake to begin with to have
them include setup(). Those should have remained separate.

What about a compromise here:

Instead of walking the array and testing for preferred_console as we do
so, we first test the array entry pointed to by preferred_console
(doing both match & setup as today) and if that doesn't work, fallback
to our existing mechanism ?

It would be a first step. It wouldn't fix all cases but would be
something reasonable to backport.

Another approach woudl be to pass to add_preferred_console an argument
"bool user_specified" which we would use to set a CON_USER flag.

We could then do a two-pass lookup of the array where we first only
match user specified entries, then match the rest.

> [..]
> >  - Another match that is marked preferred_console, in which case in
> > addition to being enabled, the newly registered console will also be
> > made the default console (ie, first in the list with CONSDEV set). This
> > is actually what we want ! IE. The console matches the last specified
> > one on the command line.
> 
> Well, it still looks to me that what you want is to "ignore alias
> match and prefer exact match".

We don't want to ignore the alias match. But we do want to prefer the
exact match. We still want to keep the fallback to the alias match.

Cheers,
Ben.

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Sergey Senozhatsky]

On (19/12/11 15:02), Benjamin Herrenschmidt wrote:
> On Wed, 2019-12-11 at 11:01 +0900, Sergey Senozhatsky wrote:
> > On (19/12/11 09:26), Benjamin Herrenschmidt wrote:
> > 
[..]
> > If we could perform simple alias matching, without ->setup() call, and
> > exact matching (strcmp()), and then, if newcon would match two entries,
> > we would pick up the last matching entry and configure newcon only once.
> > 
> > This changes the order, tho.
> 
> Walking the array backwards might just be what we want actually for the
> case at hand, but of course if some platforms or driver call
> add_preferred_console *after* the command line parsing, then it will
> break those.

Reverse loop sounds like a nice idea. But yes I guess this can break
things.

> Simple alias matching would require re-working all the match()
> callbacks. That said I think it was a mistake to begin with to have
> them include setup(). Those should have remained separate.

Agreed. strcmp(alias) and strcmp(exact name) are the same things.
The latter does "match" and setup() as separate steps, but the former
does both at once.

> What about a compromise here:
>
> Instead of walking the array and testing for preferred_console as we do
> so, we first test the array entry pointed to by preferred_console
> (doing both match & setup as today) and if that doesn't work, fallback
> to our existing mechanism ?

This may do the trick.
And perform preferred_console fast-path configuration only if
`has_preferred' is false.

> > Well, it still looks to me that what you want is to "ignore alias
> > match and prefer exact match".
>
> We don't want to ignore the alias match. But we do want to prefer the
> exact match. We still want to keep the fallback to the alias match.

Yeah, "prefer" is the key word here.

	-ss

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Petr Mladek]

On Wed 2019-12-11 09:39:06, Benjamin Herrenschmidt wrote:
> On Tue, 2019-12-10 at 10:15 +0100, Petr Mladek wrote:
> > On Tue 2019-12-10 11:57:26, Benjamin Herrenschmidt wrote:
> > > In the following circumstances, the rule of selecting the console
> > > corresponding to the last "console=" entry on the command line as
> > > the preferred console (CON_CONSDEV, ie, /dev/console) fails. This
> > > is a specific example, but it could happen with different consoles
> > > that have a similar name aliasing mechanism.
> > > 
> > > This tentative fix changes the loop in register_console to continue
> > > matching with the array until the match is actually the preferred
> > > console.
> > 
> > One problem with this is that con->match() callbacks might have
> > side effects. If the console matches, the callback sometimes
> > do some changes in the console driver because it expects
> > that the console is going to be registered and used.
> 
> It will still be enabled. I am not changing that. The main issue would
> be if the match callback chokes on being called multiple times.

And this would exactly happen as pointed out by Sergey. match() does
also some setup operations. I would be scared to call them twice.

> This looks indeed a lot more invasive. Is there any reason why what I
> propose wouldn't work as a first patch that can easily be backported ?

Your solution is not acceptable because it might cause calling
match() more times.

The reverse search of list of console does not work for ttySX
consoles because the number is omitted when matching. And the messages
will appear only on the first matched serial console. There is
a paragraph about this in the commit message of my patch.


> We can continue cleaning up from there of course, but I'd be keen on
> having a minimal fix that can go back to stable first.

I would be fine to take my patch as is and do the clean up later.
It seems that more users are affected by the problem. The clean up
might be complicated and prone to regressions.

The question is how far back you would like to go. The changes
in register_console() are rare. Everybody is scared to touch
this mess ;-) And it is prone to regressions.

> I'll give it a spin. However I don't fully grasp why it's necessarily
> so complicated. Correct me if I'm wrong here but you are trying to
> address two issues in that patch:
> 
>  - The one I'm trying to address which is that we might "miss" the
> preferred console in the case of multiple matches.
> 
>  - The fact that when the preferred console isn't found, the one we
> default to (which ends up first in the list) is missing CONSDEV.

My patch is primary fixing this 2nd problem. It fixes showconsole
tool to work correctly. But it covers also the 1st one. It might
be possible to split it when needed. But I think that you could
not easily fix the 1st problem without solving the 2nd one.

The best solution would be to split the match() and setup
functionality. It would be really appreciated. But it is a lot
of work. And it might be hard to backport.

Best Regards,
Petr

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Petr Mladek]

On Wed 2019-12-11 15:02:02, Benjamin Herrenschmidt wrote:
> On Wed, 2019-12-11 at 11:01 +0900, Sergey Senozhatsky wrote:
> > On (19/12/11 09:26), Benjamin Herrenschmidt wrote:
> > 
> 
> > As far as I know, ->match() does not only match but also does ->setup().
> > If we have two console list entries that match (one via aliasing and one
> > via exact match) then the console driver is setup twice. Do all console
> > drivers handle it? [double setup]
> 
> I don't think it's an issue but I may be wrong. I had a quick look
> at some of the drivers and I don't really see why they would break but
> I couldn't look at them all and I might be mistaken.
> 
> We could skip setup if the console is already enabled but I would
> advise against that since the two calls might have different options
> (the firmware baud rate could be different from the command line one
> for example) and we want the options for the last one.

For example:

  + ip22zilog_console_setup() calls  __ip22zilog_startup() that does
    some initialization of the device

  + pl011_console_setup() does some non-trivial things as well.

Honestly, I am not familiar with these devices. I am not sure if it is
dangerous or safe to call these functions twice.

I am not sure if anybody would know this for sure. Therefore I suggest a
conservative approach and avoid calling setup() twice().

I think that it is dangerous and error-prone design. The best solution
would be to split setup() and match() functionality.

Best Regards,
Petr

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Benjamin Herrenschmidt]

On Tue, 2019-12-10 at 10:15 +0100, Petr Mladek wrote:
> 
> Anyway, here is the patch that we use. Could you please check if it
> works for you as well? Does it make sense, please?

It doesn't fix my problem. tty0 remains the default console instead
of ttyS0 with your patch applied.

I suspect for the same reason, we match uart0 which isn't preferred,
so we enable that but don't put it "first" in the list, and since
we break out of the loop we never match ttyS0.

I see 3 simple ways out of this that don't involve breaking up match()

 - Bite the bullet and use my patch assuming that calling setup()
multiple times is safe. I had a look at the two you had concerns
with, the zilog ones seems safe. pl1011 will leak a clk_prepare
reference but I think that's a non-issue for a kernel console
(I may be wrong)

 - Rework the loop to try matching against the array entry pointed
by preferred_console first.

 - Rework the loop to try matching the entries from the command line
before trying to match the entries added by the platform/arch.
(Easily done by flagging them in the array, I can cook a patch).

Let me know what you prefer or if you have a different idea and
I'll try to whip up a patch tomorrow.

Cheers,
Ben.


> From: Petr Mladek <pmladek@suse.com>
> Date: Tue, 20 Jun 2017 14:40:34 +0200
> Subject: printk/console: Correctly mark console that is used when opening /dev/console
> Patch-mainline: Never, an extensive clean up is being prepared for upstream
> References: bsc#1040020
> 
> showconsole tool shows the real name of tty device associated with
> /dev/console. It expects that the related console driver has set
> CON_CONSDEV flag.
> 
> On the other hand, kernel ignores CON_CONSDEV flag when it looks
> for the right driver. Instead, it takes the first driver that
> has the tty binding (console->device). See tty_lookup_driver()
> and console_device().
> 
> All this works most of the time because kernel puts the driver
> with CON_CONSDEV flag first into the list. There is almost always
> registered a real (non-boot) console with this flag set. The real
> consoles mostly (always?) have tty binding. Boot consoles that
> might miss the tty binding are always removed unless keep_bootcon
> command line parameter is used.
> 
> The problem is when some consoles are defined on the command line
> and the preferred one (last one) is not registered from some reason.
> Note that the consoles might be added to the command line also
> using ACPI SPCR or device tree. It might happen that, for example,
> SPCR code and user add the same console using two aliases.
> Then the first alias matches and we might miss that it matched
> also with the preferred console.
> 
> There was one attempt to fix this by searching the command line
> from the end and match the preferred alias first. But it caused
> regressions. For example, ttyS* are taken as aliases as well
> and kernel messages can appear only on one serial port.
> The reversed matching caused that the logs suddenly appeared
> on another serial port.
> 
> The right solution is to set CON_CONSDEV flag for the driver
> used by tty_lookup_driver() even when the preferred console
> is not registered.
> 
> It is a bit complicated because register_console() code is tricky.
> It expects that only the preferred driver will have CON_CONSDEV
> flag set. Also it expects that a boot console will stay first
> in the list until the preferred console is registered. These
> information are used to make various decisions:
> 
>     + Use a fallback code when none console is configured on
>       the command line. This code tries to enable any console
>       until a real one is enabled.
> 
>     + Unregister all boot consoles when the real preferred one
>       is registered. And do not reply the log on the real console
>       to avoid duplicates.
> 
> A rather invasive clean up is being prepared for upstream. This
> patch tries to be as minimalist and do not change the order
> of consoles as possible.
> 
> It keeps the logic about having a boot console first until
> the real preferred console is registered. But it makes sure
> that the first console with tty binding (console->device) will
> have CON_CONSDEV flag set. Let's look at this in more details.
> 
> The fallback code in console_register() already works as
> expected. It sets CON_CONSDEV flag for any console with
> tty binding.
> 
> The code matching all consoles from the command line newly sets
> CON_CONSDEV flag also for the fist console with the tty binding.
> But it sets "consdev_fallback" to avoid putting this console
> first into the list. Remember that we want to keep the boot
> console first until the preferred is registered. The information
> about the fallback is used also to avoid doing other actions
> that need to wait for the preferred console.
> 
> The code adding the console into the list of drivers must
> put non-preferred drivers with tty binding next to the
> console with CON_CONSDEV set. This is the only change that
> might change the order of console drivers in the list
> and eventually cause regressions. But it has an effect only
> when there are at least three drivers mentioned on the command
> line, a boot console is registered and the preferred driver
> is not registered. This should be a corner case.
> 
> Finally, unregister_console() sets CON_CONSDEV to first console
> with tty binding instead of the first one in the list.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/printk/printk.c | 59 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 94ec1aacea64..b6bb4d362b22 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2662,16 +2662,23 @@ void register_console(struct console *newcon)
>  	int i;
>  	unsigned long flags;
>  	struct console *bcon = NULL;
> +	struct console *con_consdev = NULL;
>  	struct console_cmdline *c;
>  	static bool has_preferred;
> +	bool consdev_fallback = false;
>  
> -	if (console_drivers)
> -		for_each_console(bcon)
> +	if (console_drivers) {
> +		for_each_console(bcon) {
>  			if (WARN(bcon == newcon,
>  					"console '%s%d' already registered\n",
>  					bcon->name, bcon->index))
>  				return;
>  
> +			if (bcon->flags & CON_CONSDEV && !con_consdev)
> +				con_consdev = bcon;
> +		}
> +	}
> +
>  	/*
>  	 * before we register a new CON_BOOT console, make sure we don't
>  	 * already have a valid console
> @@ -2739,8 +2746,17 @@ void register_console(struct console *newcon)
>  
>  		newcon->flags |= CON_ENABLED;
>  		if (i == preferred_console) {
> +			/* This is the last console on the command line. */
>  			newcon->flags |= CON_CONSDEV;
>  			has_preferred = true;
> +		} else if (newcon->device && !con_consdev) {
> +			/*
> +			 * This is the first console with tty binding. It will
> +			 * be used for /dev/console when the preferred one
> +			 * will not get registered for some reason.
> +			 */
> +			newcon->flags |= CON_CONSDEV;
> +			consdev_fallback = true;
>  		}
>  		break;
>  	}
> @@ -2754,7 +2770,9 @@ void register_console(struct console *newcon)
>  	 * the real console are the same physical device, it's annoying to
>  	 * see the beginning boot messages twice
>  	 */
> -	if (bcon && ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV))
> +	if (bcon &&
> +	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
> +	    !consdev_fallback)
>  		newcon->flags &= ~CON_PRINTBUFFER;
>  
>  	/*
> @@ -2762,12 +2780,28 @@ void register_console(struct console *newcon)
>  	 *	preferred driver at the head of the list.
>  	 */
>  	console_lock();
> -	if ((newcon->flags & CON_CONSDEV) || console_drivers == NULL) {
> +	if ((newcon->flags & CON_CONSDEV && !consdev_fallback) ||
> +	     console_drivers == NULL) {
> +		/* Put the preferred or the first console at the head. */
>  		newcon->next = console_drivers;
>  		console_drivers = newcon;
> -		if (newcon->next)
> -			newcon->next->flags &= ~CON_CONSDEV;
> +		/* Only one console can have CON_CONSDEV flag set */
> +		if (con_consdev)
> +			con_consdev->flags &= ~CON_CONSDEV;
> +	} else if (newcon->device && con_consdev) {
> +		/*
> +		 * Keep the driver associated with /dev/console.
> +		 * We are here only when the console was enabled by the cycle
> +		 * checking console_cmdline and this is neither preferred
> +		 * console nor the consdev fallback.
> +		 */
> +		newcon->next = con_consdev->next;
> +		con_consdev->next = newcon;
>  	} else {
> +		/*
> +		 * Keep a boot console first until the preferred real one
> +		 * is registered.
> +		 */
>  		newcon->next = console_drivers->next;
>  		console_drivers->next = newcon;
>  	}
> @@ -2808,6 +2842,7 @@ void register_console(struct console *newcon)
>  		newcon->name, newcon->index);
>  	if (bcon &&
>  	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
> +	    !consdev_fallback &&
>  	    !keep_bootcon) {
>  		/* We need to iterate through all boot consoles, to make
>  		 * sure we print everything out, before we unregister them.
> @@ -2853,10 +2888,16 @@ int unregister_console(struct console *console)
>  
>  	/*
>  	 * If this isn't the last console and it has CON_CONSDEV set, we
> -	 * need to set it on the next preferred console.
> +	 * need to set it on the first console with tty binding.
>  	 */
> -	if (console_drivers != NULL && console->flags & CON_CONSDEV)
> -		console_drivers->flags |= CON_CONSDEV;
> +	if (console_drivers != NULL && console->flags & CON_CONSDEV) {
> +		for_each_console(a) {
> +			if (a->device) {
> +				a->flags |= CON_CONSDEV;
> +				break;
> +			}
> +		}
> +	}
>  
>  	console->flags &= ~CON_ENABLED;
>  	console_unlock();

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Sergey Senozhatsky]

On (19/12/11 10:17), Petr Mladek wrote:
> And this would exactly happen as pointed out by Sergey. match() does
> also some setup operations. I would be scared to call them twice.
> 
> > This looks indeed a lot more invasive. Is there any reason why what I
> > propose wouldn't work as a first patch that can easily be backported ?
> 
> Your solution is not acceptable because it might cause calling
> match() more times.
> 
> The reverse search of list of console does not work for ttySX
> consoles because the number is omitted when matching. And the messages
> will appear only on the first matched serial console. There is
> a paragraph about this in the commit message of my patch.

A fast path for preferred_console match()/setup() sounds like
something that may work. There won't be double setup()-s and
we scan console list in the same direction. We just have
separate match()/setup() for preferred_console. Do you think
this won't do the trick?

	-ss

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Petr Mladek]

On Thu 2019-12-12 11:35:23, Benjamin Herrenschmidt wrote:
> On Tue, 2019-12-10 at 10:15 +0100, Petr Mladek wrote:
> > 
> > Anyway, here is the patch that we use. Could you please check if it
> > works for you as well? Does it make sense, please?
> 
> It doesn't fix my problem. tty0 remains the default console instead
> of ttyS0 with your patch applied.

Sigh, I see.

> I suspect for the same reason, we match uart0 which isn't preferred,
> so we enable that but don't put it "first" in the list, and since
> we break out of the loop we never match ttyS0.

Yeah.

> I see 3 simple ways out of this that don't involve breaking up match()
> 
>  - Bite the bullet and use my patch assuming that calling setup()
> multiple times is safe. I had a look at the two you had concerns
> with, the zilog ones seems safe. pl1011 will leak a clk_prepare
> reference but I think that's a non-issue for a kernel console
> (I may be wrong)

This does not sound much convincing to me. Leaking reference
is an issue, definitely.


>  - Rework the loop to try matching against the array entry pointed
> by preferred_console first.

IMHO, in principle, we are trying to solve the same problem as
the commit cf39bf58afdaabc0b86f141 ("printk: fix double
printing with earlycon").

And it was reverted because it broke some setups, see
the commit dac8bbbae1d0ccba96402d25d ("Revert "printk: fix double
printing with earlycon").

Trying only the preferred console first is less invasive but
it might cause exactly the same regression.


>  - Rework the loop to try matching the entries from the command line
> before trying to match the entries added by the platform/arch.
> (Easily done by flagging them in the array, I can cook a patch).

This makes some sense. It would allow user to override the fallback
defined by platform/arch.

IMHO, it would solve all the problems that motivated people working
on this. And it should not cause regression that forced us to
revert the backward search.

There is still some risk of regressions. But I would give it a try.

Best Regards,
Petr

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Benjamin Herrenschmidt]

On Wed, 2019-12-11 at 10:17 +0100, Petr Mladek wrote:
> The reverse search of list of console does not work for ttySX
> consoles because the number is omitted when matching. And the messages
> will appear only on the first matched serial console. There is
> a paragraph about this in the commit message of my patch.

About that specific issue...

I see indeed that 8250_core.c registers a "generic" console with index
-1 which will match whetever we hit first in the array.

This is actually wrong isn't it ? Without any change such as what we've
been proposing, it means that an arch doing add_preferred_console of
any ttyS* will override anything on the command line, and it also means
that a command line with multiple ttyS entries will stop at the first
one, not the last one.

IE. In both case the code will select a console that isn't
preferred_console... or am I missing something subtle ?

So yes, fixing that will "regress" in the sense that it will change the
behaviour, but to make it match what's documented... am I wrong ?

The question then becomes what's the most broken ? Changing the
behaviour that might have become expected or leaving the (alledgedly)
broken behaviour in place ?

Cheers,
Ben.

Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

[Petr Mladek]

On Mon 2019-12-16 11:09:33, Benjamin Herrenschmidt wrote:
> On Wed, 2019-12-11 at 10:17 +0100, Petr Mladek wrote:
> > The reverse search of list of console does not work for ttySX
> > consoles because the number is omitted when matching. And the messages
> > will appear only on the first matched serial console. There is
> > a paragraph about this in the commit message of my patch.
> 
> About that specific issue...
> 
> I see indeed that 8250_core.c registers a "generic" console with index
> -1 which will match whetever we hit first in the array.
> 
> This is actually wrong isn't it ? Without any change such as what we've
> been proposing, it means that an arch doing add_preferred_console of
> any ttyS* will override anything on the command line, and it also means
> that a command line with multiple ttyS entries will stop at the first
> one, not the last one.
> 
> IE. In both case the code will select a console that isn't
> preferred_console... or am I missing something subtle ?
> 
> So yes, fixing that will "regress" in the sense that it will change the
> behaviour, but to make it match what's documented... am I wrong ?
> 
> The question then becomes what's the most broken ? Changing the
> behaviour that might have become expected or leaving the (alledgedly)
> broken behaviour in place ?

I though the same. But then I found the following in
Documentation/admin-guide/serial-console.rst

  "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
  first looks for a VGA card and then for a serial port. So if you don't
  have a VGA card in your system the first serial port will automatically
  become the console."


In addition, there is the "documentation" how systemd handles serial
console and login prompts, see
http://0pointer.de/blog/projects/serial-console.html


I agree that the current behavior is wrong. I really would like
to change it. But the result would be that some users will not
get login prompt after a kernel update. I am afraid that we are
blocked by the "do not break userspace" rule.

Best Regards,
Petr