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?
> 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?
> - if (console_drivers)
> - for_each_console(bcon)
> + if (console_drivers) {
> + for_each_console(bcon) {
> if (WARN(bcon == newcon,
> "console '%s%d' already registered\n",
> bcon->name, bcon->index))
> return;
>
> - /*
> - * before we register a new CON_BOOT console, make sure we don't
> - * already have a valid console
'a valid console'. so there are boot consoles, valid consoles and real
consoles :) it's good to see this comment being removed then.
> - */
> - if (console_drivers && newcon->flags & CON_BOOT) {
> - /* find the last or real console */
> - for_each_console(bcon) {
> - if (!(bcon->flags & CON_BOOT)) {
> - pr_info("Too late to register bootconsole
> %s%d\n",
> - newcon->name, newcon->index);
> - return;
> - }
> + if (!(bcon->flags & CON_BOOT))
> + have_real_console = true;
> }
> }
>
> + if (have_real_console && newcon->flags & CON_BOOT) {
> + pr_info("Too late to register bootconsole %s%d\n",
> + newcon->name, newcon->index);
> + return;
> + }
> +
> + /*
> + * We have not registered the real preferred console if a boot
> + * console is still the first one.
> + */
> if (console_drivers && console_drivers->flags & CON_BOOT)
> bcon = console_drivers;
>
> - if (!has_preferred || bcon || !console_drivers)
> - has_preferred = preferred_console >= 0;
> -
> /*
> - * See if we want to use this console driver. If we
> - * didn't select a console we take the first one
> - * that registers here.
> + * Enable any boot console and the first real one when
> + * consoles are not configured.
> */
> - if (!has_preferred) {
> + if (!have_real_console && preferred_console < 0) {
> if (newcon->index < 0)
> newcon->index = 0;
> if (newcon->setup == NULL ||
> newcon->setup(newcon, NULL) == 0) {
> newcon->flags |= CON_ENABLED;
> - if (newcon->device) {
> + if (newcon->device)
> newcon->flags |= CON_CONSDEV;
> - has_preferred = true;
> - }
> }
> }
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)
#define CON_ENABLED (4)
#define CON_BOOT (8)
#define CON_ANYTIME (16) /* Safe to call when cpu is offline */
---
CON_CONSDEV can be at any place on the command line. right? and
unregister_console() even takes care of it.
/*
* If this isn't the last console and it has CON_CONSDEV set, we
* need to set it on the next preferred console.
*/
if (console_drivers != NULL && console->flags & CON_CONSDEV)
console_drivers->flags |= CON_CONSDEV;
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.
-ss
