lkml.org 
[lkml]   [2017]   [Jun]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2017-06-14 10:41    [W:0.097 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site