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

[Petr Mladek]

On Fri 2026-05-15 12:29:14, John Ogness wrote:
> On 2026-04-23, Petr Mladek <pmladek@xxxxxxxx> wrote:
> > 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.
> 
> Perhaps the description should be:
> 
> Update or add a given entry in the preferred_consoles[] table.
> 
> If the caller didn't match an existing preferred_console, this function
> is called to add a new entry. This is obvious if you are looking at the
> caller, but it is not obvious at all if you are only looking at this
> function, i.e. strscpy'ing in @name or @devname is the only clue that
> you are dealing with a new entry.

Good point! I'll update the comment in v3. Also I'll rename the function to
add_or_update_preferred_console(). It is long but the function is
used only once so it does not harm that much ;-)

> > + * @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;
> 
> You are adding detailed error messages for all possible wrong calls
> except for this on. Maybe this should be covered as well with:
> 
> 	if (WARN_ON(!name && !devname))
> 		return -EINVAL;

Makes sense.

Also I am going print an error when reached the maximal number of
preferred consoles in the above check. It would look like:

	if (i >= MAX_PREFERRED_CONSOLES) {
		pr_err_once("Reached maximal number of preferred consoles.\n");
		return -E2BIG;
	}

It would be nice to print some more details. But it is not that easy
because we do not know whether "name" or "devname" is defined, ...
IMHO, it is not worth the complexity. I guess not nobody has reached
the limit in a real life scenario yet.

Best Regards,
Petr