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

[Tony Lindgren]

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.
> > 
> > Signed-off-by: David Engraf <david.engraf@xxxxxxxxx>
> > ---
> >  kernel/printk/printk.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 6d3d18a50da74..f4803fe05a0aa 100644
> > --- 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 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.