[PATCH v4 0/3] printk/console: Fix preferred console handling

[PATCH v4 0/3] printk/console: Fix preferred console handling

[Petr Mladek]

Hi,

I send this on behalf of Benjamin who is traveling at the moment.
It is an interesting approach to long terms problems with matching
the console preferred on the command line.

Changes against v3:

	+ better check when accepting pre-enabled consoles
	+ correct reasoning in the 3rd patch
	+ update a comment of CON_CONSDEV definition
	+ fixed checkpatch warnings

Best Regards,
Petr

Benjamin Herrenschmidt (3):
  printk: Move console matching logic into a separate function
  printk: Fix preferred console selection with multiple matches
  printk: Correctly set CON_CONSDEV even when preferred console was not
    registered

 include/linux/console.h         |   2 +-
 kernel/printk/console_cmdline.h |   1 +
 kernel/printk/printk.c          | 122 +++++++++++++++++++++++++---------------
 3 files changed, 80 insertions(+), 45 deletions(-)

-- 
2.16.4

[PATCH v4 1/3] printk: Move console matching logic into a separate function

[Petr Mladek]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

This moves the loop that tries to match a newly registered console
with the command line or add_preferred_console list into a separate
helper, in order to be able to call it multiple times in subsequent
patches.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 kernel/printk/printk.c | 105 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 65 insertions(+), 40 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fada22dc4ab6..0ebcdf53e75d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -280,6 +280,7 @@ static struct console *exclusive_console;
 static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
 
 static int preferred_console = -1;
+static bool has_preferred_console;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
 
@@ -2626,6 +2627,60 @@ static int __init keep_bootcon_setup(char *str)
 
 early_param("keep_bootcon", keep_bootcon_setup);
 
+/*
+ * This is called by register_console() to try to match
+ * the newly registered console with any of the ones selected
+ * by either the command line or add_preferred_console() and
+ * setup/enable it.
+ *
+ * Care need to be taken with consoles that are statically
+ * enabled such as netconsole
+ */
+static int try_enable_new_console(struct console *newcon)
+{
+	struct console_cmdline *c;
+	int i;
+
+	for (i = 0, c = console_cmdline;
+	     i < MAX_CMDLINECONSOLES && c->name[0];
+	     i++, c++) {
+		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 &&
+			    newcon->setup(newcon, c->options) != 0)
+				return -EIO;
+		}
+		newcon->flags |= CON_ENABLED;
+		if (i == preferred_console) {
+			newcon->flags |= CON_CONSDEV;
+			has_preferred_console = true;
+		}
+		return 0;
+	}
+
+	/*
+	 * Some consoles, such as pstore and netconsole, can be enabled even
+	 * without matching.
+	 */
+	if (newcon->flags & CON_ENABLED)
+		return 0;
+
+	return -ENOENT;
+}
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -2647,11 +2702,9 @@ early_param("keep_bootcon", keep_bootcon_setup);
  */
 void register_console(struct console *newcon)
 {
-	int i;
 	unsigned long flags;
 	struct console *bcon = NULL;
-	struct console_cmdline *c;
-	static bool has_preferred;
+	int err;
 
 	if (console_drivers)
 		for_each_console(bcon)
@@ -2678,15 +2731,15 @@ void register_console(struct console *newcon)
 	if (console_drivers && console_drivers->flags & CON_BOOT)
 		bcon = console_drivers;
 
-	if (!has_preferred || bcon || !console_drivers)
-		has_preferred = preferred_console >= 0;
+	if (!has_preferred_console || bcon || !console_drivers)
+		has_preferred_console = preferred_console >= 0;
 
 	/*
 	 *	See if we want to use this console driver. If we
 	 *	didn't select a console we take the first one
 	 *	that registers here.
 	 */
-	if (!has_preferred) {
+	if (!has_preferred_console) {
 		if (newcon->index < 0)
 			newcon->index = 0;
 		if (newcon->setup == NULL ||
@@ -2694,47 +2747,19 @@ void register_console(struct console *newcon)
 			newcon->flags |= CON_ENABLED;
 			if (newcon->device) {
 				newcon->flags |= CON_CONSDEV;
-				has_preferred = true;
+				has_preferred_console = true;
 			}
 		}
 	}
 
 	/*
-	 *	See if this console matches one we selected on
-	 *	the command line.
+	 * See if this console matches one we selected on
+	 * the command line or if it was statically enabled
 	 */
-	for (i = 0, c = console_cmdline;
-	     i < MAX_CMDLINECONSOLES && c->name[0];
-	     i++, c++) {
-		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;
-
-			if (newcon->setup &&
-			    newcon->setup(newcon, c->options) != 0)
-				break;
-		}
-
-		newcon->flags |= CON_ENABLED;
-		if (i == preferred_console) {
-			newcon->flags |= CON_CONSDEV;
-			has_preferred = true;
-		}
-		break;
-	}
+	err = try_enable_new_console(newcon);
 
-	if (!(newcon->flags & CON_ENABLED))
+	/* printk() messages are not printed to the Braille console. */
+	if (err || newcon->flags & CON_BRL)
 		return;
 
 	/*
-- 
2.16.4

[PATCH v4 2/3] printk: Fix preferred console selection with multiple matches

[Petr Mladek]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

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 register_console() to scan first for consoles
specified on the command line, and only if none is found, to then
scan for consoles specified by the architecture.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/console_cmdline.h |  1 +
 kernel/printk/printk.c          | 29 ++++++++++++++++++-----------
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_cmdline.h
index 11f19c466af5..3ca74ad391d6 100644
--- a/kernel/printk/console_cmdline.h
+++ b/kernel/printk/console_cmdline.h
@@ -6,6 +6,7 @@ struct console_cmdline
 {
 	char	name[16];			/* Name of the driver	    */
 	int	index;				/* Minor dev. to use	    */
+	bool	user_specified;			/* Specified by command line vs. platform */
 	char	*options;			/* Options for the driver   */
 #ifdef CONFIG_A11Y_BRAILLE_CONSOLE
 	char	*brl_options;			/* Options for braille driver */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 0ebcdf53e75d..f76ef3f0efca 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2116,7 +2116,7 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
 #endif
 
 static int __add_preferred_console(char *name, int idx, char *options,
-				   char *brl_options)
+				   char *brl_options, bool user_specified)
 {
 	struct console_cmdline *c;
 	int i;
@@ -2131,6 +2131,8 @@ static int __add_preferred_console(char *name, int idx, char *options,
 		if (strcmp(c->name, name) == 0 && c->index == idx) {
 			if (!brl_options)
 				preferred_console = i;
+			if (user_specified)
+				c->user_specified = true;
 			return 0;
 		}
 	}
@@ -2140,6 +2142,7 @@ static int __add_preferred_console(char *name, int idx, char *options,
 		preferred_console = i;
 	strlcpy(c->name, name, sizeof(c->name));
 	c->options = options;
+	c->user_specified = user_specified;
 	braille_set_options(c, brl_options);
 
 	c->index = idx;
@@ -2194,7 +2197,7 @@ static int __init console_setup(char *str)
 	idx = simple_strtoul(s, NULL, 10);
 	*s = 0;
 
-	__add_preferred_console(buf, idx, options, brl_options);
+	__add_preferred_console(buf, idx, options, brl_options, true);
 	console_set_on_cmdline = 1;
 	return 1;
 }
@@ -2215,7 +2218,7 @@ __setup("console=", console_setup);
  */
 int add_preferred_console(char *name, int idx, char *options)
 {
-	return __add_preferred_console(name, idx, options, NULL);
+	return __add_preferred_console(name, idx, options, NULL, false);
 }
 
 bool console_suspend_enabled = true;
@@ -2636,7 +2639,7 @@ early_param("keep_bootcon", keep_bootcon_setup);
  * Care need to be taken with consoles that are statically
  * enabled such as netconsole
  */
-static int try_enable_new_console(struct console *newcon)
+static int try_enable_new_console(struct console *newcon, bool user_specified)
 {
 	struct console_cmdline *c;
 	int i;
@@ -2644,6 +2647,8 @@ static int try_enable_new_console(struct console *newcon)
 	for (i = 0, c = console_cmdline;
 	     i < MAX_CMDLINECONSOLES && c->name[0];
 	     i++, c++) {
+		if (c->user_specified != user_specified)
+			continue;
 		if (!newcon->match ||
 		    newcon->match(newcon, c->name, c->index, c->options) != 0) {
 			/* default matching */
@@ -2673,9 +2678,10 @@ static int try_enable_new_console(struct console *newcon)
 
 	/*
 	 * Some consoles, such as pstore and netconsole, can be enabled even
-	 * without matching.
+	 * without matching. Accept the pre-enabled consoles only when match()
+	 * and setup() had a change to be called.
 	 */
-	if (newcon->flags & CON_ENABLED)
+	if (newcon->flags & CON_ENABLED && c->user_specified ==	user_specified)
 		return 0;
 
 	return -ENOENT;
@@ -2752,11 +2758,12 @@ void register_console(struct console *newcon)
 		}
 	}
 
-	/*
-	 * See if this console matches one we selected on
-	 * the command line or if it was statically enabled
-	 */
-	err = try_enable_new_console(newcon);
+	/* See if this console matches one we selected on the command line */
+	err = try_enable_new_console(newcon, true);
+
+	/* If not, try to match against the platform default(s) */
+	if (err == -ENOENT)
+		err = try_enable_new_console(newcon, false);
 
 	/* printk() messages are not printed to the Braille console. */
 	if (err || newcon->flags & CON_BRL)
-- 
2.16.4

[PATCH v4 3/3] printk: Correctly set CON_CONSDEV even when preferred console was not registered

[Petr Mladek]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

CON_CONSDEV flag was historically used to put/keep the preferred console
first in console_drivers list. Where the preferred console is the last
on the command line.

The ordering is important only when opening /dev/console:

  + tty_kopen()
    + tty_lookup_driver()
      + console_device()

The flag was originally an implementation detail. But it was later
made accessible from userspace via /proc/consoles. It was used,
for example, by the tool "showconsole" to show the real tty
accessible via /dev/console, see
https://github.com/bitstreamout/showconsole

Now, the current code sets CON_CONSDEV only for the preferred
console or when a fallback console is added. The flag is not
set when the preferred console is defined on the command line
but it is not registered from some reasons.

Simple solution is to set CON_CONSDEV flag for the first
registered console. It will work most of the time because:

  + Most real consoles have console->device defined.

  + Boot consoles are removed in printk_late_init().

  + unregister_console() moves CON_CONSDEV flag to the next
    console.

Clean solution would require checking con->device when the
preferred console is registered and in unregister_console().

Conclusion:

Use the simple solution for now. It is better than the current
state and good enough.

The clean solution is not worth it. It would complicate the already
complicated code without too much gain. Instead the code would deserve
a complete rewrite.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
[pmladek@suse.com: Correct reasoning in the commit message, comment update.]
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/console.h | 2 +-
 kernel/printk/printk.c  | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index f33016b3a401..57ae2dedb51f 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -134,7 +134,7 @@ static inline int con_debug_leave(void)
  */
 
 #define CON_PRINTBUFFER	(1)
-#define CON_CONSDEV	(2) /* Last on the command line */
+#define CON_CONSDEV	(2) /* Preferred console, /dev/console */
 #define CON_ENABLED	(4)
 #define CON_BOOT	(8)
 #define CON_ANYTIME	(16) /* Safe to call when cpu is offline */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index f76ef3f0efca..cf0ceacdae2f 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2788,6 +2788,8 @@ void register_console(struct console *newcon)
 		console_drivers = newcon;
 		if (newcon->next)
 			newcon->next->flags &= ~CON_CONSDEV;
+		/* Ensure this flag is always set for the head of the list */
+		newcon->flags |= CON_CONSDEV;
 	} else {
 		newcon->next = console_drivers->next;
 		console_drivers->next = newcon;
-- 
2.16.4

Re: [PATCH v4 0/3] printk/console: Fix preferred console handling

[Sergey Senozhatsky]

On (20/02/13 10:51), Petr Mladek wrote:
> Hi,
> 
> I send this on behalf of Benjamin who is traveling at the moment.
> It is an interesting approach to long terms problems with matching
> the console preferred on the command line.
> 
> Changes against v3:
> 
> 	+ better check when accepting pre-enabled consoles
> 	+ correct reasoning in the 3rd patch
> 	+ update a comment of CON_CONSDEV definition
> 	+ fixed checkpatch warnings

Looks good to me,

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH v4 0/3] printk/console: Fix preferred console handling

[Petr Mladek]

On Mon 2020-02-17 22:03:08, Sergey Senozhatsky wrote:
> On (20/02/13 10:51), Petr Mladek wrote:
> > Hi,
> > 
> > I send this on behalf of Benjamin who is traveling at the moment.
> > It is an interesting approach to long terms problems with matching
> > the console preferred on the command line.
> > 
> > Changes against v3:
> > 
> > 	+ better check when accepting pre-enabled consoles
> > 	+ correct reasoning in the 3rd patch
> > 	+ update a comment of CON_CONSDEV definition
> > 	+ fixed checkpatch warnings
> 
> Looks good to me,
> 
> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

The patchset is commited in printk.git, branch
for-5.7-preferred-console.

Best Regards,
Petr

Re: [PATCH v4 0/3] printk/console: Fix preferred console handling

[Benjamin Herrenschmidt]

On Tue, 2020-02-18 at 10:52 +0100, Petr Mladek wrote:
> On Mon 2020-02-17 22:03:08, Sergey Senozhatsky wrote:
> > On (20/02/13 10:51), Petr Mladek wrote:
> > > Hi,
> > > 
> > > I send this on behalf of Benjamin who is traveling at the moment.
> > > It is an interesting approach to long terms problems with
> > > matching
> > > the console preferred on the command line.
> > > 
> > > Changes against v3:
> > > 
> > > 	+ better check when accepting pre-enabled consoles
> > > 	+ correct reasoning in the 3rd patch
> > > 	+ update a comment of CON_CONSDEV definition
> > > 	+ fixed checkpatch warnings
> > 
> > Looks good to me,
> > 
> > Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> The patchset is commited in printk.git, branch
> for-5.7-preferred-console.

Thanks a lot guys ! (I'm back now btw :-)

Cheers,
Ben.

Re: [PATCH v4 0/3] printk/console: Fix preferred console handling

[Benjamin Herrenschmidt]

On Tue, 2020-02-18 at 10:52 +0100, Petr Mladek wrote:
> On Mon 2020-02-17 22:03:08, Sergey Senozhatsky wrote:
> > On (20/02/13 10:51), Petr Mladek wrote:
> > > Hi,
> > > 
> > > I send this on behalf of Benjamin who is traveling at the moment.
> > > It is an interesting approach to long terms problems with
> > > matching
> > > the console preferred on the command line.
> > > 
> > > Changes against v3:
> > > 
> > > 	+ better check when accepting pre-enabled consoles
> > > 	+ correct reasoning in the 3rd patch
> > > 	+ update a comment of CON_CONSDEV definition
> > > 	+ fixed checkpatch warnings
> > 
> > Looks good to me,
> > 
> > Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> The patchset is commited in printk.git, branch
> for-5.7-preferred-console.
> 
Do you plan to send any of this to -stable ?

Cheers,
Ben.

Re: [PATCH v4 0/3] printk/console: Fix preferred console handling

[Petr Mladek]

On Fri 2020-02-28 09:14:47, Benjamin Herrenschmidt wrote:
> On Tue, 2020-02-18 at 10:52 +0100, Petr Mladek wrote:
> > On Mon 2020-02-17 22:03:08, Sergey Senozhatsky wrote:
> > > On (20/02/13 10:51), Petr Mladek wrote:
> > > > Hi,
> > > > 
> > > > I send this on behalf of Benjamin who is traveling at the moment.
> > > > It is an interesting approach to long terms problems with
> > > > matching
> > > > the console preferred on the command line.
> > > > 
> > > > Changes against v3:
> > > > 
> > > > 	+ better check when accepting pre-enabled consoles
> > > > 	+ correct reasoning in the 3rd patch
> > > > 	+ update a comment of CON_CONSDEV definition
> > > > 	+ fixed checkpatch warnings
> > > 
> > > Looks good to me,
> > > 
> > > Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > 
> > The patchset is commited in printk.git, branch
> > for-5.7-preferred-console.
> > 
> Do you plan to send any of this to -stable ?

Good question. I would prefer to wait until 5.7 gets release or even
longer. Changes in console registration order are prone to
regressions. People then complain that they do not longer see console
after reboot.

linux-next and rc phase has only pretty limited number of users.
Released kernels hit much bigger user base, for example, via
OpenSUSE Tumbleweed.

Best Regards,
Petr

Re: [PATCH v4 0/3] printk/console: Fix preferred console handling

[Sergey Senozhatsky]

On (20/02/28 15:58), Petr Mladek wrote:
> > > 
> > Do you plan to send any of this to -stable ?
> 
> Good question. I would prefer to wait until 5.7 gets release or even
> longer. Changes in console registration order are prone to
> regressions. People then complain that they do not longer see console
> after reboot.
> 
> linux-next and rc phase has only pretty limited number of users.
> Released kernels hit much bigger user base, for example, via
> OpenSUSE Tumbleweed.

Agreed. Let's not rather rush it.

	-ss

Re: [PATCH v4 0/3] printk/console: Fix preferred console handling

[Benjamin Herrenschmidt]

On Fri, 2020-02-28 at 15:58 +0100, Petr Mladek wrote:
> > > The patchset is commited in printk.git, branch
> > > for-5.7-preferred-console.
> > > 
> > 
> > Do you plan to send any of this to -stable ?
> 
> Good question. I would prefer to wait until 5.7 gets release or even
> longer. Changes in console registration order are prone to
> regressions. People then complain that they do not longer see console
> after reboot.
> 
> linux-next and rc phase has only pretty limited number of users.
> Released kernels hit much bigger user base, for example, via
> OpenSUSE Tumbleweed.

Ok. Thanks.

Cheers,
Ben.

Re: [PATCH v4 0/3] printk/console: Fix preferred console handling

[Benjamin Herrenschmidt]

On Fri, 2020-02-28 at 15:58 +0100, Petr Mladek wrote:
> > > > Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > 
> > > The patchset is commited in printk.git, branch
> > > for-5.7-preferred-console.
> > > 
> > 
> > Do you plan to send any of this to -stable ?
> 
> Good question. I would prefer to wait until 5.7 gets release or even
> longer. Changes in console registration order are prone to
> regressions. People then complain that they do not longer see console
> after reboot.
> 
> linux-next and rc phase has only pretty limited number of users.
> Released kernels hit much bigger user base, for example, via
> OpenSUSE Tumbleweed

Hi Petr !

I don't see this upstream yet, what happened ?

Cheers,
Ben.