Re: [PATCH v2] printk: Fix preferred console selection with multiple matches

[Petr Mladek]

On Fri 2019-12-20 08:48:24, Benjamin Herrenschmidt wrote:
> On Thu, 2019-12-19 at 14:50 +0100, Petr Mladek wrote:
> > The code really need simplification. I would prefer to take the risk
> > and reduce the amount of added conditions as much as possible.
> > I have an idea, see below.
> 
> I wanted you to say that :-) I'll rework along those lines. Just a nit
> or two:
> > 
> > 
> > > --- a/kernel/printk/printk.c
> > > +++ b/kernel/printk/printk.c
> > > @@ -2542,6 +2545,53 @@ static int __init keep_bootcon_setup(char *str)
> > >  
> > >  early_param("keep_bootcon", keep_bootcon_setup);
> > >  
> > > +enum con_match {
> > > +	con_matched,
> > > +	con_matched_preferred,
> > > +	con_braille,
> > > +	con_failed,
> > > +	con_no_match,
> > > +};
> > 
> > Please, replace this with int, where:
> > 
> >    + con_matched -> 0
> >    + con_matched_preferred -> 0 and make "has_preferred" global variable
> >    + con_braile -> 0		later check for CON_BRL flag
> >    + con_failed -> -EFAULT
> >    + con_no_match -> -ENOENT
> 
> Not fan of using -EFAULT here, it's a detail since it's rather kernel
> internal, but I'd rather use -ENXIO for no match and -EIO for failed
> (or pass the original error code up if any). That said it's really bike
> shed painting at this point :-)

Sigh, either variant is somehow confusing.

I think that -ENOENT is a bit better than -EIO. It is abbreviation of
"No entry or No entity" which quite fits here. Also the device might
exist but it is not used when not requested.

I do not mind about -EFAULT vs -EIO. Well, -EIO might actually
better describe the reality.

That said, I do not want to spend much time on bikesheding. Feel free
to use whatever looks better to you.


> > > @@ -2615,41 +2664,19 @@ void register_console(struct console *newcon)
> > 	/* Prefer command line over platform specific defaults. */
> > 	err = try_match_new_console(newcon, true);
> > 	if (err = -ENOENT)
> > 		err = try_match_new_console(newcon, false);
> > 
> > 	/* printk() messages are not printed to Braille consoles. */
> > 	if (err || console->flags | CON_BRL)
> > 		return;
> 
> So this changes the existing behaviour in one way that may or may not
> matter, I don't know:
> 
> If setup() fails, the existing code will not exit. That means that if
> the console has CON_ENABLED already set (some do set it statically or
> set it from outside this function, I haven't looked into details the
> various circumstances this can happen), the existing code will still
> insert it. Your patch will make us not insert it.

Great catch! There is still a lot to learn about this code.

It seems that, for example, pstore and netconsole are not added
into console_cmdline and do not match. They relly on the explicitely
set CON_ENABLED.

I would prefer to hide this into the new "shorter" function. I would
rename it to try_enable_new_console() and add the following at the end:

	/*
	 * For example, pstore, netconsole, are enabled even
	 * without matching.
	 */
	if (console->flags & CON_ENABLED)
		return 0;

Best Regards,
Petr