[PATCH 0/8] printk: Clean up preferred console handling

[PATCH 0/8] printk: Clean up preferred console handling

[Petr Mladek]

Hi,

this patches does some clean up of the code for handling preferred consoles
in the console registration code.

  + 1st and 2nd patch try to improve a naming.

    The meaning of struct console_cmdline has changed over time.
    The meaning of "preferred_console" variable has always been a bit
    confusing and it has got even worse after adding the support for
    Braille consoles.


  + 3rd patch remove some code duplication. It better defines and describes
    the rules for adding and updating preferred consoles. It uses a more
    defensive coding style.


  + 4th, 5th, and 6th patch improve handling of Braille consoles.
    They are preferred via the command line but they do not get
    printk() messages and are not associated with /dev/console.

    The new code makes this more obvious. Also it explicitly
    defines the relation against default consoles and other
    non-Braille preferred consoles.


  + 7th patch removes a hidden side effect of
    try_enable_preferred_console()


  + 8th patch removes the last hidden side effect of
    try_enable_preferred_console() and allows to call it
    only when there are any non-Braille preferred consoles.


I tested many scenarios of fixed several bugs. I did my best to
prevent regressions.

This patchset is a prerequisite for Marcos' clean up of CON_ENABLE
flag handling. It should prevent regressions caused by the
hidden effects of try_enable_preferred_console(), for example,
see https://lore.kernel.org/r/89409a0f48e6998ff6dd2245691b9954f0e1e435.camel@suse.com

Also I am working on a feature which would allow to explicitly
enable/prefer consoles proposed by SPCR, device tree, or
platform-specific code using a generic "console=platform".
This clean up is a prerequisite, see
https://github.com/pmladek/linux/tree/console-platform-poc1-iter9

The patchset has been re-based on top of v6.19-rc8.

Petr Mladek (8):
  printk: Rename struct console_cmdline to preferred_console
  printk: Rename preferred_console to preferred_dev_console
  printk: Separate code for adding/updating preferred console metadata
  printk: Cleanup _braille_(un)register_console() wrappers
  printk: Try to register each console as Braille first
  printk: Do not set Braille console as preferred_console
  printk: Handle pre-enabled consoles directly in register_console()
  printk: Try enable preferred consoles only when there are any

 .../accessibility/braille/braille_console.c   |   7 +-
 kernel/printk/braille.c                       |  20 +-
 kernel/printk/braille.h                       |  23 +-
 .../{console_cmdline.h => console_register.h} |   6 +-
 kernel/printk/printk.c                        | 295 ++++++++++++------
 5 files changed, 223 insertions(+), 128 deletions(-)
 rename kernel/printk/{console_cmdline.h => console_register.h} (83%)

-- 
2.52.0

[PATCH 1/8] printk: Rename struct console_cmdline to preferred_console

[Petr Mladek]

The structure 'console_cmdline' was originally intended to store details
about consoles defined on the kernel command line. However, its usage
has since expanded; it now stores information for consoles preferred
via SPCR, device tree, or by particular platforms, e.g. XEN.

The current naming is misleading as it implies the configuration only
originates from the command line.

Rename the structure and associated artifacts to better reflect their
current purpose, for example:

  - struct console_cmdline c -> struct preferred_console pc
  - console_cmdline[]        -> preferred_consoles[]
  - console_cmdline.h        -> console_register.h
  - c			     -> pc

Additionally, renaming the header file to console_register.h would
eventually allow to decouple console registration logic from
the monolithic printk.c.

Finally, renaming the local variable from "c" to "pc" helps to distinguish
it from struct console variables. Note that "c" is used for struct console
in some code, for example see vt_console_device() function definition.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/braille.c                       | 10 +--
 kernel/printk/braille.h                       | 10 +--
 .../{console_cmdline.h => console_register.h} |  6 +-
 kernel/printk/printk.c                        | 84 ++++++++++---------
 4 files changed, 56 insertions(+), 54 deletions(-)
 rename kernel/printk/{console_cmdline.h => console_register.h} (83%)

diff --git a/kernel/printk/braille.c b/kernel/printk/braille.c
index 17a9591e54ff..9d21a2bb1d38 100644
--- a/kernel/printk/braille.c
+++ b/kernel/printk/braille.c
@@ -6,7 +6,7 @@
 #include <linux/errno.h>
 #include <linux/string.h>
 
-#include "console_cmdline.h"
+#include "console_register.h"
 #include "braille.h"
 
 int _braille_console_setup(char **str, char **brl_options)
@@ -35,14 +35,14 @@ int _braille_console_setup(char **str, char **brl_options)
 }
 
 int
-_braille_register_console(struct console *console, struct console_cmdline *c)
+_braille_register_console(struct console *console, struct preferred_console *pc)
 {
 	int rtn = 0;
 
-	if (c->brl_options) {
+	if (pc->brl_options) {
 		console->flags |= CON_BRL;
-		rtn = braille_register_console(console, c->index, c->options,
-					       c->brl_options);
+		rtn = braille_register_console(console, pc->index, pc->options,
+					       pc->brl_options);
 	}
 
 	return rtn;
diff --git a/kernel/printk/braille.h b/kernel/printk/braille.h
index 123154f86304..55cd3178a17a 100644
--- a/kernel/printk/braille.h
+++ b/kernel/printk/braille.h
@@ -5,9 +5,9 @@
 #ifdef CONFIG_A11Y_BRAILLE_CONSOLE
 
 static inline void
-braille_set_options(struct console_cmdline *c, char *brl_options)
+braille_set_options(struct preferred_console *pc, char *brl_options)
 {
-	c->brl_options = brl_options;
+	pc->brl_options = brl_options;
 }
 
 /*
@@ -21,7 +21,7 @@ int
 _braille_console_setup(char **str, char **brl_options);
 
 int
-_braille_register_console(struct console *console, struct console_cmdline *c);
+_braille_register_console(struct console *console, struct preferred_console *pc);
 
 int
 _braille_unregister_console(struct console *console);
@@ -29,7 +29,7 @@ _braille_unregister_console(struct console *console);
 #else
 
 static inline void
-braille_set_options(struct console_cmdline *c, char *brl_options)
+braille_set_options(struct preferred_console *pc, char *brl_options)
 {
 }
 
@@ -40,7 +40,7 @@ _braille_console_setup(char **str, char **brl_options)
 }
 
 static inline int
-_braille_register_console(struct console *console, struct console_cmdline *c)
+_braille_register_console(struct console *console, struct preferred_console *pc)
 {
 	return 0;
 }
diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_register.h
similarity index 83%
rename from kernel/printk/console_cmdline.h
rename to kernel/printk/console_register.h
index 0ab573b6d4dc..9ab3e1cc749b 100644
--- a/kernel/printk/console_cmdline.h
+++ b/kernel/printk/console_register.h
@@ -1,8 +1,8 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _CONSOLE_CMDLINE_H
-#define _CONSOLE_CMDLINE_H
+#ifndef _CONSOLE_REGISTER_H
+#define _CONSOLE_REGISTER_H
 
-struct console_cmdline
+struct preferred_console
 {
 	char	name[16];			/* Name of the driver	    */
 	int	index;				/* Minor dev. to use	    */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1d765ad242b8..86a908e74445 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -58,7 +58,7 @@
 #include <trace/events/printk.h>
 
 #include "printk_ringbuffer.h"
-#include "console_cmdline.h"
+#include "console_register.h"
 #include "braille.h"
 #include "internal.h"
 
@@ -360,9 +360,9 @@ static int console_locked;
  *	Array of consoles built from command line options (console=)
  */
 
-#define MAX_CMDLINECONSOLES 8
+#define MAX_PREFERRED_CONSOLES 8
 
-static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
+static struct preferred_console preferred_consoles[MAX_PREFERRED_CONSOLES];
 
 static int preferred_console = -1;
 int console_set_on_cmdline;
@@ -2491,7 +2491,7 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
 }
 #endif
 
-static void set_user_specified(struct console_cmdline *c, bool user_specified)
+static void set_user_specified(struct preferred_console *pc, bool user_specified)
 {
 	if (!user_specified)
 		return;
@@ -2500,7 +2500,7 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified)
 	 * @c console was defined by the user on the command line.
 	 * Do not clear when added twice also by SPCR or the device tree.
 	 */
-	c->user_specified = true;
+	pc->user_specified = true;
 	/* At least one console defined by the user on the command line. */
 	console_set_on_cmdline = 1;
 }
@@ -2509,7 +2509,7 @@ static int __add_preferred_console(const char *name, const short idx,
 				   const char *devname, char *options,
 				   char *brl_options, bool user_specified)
 {
-	struct console_cmdline *c;
+	struct preferred_console *pc;
 	int i;
 
 	if (!name && !devname)
@@ -2528,30 +2528,30 @@ static int __add_preferred_console(const char *name, const short idx,
 	 *	See if this tty is not yet registered, and
 	 *	if we have a slot free.
 	 */
-	for (i = 0, c = console_cmdline;
-	     i < MAX_CMDLINECONSOLES && (c->name[0] || c->devname[0]);
-	     i++, c++) {
-		if ((name && strcmp(c->name, name) == 0 && c->index == idx) ||
-		    (devname && strcmp(c->devname, devname) == 0)) {
+	for (i = 0, pc = preferred_consoles;
+	     i < MAX_PREFERRED_CONSOLES && (pc->name[0] || pc->devname[0]);
+	     i++, pc++) {
+		if ((name && strcmp(pc->name, name) == 0 && pc->index == idx) ||
+		    (devname && strcmp(pc->devname, devname) == 0)) {
 			if (!brl_options)
 				preferred_console = i;
-			set_user_specified(c, user_specified);
+			set_user_specified(pc, user_specified);
 			return 0;
 		}
 	}
-	if (i == MAX_CMDLINECONSOLES)
+	if (i == MAX_PREFERRED_CONSOLES)
 		return -E2BIG;
 	if (!brl_options)
 		preferred_console = i;
 	if (name)
-		strscpy(c->name, name);
+		strscpy(pc->name, name);
 	if (devname)
-		strscpy(c->devname, devname);
-	c->options = options;
-	set_user_specified(c, user_specified);
-	braille_set_options(c, brl_options);
+		strscpy(pc->devname, devname);
+	pc->options = options;
+	set_user_specified(pc, user_specified);
+	braille_set_options(pc, brl_options);
 
-	c->index = idx;
+	pc->index = idx;
 	return 0;
 }
 
@@ -2571,8 +2571,10 @@ __setup("console_msg_format=", console_msg_format_setup);
  */
 static int __init console_setup(char *str)
 {
-	static_assert(sizeof(console_cmdline[0].devname) >= sizeof(console_cmdline[0].name) + 4);
-	char buf[sizeof(console_cmdline[0].devname)];
+	static_assert(sizeof(preferred_consoles[0].devname) >=
+		      sizeof(preferred_consoles[0].name) + 4);
+
+	char buf[sizeof(preferred_consoles[0].devname)];
 	char *brl_options = NULL;
 	char *ttyname = NULL;
 	char *devname = NULL;
@@ -2677,19 +2679,19 @@ int match_devname_and_update_preferred_console(const char *devname,
 					       const char *name,
 					       const short idx)
 {
-	struct console_cmdline *c = console_cmdline;
+	struct preferred_console *pc = preferred_consoles;
 	int i;
 
 	if (!devname || !strlen(devname) || !name || !strlen(name) || idx < 0)
 		return -EINVAL;
 
-	for (i = 0; i < MAX_CMDLINECONSOLES && (c->name[0] || c->devname[0]);
-	     i++, c++) {
-		if (!strcmp(devname, c->devname)) {
+	for (i = 0; i < MAX_PREFERRED_CONSOLES && (pc->name[0] || pc->devname[0]);
+	     i++, pc++) {
+		if (!strcmp(devname, pc->devname)) {
 			pr_info("associate the preferred console \"%s\" with \"%s%d\"\n",
 				devname, name, idx);
-			strscpy(c->name, name);
-			c->index = idx;
+			strscpy(pc->name, name);
+			pc->index = idx;
 			return 0;
 		}
 	}
@@ -3859,33 +3861,33 @@ static int console_call_setup(struct console *newcon, char *options)
 static int try_enable_preferred_console(struct console *newcon,
 					bool user_specified)
 {
-	struct console_cmdline *c;
+	struct preferred_console *pc;
 	int i, err;
 
-	for (i = 0, c = console_cmdline;
-	     i < MAX_CMDLINECONSOLES && (c->name[0] || c->devname[0]);
-	     i++, c++) {
+	for (i = 0, pc = preferred_consoles;
+	     i < MAX_PREFERRED_CONSOLES && (pc->name[0] || pc->devname[0]);
+	     i++, pc++) {
 		/* Console not yet initialized? */
-		if (!c->name[0])
+		if (!pc->name[0])
 			continue;
-		if (c->user_specified != user_specified)
+		if (pc->user_specified != user_specified)
 			continue;
 		if (!newcon->match ||
-		    newcon->match(newcon, c->name, c->index, c->options) != 0) {
+		    newcon->match(newcon, pc->name, pc->index, pc->options) != 0) {
 			/* default matching */
-			BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
-			if (strcmp(c->name, newcon->name) != 0)
+			BUILD_BUG_ON(sizeof(pc->name) != sizeof(newcon->name));
+			if (strcmp(pc->name, newcon->name) != 0)
 				continue;
 			if (newcon->index >= 0 &&
-			    newcon->index != c->index)
+			    newcon->index != pc->index)
 				continue;
 			if (newcon->index < 0)
-				newcon->index = c->index;
+				newcon->index = pc->index;
 
-			if (_braille_register_console(newcon, c))
+			if (_braille_register_console(newcon, pc))
 				return 0;
 
-			err = console_call_setup(newcon, c->options);
+			err = console_call_setup(newcon, pc->options);
 			if (err)
 				return err;
 		}
@@ -3900,7 +3902,7 @@ static int try_enable_preferred_console(struct console *newcon,
 	 * without matching. Accept the pre-enabled consoles only when match()
 	 * and setup() had a chance to be called.
 	 */
-	if (newcon->flags & CON_ENABLED && c->user_specified ==	user_specified)
+	if (newcon->flags & CON_ENABLED && pc->user_specified == user_specified)
 		return 0;
 
 	return -ENOENT;
-- 
2.52.0

[PATCH 2/8] printk: Rename preferred_console to preferred_dev_console

[Petr Mladek]

The preferred_consoles[] array stores information about consoles requested
via the command line, SPCR, Device Tree, or platform-specific code.
Within this array, the 'preferred_console' variable tracks the specific
index that should be associated with /dev/console (typically the last
non-braille console defined).

The current name "preferred_console" is ambiguous and leads to confusion.
It does not clearly communicate why one console is "more preferred" than
others in the array. Furthermore, entries for Braille consoles can exist
within the preferred_consoles[] array, yet they are never associated with
/dev/console and do not receive standard printk() output. Consequently,
the 'preferred_console' index must skip these entries, which is not
immediately obvious from the name.

Rename the variable to 'preferred_dev_console' to explicitly clarify its
role in identifying which entry is linked to /dev/console.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 86a908e74445..3f856a438e74 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -364,7 +364,7 @@ static int console_locked;
 
 static struct preferred_console preferred_consoles[MAX_PREFERRED_CONSOLES];
 
-static int preferred_console = -1;
+static int preferred_dev_console = -1;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
 
@@ -2534,7 +2534,7 @@ static int __add_preferred_console(const char *name, const short idx,
 		if ((name && strcmp(pc->name, name) == 0 && pc->index == idx) ||
 		    (devname && strcmp(pc->devname, devname) == 0)) {
 			if (!brl_options)
-				preferred_console = i;
+				preferred_dev_console = i;
 			set_user_specified(pc, user_specified);
 			return 0;
 		}
@@ -2542,7 +2542,7 @@ static int __add_preferred_console(const char *name, const short idx,
 	if (i == MAX_PREFERRED_CONSOLES)
 		return -E2BIG;
 	if (!brl_options)
-		preferred_console = i;
+		preferred_dev_console = i;
 	if (name)
 		strscpy(pc->name, name);
 	if (devname)
@@ -3892,7 +3892,7 @@ static int try_enable_preferred_console(struct console *newcon,
 				return err;
 		}
 		newcon->flags |= CON_ENABLED;
-		if (i == preferred_console)
+		if (i == preferred_dev_console)
 			newcon->flags |= CON_CONSDEV;
 		return 0;
 	}
@@ -4073,7 +4073,7 @@ void register_console(struct console *newcon)
 	 * Note that a console with tty binding will have CON_CONSDEV
 	 * flag set and will be first in the list.
 	 */
-	if (preferred_console < 0) {
+	if (preferred_dev_console < 0) {
 		if (hlist_empty(&console_list) || !console_first()->device ||
 		    console_first()->flags & CON_BOOT) {
 			try_enable_default_console(newcon);
-- 
2.52.0

[PATCH 3/8] printk: Separate code for adding/updating preferred console metadata

[Petr Mladek]

The logic for adding or updating a preferred console is currently
duplicated within __add_preferred_console(), making the code difficult
to follow and prone to consistency issues.

Introduce update_preferred_console() to centralize the initialization
and updating of struct preferred_console entries. This refactoring
explicitly defines and enforces the following rules:

1. Console names and/or indexes are not set when a console is preferred
   via devname; these are resolved later during device matching.
2. Console names are only added alongside a valid index.
3. Only matching entries are updated.
4. Console and Braille options are never cleared. They are updated
   only via the command line.
5. The global 'preferred_dev_console' index and 'console_set_on_cmdline'
   flag are updated consistently.

Additionally, rename braille_set_options() to braille_update_options()
to better reflect its conditional behavior.

While this is primarily a refactoring, it fixes two minor behavioral
consistencies: console options can now be overridden via the command
line, and Braille options are preserved even if a non-Braille console
with the same name was previously defined.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/braille.h |   7 +--
 kernel/printk/printk.c  | 110 +++++++++++++++++++++++++++-------------
 2 files changed, 80 insertions(+), 37 deletions(-)

diff --git a/kernel/printk/braille.h b/kernel/printk/braille.h
index 55cd3178a17a..0bdac303f8b1 100644
--- a/kernel/printk/braille.h
+++ b/kernel/printk/braille.h
@@ -5,9 +5,10 @@
 #ifdef CONFIG_A11Y_BRAILLE_CONSOLE
 
 static inline void
-braille_set_options(struct preferred_console *pc, char *brl_options)
+braille_update_options(struct preferred_console *pc, char *brl_options)
 {
-	pc->brl_options = brl_options;
+	if (brl_options)
+		pc->brl_options = brl_options;
 }
 
 /*
@@ -29,7 +30,7 @@ _braille_unregister_console(struct console *console);
 #else
 
 static inline void
-braille_set_options(struct preferred_console *pc, char *brl_options)
+braille_update_options(struct preferred_console *pc, char *brl_options)
 {
 }
 
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 3f856a438e74..ee57c7ac9d02 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2491,18 +2491,82 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
 }
 #endif
 
-static void set_user_specified(struct preferred_console *pc, bool user_specified)
+static int update_preferred_console(int i, const char *name, const short idx,
+				    const char *devname, char *options,
+				    char *brl_options, bool user_specified)
 {
-	if (!user_specified)
-		return;
+	struct preferred_console *pc;
+
+	if (i >= MAX_PREFERRED_CONSOLES)
+		return -E2BIG;
+
+	pc = &preferred_consoles[i];
+
+	if (!name && !devname)
+		return -EINVAL;
+
+	if (devname) {
+		/*
+		 * A valid console name and index will get assigned when
+		 * a matching device gets registered.
+		 */
+		if (name) {
+			pr_err("Adding a preferred console devname with a hard-coded console name: %s, %s\n",
+			       devname, name);
+			return -EINVAL;
+		}
+		if (idx != -1) {
+			pr_err("Adding a preferred console devname with a hard-coded index: %s, %d\n",
+			       devname, idx);
+			return -EINVAL;
+		}
+
+		if (!pc->devname[0]) {
+			strscpy(pc->devname, devname);
+			pc->index = idx;
+		} else if (strcmp(pc->devname, devname) != 0) {
+			pr_err("Updating a preferred console with an invalid devname: %s vs. %s\n",
+			       pc->devname, devname);
+			return -EINVAL;
+		}
+	}
+
+	if (name) {
+		/* A console name must be defined with a valid index. */
+		if (idx < 0) {
+			pr_err("Adding a preferred console with an invalid index: %s, %d\n",
+			       name, idx);
+			return -EINVAL;
+		}
+
+		if (!pc->name[0]) {
+			strscpy(pc->name, name);
+			pc->index = idx;
+		} else if (strcmp(pc->name, name) != 0 || pc->index != idx) {
+			pr_err("Updating a preferred console with an invalid name or index: %s%d vs. %s%d\n",
+			       pc->name, pc->index, name, idx);
+			return -EINVAL;
+		}
+	}
+
+	if (!pc->options || (user_specified && options))
+		pc->options = options;
+
+	braille_update_options(pc, brl_options);
+
+	if (!brl_options)
+		preferred_dev_console = i;
 
 	/*
 	 * @c console was defined by the user on the command line.
 	 * Do not clear when added twice also by SPCR or the device tree.
 	 */
-	pc->user_specified = true;
-	/* At least one console defined by the user on the command line. */
-	console_set_on_cmdline = 1;
+	if (user_specified) {
+		pc->user_specified = true;
+		console_set_on_cmdline = 1;
+	}
+
+	return 0;
 }
 
 static int __add_preferred_console(const char *name, const short idx,
@@ -2515,14 +2579,6 @@ static int __add_preferred_console(const char *name, const short idx,
 	if (!name && !devname)
 		return -EINVAL;
 
-	/*
-	 * We use a signed short index for struct console for device drivers to
-	 * indicate a not yet assigned index or port. However, a negative index
-	 * value is not valid when the console name and index are defined on
-	 * the command line.
-	 */
-	if (name && idx < 0)
-		return -EINVAL;
 
 	/*
 	 *	See if this tty is not yet registered, and
@@ -2531,28 +2587,14 @@ static int __add_preferred_console(const char *name, const short idx,
 	for (i = 0, pc = preferred_consoles;
 	     i < MAX_PREFERRED_CONSOLES && (pc->name[0] || pc->devname[0]);
 	     i++, pc++) {
-		if ((name && strcmp(pc->name, name) == 0 && pc->index == idx) ||
-		    (devname && strcmp(pc->devname, devname) == 0)) {
-			if (!brl_options)
-				preferred_dev_console = i;
-			set_user_specified(pc, user_specified);
-			return 0;
-		}
+		if (name && strcmp(pc->name, name) == 0 && pc->index == idx)
+			break;
+		if (devname && strcmp(pc->devname, devname) == 0)
+			break;
 	}
-	if (i == MAX_PREFERRED_CONSOLES)
-		return -E2BIG;
-	if (!brl_options)
-		preferred_dev_console = i;
-	if (name)
-		strscpy(pc->name, name);
-	if (devname)
-		strscpy(pc->devname, devname);
-	pc->options = options;
-	set_user_specified(pc, user_specified);
-	braille_set_options(pc, brl_options);
 
-	pc->index = idx;
-	return 0;
+	return update_preferred_console(i, name, idx, devname, options,
+					brl_options, user_specified);
 }
 
 static int __init console_msg_format_setup(char *str)
-- 
2.52.0

[PATCH 4/8] printk: Cleanup _braille_(un)register_console() wrappers

[Petr Mladek]

The _braille_(un)register_console() wrappers currently attempt to hide
implementation details like the CON_BRL flag and pc->brl_options. This
forces callers to handle an unconventional tri-state return value (0 for
NOP, >0 for success, <0 for error), which makes the control flow harder
to follow and non-standard.

Refactor the wrappers to use standard kernel return codes (0 for success,
-ERRCODE on failure). Move the responsibility of checking brl_options
to the caller to make the logic more explicit.

Additionally, move the assignment of the CON_BRL flag from the internal
wrapper to braille_register_console(). This aligns it with how
CON_ENABLED is handled. To maintain symmetry and fix a potential bug
where flags might persist after removal, explicitly clear both
CON_ENABLED and CON_BRL in braille_unregister_console().

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 drivers/accessibility/braille/braille_console.c |  7 ++++---
 kernel/printk/braille.c                         | 16 +++-------------
 kernel/printk/braille.h                         | 12 ++++++++++++
 kernel/printk/printk.c                          | 13 +++++--------
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/accessibility/braille/braille_console.c b/drivers/accessibility/braille/braille_console.c
index 06b43b678d6e..7b324329882f 100644
--- a/drivers/accessibility/braille/braille_console.c
+++ b/drivers/accessibility/braille/braille_console.c
@@ -360,12 +360,12 @@ int braille_register_console(struct console *console, int index,
 		if (ret != 0)
 			return ret;
 	}
-	console->flags |= CON_ENABLED;
+	console->flags |= CON_ENABLED | CON_BRL;
 	console->index = index;
 	braille_co = console;
 	register_keyboard_notifier(&keyboard_notifier_block);
 	register_vt_notifier(&vt_notifier_block);
-	return 1;
+	return 0;
 }
 
 int braille_unregister_console(struct console *console)
@@ -375,5 +375,6 @@ int braille_unregister_console(struct console *console)
 	unregister_keyboard_notifier(&keyboard_notifier_block);
 	unregister_vt_notifier(&vt_notifier_block);
 	braille_co = NULL;
-	return 1;
+	console->flags &= ~(CON_ENABLED | CON_BRL);
+	return 0;
 }
diff --git a/kernel/printk/braille.c b/kernel/printk/braille.c
index 9d21a2bb1d38..593f83eb0487 100644
--- a/kernel/printk/braille.c
+++ b/kernel/printk/braille.c
@@ -37,22 +37,12 @@ int _braille_console_setup(char **str, char **brl_options)
 int
 _braille_register_console(struct console *console, struct preferred_console *pc)
 {
-	int rtn = 0;
-
-	if (pc->brl_options) {
-		console->flags |= CON_BRL;
-		rtn = braille_register_console(console, pc->index, pc->options,
-					       pc->brl_options);
-	}
-
-	return rtn;
+	return braille_register_console(console, pc->index, pc->options,
+					pc->brl_options);
 }
 
 int
 _braille_unregister_console(struct console *console)
 {
-	if (console->flags & CON_BRL)
-		return braille_unregister_console(console);
-
-	return 0;
+	return braille_unregister_console(console);
 }
diff --git a/kernel/printk/braille.h b/kernel/printk/braille.h
index 0bdac303f8b1..ec5feac1f508 100644
--- a/kernel/printk/braille.h
+++ b/kernel/printk/braille.h
@@ -11,6 +11,12 @@ braille_update_options(struct preferred_console *pc, char *brl_options)
 		pc->brl_options = brl_options;
 }
 
+static inline bool
+is_braille_console_preferred(struct preferred_console *pc)
+{
+	return (!!pc->brl_options);
+}
+
 /*
  * Setup console according to braille options.
  * Return -EINVAL on syntax error, 0 on success (or no braille option was
@@ -34,6 +40,12 @@ braille_update_options(struct preferred_console *pc, char *brl_options)
 {
 }
 
+static inline bool
+is_braille_console_preferred(struct preferred_console *pc)
+{
+	return false;
+}
+
 static inline int
 _braille_console_setup(char **str, char **brl_options)
 {
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ee57c7ac9d02..4b2865831a62 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3926,8 +3926,8 @@ static int try_enable_preferred_console(struct console *newcon,
 			if (newcon->index < 0)
 				newcon->index = pc->index;
 
-			if (_braille_register_console(newcon, pc))
-				return 0;
+			if (is_braille_console_preferred(pc))
+				return _braille_register_console(newcon, pc);
 
 			err = console_call_setup(newcon, pc->options);
 			if (err)
@@ -4239,17 +4239,14 @@ static int unregister_console_locked(struct console *console)
 	bool found_boot_con = false;
 	unsigned long flags;
 	struct console *c;
-	int res;
+	int res = 0;
 
 	lockdep_assert_console_list_lock_held();
 
 	con_printk(KERN_INFO, console, "disabled\n");
 
-	res = _braille_unregister_console(console);
-	if (res < 0)
-		return res;
-	if (res > 0)
-		return 0;
+	if (console->flags & CON_BRL)
+		return _braille_unregister_console(console);
 
 	if (!console_is_registered_locked(console))
 		res = -ENODEV;
-- 
2.52.0

[PATCH 5/8] printk: Try to register each console as Braille first

[Petr Mladek]

Braille consoles are unique: they can only be enabled via the command
line and use the preferred console framework for initialization, yet
they are never added to the console_list or associated with
/dev/console. Because of this, the preferred_console variable remains
unset when only a Braille console is requested.

Currently, try_enable_preferred_console() must be called even when
"preferred_dev_console" variable is not set, just to catch these "hidden"
Braille requests. The logic relies on non-obvious shortcuts within both
try_enable_preferred_console() and register_console(), making the
initialization flow fragile and difficult to follow.

Refactor the logic by adding a parameter to try_enable_preferred_console()
to explicitly handle Braille vs. non-Braille cases. This removes the
need for implicit shortcuts in register_console(). It will eventually
allow to skip try_enable_preferred_console() when there are no preferred
consoles. This improves code robustness by ensuring the console
setup is explicit and only performed once.

The refactoring even fixes two subtle bugs:

1. When only the Braille console is defined on the command line,
   the original code might attempt to enable the same console twice—once
   as a default and once as a Braille console. This results in calling
   the setup() callback twice and incorrectly setting the CON_CONSDEV flag.

2. When the same console is defined on the command line using devname
   and then as Braille console, for example:

      console=00:00:0.0,115200 console=brl,ttyS0,115200

   It would have two separate entries in preferred_consoles[] array.

   The 2nd (Braille) entry would be used when univ8250_console_init()
   tries to register the generic "ttyS" console driver. Note that
   the 1st entry still does not have defined the "name" entry at
   this stage.

   The 1st non-Braille entry would be used later when serial8250_init()
   registers all found devices, assigns "tty0" for the given "00:00:0.0"
   devname and tries to register the same struct console once again.

   The original code would call newcon->setup() twice in this scenario.
   It won't add the console into console_list only because the later
   check in register_console() would detect CON_BRL flag set from
   the 1st registration and return early.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 4b2865831a62..279b36ef90bd 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -365,6 +365,7 @@ static int console_locked;
 static struct preferred_console preferred_consoles[MAX_PREFERRED_CONSOLES];
 
 static int preferred_dev_console = -1;
+static bool want_braille_console;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
 
@@ -2554,7 +2555,9 @@ static int update_preferred_console(int i, const char *name, const short idx,
 
 	braille_update_options(pc, brl_options);
 
-	if (!brl_options)
+	if (brl_options)
+		want_braille_console = true;
+	else
 		preferred_dev_console = i;
 
 	/*
@@ -3900,8 +3903,9 @@ static int console_call_setup(struct console *newcon, char *options)
  * Care need to be taken with consoles that are statically
  * enabled such as netconsole
  */
-static int try_enable_preferred_console(struct console *newcon,
-					bool user_specified)
+static int __try_enable_preferred_console(struct console *newcon,
+					  bool user_specified,
+					  bool try_only_braille)
 {
 	struct preferred_console *pc;
 	int i, err;
@@ -3918,6 +3922,12 @@ static int try_enable_preferred_console(struct console *newcon,
 		    newcon->match(newcon, pc->name, pc->index, pc->options) != 0) {
 			/* default matching */
 			BUILD_BUG_ON(sizeof(pc->name) != sizeof(newcon->name));
+			/*
+			 * Two entries might have the same pc->name when one was
+			 * defined via "devname".
+			 */
+			if (try_only_braille && !is_braille_console_preferred(pc))
+				continue;
 			if (strcmp(pc->name, newcon->name) != 0)
 				continue;
 			if (newcon->index >= 0 &&
@@ -3926,7 +3936,7 @@ static int try_enable_preferred_console(struct console *newcon,
 			if (newcon->index < 0)
 				newcon->index = pc->index;
 
-			if (is_braille_console_preferred(pc))
+			if (try_only_braille)
 				return _braille_register_console(newcon, pc);
 
 			err = console_call_setup(newcon, pc->options);
@@ -3950,6 +3960,17 @@ static int try_enable_preferred_console(struct console *newcon,
 	return -ENOENT;
 }
 
+static int try_enable_preferred_console(struct console *newcon,
+					bool user_specified)
+{
+	return __try_enable_preferred_console(newcon, user_specified, false);
+}
+
+static int try_enable_braille_console(struct console *newcon)
+{
+	return __try_enable_preferred_console(newcon, true, true);
+}
+
 /* Try to enable the console unconditionally */
 static void try_enable_default_console(struct console *newcon)
 {
@@ -4103,6 +4124,20 @@ void register_console(struct console *newcon)
 			goto unlock;
 	}
 
+	/*
+	 * First, try to enable the console driver as a Braille console.
+	 * It would have metadata in the preferred_consoles[] array.
+	 * But it won't be counted as @preferred_console because
+	 * it does not get printk() messages and is not associated
+	 * with /dev/console.
+	 */
+	if (want_braille_console) {
+		err = try_enable_braille_console(newcon);
+		/* Return on success or when con->setup failed. */
+		if (err != -ENOENT)
+			goto unlock_free;
+	}
+
 	/*
 	 * See if we want to enable this console driver by default.
 	 *
@@ -4129,12 +4164,8 @@ void register_console(struct console *newcon)
 	if (err == -ENOENT)
 		err = try_enable_preferred_console(newcon, false);
 
-	/* printk() messages are not printed to the Braille console. */
-	if (err || newcon->flags & CON_BRL) {
-		if (newcon->flags & CON_NBCON)
-			nbcon_free(newcon);
-		goto unlock;
-	}
+	if (err)
+		goto unlock_free;
 
 	/*
 	 * If we have a bootconsole, and are switching to a real console,
@@ -4227,6 +4258,11 @@ void register_console(struct console *newcon)
 	printk_kthreads_check_locked();
 unlock:
 	console_list_unlock();
+	return;
+
+unlock_free:
+	nbcon_free(newcon);
+	goto unlock;
 }
 EXPORT_SYMBOL(register_console);
 
-- 
2.52.0

[PATCH 6/8] printk: Do not set Braille console as preferred_console

[Petr Mladek]

The Braille console reuses the framework for handling consoles preferred
via the command line. However, it is a special-purpose interface that
neither receives standard printk messages nor associates with
/dev/console.

Currently, the "preferred_dev_console" variable can point to a Braille
console entry in the "preferred_consoles[]" array. This occurs if an
entry was first created for a non-Braille console, but a later 'console='
parameter redefined it as a Braille console.

Since a Braille console will only ever be enabled as such, it should not
be tracked as the primary system console. Adjust the logic to ensure
"preferred_dev_console" continues to point to the previously designated
normal console instead.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 279b36ef90bd..eb224eaace64 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -365,6 +365,7 @@ static int console_locked;
 static struct preferred_console preferred_consoles[MAX_PREFERRED_CONSOLES];
 
 static int preferred_dev_console = -1;
+static int preferred_dev_console_prev = -1;
 static bool want_braille_console;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
@@ -2555,10 +2556,23 @@ static int update_preferred_console(int i, const char *name, const short idx,
 
 	braille_update_options(pc, brl_options);
 
-	if (brl_options)
+	if (brl_options) {
 		want_braille_console = true;
-	else
+		/*
+		 * This console name will always get enabled as Braille
+		 * console. It takes special code paths in register_console().
+		 * Do not treat it as a normal preferred_console.
+		 */
+		if (preferred_dev_console == i)
+			preferred_dev_console = preferred_dev_console_prev;
+	} else {
+		/*
+		 * Only the VisioBraille device is supported at the moment.
+		 * One level history should be enough.
+		 */
+		preferred_dev_console_prev = preferred_dev_console;
 		preferred_dev_console = i;
+	}
 
 	/*
 	 * @c console was defined by the user on the command line.
-- 
2.52.0

[PATCH 7/8] printk: Handle pre-enabled consoles directly in register_console()

[Petr Mladek]

The function try_enable_preferred_console() currently has the
non-obvious side effect of returning success for consoles that are
already pre-enabled. This obscures the logic flow during console
registration.

Move the check for pre-enabled consoles directly into
register_console(). This change makes the handling of pre-enabled
consoles explicit and easier to follow.

Furthermore, this separation lays the groundwork for future cleanups
where try_enable_preferred_console() can be restricted to cases where
an entry actually exists in the preferred_consoles[] array.

Possible behavior change:

try_enable_preferred_console() will newly be called also with
@user_specified parameter set to "false" when it failed with the "true"
variant. But it looks like the right way to do. It will allow to call
newcon->setup() when the console was preferred by some platform
specific code.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index eb224eaace64..db91be780de9 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3913,9 +3913,6 @@ static int console_call_setup(struct console *newcon, char *options)
  * 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_preferred_console(struct console *newcon,
 					  bool user_specified,
@@ -3963,14 +3960,6 @@ static int __try_enable_preferred_console(struct console *newcon,
 		return 0;
 	}
 
-	/*
-	 * Some consoles, such as pstore and netconsole, can be enabled even
-	 * without matching. Accept the pre-enabled consoles only when match()
-	 * and setup() had a chance to be called.
-	 */
-	if (newcon->flags & CON_ENABLED && pc->user_specified == user_specified)
-		return 0;
-
 	return -ENOENT;
 }
 
@@ -4178,6 +4167,14 @@ void register_console(struct console *newcon)
 	if (err == -ENOENT)
 		err = try_enable_preferred_console(newcon, false);
 
+	/*
+	 * Some consoles, such as pstore and netconsole, can be enabled even
+	 * without matching. Accept them at this stage when they had a chance
+	 * to match() and call setup().
+	 */
+	if (err == -ENOENT && (newcon->flags & CON_ENABLED))
+		err = 0;
+
 	if (err)
 		goto unlock_free;
 
-- 
2.52.0

[PATCH 8/8] printk: Try enable preferred consoles only when there are any

[Petr Mladek]

try_enable_preferred_console() used to be always called because it
had several hidden effects, namely:

- enabled Braille consoles which were ignored by "preferred_dev_console"
  because they were not associated with /dev/console.

- returned success when a console was pre-enabled using CON_ENABLED
  flag.

- returned success when a console was enabled by default because
  try_enable_default_console() did not return success.

The first two hidden effects were removed in previous patches. Remove
the last one so that try_enable_preferred_console() can be called only
when any non-Braille console is preferred.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index db91be780de9..462d870feaf2 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3975,18 +3975,23 @@ static int try_enable_braille_console(struct console *newcon)
 }
 
 /* Try to enable the console unconditionally */
-static void try_enable_default_console(struct console *newcon)
+static int try_enable_default_console(struct console *newcon)
 {
+	int err;
+
 	if (newcon->index < 0)
 		newcon->index = 0;
 
-	if (console_call_setup(newcon, NULL) != 0)
-		return;
+	err = console_call_setup(newcon, NULL);
+	if (err)
+		return err;
 
 	newcon->flags |= CON_ENABLED;
 
 	if (newcon->device)
 		newcon->flags |= CON_CONSDEV;
+
+	return 0;
 }
 
 /* Return the starting sequence number for a newly registered console. */
@@ -4156,17 +4161,15 @@ void register_console(struct console *newcon)
 	if (preferred_dev_console < 0) {
 		if (hlist_empty(&console_list) || !console_first()->device ||
 		    console_first()->flags & CON_BOOT) {
-			try_enable_default_console(newcon);
+			err = try_enable_default_console(newcon);
 		}
+	} else {
+		err = try_enable_preferred_console(newcon, true);
+
+		if (err == -ENOENT)
+			err = try_enable_preferred_console(newcon, false);
 	}
 
-	/* See if this console matches one we selected on the command line */
-	err = try_enable_preferred_console(newcon, true);
-
-	/* If not, try to match against the platform default(s) */
-	if (err == -ENOENT)
-		err = try_enable_preferred_console(newcon, false);
-
 	/*
 	 * Some consoles, such as pstore and netconsole, can be enabled even
 	 * without matching. Accept them at this stage when they had a chance
-- 
2.52.0

Re: [PATCH 3/8] printk: Separate code for adding/updating preferred console metadata

[John Ogness]

Hi Petr,

On 2026-02-06, Petr Mladek <pmladek@suse.com> wrote:
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 3f856a438e74..ee57c7ac9d02 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2491,18 +2491,82 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
>  }
>  #endif
>  
> -static void set_user_specified(struct preferred_console *pc, bool user_specified)
> +static int update_preferred_console(int i, const char *name, const short idx,

Perhaps @i should be unsigned in order to guarantee no possibility of
negative array indexing.

It would need to be defined that way in __add_preferred_console() as well.

> +				    const char *devname, char *options,
> +				    char *brl_options, bool user_specified)
>  {
> -	if (!user_specified)
> -		return;
> +	struct preferred_console *pc;
> +
> +	if (i >= MAX_PREFERRED_CONSOLES)
> +		return -E2BIG;
> +
> +	pc = &preferred_consoles[i];
> +
> +	if (!name && !devname)
> +		return -EINVAL;
> +
> +	if (devname) {
> +		/*
> +		 * A valid console name and index will get assigned when
> +		 * a matching device gets registered.
> +		 */
> +		if (name) {
> +			pr_err("Adding a preferred console devname with a hard-coded console name: %s, %s\n",
> +			       devname, name);
> +			return -EINVAL;
> +		}
> +		if (idx != -1) {
> +			pr_err("Adding a preferred console devname with a hard-coded index: %s, %d\n",
> +			       devname, idx);
> +			return -EINVAL;
> +		}
> +
> +		if (!pc->devname[0]) {
> +			strscpy(pc->devname, devname);
> +			pc->index = idx;
> +		} else if (strcmp(pc->devname, devname) != 0) {
> +			pr_err("Updating a preferred console with an invalid devname: %s vs. %s\n",
> +			       pc->devname, devname);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (name) {
> +		/* A console name must be defined with a valid index. */
> +		if (idx < 0) {
> +			pr_err("Adding a preferred console with an invalid index: %s, %d\n",
> +			       name, idx);
> +			return -EINVAL;
> +		}
> +
> +		if (!pc->name[0]) {
> +			strscpy(pc->name, name);
> +			pc->index = idx;
> +		} else if (strcmp(pc->name, name) != 0 || pc->index != idx) {
> +			pr_err("Updating a preferred console with an invalid name or index: %s%d vs. %s%d\n",
> +			       pc->name, pc->index, name, idx);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (!pc->options || (user_specified && options))
> +		pc->options = options;
> +
> +	braille_update_options(pc, brl_options);
> +
> +	if (!brl_options)
> +		preferred_dev_console = i;
>  
>  	/*
>  	 * @c console was defined by the user on the command line.
>  	 * Do not clear when added twice also by SPCR or the device tree.
>  	 */
> -	pc->user_specified = true;
> -	/* At least one console defined by the user on the command line. */
> -	console_set_on_cmdline = 1;
> +	if (user_specified) {
> +		pc->user_specified = true;
> +		console_set_on_cmdline = 1;
> +	}
> +
> +	return 0;
>  }
>  
>  static int __add_preferred_console(const char *name, const short idx,

There are a lot of rules to arguments of __add_preferred_console(). Can
you add some comment above the __add_preferred_console() function
definition about these rules? I think this patch is an appropriate place
to do that since the rules are quite visible with your changes to
update_preferred_console(). For example, mentioning:

- required: either @name and a valid @idx OR @devname and idx=-1
- specify @brl_options if it is a Braille console
- Braille consoles will never be associated with /dev/console

And a simple description like "Add a new preferred console or update the
options of an already registered preferred console."

John Ogness

Re: [PATCH 6/8] printk: Do not set Braille console as preferred_console

[John Ogness]

On 2026-02-06, Petr Mladek <pmladek@suse.com> wrote:
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 279b36ef90bd..eb224eaace64 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -365,6 +365,7 @@ static int console_locked;
>  static struct preferred_console preferred_consoles[MAX_PREFERRED_CONSOLES];
>  
>  static int preferred_dev_console = -1;
> +static int preferred_dev_console_prev = -1;
>  static bool want_braille_console;
>  int console_set_on_cmdline;
>  EXPORT_SYMBOL(console_set_on_cmdline);
> @@ -2555,10 +2556,23 @@ static int update_preferred_console(int i, const char *name, const short idx,
>  
>  	braille_update_options(pc, brl_options);
>  
> -	if (brl_options)
> +	if (brl_options) {
>  		want_braille_console = true;
> -	else
> +		/*
> +		 * This console name will always get enabled as Braille
> +		 * console. It takes special code paths in register_console().
> +		 * Do not treat it as a normal preferred_console.
> +		 */
> +		if (preferred_dev_console == i)
> +			preferred_dev_console = preferred_dev_console_prev;

I am wondering if in this case it should also do:

			preferred_dev_console_prev = -1;

to make sure that @preferred_dev_console never ends up at a Braille
device.

> +	} else {
> +		/*
> +		 * Only the VisioBraille device is supported at the moment.
> +		 * One level history should be enough.
> +		 */
> +		preferred_dev_console_prev = preferred_dev_console;
>  		preferred_dev_console = i;
> +	}
>  
>  	/*
>  	 * @c console was defined by the user on the command line.

John Ogness

Re: [PATCH 0/8] printk: Clean up preferred console handling

[John Ogness]

Hi Petr,

On 2026-02-06, Petr Mladek <pmladek@suse.com> wrote:
> this patches does some clean up of the code for handling preferred consoles
> in the console registration code.
>
>   + 1st and 2nd patch try to improve a naming.
>
>     The meaning of struct console_cmdline has changed over time.
>     The meaning of "preferred_console" variable has always been a bit
>     confusing and it has got even worse after adding the support for
>     Braille consoles.
>
>
>   + 3rd patch remove some code duplication. It better defines and describes
>     the rules for adding and updating preferred consoles. It uses a more
>     defensive coding style.
>
>
>   + 4th, 5th, and 6th patch improve handling of Braille consoles.
>     They are preferred via the command line but they do not get
>     printk() messages and are not associated with /dev/console.
>
>     The new code makes this more obvious. Also it explicitly
>     defines the relation against default consoles and other
>     non-Braille preferred consoles.
>
>
>   + 7th patch removes a hidden side effect of
>     try_enable_preferred_console()
>
>
>   + 8th patch removes the last hidden side effect of
>     try_enable_preferred_console() and allows to call it
>     only when there are any non-Braille preferred consoles.
>
>
> I tested many scenarios of fixed several bugs. I did my best to
> prevent regressions.
>
> This patchset is a prerequisite for Marcos' clean up of CON_ENABLE
> flag handling. It should prevent regressions caused by the
> hidden effects of try_enable_preferred_console(), for example,
> see https://lore.kernel.org/r/89409a0f48e6998ff6dd2245691b9954f0e1e435.camel@suse.com
>
> Also I am working on a feature which would allow to explicitly
> enable/prefer consoles proposed by SPCR, device tree, or
> platform-specific code using a generic "console=platform".
> This clean up is a prerequisite, see
> https://github.com/pmladek/linux/tree/console-platform-poc1-iter9
>
> The patchset has been re-based on top of v6.19-rc8.

I have finished my review. I just had feedback relating to
update_preferred_console() and also adding some function description
comments to __add_preferred_console().

John

Re: [PATCH 3/8] printk: Separate code for adding/updating preferred console metadata

[Petr Mladek]

On Mon 2026-02-16 15:11:59, John Ogness wrote:
> Hi Petr,
> 
> On 2026-02-06, Petr Mladek <pmladek@suse.com> wrote:
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 3f856a438e74..ee57c7ac9d02 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2491,18 +2491,82 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
> >  }
> >  #endif
> >  
> > -static void set_user_specified(struct preferred_console *pc, bool user_specified)
> > +static int update_preferred_console(int i, const char *name, const short idx,
> 
> Perhaps @i should be unsigned in order to guarantee no possibility of
> negative array indexing.
> 
> It would need to be defined that way in __add_preferred_console() as well.

Makes sense. I'll do it in v2.

> > +				    const char *devname, char *options,
> > +				    char *brl_options, bool user_specified)
> >  {
> > -	if (!user_specified)
> > -		return;
> > +	struct preferred_console *pc;
> > +
> > +	if (i >= MAX_PREFERRED_CONSOLES)
> > +		return -E2BIG;
> > +
> > +	pc = &preferred_consoles[i];
> > +
> > +	if (!name && !devname)
> > +		return -EINVAL;
> > +
> > +	if (devname) {
> > +		/*
> > +		 * A valid console name and index will get assigned when
> > +		 * a matching device gets registered.
> > +		 */
> > +		if (name) {
> > +			pr_err("Adding a preferred console devname with a hard-coded console name: %s, %s\n",
> > +			       devname, name);
> > +			return -EINVAL;
> > +		}
> > +		if (idx != -1) {
> > +			pr_err("Adding a preferred console devname with a hard-coded index: %s, %d\n",
> > +			       devname, idx);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (!pc->devname[0]) {
> > +			strscpy(pc->devname, devname);
> > +			pc->index = idx;
> > +		} else if (strcmp(pc->devname, devname) != 0) {
> > +			pr_err("Updating a preferred console with an invalid devname: %s vs. %s\n",
> > +			       pc->devname, devname);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	if (name) {
> > +		/* A console name must be defined with a valid index. */
> > +		if (idx < 0) {
> > +			pr_err("Adding a preferred console with an invalid index: %s, %d\n",
> > +			       name, idx);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (!pc->name[0]) {
> > +			strscpy(pc->name, name);
> > +			pc->index = idx;
> > +		} else if (strcmp(pc->name, name) != 0 || pc->index != idx) {
> > +			pr_err("Updating a preferred console with an invalid name or index: %s%d vs. %s%d\n",
> > +			       pc->name, pc->index, name, idx);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	if (!pc->options || (user_specified && options))
> > +		pc->options = options;
> > +
> > +	braille_update_options(pc, brl_options);
> > +
> > +	if (!brl_options)
> > +		preferred_dev_console = i;
> >  
> >  	/*
> >  	 * @c console was defined by the user on the command line.
> >  	 * Do not clear when added twice also by SPCR or the device tree.
> >  	 */
> > -	pc->user_specified = true;
> > -	/* At least one console defined by the user on the command line. */
> > -	console_set_on_cmdline = 1;
> > +	if (user_specified) {
> > +		pc->user_specified = true;
> > +		console_set_on_cmdline = 1;
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  static int __add_preferred_console(const char *name, const short idx,
> 
> There are a lot of rules to arguments of __add_preferred_console(). Can
> you add some comment above the __add_preferred_console() function
> definition about these rules? I think this patch is an appropriate place
> to do that since the rules are quite visible with your changes to
> update_preferred_console(). For example, mentioning:
> 
> - required: either @name and a valid @idx OR @devname and idx=-1
> - specify @brl_options if it is a Braille console
> - Braille consoles will never be associated with /dev/console
> 
> And a simple description like "Add a new preferred console or update the
> options of an already registered preferred console."

I am preparing v2 and added this:

/** update_preferred_console - Update a given entry in the preferred_consoles[]
 *	table.
 * @i: index of the entry in @preferred_consoles table which should get updated.
 * @name: The name of the preferred console driver.
 * @idx: Preferred console index, e.g. port number.
 * @devname: The name of the preferred physical device.
 * @options: Options used when setting up the console driver.
 * @brl_options: Options used when setting up the console driver
 *	as a braille console.
 * @user_specified: True if preferred via the kernel command line.
 *
 * The function ensures that the given values are consistent. Also
 * it updates some global variables which are used to make the right
 * decisions in register_console().
 *
 * Rules:
 *
 *   1. Either @name and valid @idx OR @devname and @idx=-1 are allowed.
 *	Note that a valid @name and @idx will get assigned later when
 *	@devname matches during the device initialization.
 *   2. Specify @brl_options if the console should be enabled as
 *	a Braille console [*]
 *   3. Only matching entries can be updated.
 *   4. @options passed via the command line are used when the same
 *	console is is preferred also by some platform-specific code.
 *
 * [*] Braille console is using the mechanism for registering consoles
 *     but it is very special. It is primary used for an user interaction
 *     with the system. It neither gets printk() messages nor is
 *     associated with /dev/cosnole.
 */

Best Regards,
Petr

Re: [PATCH 3/8] printk: Separate code for adding/updating preferred console metadata

[John Ogness]

On 2026-02-19, Petr Mladek <pmladek@suse.com> wrote:
>> On 2026-02-06, Petr Mladek <pmladek@suse.com> wrote:
>> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> > index 3f856a438e74..ee57c7ac9d02 100644
>> > --- a/kernel/printk/printk.c
>> > +++ b/kernel/printk/printk.c
>> > @@ -2491,18 +2491,82 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
>> >  }
>> >  #endif
>> >  
>> > -static void set_user_specified(struct preferred_console *pc, bool user_specified)
>> > +static int update_preferred_console(int i, const char *name, const short idx,
>> 
>> Perhaps @i should be unsigned in order to guarantee no possibility of
>> negative array indexing.
>> 
>> It would need to be defined that way in __add_preferred_console() as well.
>
> Makes sense. I'll do it in v2.

Note that it might cause some warnings due to comparing signed and
unsigned. It might be preferrable to check for non-negative rather than
making it unsigned.

> I am preparing v2 and added this:
>
> /** update_preferred_console - Update a given entry in the preferred_consoles[]
>  *	table.
>  * @i: index of the entry in @preferred_consoles table which should get updated.
>  * @name: The name of the preferred console driver.
>  * @idx: Preferred console index, e.g. port number.
>  * @devname: The name of the preferred physical device.
>  * @options: Options used when setting up the console driver.
>  * @brl_options: Options used when setting up the console driver
>  *	as a braille console.
>  * @user_specified: True if preferred via the kernel command line.
>  *
>  * The function ensures that the given values are consistent. Also
>  * it updates some global variables which are used to make the right
>  * decisions in register_console().
>  *
>  * Rules:
>  *
>  *   1. Either @name and valid @idx OR @devname and @idx=-1 are allowed.
>  *	Note that a valid @name and @idx will get assigned later when
>  *	@devname matches during the device initialization.
>  *   2. Specify @brl_options if the console should be enabled as
>  *	a Braille console [*]
>  *   3. Only matching entries can be updated.
>  *   4. @options passed via the command line are used when the same
>  *	console is is preferred also by some platform-specific code.
>  *
>  * [*] Braille console is using the mechanism for registering consoles
>  *     but it is very special. It is primary used for an user interaction
>  *     with the system.

Grammar fixup:

It is primarily used for user interaction with the system.
>  *     with the system. It neither gets printk() messages nor is
>  *     associated with /dev/cosnole.

/dev/console spelled wrong.

>  */

Otherwise, LGTM.

I can provide reviewed-by tags once I look over v2.

John

Re: [PATCH 1/8] printk: Rename struct console_cmdline to preferred_console

[Chris Down]

The rename itself looks correct and is a clear improvement. Only one small 
thing:

Petr Mladek writes:
>-static void set_user_specified(struct console_cmdline *c, bool user_specified)
>+static void set_user_specified(struct preferred_console *pc, bool user_specified)
> {
> 	if (!user_specified)
> 		return;
>@@ -2500,7 +2500,7 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified)
> 	 * @c console was defined by the user on the command line.
> 	 * Do not clear when added twice also by SPCR or the device tree.
> 	 */

The comment is stale, it should be @pc.

With that fix:

Acked-by: Chris Down <chris@chrisdown.name>

Re: [PATCH 2/8] printk: Rename preferred_console to preferred_dev_console

[Chris Down]

Petr Mladek writes:
>The preferred_consoles[] array stores information about consoles requested
>via the command line, SPCR, Device Tree, or platform-specific code.
>Within this array, the 'preferred_console' variable tracks the specific
>index that should be associated with /dev/console (typically the last
>non-braille console defined).
>
>The current name "preferred_console" is ambiguous and leads to confusion.
>It does not clearly communicate why one console is "more preferred" than
>others in the array. Furthermore, entries for Braille consoles can exist
>within the preferred_consoles[] array, yet they are never associated with
>/dev/console and do not receive standard printk() output. Consequently,
>the 'preferred_console' index must skip these entries, which is not
>immediately obvious from the name.
>
>Rename the variable to 'preferred_dev_console' to explicitly clarify its
>role in identifying which entry is linked to /dev/console.
>
>Signed-off-by: Petr Mladek <pmladek@suse.com>

Acked-by: Chris Down <chris@chrisdown.name>

Re: [PATCH 3/8] printk: Separate code for adding/updating preferred console metadata

[Chris Down]

Petr Mladek writes:
>@@ -2531,28 +2587,14 @@ static int __add_preferred_console(const char *name, const short idx,
> 	for (i = 0, pc = preferred_consoles;
> 	     i < MAX_PREFERRED_CONSOLES && (pc->name[0] || pc->devname[0]);
> 	     i++, pc++) {
>-		if ((name && strcmp(pc->name, name) == 0 && pc->index == idx) ||
>-		    (devname && strcmp(pc->devname, devname) == 0)) {
>-			if (!brl_options)
>-				preferred_dev_console = i;
>-			set_user_specified(pc, user_specified);
>-			return 0;
>-		}

So, in this old code when __add_preferred_console() finds an existing matching 
entry, it returns immediately without touching the options at all.

But in the new patch the loop breaks out and falls through to 
update_preferred_console(), which only conditionally updates options.

This has different behaviour. For example if you imagine SPCR/DT setting the 
options, before they win out against the command line, but now they do not.

This may well be intended, since the user cmdline probably should win over the 
platform defaults anyway. I'd argue that sounds like the right behaviour 
anyway. But I don't see this change being mentioned as intentional in the 
changelog. Is it?  If it is, let's mention it there.

With this clarified:

Acked-by: Chris Down <chris@chrisdown.name>

Re: [PATCH 4/8] printk: Cleanup _braille_(un)register_console() wrappers

[Chris Down]

Petr Mladek writes:
>The _braille_(un)register_console() wrappers currently attempt to hide
>implementation details like the CON_BRL flag and pc->brl_options. This
>forces callers to handle an unconventional tri-state return value (0 for
>NOP, >0 for success, <0 for error), which makes the control flow harder
>to follow and non-standard.
>
>Refactor the wrappers to use standard kernel return codes (0 for success,
>-ERRCODE on failure). Move the responsibility of checking brl_options
>to the caller to make the logic more explicit.
>
>Additionally, move the assignment of the CON_BRL flag from the internal
>wrapper to braille_register_console(). This aligns it with how
>CON_ENABLED is handled. To maintain symmetry and fix a potential bug
>where flags might persist after removal, explicitly clear both
>CON_ENABLED and CON_BRL in braille_unregister_console().
>
>Signed-off-by: Petr Mladek <pmladek@suse.com>

Nice, looks good. Thanks!

Acked-by: Chris Down <chris@chrisdown.name>

Re: [PATCH 6/8] printk: Do not set Braille console as preferred_console

[Petr Mladek]

On Mon 2026-02-16 17:13:10, John Ogness wrote:
> On 2026-02-06, Petr Mladek <pmladek@suse.com> wrote:
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 279b36ef90bd..eb224eaace64 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -365,6 +365,7 @@ static int console_locked;
> >  static struct preferred_console preferred_consoles[MAX_PREFERRED_CONSOLES];
> >  
> >  static int preferred_dev_console = -1;
> > +static int preferred_dev_console_prev = -1;
> >  static bool want_braille_console;
> >  int console_set_on_cmdline;
> >  EXPORT_SYMBOL(console_set_on_cmdline);
> > @@ -2555,10 +2556,23 @@ static int update_preferred_console(int i, const char *name, const short idx,
> >  
> >  	braille_update_options(pc, brl_options);
> >  
> > -	if (brl_options)
> > +	if (brl_options) {
> >  		want_braille_console = true;
> > -	else
> > +		/*
> > +		 * This console name will always get enabled as Braille
> > +		 * console. It takes special code paths in register_console().
> > +		 * Do not treat it as a normal preferred_console.
> > +		 */
> > +		if (preferred_dev_console == i)
> > +			preferred_dev_console = preferred_dev_console_prev;
> 
> I am wondering if in this case it should also do:
> 
> 			preferred_dev_console_prev = -1;
> 
> to make sure that @preferred_dev_console never ends up at a Braille
> device.

Great catch!

> > +	} else {
> > +		/*
> > +		 * Only the VisioBraille device is supported at the moment.
> > +		 * One level history should be enough.
> > +		 */
> > +		preferred_dev_console_prev = preferred_dev_console;
> >  		preferred_dev_console = i;
> > +	}

We actually also need to prevent setting "preferred_dev_console" when
the same console has been preferred as Braille before, for example:

       console=brl,ttyS0,115200 console=ttyS0,115200

One might argue that the later definition should win. But it would
be a regression.

The motivation to keep the current behavior is that the Braille console
is special. IMHO, it should always win. Otherwise, the user might have
hard times to debug the problem.

My new version of this code is:

	/*
	 * The last preferred console should get associated with /dev/console.
	 * Except for the Braille console which can't get associated with
	 * /dev/console. One level history should be enough because only one,
	 * the VisioBraille device, is supported at the moment.
	 */
	if (brl_options) {
		want_braille_console = true;
		if (preferred_dev_console == i) {
			preferred_dev_console = preferred_dev_console_prev;
			preferred_dev_console_prev = -1;
		}
	} else if (!is_braille_console_preferred(pc)) {
		preferred_dev_console_prev = preferred_dev_console;
		preferred_dev_console = i;
	}

Best Regards,
Petr

Re: [PATCH 5/8] printk: Try to register each console as Braille first

[Chris Down]

Petr Mladek writes:
>@@ -3918,6 +3922,12 @@ static int try_enable_preferred_console(struct console *newcon,
> 		    newcon->match(newcon, pc->name, pc->index, pc->options) != 0) {
> 			/* default matching */
> 			BUILD_BUG_ON(sizeof(pc->name) != sizeof(newcon->name));
>+			/*
>+			 * Two entries might have the same pc->name when one was
>+			 * defined via "devname".
>+			 */
>+			if (try_only_braille && !is_braille_console_preferred(pc))
>+				continue;
> 			if (strcmp(pc->name, newcon->name) != 0)
> 				continue;
> 			if (newcon->index >= 0 &&
>@@ -3926,7 +3936,7 @@ static int try_enable_preferred_console(struct console *newcon,
> 			if (newcon->index < 0)
> 				newcon->index = pc->index;
>
>-			if (is_braille_console_preferred(pc))
>+			if (try_only_braille)
> 				return _braille_register_console(newcon, pc);
>
> 			err = console_call_setup(newcon, pc->options);

This doesn't look right to me. By putting the Braille dispatch inside the if 
block, you bypass it whenever newcon->match matches.

So that means if a console driver matches, the execution skips this 
default matching block completely, falls through, and silently sets CON_ENABLED 
without ever registering it as a Braille console or adding it to console_list, 
and the console is silently lost.

The try_only_braille and is_braille_console_preferred(pc) checks likely need to 
happen before or independently of the match() vs. default matching branch.

Re: [PATCH 6/8] printk: Do not set Braille console as preferred_console

[Chris Down]

Petr Mladek writes:
>The Braille console reuses the framework for handling consoles preferred
>via the command line. However, it is a special-purpose interface that
>neither receives standard printk messages nor associates with
>/dev/console.
>
>Currently, the "preferred_dev_console" variable can point to a Braille
>console entry in the "preferred_consoles[]" array. This occurs if an
>entry was first created for a non-Braille console, but a later 'console='
>parameter redefined it as a Braille console.
>
>Since a Braille console will only ever be enabled as such, it should not
>be tracked as the primary system console. Adjust the logic to ensure
>"preferred_dev_console" continues to point to the previously designated
>normal console instead.
>
>Signed-off-by: Petr Mladek <pmladek@suse.com>

Looks good to me, after fixing John's comment about resetting 
preferred_dev_console_prev, please feel free to add:

Acked-by: Chris Down <chris@chrisdown.name>

Re: [PATCH 7/8] printk: Handle pre-enabled consoles directly in register_console()

[Chris Down]

Petr Mladek writes:
>The function try_enable_preferred_console() currently has the
>non-obvious side effect of returning success for consoles that are
>already pre-enabled. This obscures the logic flow during console
>registration.
>
>Move the check for pre-enabled consoles directly into
>register_console(). This change makes the handling of pre-enabled
>consoles explicit and easier to follow.
>
>Furthermore, this separation lays the groundwork for future cleanups
>where try_enable_preferred_console() can be restricted to cases where
>an entry actually exists in the preferred_consoles[] array.
>
>Possible behavior change:
>
>try_enable_preferred_console() will newly be called also with
>@user_specified parameter set to "false" when it failed with the "true"
>variant. But it looks like the right way to do. It will allow to call
>newcon->setup() when the console was preferred by some platform
>specific code.
>
>Signed-off-by: Petr Mladek <pmladek@suse.com>

Looks cracking, thanks.

Acked-by: Chris Down <chris@chrisdown.name>

Re: [PATCH 8/8] printk: Try enable preferred consoles only when there are any

[Chris Down]

Petr Mladek writes:
>try_enable_preferred_console() used to be always called because it
>had several hidden effects, namely:
>
>- enabled Braille consoles which were ignored by "preferred_dev_console"
>  because they were not associated with /dev/console.
>
>- returned success when a console was pre-enabled using CON_ENABLED
>  flag.
>
>- returned success when a console was enabled by default because
>  try_enable_default_console() did not return success.
>
>The first two hidden effects were removed in previous patches. Remove
>the last one so that try_enable_preferred_console() can be called only
>when any non-Braille console is preferred.

There is a fourth hidden effect that was also removed, unconditionally 
initialising err :-) Unfortunately removing that causes an uninitialised read.

>@@ -4156,17 +4161,15 @@ void register_console(struct console *newcon)
> 	if (preferred_dev_console < 0) {
> 		if (hlist_empty(&console_list) || !console_first()->device ||
> 		    console_first()->flags & CON_BOOT) {
>-			try_enable_default_console(newcon);
>+			err = try_enable_default_console(newcon);
> 		}
>+	} else {
>+		err = try_enable_preferred_console(newcon, true);
>+
>+		if (err == -ENOENT)
>+			err = try_enable_preferred_console(newcon, false);
> 	}

When preferred_dev_console < 0 and the inner condition is false, nothing will 
initialise err.

>
>-	/* See if this console matches one we selected on the command line */
>-	err = try_enable_preferred_console(newcon, true);
>-
>-	/* If not, try to match against the platform default(s) */
>-	if (err == -ENOENT)
>-		err = try_enable_preferred_console(newcon, false);

You can see previously it was initialised without conditions here.

Later we read err with `if (err == -ENOENT)`, so it's possible to do an 
uninitialised read.

Doing `int err = -ENOENT` at declaration should fix it by the look of it.

Re: [PATCH 0/8] printk: Clean up preferred console handling

[Chris Down]

Hi Petr,

Thanks for this series! The overall concept looks great and having reviewed it 
I can say the resulting code is definitely much easier to follow.

I left some comments on individual patches. With the bugs in patch 5 and 8 
fixed I would ack the whole series. There are also some more minor comments on 
some of the other patches.

Best,

Chris

Re: [PATCH 6/8] printk: Do not set Braille console as preferred_console

[John Ogness]

On 2026-02-19, Petr Mladek <pmladek@suse.com> wrote:
> We actually also need to prevent setting "preferred_dev_console" when
> the same console has been preferred as Braille before, for example:
>
>        console=brl,ttyS0,115200 console=ttyS0,115200
>
> One might argue that the later definition should win. But it would
> be a regression.
>
> The motivation to keep the current behavior is that the Braille console
> is special. IMHO, it should always win. Otherwise, the user might have
> hard times to debug the problem.

I have no experience about how a Braille console is used/configured in
the real world. But your reasoning is clear. Once a device has been
flagged as a Braille device, it can never be un-flagged.

> My new version of this code is:
>
> 	/*
> 	 * The last preferred console should get associated with /dev/console.
> 	 * Except for the Braille console which can't get associated with
> 	 * /dev/console. One level history should be enough because only one,
> 	 * the VisioBraille device, is supported at the moment.
> 	 */
> 	if (brl_options) {
> 		want_braille_console = true;
> 		if (preferred_dev_console == i) {
> 			preferred_dev_console = preferred_dev_console_prev;
> 			preferred_dev_console_prev = -1;
> 		}
> 	} else if (!is_braille_console_preferred(pc)) {
> 		preferred_dev_console_prev = preferred_dev_console;
> 		preferred_dev_console = i;
> 	}

Yes, this looks good. Thanks.

John

Re: [PATCH 3/8] printk: Separate code for adding/updating preferred console metadata

[Petr Mladek]

On Thu 2026-02-19 22:48:49, Chris Down wrote:
> Petr Mladek writes:
> > @@ -2531,28 +2587,14 @@ static int __add_preferred_console(const char *name, const short idx,
> > 	for (i = 0, pc = preferred_consoles;
> > 	     i < MAX_PREFERRED_CONSOLES && (pc->name[0] || pc->devname[0]);
> > 	     i++, pc++) {
> > -		if ((name && strcmp(pc->name, name) == 0 && pc->index == idx) ||
> > -		    (devname && strcmp(pc->devname, devname) == 0)) {
> > -			if (!brl_options)
> > -				preferred_dev_console = i;
> > -			set_user_specified(pc, user_specified);
> > -			return 0;
> > -		}
> 
> So, in this old code when __add_preferred_console() finds an existing
> matching entry, it returns immediately without touching the options at all.
> 
> But in the new patch the loop breaks out and falls through to
> update_preferred_console(), which only conditionally updates options.
> 
> This has different behaviour. For example if you imagine SPCR/DT setting the
> options, before they win out against the command line, but now they do not.
> 
> This may well be intended, since the user cmdline probably should win over
> the platform defaults anyway. I'd argue that sounds like the right behaviour
> anyway. But I don't see this change being mentioned as intentional in the
> changelog. Is it?  If it is, let's mention it there.

Good point!

I considered this as a bug fix and I described this in the last
paragraph of the commit message:

<paste>
While this is primarily a refactoring, it fixes two minor behavioral
consistencies: console options can now be overridden via the command
line, and Braille options are preserved even if a non-Braille console
with the same name was previously defined.
</paste>


I hope that this change should be on the safe size because:

  1. People should be able to override any default value on the
     command line. It seems to be the only sane approach.

  2. 20 of 33 add_preferred_console() callers do not pass options[*]

  3. I somehow hooped that add_preferred_console() is mostly called
     before the command line is proceed. Otherwise, the command line
     would not be able to overwrite the options. Also /dev/console
     is supposed to be associated with the last console= defined
     on the command line.

     I have to admit that I checked this only on x86_64 where the spcr
     is handled before the command line, see:

       + setup_arch()
	 + acpi_boot_init()
	   + acpi_parse_spcr()

      Hmm, I see that the device tree seems to be handled later, for example,

	+ serial_core_add_one_port()
	  + of_console_check()


Honestly, I am a bit afraid of regressions caused by this change.
But I hope that it will be a corner case and users might
be persuaded that their command line was bogus.

As I said, people should be able to override the defaults via the
command line. And I do not see any other way how to achieve it.

By other words, the sooner we enforce a sane rule the better.
I hope that it is not too late. And the only way to check it
in the real life is to try it. /o\.

But you are right, I should be more clear about this in
the commit message. I am going to improve it in v2.

[*] 

$> git grep add_preferred_console | grep -v -e "__add_pre" -e "xen_add_pref" -e "printk" -e "console.h" | wc -l
33
$> git grep add_preferred_console | grep -v -e "__add_pre" -e "xen_add_pref" -e "printk" -e "console.h" | grep ", NULL);" | wc -l
20


Best Regards,
Petr

Re: [PATCH 5/8] printk: Try to register each console as Braille first

[Petr Mladek]

On Thu 2026-02-19 22:59:38, Chris Down wrote:
> Petr Mladek writes:
> > @@ -3918,6 +3922,12 @@ static int try_enable_preferred_console(struct console *newcon,
> > 		    newcon->match(newcon, pc->name, pc->index, pc->options) != 0) {
> > 			/* default matching */
> > 			BUILD_BUG_ON(sizeof(pc->name) != sizeof(newcon->name));
> > +			/*
> > +			 * Two entries might have the same pc->name when one was
> > +			 * defined via "devname".
> > +			 */
> > +			if (try_only_braille && !is_braille_console_preferred(pc))
> > +				continue;
> > 			if (strcmp(pc->name, newcon->name) != 0)
> > 				continue;
> > 			if (newcon->index >= 0 &&
> > @@ -3926,7 +3936,7 @@ static int try_enable_preferred_console(struct console *newcon,
> > 			if (newcon->index < 0)
> > 				newcon->index = pc->index;
> > 
> > -			if (is_braille_console_preferred(pc))
> > +			if (try_only_braille)
> > 				return _braille_register_console(newcon, pc);
> > 
> > 			err = console_call_setup(newcon, pc->options);
> 
> This doesn't look right to me. By putting the Braille dispatch inside the if
> block, you bypass it whenever newcon->match matches.
> 
> So that means if a console driver matches, the execution skips this default
> matching block completely, falls through, and silently sets CON_ENABLED
> without ever registering it as a Braille console or adding it to
> console_list, and the console is silently lost.
> 
> The try_only_braille and is_braille_console_preferred(pc) checks likely need
> to happen before or independently of the match() vs. default matching
> branch.

This should never happen because register_console() always tries to
register Braille consoles first.

This patch adds the following hunk into register_console():

@@ -4173,6 +4194,20 @@ void register_console(struct console *newcon)
 			goto unlock;
 	}
 
+	/*
+	 * First, try to enable the console driver as a Braille console.
+	 * It would have metadata in the preferred_consoles[] array.
+	 * But it won't be counted as @preferred_console because
+	 * it does not get printk() messages and is not associated
+	 * with /dev/console.
+	 */
+	if (want_braille_console) {
+		err = try_enable_braille_console(newcon);
+		/* Return on success or when con->setup failed. */
+		if (err != -ENOENT)
+			goto unlock_free;
+	}
+
 	/*
 	 * See if we want to enable this console driver by default.
 	 *

I am going to add a comment into try_enable_preferred_console() about
this.

Best Regards,
Petr

Re: [PATCH 1/8] printk: Rename struct console_cmdline to preferred_console

[Marcos Paulo de Souza]

On Fri, 2026-02-06 at 17:49 +0100, Petr Mladek wrote:
> The structure 'console_cmdline' was originally intended to store
> details
> about consoles defined on the kernel command line. However, its usage
> has since expanded; it now stores information for consoles preferred
> via SPCR, device tree, or by particular platforms, e.g. XEN.
> 
> The current naming is misleading as it implies the configuration only
> originates from the command line.
> 
> Rename the structure and associated artifacts to better reflect their
> current purpose, for example:
> 
>   - struct console_cmdline c -> struct preferred_console pc
>   - console_cmdline[]        -> preferred_consoles[]
>   - console_cmdline.h        -> console_register.h
>   - c			     -> pc
> 
> Additionally, renaming the header file to console_register.h would
> eventually allow to decouple console registration logic from
> the monolithic printk.c.
> 
> Finally, renaming the local variable from "c" to "pc" helps to
> distinguish
> it from struct console variables. Note that "c" is used for struct
> console
> in some code, for example see vt_console_device() function
> definition.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

The renaming is very welcome, since it was indeed confusing to
distinguish between console_cmdline, struct console and whatnot.

Acked-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Re: [PATCH 2/8] printk: Rename preferred_console to preferred_dev_console

[Marcos Paulo de Souza]

On Fri, 2026-02-06 at 17:49 +0100, Petr Mladek wrote:
> The preferred_consoles[] array stores information about consoles
> requested
> via the command line, SPCR, Device Tree, or platform-specific code.
> Within this array, the 'preferred_console' variable tracks the
> specific
> index that should be associated with /dev/console (typically the last
> non-braille console defined).
> 
> The current name "preferred_console" is ambiguous and leads to
> confusion.
> It does not clearly communicate why one console is "more preferred"
> than
> others in the array. Furthermore, entries for Braille consoles can
> exist
> within the preferred_consoles[] array, yet they are never associated
> with
> /dev/console and do not receive standard printk() output.
> Consequently,
> the 'preferred_console' index must skip these entries, which is not
> immediately obvious from the name.
> 
> Rename the variable to 'preferred_dev_console' to explicitly clarify
> its
> role in identifying which entry is linked to /dev/console.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---

Acked-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Re: [PATCH 4/8] printk: Cleanup _braille_(un)register_console() wrappers

[Marcos Paulo de Souza]

On Fri, 2026-02-06 at 17:49 +0100, Petr Mladek wrote:
> The _braille_(un)register_console() wrappers currently attempt to
> hide
> implementation details like the CON_BRL flag and pc->brl_options.
> This
> forces callers to handle an unconventional tri-state return value (0
> for
> NOP, >0 for success, <0 for error), which makes the control flow
> harder
> to follow and non-standard.
> 
> Refactor the wrappers to use standard kernel return codes (0 for
> success,
> -ERRCODE on failure). Move the responsibility of checking brl_options
> to the caller to make the logic more explicit.
> 
> Additionally, move the assignment of the CON_BRL flag from the
> internal
> wrapper to braille_register_console(). This aligns it with how
> CON_ENABLED is handled. To maintain symmetry and fix a potential bug
> where flags might persist after removal, explicitly clear both
> CON_ENABLED and CON_BRL in braille_unregister_console().
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---


I had a very similar patch locally, thanks a lot for cleaning this up!

Reviewed-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Re: [PATCH 5/8] printk: Try to register each console as Braille first

[Chris Down]

Petr Mladek writes:
>> The try_only_braille and is_braille_console_preferred(pc) checks likely need
>> to happen before or independently of the match() vs. default matching
>> branch.
>
>This should never happen because register_console() always tries to
>register Braille consoles first.

I think maybe we're talking a bit past each other :-) It does seem I 
misunderstood the ->match() semantics a bit though, I don't think there's a bug 
here any more, but maybe worth a clarifying code comment.

My concern wasn't so much about the ordering between 
try_enable_braille_console() and try_enable_preferred_console(). It was more 
about what happens inside __try_enable_preferred_console() when it takes 
try_only_braille=true. _braille_register_console and 
is_braille_console_preferred are both inside the default match block, which is 
only entered when ->match() returns non-zero or is absent. So, if ->match() 
returns zero, both would be skipped and the console would fall through to 
CON_ENABLED without _braille_register_console() being called.

But looking more closely I don't think this can trigger in practice as the tree 
is right now. The only ->match() callbacks in the tree only match 
earlycon-style names, and always return -ENODEV for other kinds of inputs. So 
actually they return -ENODEV in this case and we enter the match block 
normally.

But I still feel like maybe the structure is a bit subtle here, this confused 
me. The correctness of the Braille path seems to depend on ->match() never 
returning 0 for these kinds of console names, which I'm not sure is documented 
or enforced anywhere else. Maybe a comment mentioning this would help future 
readers? I won't block on it though.

Re: [PATCH 5/8] printk: Try to register each console as Braille first

[Petr Mladek]

On Fri 2026-02-20 12:52:09, Chris Down wrote:
> Petr Mladek writes:
> > > The try_only_braille and is_braille_console_preferred(pc) checks likely need
> > > to happen before or independently of the match() vs. default matching
> > > branch.
> > 
> > This should never happen because register_console() always tries to
> > register Braille consoles first.
> 
> I think maybe we're talking a bit past each other :-) It does seem I
> misunderstood the ->match() semantics a bit though, I don't think there's a
> bug here any more, but maybe worth a clarifying code comment.
> 
> My concern wasn't so much about the ordering between
> try_enable_braille_console() and try_enable_preferred_console(). It was more
> about what happens inside __try_enable_preferred_console() when it takes
> try_only_braille=true. _braille_register_console and
> is_braille_console_preferred are both inside the default match block, which
> is only entered when ->match() returns non-zero or is absent. So, if
> ->match() returns zero, both would be skipped and the console would fall
> through to CON_ENABLED without _braille_register_console() being called.

You are right. I have completely missed this variant.

> But looking more closely I don't think this can trigger in practice as the
> tree is right now. The only ->match() callbacks in the tree only match
> earlycon-style names, and always return -ENODEV for other kinds of inputs.
> So actually they return -ENODEV in this case and we enter the match block
> normally.
> 
> But I still feel like maybe the structure is a bit subtle here, this
> confused me. The correctness of the Braille path seems to depend on
> ->match() never returning 0 for these kinds of console names, which I'm not
> sure is documented or enforced anywhere else. Maybe a comment mentioning
> this would help future readers? I won't block on it though.

You are right that these are subtle checks.

Hmm ,we might call __try_enable_preferred_console() three times and
we are always interested in some particular entries:

   + Braille consoles
   + User specified
   + Platform specified

I agree that we should distinguish this in the main loop. I am going
to add the following into v2:

static int __try_enable_preferred_console(struct console *newcon,
					  bool user_specified,
					  bool try_only_braille)
{
[...]
	for (i = 0, pc = preferred_consoles;
	     i < MAX_PREFERRED_CONSOLES && (pc->name[0] || pc->devname[0]);
	     i++, pc++) {
		/* Console not yet initialized? */
		if (!pc->name[0])
			continue;
+ 
+ 		/*
+ 		 * @try_only_braille and @user_specifified define which
+ 		 * preferred console entries are handled in this round.
+ 		 */
+ 		if (try_only_braille) {
+ 			if (!is_braille_console_preferred(pc))
+ 				continue;
+ 		} else {
+ 			if (pc->user_specified != user_specified)
+ 				continue;
+ 		}
+
		if (!newcon->match ||
		    newcon->match(newcon, pc->name, pc->index, pc->options) != 0) {
			/* default matching */
[...]


But there is still the problem that there might be two entries with
the same pc->name. The 2nd entry might appear later when pc->devname
matches, see match_devname_and_update_preferred_console().

I am afraid that the same HW device might get registered as both
Braille and non-Braille console.

I though about catching this in
match_devname_and_update_preferred_console() and returning -EBUSY.
But there is similar problem that another entry might match
also via con->match() callback.

But maybe this is not a problem at all. The above proposed check
and calling try_enable_braille_console() first ensures that
the Braille entry is preferred.

And each HW device should get associated with a particular
"struct console". The subsystem should make sure that it does not
register the same "struct console" twice. Maybe we could check
console_is_registered_locked() at the beginning on register_console()
to be on the safe side.

Best Regards,
Petr

PS: I hope that the above makes sense. My head is spinning a bit now.
    And I am leaving early today for a week long vacation which
    affects my concetration a bit...

    I think that I need to make more tests after I am back.