printk/console: Simplify the logic and always have a preferred console

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

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").

---
 kernel/printk/printk.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index a1db38abac5b..8ebc480fdbc6 100644
@@ -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

agree.

	-ss

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.

---
 kernel/printk/printk.c | 44 +++++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

index 8ebc480fdbc6..6e651f68bffd 100644
@@ -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

Thanks for taking a look at this.

index b8920a031a3e..a2525e0d7605 100644
@@ -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 */

	/*
	 * 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;

	-ss

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 ;-)

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.

Thanks a lot for great review.

Best Regards,
Petr

On (06/15/17 17:31), Petr Mladek wrote:
[..]

that's better, indeed.

true.

	-ss

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.

---
 kernel/printk/printk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 6e651f68bffd..76b1159f2004 100644
@@ -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

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

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.

need to think more.

	-ss

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

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.

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

yep. seems so.

	-ss