Messages in this thread Patch in this message | | | Date | Wed, 14 Jun 2017 17:38:03 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [PATCH 2/3] printk/console: Clean up logic around fallback console |
| |
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
| |