lkml.org 
[lkml]   [2019]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] printk: Fix preferred console selection with multiple matches
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

\
 
 \ /
  Last update: 2019-12-20 10:12    [W:0.041 / U:2.672 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site