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

[Marcos Paulo de Souza <mpdesouza@suse.com>]

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)