[PATCH 0/3] printk/console: Simplify the logic and always have a preferred console

[PATCH 0/3] printk/console: Simplify the logic and always have a preferred console

[Petr Mladek]

The regression caused by the commit cf39bf58afdaabc0b ("printk: fix
double printing with earlycon") made me to investigate the console
registration code from many angles. And it was not an easy reading.

Especially the logic around the preferred console was somehow
twisted. The first two patches try to make it more straightforward.
The good thing is that they should not change the behavior.

Another problem was inconsistency in handling a fallback for
the preferred console. This is solved by the 3rd patch.
Also it solves a bug report related to "showconsole" that
I got 2 weeks ago. This fix slightly changes the order
of non-preferred consoles. But I believe that it is behind
the border when we need to take care of backward compatibility.

Sadly, we had to revert the fix of the duplicated output. It is
not solved by this patch set. I consider this a cosmetic issue.
A proper solution would be quite complicated. It might involve
rework of the console->match() functions and allow to match
aliases without side effects. Alternative solution would be
an additional command line option that would force handling
of CON_PRINTBUFFER flag and "never", "auto", or "always"
reply the buffer for newly registered consoles. But I am
not sure if it is worth it.


Petr Mladek (3):
  printk/console: Remove superfluous setting of has_preferred state
    value
  printk/console: Clean up logic around fallback console
  printk/console: Always have a preferred console

 kernel/printk/printk.c | 48 ++++++++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

-- 
1.8.5.6

[PATCH 1/3] printk/console: Remove superfluous setting of has_preferred state value

[Petr Mladek]

It is superfluous to set "has_preferred" when the really preferred
console is enabled. The variable "i" is index of an array. Therefore
"has_preferred" is set only when preferred_console is >= 0. But then
it was already true because of the above code:

	if (!has_preferred || bcon || !console_drivers)
		has_preferred = preferred_console >= 0;

This line was added by the commit ab4af03a4054bd78bc ("[PATCH] CON_CONSDEV
bit not set correctly on last console"). It was kind of logic to store
the really used index there. But this commit actually caused that
"preferred_console" started to be used only as a boolean. This was
discovered and corrected by the commit b077bafa2f3848ddfcef ("printk:
fix name/type/scope of preferred_console var").

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index a1db38abac5b..8ebc480fdbc6 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2489,10 +2489,8 @@ void register_console(struct console *newcon)
 		}
 
 		newcon->flags |= CON_ENABLED;
-		if (i == preferred_console) {
+		if (i == preferred_console)
 			newcon->flags |= CON_CONSDEV;
-			has_preferred = true;
-		}
 		break;
 	}
 
-- 
1.8.5.6

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

[Petr Mladek]

register_console() has a fallback code that enables the first
usable console when a preferred one is not set. Though the logic
is really tangled by the variable "has_preferred"

The only purpose of the variable is to decide whether we use the fallback
code or not. It is set when the console setup succeeds and the fallback
console is enabled. It makes some sense. We have enabled a console.
We have a fallback. Therefore the fallback code is not longer needed.

But the value is reset in the next register_console() call by the code:

	if (!has_preferred || bcon || !console_drivers)
		has_preferred = preferred_console >= 0

Now, keep your head firmly in hands and try to understand all the twists
here.

"!has_preferred" is true when register_console() is called for the 1st
time or it was false after the previous call. The second possibility
means that "preferred_console" was not set in the previous call.
Therefore the previous call tried to setup a console as a fallback
and it must have failed.

"bcon" is true when a boot console is first in the console_drivers
list. This is a complicated clue in itself. We need to look on how
the list is manipulated. New consoles are put at the beginning of
the list. But the preferred console (with CON_CONSDEV flag) is kept
at the beginning.

Note that we might add a preferred console when there already
is one. Then the new preferred console obsoletes the old one.
One case is when the early and real console have the same
definition and entry in the preferred_console list. Then they
both are preferred. Rather theoretical case is when the first
one is registered using the fallback mechanism before
any preferred_console is set, e.g. by the command line.

OK, let's go back to "bcon". It is "false" when there are
no console drivers. Or it can become "false" when a real console
is put at the beginning of the list. The are basically two
possibilities.

If the first boot console does _not_ have CON_CONSDEV flag set then
any new console will be put at the beginning of the list. By other
words, "bcon" will become "false" once we register the first real
console.

If the first boot console has CON_CONSDEV flag set then "bcon"
will become "false" only when a real console with CON_CONSDEV
flag set is registered. There are only two possibilities.
Either if "preferred_console" is set then we need to wait
until this one is registered. But then "has_preferred" is
true and the fallback code is not used. Or if "preferred_console"
is not set, it is enough to register any real console using
the fallback code that always sets CON_CONSDEV.

Please note that only one real console can be registered using
the fallback code. It will cause "bcon" to become "false"
and "as_preferred" to be "true". This will be important later.

The only exception is when the real console is unregistered
again. There will be no real console in the list. A boot
console will become the first in the list again or console_drivers
will become NULL.

Finally, "!console_drivers" is true if none driver was registered
before or when all drivers were unregistered, e.g. early drivers
by late_initcall(printk_late_init).

Now, if any of the three checks are true, we (re)set "has_preferred"
value. Let's repeat. The variable is set to "true" when any preferred
console is configured. And this information is used to decide whether
we will try to setup this console as a fallback.

If you still have a head close your neck, you might ask what is the exact
purpose of the code. The answer is in the commit 69331af79cf29e26d1231
("Fixes and cleanups for earlyprintk aka boot console"). In short:

  + The check of "bcon" allows to use the fallback code more times
    until we have a real console registered.

  + The "!console_drivers" check is moved from console_unregister().
    It allows to re-enable the fallback mechanism when the last
    console is unregistered.

OK, we know that the current code is not trivial and we know what
we want to achieve. Could we do it an easier way?

This patch gets rid of the "has_preferred" variable. Instead it
uses the following check:

	if (!have_real_console && preferred_console < 0)
		/* try to setup fallback */

Where "have_real_console" is detected using the same code that prevented
registering boot consoles once a real one was in the list. It basically
replaces the check of "!bcon", "console_drivers", and "has_preferred".

IMHO, it is much more straightforward. It perfectly fits the expected
logic that was described in the above mentioned commit and also
in the commit 4d09161196c9a836eac ("printk: Enable the use of more
than one CON_BOOT (early console)").

In short, we try to make debugging easier. Therefore we try to enable
any boot console and one real console if no console is configured
(preferred one is not selected). The fallback code is not used
if a console is configured.

Note that the fallback code might actually be used to register boot
consoles. They might be registered before the "console=" commandline
parameters are handled.

The patch does not change the existing behavior. If I did not miss
anything, the fallback code is called in exactly the same situations
where it was called before.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 44 +++++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

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;
 	struct console_cmdline *c;
-	static bool has_preferred;
+	bool have_real_console = false;
 
-	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
-	 */
-	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;
-			}
 		}
 	}
 
-- 
1.8.5.6

[PATCH 3/3] printk/console: Always have a preferred console

[Petr Mladek]

More consoles can be registered but one might be special. It is the one
with CON_CONSDEV flag set. It must be the first in the console_drivers
list. It is also sometimes called as a preferred one.

It is the console that is associated with /dev/console. It is shown
by "showconsole" binary. But if none of the consoles have CON_CONSDEV
flag set, the state is unclear and "showconsole" is unable to find it:

     showconsole: real console unknown: Success

Documentation/admin-guide/serial-console.rst says that the preferred
console is the last one on the command line. But there already exists
some fallbacks.

First, there is a fallback code that tries to enable any boot
console and one real console if no consoles are configured.
This code always sets CON_CONSDEV if console setup succeeded.

Second, console_unregister() sets the flag CON_CONSDEV for the next
console in the list when a console with this flag is being removed.

Now, the flag is not set if some consoles are configured and
the preferred one is never registered from some reason.

This patch modifies the code that enables the configured consoles.
It sets the CON_CONSDEV flag also when we register the first
console. It causes that one of the registered consoles will
always have CON_CONSDEV flag set.

It might have side effects. The first registered console will be
marked as preferred and kept first in the console_drivers list
until the really preferred one is registered. This might change
the order of consoles in console_drivers list. As a consequence,
another console might be selected when the really preferred one
is unregistered. But this should require some manual intervention.
The order was never guarantied. Therefore it does not look
worth the effort to keep the original order.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 6e651f68bffd..76b1159f2004 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2483,7 +2483,7 @@ void register_console(struct console *newcon)
 		}
 
 		newcon->flags |= CON_ENABLED;
-		if (i == preferred_console)
+		if (i == preferred_console || !console_drivers)
 			newcon->flags |= CON_CONSDEV;
 		break;
 	}
-- 
1.8.5.6

Re: [PATCH 1/3] printk/console: Remove superfluous setting of has_preferred state value

[Sergey Senozhatsky]

On (06/13/17 14:54), Petr Mladek wrote:
> It is superfluous to set "has_preferred" when the really preferred
> console is enabled. The variable "i" is index of an array. Therefore
> "has_preferred" is set only when preferred_console is >= 0. But then
> it was already true because of the above code:
> 
> 	if (!has_preferred || bcon || !console_drivers)
> 		has_preferred = preferred_console >= 0;
> 
> This line was added by the commit ab4af03a4054bd78bc ("[PATCH] CON_CONSDEV
> bit not set correctly on last console"). It was kind of logic to store
> the really used index there. But this commit actually caused that
> "preferred_console" started to be used only as a boolean. This was
> discovered and corrected by the commit b077bafa2f3848ddfcef ("printk:
> fix name/type/scope of preferred_console var").
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

agree.

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

> ---
>  kernel/printk/printk.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index a1db38abac5b..8ebc480fdbc6 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2489,10 +2489,8 @@ void register_console(struct console *newcon)
>  		}
>  
>  		newcon->flags |= CON_ENABLED;
> -		if (i == preferred_console) {
> +		if (i == preferred_console)
>  			newcon->flags |= CON_CONSDEV;
> -			has_preferred = true;
> -		}
>  		break;
>  	}
>  
> -- 
> 1.8.5.6
> 

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

[Sergey Senozhatsky]

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

Re: [PATCH 3/3] printk/console: Always have a preferred console

[Sergey Senozhatsky]

On (06/13/17 14:54), Petr Mladek wrote:
> More consoles can be registered but one might be special. It is the one
> with CON_CONSDEV flag set. It must be the first in the console_drivers
> list. It is also sometimes called as a preferred one.
> 
> It is the console that is associated with /dev/console. It is shown
> by "showconsole" binary. But if none of the consoles have CON_CONSDEV
> flag set, the state is unclear and "showconsole" is unable to find it:
> 
>      showconsole: real console unknown: Success
> 
> Documentation/admin-guide/serial-console.rst says that the preferred
> console is the last one on the command line. But there already exists
> some fallbacks.
> 
> First, there is a fallback code that tries to enable any boot
> console and one real console if no consoles are configured.
> This code always sets CON_CONSDEV if console setup succeeded.
> 
> Second, console_unregister() sets the flag CON_CONSDEV for the next
> console in the list when a console with this flag is being removed.
> 
> Now, the flag is not set if some consoles are configured and
> the preferred one is never registered from some reason.

haha, nice. that's exactly what I was talking about a moment ago.

> This patch modifies the code that enables the configured consoles.
> It sets the CON_CONSDEV flag also when we register the first
> console. It causes that one of the registered consoles will
> always have CON_CONSDEV flag set.

hm.... my impression was that we shouldn't set CON_CONSDEV if the
console has no ->device. but then, once again, unregister_console()
does not care and register_console() cares only in one place. so I'm
a bit in doubt.

> It might have side effects. The first registered console will be
> marked as preferred and kept first in the console_drivers list
> until the really preferred one is registered. This might change
> the order of consoles in console_drivers list. As a consequence,
> another console might be selected when the really preferred one
> is unregistered. But this should require some manual intervention.
> The order was never guarantied. Therefore it does not look
> worth the effort to keep the original order.

need to think more.

	-ss

> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/printk/printk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 6e651f68bffd..76b1159f2004 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2483,7 +2483,7 @@ void register_console(struct console *newcon)
>  		}
>  
>  		newcon->flags |= CON_ENABLED;
> -		if (i == preferred_console)
> +		if (i == preferred_console || !console_drivers)
>  			newcon->flags |= CON_CONSDEV;
>  		break;
>  	}

Re: [PATCH 3/3] printk/console: Always have a preferred console

[Petr Mladek]

On Wed 2017-06-14 18:11:28, Sergey Senozhatsky wrote:
> On (06/13/17 14:54), Petr Mladek wrote:
> 
> > This patch modifies the code that enables the configured consoles.
> > It sets the CON_CONSDEV flag also when we register the first
> > console. It causes that one of the registered consoles will
> > always have CON_CONSDEV flag set.
> 
> hm.... my impression was that we shouldn't set CON_CONSDEV if the
> console has no ->device. but then, once again, unregister_console()
> does not care and register_console() cares only in one place. so I'm
> a bit in doubt.

Great catch! It helped me to get even better picture.

If I get it correctly, the console driver for /dev/console is
found in tty_lookup_driver() by console_device(). Where
console_device() skips console drivers without any device.

CON_CONSDEV is basically handled only in register_console()
and unregister_console(). The driver with this flag is kept at
the beginning of the console_drivers list. It causes that
console_device() will return the console with this flag.

Also it is in sync with the commit cd3a1b8562d28490b33 ("printk: don't
prefer unsuited consoles on registration"). It added the check
for existing device into the fallback code in console_register()
so that console_device() might find a valid one.

BTW: The flag CON_CONSDEV first appeared in Linux 2.1.92.
It replaced CON_FIRST. Documentation/serial-console.txt
was modified so that the last console on the command line
will be used for /dev/console instead of the first one.

In each case, it seems that CON_CONSDEV is closely related
to an existing device and does not make much sense without it.


Heh, the handling in unregister_console() looks buggy. Well,
it probably does not break anything. The function does not change
the order of drivers and console_device() searches
the entire list until it finds a console with device.
Anyway, it would make sense to clean it as well.

Thanks a lot for review.

Best Regards,
Petr

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

Re: [PATCH 3/3] printk/console: Always have a preferred console

[Sergey Senozhatsky]

On (06/15/17 16:54), Petr Mladek wrote:
> On Wed 2017-06-14 18:11:28, Sergey Senozhatsky wrote:
> > On (06/13/17 14:54), Petr Mladek wrote:
> > 
> > > This patch modifies the code that enables the configured consoles.
> > > It sets the CON_CONSDEV flag also when we register the first
> > > console. It causes that one of the registered consoles will
> > > always have CON_CONSDEV flag set.
> > 
> > hm.... my impression was that we shouldn't set CON_CONSDEV if the
> > console has no ->device. but then, once again, unregister_console()
> > does not care and register_console() cares only in one place. so I'm
> > a bit in doubt.
> 
> Great catch! It helped me to get even better picture.
> 
> If I get it correctly, the console driver for /dev/console is
> found in tty_lookup_driver() by console_device(). Where
> console_device() skips console drivers without any device.
> 
> CON_CONSDEV is basically handled only in register_console()
> and unregister_console(). The driver with this flag is kept at
> the beginning of the console_drivers list. It causes that
> console_device() will return the console with this flag.

yeah, I grep-ed and didn't find anything that would depend
on CON_CONSDEV flag (except for printk), we check ->device
in critical places.

> Also it is in sync with the commit cd3a1b8562d28490b33 ("printk: don't
> prefer unsuited consoles on registration"). It added the check
> for existing device into the fallback code in console_register()
> so that console_device() might find a valid one.
> 
> BTW: The flag CON_CONSDEV first appeared in Linux 2.1.92.
> It replaced CON_FIRST. Documentation/serial-console.txt
> was modified so that the last console on the command line
> will be used for /dev/console instead of the first one.

good to know that, thanks.  s/CON_FIRST/CON_LAST/g  :)

> In each case, it seems that CON_CONSDEV is closely related
> to an existing device and does not make much sense without it.
> 
> 
> Heh, the handling in unregister_console() looks buggy. Well,
> it probably does not break anything. The function does not change
> the order of drivers and console_device() searches
> the entire list until it finds a console with device.

yep. seems so.

	-ss

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

[Sergey Senozhatsky]

On (06/15/17 17:31), Petr Mladek wrote:
[..]
> > 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 */

that's better, indeed.

> There are some situations (bugs) where this is not true.

true.

	-ss