Re: [PATCH v5 04/16] printk: Separate code for adding/updating preferred console metadata

[Petr Mladek]

On Tue 2026-10-27 13:11:33, John Ogness wrote:
> On 2026-09-29, Petr Mladek <pmladek@xxxxxxxx> 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.
> >
> > Split the code for adding or updating entries to avoid code duplication.
> > Make it clear what operations are done only for new entries.
> >
> > Use this opportunity and replace an int with an unsigned int for
> > iterating over the preferred_consoles array.
> >
> > It is a preparation step before adding more consistency checks.
> > No intended functional changes.
> >
> > Signed-off-by: Petr Mladek <pmladek@xxxxxxxx>
> 
> Reviewed-by: John Ogness <john.ogness@xxxxxxxxxxxxx>
> 
> Some minor comments below...
> 
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index c3e50f19a32b..174aee533a53 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2577,35 +2584,56 @@ static int __add_preferred_console(const char *name, const short idx,
> >  	if (name && idx < 0)
> >  		return -EINVAL;
> >  
> > +	if (!pc->name[0] && !pc->devname[0])
> > +		is_new_entry = true;
> > +
> > +	if (is_new_entry) {
> > +		if (devname)
> > +			strscpy(pc->devname, devname);
> > +		if (name)
> > +			strscpy(pc->name, name);
> > +		pc->index = idx;
> > +	}
> > +
> >  	/*
> > -	 *	See if this tty is not yet registered, and
> > -	 *	if we have a slot free.
> > +	 * Historically, the preferred options are stored only by the first
> > +	 * caller who prefers the particular console.
> >  	 */
> > +	if (is_new_entry) {
> > +		pc->options = options;
> > +		braille_set_options(pc, brl_options);
> > +	}
> > +
> > +	if (!brl_options)
> > +		preferred_dev_console = i;
> > +
> > +	if (user_specified) {
> 
> Here you drop the comments:
> 
> 	/*
> 	 * @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;
> 
> And here you drop the comments:
> 
> 	/* At least one console defined by the user on the command line. */
> 
> > +		console_set_on_cmdline = 1;
> > +	}
> 
> The comments seem useful to help track all this. I do not see this
> information preserved anywhere by the end of your series. But you have a
> better understanding of the big picture. If you think the comments are
> insignificant and/or misleading, I am fine with you dropping them.

Heh, they did not fit 80-characters per-line, so I dropped them ;-)
OK, I am going to add them back the following way in v6:

--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -366,6 +366,7 @@ static int console_locked;
 static struct preferred_console preferred_consoles[MAX_PREFERRED_CONSOLES];
 
 static int preferred_dev_console = -1;
+/* Set when there is at least one console preferred via the command line. */
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
 
@@ -2607,6 +2608,10 @@ static int add_or_update_preferred_console(unsigned int i,
 		preferred_dev_console = i;
 
 	if (user_specified) {
+		/*
+		 * @pc console was defined by the user on the command line.
+		 * Do not clear when added twice also by SPCR or DT.
+		 */
 		pc->user_specified = true;
 		console_set_on_cmdline = 1;
 	}

I'll keep your Reviewed-by. Speak if you do not like it, please.

Best Regards,
Petr