[PATCH 0/5] printk/console: Registration code cleanup - part 1

[PATCH 0/5] printk/console: Registration code cleanup - part 1

[Petr Mladek]

The console registration code has several twists that are hard to follow.
It is result of various features added over the last few decades.

I have already spent many days to understand all the effects and
the desired behavior. I am always scared to touch the console registration
code even after years working as printk maintainer.

There were several changes in the code that had to be reverted because
they caused regression, for example:

   + commit dac8bbbae1d0ccba96402 ("Revert "printk: fix double printing
     with earlycon")

   + commit c6c7d83b9c9e6a8b3e6d ("Revert "console: don't prefer first
     registered if DT specifies stdout-path")


This patchset removes the most tricky twists from my POV. I have worked
on it when time permitted since January. I have spent most of the time
writing the commit message, understanding, and explaining the effects.
I am not sure if I succeeded but it is time to send this.


Behavior change:

There is one behavior change caused by 4th patch. I consider it bug fix.
It should be acceptable. See the commit message for more details.


Future plans:

I already have additional 18 patches that do further clean up of
the console registration code. They still need more love.

I send this 5 patchset first because they are clear win from my POV.
And I wanted to do it by smaller steps.

I would appreciate if anyone double checks logic of the changes.
Anyway, we could put it into linux-next and see. I am quite
confident and optimistic ;-)


Petr Mladek (5):
  printk/console: Split out code that enables default console
  printk/console: Rename has_preferred_console to need_default_console
  printk/console: Remove unnecessary need_default_console manipulation
  printk/console: Remove need_default_console variable
  printk/console: Clean up boot console handling in register_console()

 kernel/printk/printk.c | 104 +++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 46 deletions(-)

-- 
2.26.2

[PATCH 1/5] printk/console: Split out code that enables default console

[Petr Mladek]

Put the code enabling a console by default into a separate function
called try_enable_default_console().

Rename try_enable_new_console() to try_enable_preferred_console() to
make the purpose of the different variants more clear.

It is a code refactoring without any functional change.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 57b132b658e1..1acbe39dd47c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2861,7 +2861,8 @@ early_param("keep_bootcon", keep_bootcon_setup);
  * Care need to be taken with consoles that are statically
  * enabled such as netconsole
  */
-static int try_enable_new_console(struct console *newcon, bool user_specified)
+static int try_enable_preferred_console(struct console *newcon,
+					bool user_specified)
 {
 	struct console_cmdline *c;
 	int i, err;
@@ -2909,6 +2910,23 @@ static int try_enable_new_console(struct console *newcon, bool user_specified)
 	return -ENOENT;
 }
 
+/* Try to enable the console unconditionally */
+static void try_enable_default_console(struct console *newcon)
+{
+	if (newcon->index < 0)
+		newcon->index = 0;
+
+	if (newcon->setup && newcon->setup(newcon, NULL) != 0)
+		return;
+
+	newcon->flags |= CON_ENABLED;
+
+	if (newcon->device) {
+		newcon->flags |= CON_CONSDEV;
+		has_preferred_console = true;
+	}
+}
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -2964,25 +2982,15 @@ void register_console(struct console *newcon)
 	 *	didn't select a console we take the first one
 	 *	that registers here.
 	 */
-	if (!has_preferred_console) {
-		if (newcon->index < 0)
-			newcon->index = 0;
-		if (newcon->setup == NULL ||
-		    newcon->setup(newcon, NULL) == 0) {
-			newcon->flags |= CON_ENABLED;
-			if (newcon->device) {
-				newcon->flags |= CON_CONSDEV;
-				has_preferred_console = true;
-			}
-		}
-	}
+	if (!has_preferred_console)
+		try_enable_default_console(newcon);
 
 	/* See if this console matches one we selected on the command line */
-	err = try_enable_new_console(newcon, true);
+	err = try_enable_preferred_console(newcon, true);
 
 	/* If not, try to match against the platform default(s) */
 	if (err == -ENOENT)
-		err = try_enable_new_console(newcon, false);
+		err = try_enable_preferred_console(newcon, false);
 
 	/* printk() messages are not printed to the Braille console. */
 	if (err || newcon->flags & CON_BRL)
-- 
2.26.2

[PATCH 2/5] printk/console: Rename has_preferred_console to need_default_console

[Petr Mladek]

The logic around the variable @has_preferred_console made my head
spin many times. Part of the problem is the ambiguous name.

There is the variable @preferred_console. It points to the last
non-braille console in @console_cmdline array. This array contains
consoles preferred via the command line, device tree, or SPCR.

Then there is the variable @has_preferred_console. It is set to
"true" when @preferred_console is enabled or when a console with
tty binding gets enabled by default.

It might get reset back by the magic condition:

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

It is a puzzle. Dumb explanation is that it gets re-evaluated
when:

	+ it was not set before (see above when it gets set)
	+ there is still an early console enabled (bcon)
	+ there is no console enabled (!console_drivers)

This is still a puzzle.

It gets more clear when we see where the value is checked. The only
meaning of the variable is to decide whether we should try to enable
the new console by default.

Rename the variable according to the single situation where
the value is checked.

The rename requires an inverted logic. Otherwise, it is a simple
search & replace. It does not change the functionality.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1acbe39dd47c..4c5f496877b0 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -280,7 +280,7 @@ static struct console *exclusive_console;
 static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
 
 static int preferred_console = -1;
-static bool has_preferred_console;
+static bool need_default_console = true;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
 
@@ -2894,7 +2894,7 @@ static int try_enable_preferred_console(struct console *newcon,
 		newcon->flags |= CON_ENABLED;
 		if (i == preferred_console) {
 			newcon->flags |= CON_CONSDEV;
-			has_preferred_console = true;
+			need_default_console = false;
 		}
 		return 0;
 	}
@@ -2923,7 +2923,7 @@ static void try_enable_default_console(struct console *newcon)
 
 	if (newcon->device) {
 		newcon->flags |= CON_CONSDEV;
-		has_preferred_console = true;
+		need_default_console = false;
 	}
 }
 
@@ -2974,15 +2974,15 @@ void register_console(struct console *newcon)
 	if (console_drivers && console_drivers->flags & CON_BOOT)
 		bcon = console_drivers;
 
-	if (!has_preferred_console || bcon || !console_drivers)
-		has_preferred_console = preferred_console >= 0;
+	if (need_default_console || bcon || !console_drivers)
+		need_default_console = 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.
 	 */
-	if (!has_preferred_console)
+	if (need_default_console)
 		try_enable_default_console(newcon);
 
 	/* See if this console matches one we selected on the command line */
-- 
2.26.2

[PATCH 3/5] printk/console: Remove unnecessary need_default_console manipulation

[Petr Mladek]

There is no need to clear @need_default_console when a console
preferred by the command line, device tree, or SPCR, gets enabled.

The code is called only when some non-braille console matched a console
in @console_cmdline array. It means that a non-braille console was added
in __add_preferred_console() and the variable preferred_console is set
to a number >= 0. As a result, @need_default_console is always set to
"false" in the magic condition:

	if (need_default_console || bcon || !console_drivers)
		need_default_console = preferred_console < 0;

This is one small step in removing the above magic condition
that is hard to follow.

The patch removes one superfluous assignment and should not change
the functionality.

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 4c5f496877b0..3f845daa3a4a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2892,10 +2892,8 @@ static int try_enable_preferred_console(struct console *newcon,
 				return err;
 		}
 		newcon->flags |= CON_ENABLED;
-		if (i == preferred_console) {
+		if (i == preferred_console)
 			newcon->flags |= CON_CONSDEV;
-			need_default_console = false;
-		}
 		return 0;
 	}
 
-- 
2.26.2

[PATCH 4/5] printk/console: Remove need_default_console variable

[Petr Mladek]

The variable @need_default_console is used to decide whether a newly
registered console should get enabled by default.

The logic is complicated. It can be modified in a register_console()
call. But it is always re-evaluated in the next call by the following
condition:

	if (need_default_console || bcon || !console_drivers)
		need_default_console = preferred_console < 0;

In short, the value is updated when either of the condition is valid:

  + the value is still, or again, "true"
  + boot/early console is still the first in @console_driver list
  + @console_driver list is empty

The value is updated according to @preferred_console. In particular,
it is set to "false" when a @preferred_console was set by
__add_preferred_console(). This happens when a non-braille console
was added via the command line, device tree, or SPCR.

It far from clear what this all means together. Let's look at
@need_default_console from another angle:

1. The value is "true" by default. It means that it is always set
   according to @preferred_console during the first register_console()
   call.

   By other words, the first register_console() call will register
   the console by default only when none non-braille console was defined
   via the command line, device tree, or SPCR.

2. The value will always stay "false" when @preferred_console is set.

   By other words, try_enable_default_console() will never get called
   when a non-braille console is explicitly required.

4. The value might be set to "false" in try_enable_default_console()
   when a console with tty binding (driver) gets enabled.

   In this case CON_CONSDEV is set as well. It causes that the console
   will be inserted as first into the list @console_driver. It might
   be either real or boot/early console.

5. The value will be set _back_ to "true" in the next register_console()
   call when:

      + The console added by the previous register_console() had been
	a boot/early one.

      + The last console has been unregistered in the meantime and
	a boot/early console became first in @console_drivers list
	again. Or the list became empty.

   By other words, the value will stay "false" only when the last
   registered console was real, had tty binding, and was not removed
   in the mean time.

The main logic looks clear:

  + Consoles are enabled by default only when no one is preferred
    via the command line, device tree, or SPCR.

  + By default, any console is enabled until a real console
    with tty binding gets registered.

The behavior when the real console with tty binding is later removed
is a bit unclear:

  + By default, any new console is registered again only when there
    is no console or the first console in the list is a boot one.

The question is why the code is suddenly happy when a real console
without tty binding is the first in the list. It looks like an overlook
and bug.

Conclusion:

The state of @preferred_console and the first console in @console_driver
list should be enough to decide whether we need to enable the given console
by default.

The rules are simple. New consoles are _not_ enabled by default
when either of the following conditions is true:

  + @preferred_console is set. It means that a non-braille console
    is explicitly configured via the command line, device tree, or SPCR.

  + A real console with tty binding is registered. Such a console will
    have CON_CONSDEV flag set and will always be the first in
    @console_drivers list.

Note:

The new code does not use @bcon variable. The meaning of the variable
is far from clear. The direct check of the first console in the list
makes it more clear that only real console fulfills requirements
of the default console.

Behavior change:

As already discussed above. There was one situation where the original
code worked a strange way. Let's have:

	+ console A: real console without tty binding
	+ console B: real console with tty binding

and do:

	register_console(A);	/* 1st step */
	register_console(B);	/* 2nd step */
	unregister_console(B);	/* 3rd step */
	register_console(B);	/* 4th step */

The original code will not register the console B in the 4th step.
@need_default_console is set to "false" in 2nd step. The real console
with tty binding (driver) is then removed in the 3rd step.
But @need_default_console will stay "false" in the 4th step because
there is no boot/early console and @registered_consoles list is not
empty.

The new code will register the console B in the 4th step because
it checks whether the first console has tty binding (->driver)

This behavior change should acceptable:

  1. The scenario requires manual intervention (console removal).
     The system should boot with the same consoles as before.

  2. Console B is registered again probably because the user wants
     to use it. The most likely scenario is that the related
     module is reloaded.

  3. It makes the behavior more consistent and predictable.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 3f845daa3a4a..6591da285a83 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -280,7 +280,6 @@ static struct console *exclusive_console;
 static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES];
 
 static int preferred_console = -1;
-static bool need_default_console = true;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
 
@@ -2919,10 +2918,8 @@ static void try_enable_default_console(struct console *newcon)
 
 	newcon->flags |= CON_ENABLED;
 
-	if (newcon->device) {
+	if (newcon->device)
 		newcon->flags |= CON_CONSDEV;
-		need_default_console = false;
-	}
 }
 
 /*
@@ -2972,16 +2969,24 @@ void register_console(struct console *newcon)
 	if (console_drivers && console_drivers->flags & CON_BOOT)
 		bcon = console_drivers;
 
-	if (need_default_console || bcon || !console_drivers)
-		need_default_console = 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.
+	 * See if we want to enable this console driver by default.
+	 *
+	 * Nope when a console is preferred by the command line, device
+	 * tree, or SPCR.
+	 *
+	 * The first real console with tty binding (driver) wins. More
+	 * consoles might get enabled before the right one is found.
+	 *
+	 * Note that a console with tty binding will have CON_CONSDEV
+	 * flag set and will be first in the list.
 	 */
-	if (need_default_console)
-		try_enable_default_console(newcon);
+	if (preferred_console < 0) {
+		if (!console_drivers || !console_drivers->device ||
+		    console_drivers->flags & CON_BOOT) {
+			try_enable_default_console(newcon);
+		}
+	}
 
 	/* See if this console matches one we selected on the command line */
 	err = try_enable_preferred_console(newcon, true);
-- 
2.26.2

[PATCH 5/5] printk/console: Clean up boot console handling in register_console()

[Petr Mladek]

The variable @bcon has two meanings. It is used several times for iterating
the list of registered consoles. In the meantime, it holds the information
whether a boot console is first in @console_drivers list.

The information about the 1st console driver used to be important for
the decision whether to install the new console by default or not.
It allowed to re-evaluate the variable @need_default_console when
a real console with tty binding has been unregistered in the meantime.

The decision about the default console is not longer affected by @bcon
variable. The current code checks whether the first driver is real
and has tty binding directly.

The information about the first console is still used for two more
decisions:

  1. It prevents duplicate output on non-boot consoles with
     CON_CONSDEV flag set.

  2. Early/boot consoles are unregistered when a real console with
     CON_CONSDEV is registered and @keep_bootcon is not set.

The behavior in the real life is far from obvious. @bcon is set according
to the first console @console_drivers list. But the first position in
the list is special:

  1. Consoles with CON_CONSDEV flag are put at the beginning of
     the list. It is either the preferred console or any console
     with tty binding registered by default.

  2. Another console might become the first in the list when
     the first console in the list is unregistered. It might
     happen either explicitly or automatically when boot
     consoles are unregistered.

There is one more important rule:

  + Boot consoles can't be registered when any real console
    is already registered.

It is a puzzle. The main complication is the dependency on the first
position is the list and the complicated rules around it.

Let's try to make it easier:

1. Add variable @bootcon_enabled and set it by iterating all registered
   consoles. The variable has obvious meaning and more predictable
   behavior. Any speed optimization and other tricks are not worth it.

2. Use a generic name for the variable that is used to iterate
   the list on registered console drivers.

Behavior change:

No, maybe surprisingly, there is _no_ behavior change!

Let's provide the proof by contradiction. Both operations, duplicate
output prevention and boot consoles removal, are done only when
the newly added console has CON_CONSDEV flag set. The behavior
would change when the new @bootcon_enabled has different value
than the original @bcon.

By other words, the behavior would change when the following conditions
are true:

   + a console with CON_CONSDEV flag is added
   + a real (non-boot) console is the first in the list
   + a boot console is later in the list

Now, a real console might be first in the list only when:

   + It was the first registered console. In this case, there can't be
     any boot console because any later ones were rejected.

   + It was put at the first position because it had CON_CONSDEV flag
     set. It was either the preferred console or it was a console with
     tty binding registered by default. We are interested only in
     a real consoles here. And real console with tty binding fulfills
     conditions of the default console.

     Now, there is always only one console that is either preferred
     or fulfills conditions of the default console. It can't be already
     in the list and being registered at the same time.

As a result, the above three conditions could newer be "true" at
the same time. Therefore the behavior can't change.

Final dilemma:

OK, the new code has the same behavior. But is the change in the right
direction? What if the handling of @console_drivers is updated in
the future?

OK, let's look at it from another angle:

1. The ordering of @console_drivers list is important only in
   console_device() function. The first console driver with tty
   binding gets associated with /dev/console.

2. CON_CONSDEV flag is shown in /proc/consoles. And it should be set
   for the driver that is returned by console_device().

3. A boot console is removed and the duplicated output is prevented
   when the real console with CON_CONSDEV flag is registered.

Now, in the ideal world:

+ The driver associated with /dev/console should be either a console
  preferred via the command line, device tree, or SPCR. Or it should
  be the first real console with tty binding registered by default.

+ The code should match the related boot and real console drivers.
  It should unregister only the obsolete boot driver. And the duplicated
  output should be prevented only on the related real driver.

It is clear that it is not guaranteed by the current code. Instead,
the current code looks like a maze of heuristics that try to achieve
the above.

It is result of adding several features over last few decades. For example,
a possibility to register more consoles, unregister consoles, boot
consoles, consoles without tty binding, device tree, SPCR, braille
consoles.

Anyway, there is no reason why the decision, about removing boot consoles
and preventing duplicated output, should depend on the first console
in the list. The current code does the decisions primary by CON_CONSDEV
flag that is used for the preferred console. It looks like a
good compromise. And the change seems to be in the right direction.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 6591da285a83..155229f0cf0f 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2943,31 +2943,30 @@ static void try_enable_default_console(struct console *newcon)
  */
 void register_console(struct console *newcon)
 {
-	struct console *bcon = NULL;
+	struct console *con;
+	bool bootcon_enabled = false;
+	bool realcon_enabled = false;
 	int err;
 
-	for_each_console(bcon) {
-		if (WARN(bcon == newcon, "console '%s%d' already registered\n",
-					 bcon->name, bcon->index))
+	for_each_console(con) {
+		if (WARN(con == newcon, "console '%s%d' already registered\n",
+					 con->name, con->index))
 			return;
 	}
 
-	/*
-	 * before we register a new CON_BOOT console, make sure we don't
-	 * already have a valid console
-	 */
-	if (newcon->flags & CON_BOOT) {
-		for_each_console(bcon) {
-			if (!(bcon->flags & CON_BOOT)) {
-				pr_info("Too late to register bootconsole %s%d\n",
-					newcon->name, newcon->index);
-				return;
-			}
-		}
+	for_each_console(con) {
+		if (con->flags & CON_BOOT)
+			bootcon_enabled = true;
+		else
+			realcon_enabled = true;
 	}
 
-	if (console_drivers && console_drivers->flags & CON_BOOT)
-		bcon = console_drivers;
+	/* Do not register boot consoles when there already is a real one. */
+	if (newcon->flags & CON_BOOT && realcon_enabled) {
+		pr_info("Too late to register bootconsole %s%d\n",
+			newcon->name, newcon->index);
+		return;
+	}
 
 	/*
 	 * See if we want to enable this console driver by default.
@@ -3005,8 +3004,10 @@ void register_console(struct console *newcon)
 	 * the real console are the same physical device, it's annoying to
 	 * see the beginning boot messages twice
 	 */
-	if (bcon && ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV))
+	if (bootcon_enabled &&
+	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV)) {
 		newcon->flags &= ~CON_PRINTBUFFER;
+	}
 
 	/*
 	 *	Put this console in the list - keep the
@@ -3062,15 +3063,15 @@ void register_console(struct console *newcon)
 	pr_info("%sconsole [%s%d] enabled\n",
 		(newcon->flags & CON_BOOT) ? "boot" : "" ,
 		newcon->name, newcon->index);
-	if (bcon &&
+	if (bootcon_enabled &&
 	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
 	    !keep_bootcon) {
 		/* We need to iterate through all boot consoles, to make
 		 * sure we print everything out, before we unregister them.
 		 */
-		for_each_console(bcon)
-			if (bcon->flags & CON_BOOT)
-				unregister_console(bcon);
+		for_each_console(con)
+			if (con->flags & CON_BOOT)
+				unregister_console(con);
 	}
 }
 EXPORT_SYMBOL(register_console);
-- 
2.26.2

Re: [PATCH 1/5] printk/console: Split out code that enables default console

[Sergey Senozhatsky]

On (21/11/22 14:26), Petr Mladek wrote:
> Put the code enabling a console by default into a separate function
> called try_enable_default_console().
> 
> Rename try_enable_new_console() to try_enable_preferred_console() to
> make the purpose of the different variants more clear.
> 
> It is a code refactoring without any functional change.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

[..]
> -static int try_enable_new_console(struct console *newcon, bool user_specified)
> +static int try_enable_preferred_console(struct console *newcon,
> +					bool user_specified)
>  {
>  	struct console_cmdline *c;
>  	int i, err;
> @@ -2909,6 +2910,23 @@ static int try_enable_new_console(struct console *newcon, bool user_specified)
>  	return -ENOENT;
>  }
>  
> +/* Try to enable the console unconditionally */
> +static void try_enable_default_console(struct console *newcon)
> +{
> +	if (newcon->index < 0)
> +		newcon->index = 0;
> +
> +	if (newcon->setup && newcon->setup(newcon, NULL) != 0)
> +		return;
> +
> +	newcon->flags |= CON_ENABLED;
> +
> +	if (newcon->device) {
> +		newcon->flags |= CON_CONSDEV;
> +		has_preferred_console = true;
> +	}
> +}

try_enable_default_console() also sets preferred_console, as well as
try_enable_preferred_console().

Re: [PATCH 2/5] printk/console: Rename has_preferred_console to need_default_console

[Sergey Senozhatsky]

On (21/11/22 14:26), Petr Mladek wrote:
> The logic around the variable @has_preferred_console made my head
> spin many times. Part of the problem is the ambiguous name.
> 
> There is the variable @preferred_console. It points to the last
> non-braille console in @console_cmdline array. This array contains
> consoles preferred via the command line, device tree, or SPCR.
> 
> Then there is the variable @has_preferred_console. It is set to
> "true" when @preferred_console is enabled or when a console with
> tty binding gets enabled by default.
> 
> It might get reset back by the magic condition:
> 
> 	if (!has_preferred_console || bcon || !console_drivers)
> 		has_preferred_console = preferred_console >= 0;
> 
> It is a puzzle. Dumb explanation is that it gets re-evaluated
> when:
> 
> 	+ it was not set before (see above when it gets set)
> 	+ there is still an early console enabled (bcon)
> 	+ there is no console enabled (!console_drivers)
> 
> This is still a puzzle.
> 
> It gets more clear when we see where the value is checked. The only
> meaning of the variable is to decide whether we should try to enable
> the new console by default.

A nit: by "new console" you probably mean preferred_console. It sort
of suggests that try_enable_new_console() was not such a bad name,
may be, since we still refer to such consoles as "new" not "preferred".


> Rename the variable according to the single situation where
> the value is checked.
> 
> The rename requires an inverted logic. Otherwise, it is a simple
> search & replace. It does not change the functionality.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH 1/5] printk/console: Split out code that enables default console

[Petr Mladek]

On Tue 2021-11-23 11:10:18, Sergey Senozhatsky wrote:
> On (21/11/22 14:26), Petr Mladek wrote:
> > Put the code enabling a console by default into a separate function
> > called try_enable_default_console().
> > 
> > Rename try_enable_new_console() to try_enable_preferred_console() to
> > make the purpose of the different variants more clear.
> > 
> > It is a code refactoring without any functional change.
> > 
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> 
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> 
> [..]
> > -static int try_enable_new_console(struct console *newcon, bool user_specified)
> > +static int try_enable_preferred_console(struct console *newcon,
> > +					bool user_specified)
> >  {
> >  	struct console_cmdline *c;
> >  	int i, err;
> > @@ -2909,6 +2910,23 @@ static int try_enable_new_console(struct console *newcon, bool user_specified)
> >  	return -ENOENT;
> >  }
> >  
> > +/* Try to enable the console unconditionally */
> > +static void try_enable_default_console(struct console *newcon)
> > +{
> > +	if (newcon->index < 0)
> > +		newcon->index = 0;
> > +
> > +	if (newcon->setup && newcon->setup(newcon, NULL) != 0)
> > +		return;
> > +
> > +	newcon->flags |= CON_ENABLED;
> > +
> > +	if (newcon->device) {
> > +		newcon->flags |= CON_CONSDEV;
> > +		has_preferred_console = true;
> > +	}
> > +}
> 
> try_enable_default_console() also sets preferred_console, as well as
> try_enable_preferred_console().

Yes, this is one of the confusing things that this patchset is trying to solve.

The result of this patchset is that preferred_console will be used
only for consoles that are explicitely preferred via the command line,
device tree, or SPCR.

The variable @has_preferred_console is renamed to @need_default_console
in 2nd patch and completely removed in 4th patch.

Anyway, thanks for review.

Best Regards,
Petr

Re: [PATCH 2/5] printk/console: Rename has_preferred_console to need_default_console

[Petr Mladek]

On Tue 2021-11-23 11:15:52, Sergey Senozhatsky wrote:
> On (21/11/22 14:26), Petr Mladek wrote:
> > The logic around the variable @has_preferred_console made my head
> > spin many times. Part of the problem is the ambiguous name.
> > 
> > There is the variable @preferred_console. It points to the last
> > non-braille console in @console_cmdline array. This array contains
> > consoles preferred via the command line, device tree, or SPCR.
> > 
> > Then there is the variable @has_preferred_console. It is set to
> > "true" when @preferred_console is enabled or when a console with
> > tty binding gets enabled by default.
> > 
> > It might get reset back by the magic condition:
> > 
> > 	if (!has_preferred_console || bcon || !console_drivers)
> > 		has_preferred_console = preferred_console >= 0;
> > 
> > It is a puzzle. Dumb explanation is that it gets re-evaluated
> > when:
> > 
> > 	+ it was not set before (see above when it gets set)
> > 	+ there is still an early console enabled (bcon)
> > 	+ there is no console enabled (!console_drivers)
> > 
> > This is still a puzzle.
> > 
> > It gets more clear when we see where the value is checked. The only
> > meaning of the variable is to decide whether we should try to enable
> > the new console by default.
> 
> A nit: by "new console" you probably mean preferred_console. It sort
> of suggests that try_enable_new_console() was not such a bad name,
> may be, since we still refer to such consoles as "new" not "preferred".

By "new console" I mean the console that is passed being registered.
It is the console passed by @newcon parameter.

In compare, @preferred_console is only one. It is the last non-braille
consoles added by __add_preferred_console().

Outlook:

I have a followup patch set that renames @console_cmdline[] to
@preferred_consoles[]. The array includes consoles that are preferred
also by the device tree or SPCR. It is not only by the command line.

The patch also renames @preferred_console to @last_preferred_console.
It helps to distinguish it from the array name. Anyway, the last
console is just one of the preferred consoles. It is special only
because it should get associated with /dev/console.

The renaming causes a lot of noise. I am not sure if it is worth it.
It is currently done as the very last step (23rd patch) after
the rest of the logic is cleaned. But if you like it. I could
do it earlier.

Thanks for review.

Best Regards,
Petr

Re: [PATCH 0/5] printk/console: Registration code cleanup - part 1

[Petr Mladek]

On Mon 2021-11-22 14:26:44, Petr Mladek wrote:
> The console registration code has several twists that are hard to follow.
> It is result of various features added over the last few decades.
> 
> I have already spent many days to understand all the effects and
> the desired behavior. I am always scared to touch the console registration
> code even after years working as printk maintainer.
> 
> There were several changes in the code that had to be reverted because
> they caused regression, for example:
> 
>    + commit dac8bbbae1d0ccba96402 ("Revert "printk: fix double printing
>      with earlycon")
> 
>    + commit c6c7d83b9c9e6a8b3e6d ("Revert "console: don't prefer first
>      registered if DT specifies stdout-path")
> 
> 
> This patchset removes the most tricky twists from my POV. I have worked
> on it when time permitted since January. I have spent most of the time
> writing the commit message, understanding, and explaining the effects.
> I am not sure if I succeeded but it is time to send this.
> 
> 
> Behavior change:
> 
> There is one behavior change caused by 4th patch. I consider it bug fix.
> It should be acceptable. See the commit message for more details.

I am afraid that the patchset is not easy to review. I decided to help
a bit and give it a spin in linux-next.

The patchset has been committed into printk/linux.git, branch
console-registration-cleanup. It should appear in linux-next
during the next respin.

I am going to take it back when anyone reports any problem that
can't be fixed easily.

Any feedback is still appreciated.

Best Regards,
Petr