Re: [PATCH v2] printk: Remove console options before decoding the name

[Petr Mladek <pmladek@suse.com>]

On Wed 2026-09-23 13:19:15, Tony Lindgren wrote:
> On Wed, Sep 23, 2026 at 11:37:03AM +0200, Petr Mladek wrote:
> > On Thu 2026-09-17 09:05:51, David Engraf wrote:
> > > This fixes a regression when a console option includes ':'. Commit
> > > 7640f1a44eba ("printk: Add match_devname_and_update_preferred_console()")
> > > introduced console=DEVNAME:0.0 hardware style addressing by looking for a
> > > colon. If the colon is part of an option the name is handled as devname
> > > instead of ttyname.
> > > 
> > > Fix by handling the options first which will add a NULL terminator to the
> > > string.
> > > 
> > > --- a/kernel/printk/printk.c
> > > +++ b/kernel/printk/printk.c
> > > @@ -2646,24 +2646,25 @@ static int __init console_setup(char *str)
> > >  	if (_braille_console_setup(&str, &brl_options))
> > >  		return 1;
> > >  
> > > +	/*
> > > +	 * Decode str into name, index and options. Start with options, since
> > > +	 * it might also contain a ':' used for DEVNAME.
> > > +	 */
> > > +	options = strchr(str, ',');
> > > +	if (options)
> > > +		*(options++) = 0;
> > > +
> > >  	/* For a DEVNAME:0.0 style console the character device is unknown early */
> > >  	if (strchr(str, ':'))
> > >  		devname = buf;
> > >  	else
> > >  		ttyname = buf;
> > >  
> > > -	/*
> > > -	 * Decode str into name, index, options.
> > > -	 */
> > >  	if (ttyname && isdigit(str[0]))
> > >  		scnprintf(buf, sizeof(buf), "ttyS%s", str);
> > >  	else
> > >  		strscpy(buf, str);
> > 
> > Sashiko AI has the following comment:
> > 
> > | Does moving the options parsing and null-termination earlier in this function
> > | leave the loop below with an unreachable condition?
> > | 
> > | Since str is now truncated at the first comma before being copied into buf,
> > | buf will never contain a comma. This means the comma check inside the loop
> > | over buf appears to be structurally impossible to satisfy:
> > | 
> > | 	for (s = buf; *s; s++)
> > | 		if ((ttyname && isdigit(*s)) || *s == ',')
> > | 			break;
> > | 
> > | Can the comma check be safely removed from the loop condition?
> > 
> > And it is right. The original code copied the original string into
> > "buf". The new does not copy the options any longer.
> 
> OK
>  
> > It would deserve some refactoring to make the code cleaner.
> > Something like:
> > 
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index f4803fe05a0a..966744fb4bcc 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2672,17 +2672,18 @@ static int __init console_setup(char *str)
> >  		strscpy(buf, "ttyS1");
> >  #endif
> >  
> > -	for (s = buf; *s; s++)
> > -		if ((ttyname && isdigit(*s)) || *s == ',')
> > -			break;
> > -
> > -	/* @idx will get defined when devname matches. */
> > -	if (devname)
> > -		idx = -1;
> > -	else
> > +	if (ttyname) {
> > +		/* Detect @idx in ttyname and remove it. */
> > +		for (s = buf; *s; s++) {
> > +			if (isdigit(*s))
> > +				break;
> > +		}
> >  		idx = simple_strtoul(s, NULL, 10);
> > -
> > -	*s = 0;
> > +		*s = 0;
> > +	} else {
> > +		/* @idx will get defined when devname matches. */
> > +		idx = -1;
> > +	}
> >  
> >  	__add_preferred_console(ttyname, idx, devname, options, brl_options, true);
> >  	return 1;
> 
> Nice, you could now initialize idx = -1 to start with to leave out the
> else for setting devname idx?

I would personally prefer to keep the else part because it makes it
clear how the "devname" variant is handled. But I could live without
it as well.

> > I see two possibilities. We could either merge this cleanup into the
> > original patch and send v3. Or we could add it on top of the original
> > patch.
> > 
> > I would slightly prefer v3 and have both changes in a single patch.
> 
> Having a v3 sounds good to me.

Great.

Best Regards,
Petr