Re: [PATCH 2/3] printk/console: Clean up logic around fallback console

[Petr Mladek]

On Wed 2017-06-14 17:38:03, Sergey Senozhatsky wrote:
> Thanks for taking a look at this.
> 
> On (06/13/17 14:54), Petr Mladek wrote:
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 8ebc480fdbc6..6e651f68bffd 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2413,51 +2413,45 @@ void register_console(struct console *newcon)
> >  	unsigned long flags;
> >  	struct console *bcon = NULL;
> 
> I was going to ask if we can also rename `bcon' to just `con', because not
> every console we see in for_each_console() is a boot console, that's why we
> test for CON_BOOT bit set and then 'if boot_console->flags & CON_BOOT' looks
> a bit odd; but looking at the bottom of register_console() we probably better
> keep it the way it is. but who knows... suggestions?

Yeah, the variable is temporary used for iterating in a cycle
where the name is confusing. The real value is assigned and used
later. I though about cleaning this as well but I did not get a good
idea and let it for future ;-)

> 
> >  	struct console_cmdline *c;
> > -	static bool has_preferred;
> > +	bool have_real_console = false;
> 
> ok, `real console' probably can work. boot console can also be real,
> right? it's just it's not a fully featured one. may be there is a better
> naming here, can't immediately think of any tho. ideas?

There is mentioned 'normal console' in
Documentation/admin-guide/kernel-parameters.txt. But it is related to
earlyprintk.

The same document uses 'real console' several times when it talks
about the 'keep' and 'keep_bootcon' options.

I am not much happy with the name but it seems to be in sync
with the existing documentation.


> dunno, looking at this `newcon->flags |= CON_CONSDEV' I think I'd
> probably also add the following patch to the series
> 
> ---
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index b8920a031a3e..a2525e0d7605 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -127,7 +127,7 @@ static inline int con_debug_leave(void)
>   */
>  
>  #define CON_PRINTBUFFER        (1)
> -#define CON_CONSDEV    (2) /* Last on the command line */
> +#define CON_CONSDEV    (2)

The description is wrong. The name and the use suggest that
it should be something like:

#define CON_CONSDEV    (2) /* Driver used by /dev/console */

There are some situations (bugs) where this is not true.
But I will try to fix the code to fit this description.


> so, as far as I can see, the patch shouldn't change the logic of
> registration. just a clean up. need to run tests/etc. etc. I just
> read the patch so far.

Thanks a lot for great review.

Best Regards,
Petr