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

[Petr Mladek <pmladek@suse.com>]

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