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

[John Ogness]

Hi Petr,

On 2026-02-06, Petr Mladek <pmladek@xxxxxxxx> 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