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
