* Re: [PATCH v2 3/9] printk: Separate code for adding/updating preferred console metadata
[not found] ` <20260423130015.85175-4-pmladek@suse.com>
@ 2026-05-05 17:47 ` Marcos Paulo de Souza
0 siblings, 0 replies; 3+ messages in thread
From: Marcos Paulo de Souza @ 2026-05-05 17:47 UTC (permalink / raw)
To: Petr Mladek, John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Chris Down, linux-kernel
On Thu, 2026-04-23 at 15:00 +0200, Petr Mladek wrote:
> 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.
>
> Behavior change:
>
> The original code never updated the preferred console options
> when it was preferred more times, e.g. via the command line
> and/or some platform specific code, e.g. SPCR or device tree.
>
> The new code explicitly allows to update the console options
> when they are preferred over the command line.
>
> It mostly worked even before but only because the command line was
> processed early enough before handling SPCR, device tree, or other
> platform specific init code.
>
> The main behavior change is when the same console is preferred more
> times on the command line. Newly, the later or Braille variant
> wins. It is a more common and expected behavior.
Sorry for the late review. I just wanted to say that the improvement is
really great: documentation and code reorganization really improve the
code. It was too confusing and rather problematic to follow up.
Acked-by: Marcos Paulo de Souza <mpdesouza@suse.com>
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> kernel/printk/braille.h | 7 +-
> kernel/printk/printk.c | 149 ++++++++++++++++++++++++++++++--------
> --
> 2 files changed, 118 insertions(+), 38 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 13c98285892b..d251bf8e104f 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2544,18 +2544,119 @@ asmlinkage __visible void early_printk(const
> char *fmt, ...)
> }
> #endif
>
> -static void set_user_specified(struct preferred_console *pc, bool
> user_specified)
> +/** 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 preferred also by some platform-specific code.
> + *
> + * [*] Braille console is using the mechanism for registering
> consoles
> + * but it is very special. It is primarily used for user
> interaction
> + * with the system. It neither gets printk() messages nor is
> associated
> + * with /dev/console.
> + */
> +static int update_preferred_console(unsigned 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 (pc->name[0]) {
> + pr_err("Updating a preferred console entry
> with an already assigned console name via devname: %s, %s\n",
> + devname, pc->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;
>
> /*
> * @pc 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,
> @@ -2563,19 +2664,11 @@ static int __add_preferred_console(const char
> *name, const short idx,
> char *brl_options, bool
> user_specified)
> {
> struct preferred_console *pc;
> - int i;
> + unsigned int i;
>
> 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
> @@ -2584,28 +2677,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)
^ permalink raw reply [flat|nested] 3+ messages in thread