[PATCH v2 0/2] console: don't select first registered console if stdout-path used

[PATCH v2 0/2] console: don't select first registered console if stdout-path used

[Eugeniy Paltsev]

Don't select first registered console if one is specified by the device tree
via the stdout-path (or linux,stdout-path) chosen node properties.

Eugeniy Paltsev (2):
  OF: move extern declarations of of_stdout inside ifdef
  console: don't select first registered console if stdout-path used
  
Changes v1->v2:
 * Add exception for "tty0" console as current tty0 behavior is widely used
   by ARM and powerpc boards.

 include/linux/of.h     |  4 ++-
 kernel/printk/printk.c | 84 +++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 72 insertions(+), 16 deletions(-)

-- 
2.9.3

[PATCH v2 0/2] console: don't select first registered console if stdout-path used

[Eugeniy Paltsev]

Don't select first registered console if one is specified by the device tree
via the stdout-path (or linux,stdout-path) chosen node properties.

Eugeniy Paltsev (2):
  OF: move extern declarations of of_stdout inside ifdef
  console: don't select first registered console if stdout-path used
  
Changes v1->v2:
 * Add exception for "tty0" console as current tty0 behavior is widely used
   by ARM and powerpc boards.

 include/linux/of.h     |  4 ++-
 kernel/printk/printk.c | 84 +++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 72 insertions(+), 16 deletions(-)

-- 
2.9.3

[PATCH v2 1/2] OF: move extern declarations of of_stdout inside ifdef

[Eugeniy Paltsev]

Move extern declarations of "of_stdout" pointer inside "CONFIG_OF"
ifdef to be able to get rid of "CONFIG_OF" ifdef in its usage places.

Acked-by: Rob Herring <robh at kernel.org>
Suggested-by: Steven Rostedt <rostedt at goodmis.org>
Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev at synopsys.com>
---
 include/linux/of.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index cfc3411..be28cb9 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -135,7 +135,6 @@ static inline void of_node_put(struct device_node *node) { }
 extern struct device_node *of_root;
 extern struct device_node *of_chosen;
 extern struct device_node *of_aliases;
-extern struct device_node *of_stdout;
 extern raw_spinlock_t devtree_lock;
 
 /* flag descriptions (need to be visible even when !CONFIG_OF) */
@@ -147,6 +146,8 @@ extern raw_spinlock_t devtree_lock;
 #define OF_BAD_ADDR	((u64)-1)
 
 #ifdef CONFIG_OF
+extern struct device_node *of_stdout;
+
 void of_core_init(void);
 
 static inline bool is_of_node(const struct fwnode_handle *fwnode)
@@ -539,6 +540,7 @@ const char *of_prop_next_string(struct property *prop, const char *cur);
 bool of_console_check(struct device_node *dn, char *name, int index);
 
 #else /* CONFIG_OF */
+#define of_stdout	NULL
 
 static inline void of_core_init(void)
 {
-- 
2.9.3

[PATCH v2 1/2] OF: move extern declarations of of_stdout inside ifdef

[Eugeniy Paltsev]

Move extern declarations of "of_stdout" pointer inside "CONFIG_OF"
ifdef to be able to get rid of "CONFIG_OF" ifdef in its usage places.

Acked-by: Rob Herring <robh@kernel.org>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 include/linux/of.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index cfc3411..be28cb9 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -135,7 +135,6 @@ static inline void of_node_put(struct device_node *node) { }
 extern struct device_node *of_root;
 extern struct device_node *of_chosen;
 extern struct device_node *of_aliases;
-extern struct device_node *of_stdout;
 extern raw_spinlock_t devtree_lock;
 
 /* flag descriptions (need to be visible even when !CONFIG_OF) */
@@ -147,6 +146,8 @@ extern raw_spinlock_t devtree_lock;
 #define OF_BAD_ADDR	((u64)-1)
 
 #ifdef CONFIG_OF
+extern struct device_node *of_stdout;
+
 void of_core_init(void);
 
 static inline bool is_of_node(const struct fwnode_handle *fwnode)
@@ -539,6 +540,7 @@ const char *of_prop_next_string(struct property *prop, const char *cur);
 bool of_console_check(struct device_node *dn, char *name, int index);
 
 #else /* CONFIG_OF */
+#define of_stdout	NULL
 
 static inline void of_core_init(void)
 {
-- 
2.9.3

[PATCH v2 2/2] console: don't select first registered console if stdout-path used

[Eugeniy Paltsev]

In the current implementation we take the first console that
registers if we didn't select one.

But if we specify console via "stdout-path" property in device tree
we don't want first console that registers here to be selected.
Otherwise we may choose wrong console - for example if some console
is registered earlier than console is pointed in "stdout-path"
property because console pointed in "stdout-path" property can be add as
preferred quite late - when it's driver is probed.

We retain previous behavior for tty0 console (if "stdout-path" used)
as a special case:
tty0 will be registered even if it was specified neither
in "bootargs" nor in "stdout-path".
We had to retain this behavior because a lot of ARM boards (and some
powerpc) rely on it.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev at synopsys.com>
---
Changes v1->v2:
 * Add exception for "tty0" console as current behavior is widely used
   by ARM and powerpc boards.

 kernel/printk/printk.c | 84 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 69 insertions(+), 15 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 512f7c2..be40f57 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -26,6 +26,7 @@
 #include <linux/nmi.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/smp.h>
 #include <linux/security.h>
@@ -2376,6 +2377,55 @@ static int __init keep_bootcon_setup(char *str)
 
 early_param("keep_bootcon", keep_bootcon_setup);
 
+static bool console_selected_by_of(void)
+{
+	return !!of_stdout;
+}
+
+static bool can_be_tty0(struct console *newcon)
+{
+	struct console *con = NULL;
+
+	if (newcon->index > 0)
+		return false;
+
+	if (strcmp(newcon->name, "tty") != 0)
+		return false;
+
+	if (newcon->index == 0)
+		return true;
+
+	/* do we have "tty" console already registered? */
+	for_each_console(con) {
+		if (strcmp(con->name, "tty") != 0)
+			continue;
+
+		if (con->index >= 0)
+			return false;
+	}
+
+	return true;
+}
+
+static bool take_console_noopts(struct console *newcon)
+{
+	if (newcon->index < 0)
+		newcon->index = 0;
+
+	if ((newcon->setup != NULL) && (newcon->setup(newcon, NULL) != 0))
+		return false;
+
+	newcon->flags |= CON_ENABLED;
+
+	if (newcon->device && !can_be_tty0(newcon))
+		newcon->flags |= CON_CONSDEV;
+
+	if (newcon->device)
+		return true;
+
+	return false;
+}
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -2432,22 +2482,26 @@ void register_console(struct console *newcon)
 		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.
+	 * If we specify console via "stdout-path" property in device tree
+	 * we don't want first console that registers here to be selected.
 	 */
-	if (!has_preferred) {
-		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 = true;
-			}
-		}
-	}
+	if (console_selected_by_of())
+		has_preferred = true;
+
+	/*
+	 * 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)
+		has_preferred |= take_console_noopts(newcon);
+
+	/*
+	 * Treat "tty0" (in case of "stdout-path" using) as a special case:
+	 * "tty0" will be registered even if it was specified neither in
+	 * "bootargs" nor in "stdout-path".
+	 */
+	if (console_selected_by_of() && can_be_tty0(newcon))
+		has_preferred |= take_console_noopts(newcon);
 
 	/*
 	 *	See if this console matches one we selected on
-- 
2.9.3

[PATCH v2 2/2] console: don't select first registered console if stdout-path used

[Eugeniy Paltsev]

In the current implementation we take the first console that
registers if we didn't select one.

But if we specify console via "stdout-path" property in device tree
we don't want first console that registers here to be selected.
Otherwise we may choose wrong console - for example if some console
is registered earlier than console is pointed in "stdout-path"
property because console pointed in "stdout-path" property can be add as
preferred quite late - when it's driver is probed.

We retain previous behavior for tty0 console (if "stdout-path" used)
as a special case:
tty0 will be registered even if it was specified neither
in "bootargs" nor in "stdout-path".
We had to retain this behavior because a lot of ARM boards (and some
powerpc) rely on it.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
Changes v1->v2:
 * Add exception for "tty0" console as current behavior is widely used
   by ARM and powerpc boards.

 kernel/printk/printk.c | 84 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 69 insertions(+), 15 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 512f7c2..be40f57 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -26,6 +26,7 @@
 #include <linux/nmi.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/smp.h>
 #include <linux/security.h>
@@ -2376,6 +2377,55 @@ static int __init keep_bootcon_setup(char *str)
 
 early_param("keep_bootcon", keep_bootcon_setup);
 
+static bool console_selected_by_of(void)
+{
+	return !!of_stdout;
+}
+
+static bool can_be_tty0(struct console *newcon)
+{
+	struct console *con = NULL;
+
+	if (newcon->index > 0)
+		return false;
+
+	if (strcmp(newcon->name, "tty") != 0)
+		return false;
+
+	if (newcon->index == 0)
+		return true;
+
+	/* do we have "tty" console already registered? */
+	for_each_console(con) {
+		if (strcmp(con->name, "tty") != 0)
+			continue;
+
+		if (con->index >= 0)
+			return false;
+	}
+
+	return true;
+}
+
+static bool take_console_noopts(struct console *newcon)
+{
+	if (newcon->index < 0)
+		newcon->index = 0;
+
+	if ((newcon->setup != NULL) && (newcon->setup(newcon, NULL) != 0))
+		return false;
+
+	newcon->flags |= CON_ENABLED;
+
+	if (newcon->device && !can_be_tty0(newcon))
+		newcon->flags |= CON_CONSDEV;
+
+	if (newcon->device)
+		return true;
+
+	return false;
+}
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -2432,22 +2482,26 @@ void register_console(struct console *newcon)
 		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.
+	 * If we specify console via "stdout-path" property in device tree
+	 * we don't want first console that registers here to be selected.
 	 */
-	if (!has_preferred) {
-		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 = true;
-			}
-		}
-	}
+	if (console_selected_by_of())
+		has_preferred = true;
+
+	/*
+	 * 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)
+		has_preferred |= take_console_noopts(newcon);
+
+	/*
+	 * Treat "tty0" (in case of "stdout-path" using) as a special case:
+	 * "tty0" will be registered even if it was specified neither in
+	 * "bootargs" nor in "stdout-path".
+	 */
+	if (console_selected_by_of() && can_be_tty0(newcon))
+		has_preferred |= take_console_noopts(newcon);
 
 	/*
 	 *	See if this console matches one we selected on
-- 
2.9.3

[PATCH v2 2/2] console: don't select first registered console if stdout-path used

[Petr Mladek]

On Mon 2017-08-28 19:58:07, Eugeniy Paltsev wrote:
> In the current implementation we take the first console that
> registers if we didn't select one.
> 
> But if we specify console via "stdout-path" property in device tree
> we don't want first console that registers here to be selected.
> Otherwise we may choose wrong console - for example if some console
> is registered earlier than console is pointed in "stdout-path"
> property because console pointed in "stdout-path" property can be add as
> preferred quite late - when it's driver is probed.

register_console() is really twisted function. I would like to better
understand your problems before we add yet another twist there.

Could you please be more specific about your problems?
What was the output of "cat /proc/consoles" before and after the fix?
What exactly started and stopped working?


> We retain previous behavior for tty0 console (if "stdout-path" used)
> as a special case:
> tty0 will be registered even if it was specified neither
> in "bootargs" nor in "stdout-path".
> We had to retain this behavior because a lot of ARM boards (and some
> powerpc) rely on it.

My main concern is the exception for "tty". Yes, it was regiression
reported in the commit c6c7d83b9c9e6a8b3e ("Revert "console: don't
prefer first registered if DT specifies stdout-path""). But is this
the only possible regression?


All this is about the fallback code that tries to enable all
consoles until a real one with tty binding (newcon->device)
is enabled.

v1 version of you patch disabled this fallback code when a console
was defined by stdout-path in the device tree. This emulates
defining the console by console= parameter on the command line.

It might make sense until some complains that a console is not
longer automatically enabled while it was before. But wait.
Someone already complained about "tty0". We can solve this
by adding an exception for "tty0". And if anyone else complains
about another console, we might need more exceptions.

We might endup with so many exceptions that the fallback code
will be always used. But then we are back in the square
and have the original behavior before your patch.

This is why I would like to know more info about your problem.
We need to decide if it is more important than a regression.
Or if it can be fixed another way.

Best Regards,
Petr

Re: [PATCH v2 2/2] console: don't select first registered console if stdout-path used

[Petr Mladek]

On Mon 2017-08-28 19:58:07, Eugeniy Paltsev wrote:
> In the current implementation we take the first console that
> registers if we didn't select one.
> 
> But if we specify console via "stdout-path" property in device tree
> we don't want first console that registers here to be selected.
> Otherwise we may choose wrong console - for example if some console
> is registered earlier than console is pointed in "stdout-path"
> property because console pointed in "stdout-path" property can be add as
> preferred quite late - when it's driver is probed.

register_console() is really twisted function. I would like to better
understand your problems before we add yet another twist there.

Could you please be more specific about your problems?
What was the output of "cat /proc/consoles" before and after the fix?
What exactly started and stopped working?


> We retain previous behavior for tty0 console (if "stdout-path" used)
> as a special case:
> tty0 will be registered even if it was specified neither
> in "bootargs" nor in "stdout-path".
> We had to retain this behavior because a lot of ARM boards (and some
> powerpc) rely on it.

My main concern is the exception for "tty". Yes, it was regiression
reported in the commit c6c7d83b9c9e6a8b3e ("Revert "console: don't
prefer first registered if DT specifies stdout-path""). But is this
the only possible regression?


All this is about the fallback code that tries to enable all
consoles until a real one with tty binding (newcon->device)
is enabled.

v1 version of you patch disabled this fallback code when a console
was defined by stdout-path in the device tree. This emulates
defining the console by console= parameter on the command line.

It might make sense until some complains that a console is not
longer automatically enabled while it was before. But wait.
Someone already complained about "tty0". We can solve this
by adding an exception for "tty0". And if anyone else complains
about another console, we might need more exceptions.

We might endup with so many exceptions that the fallback code
will be always used. But then we are back in the square
and have the original behavior before your patch.

This is why I would like to know more info about your problem.
We need to decide if it is more important than a regression.
Or if it can be fixed another way.

Best Regards,
Petr

[PATCH v2 2/2] console: don't select first registered console if stdout-path used

[Eugeniy Paltsev]

Hi Petr,

On Tue, 2017-09-05@16:54 +0200, Petr Mladek wrote:
> On Mon 2017-08-28 19:58:07, Eugeniy Paltsev wrote:
> > In the current implementation we take the first console that
> > registers if we didn't select one.
> >?
> > But if we specify console via "stdout-path" property in device tree
> > we don't want first console that registers here to be selected.
> > Otherwise we may choose wrong console - for example if some console
> > is registered earlier than console is pointed in "stdout-path"
> > property because console pointed in "stdout-path" property can be add as
> > preferred quite late - when it's driver is probed.
>?
> register_console() is really twisted function. I would like to better
> understand your problems before we add yet another twist there.
>?
> Could you please be more specific about your problems?
> What was the output of "cat /proc/consoles" before and after the fix?
> What exactly started and stopped working?

Ok, I faced with several problems when I tried to use stdout-path and this
patch solves all of them.
There is the description of some of the problems:

-----------------------------------------------------------------------------------
Problem 1: choosing wrong serial console device

Context:
Serial console device specified via "stdout-path" property in device tree,
support for console on virtual terminal is disabled (CONFIG_VT_CONSOLE is
not selected, CONFIG_VT is selected)

In this case wrong console device can be selected.

Example:
Device tree:
-------------->8--------
chosen {
????bootargs = ""
????stdout-path = &serial_1;
};

serial_0: uart-0 at ... {} /* FAIL: serial_0 is used as console (ttyS0) as it is
?????????????????????????* probed earlier */
serial_1: uart-1 at ... {}
-------------->8--------

# cat /proc/consoles
ttyS0????????????????-W- (EC???a)????4:64????/* FAIL: ttyS0 is used instead of?
??????????????????????????????????????????????* ttyS1 */


This FAIL happens because we take the first registered console if we didn't select
a console via "console=" option in bootargs.

After my patch-v2:
# cat /proc/consoles
ttyS1????????????????-W- (EC p a)????4:67


-----------------------------------------------------------------------------------
Problem 2: printing early boot messages twice and pause in boot messages printing

Context:
We use early console. Serial console device (and early console device) specified
via "stdout-path" property in device tree.?
Support for console on virtual terminal is enabled (CONFIG_VT_CONSOLE=y)

In this case early boot messages will be printed twice - firstly by
bootconsole and after that by 'real' serial console.
Also we will get pause in boot messages printing - as bootconsole will be disabled
mush earlier than 'real' serial console is enabled.

Example:
-------------->8--------
chosen {
????bootargs = "earlycon"
????stdout-path = &serial_3;
};

serial_3: uart-3 at ... {}?
-------------->8--------

So output of serial console will be be like that:
-------------->8--------
XXX - early boot messages, printed by bootconsole
????- FAIL: pause in boot messages printing
XXX - FAIL: again early boot messages, printed by serial console
YYY - rest of boot messages, printed by serial console
-------------->8--------

So the order of enabling/disabling consoles will be like that:
-------------->8--------
bootconsole [uart0] enabled
console [tty0] enabled??????????????/* As no console is select 'tty0' was taken */
bootconsole [uart0] disabled????????/* As we have real (tty0) console we disable
?????????????????????????????????????* all bootconsoles */
console [ttyS3] enabled?????????????/* We take ttyS3 but don't reset its?
?????????????????????????????????????* CON_PRINTBUFFER flag (as there is NO enabled
				?????* bootconsoles) */
-------------->8--------


# cat /proc/consoles
ttyS3????????????????-W- (EC p a)????4:67
tty0?????????????????-WU (E?????)????4:1

As you can see CON_PRINTBUFFER flag (p) set for ttyS3 - that is wrong.

After my patch-v2:
# cat /proc/consoles
ttyS3????????????????-W- (EC???a)????4:67
tty0?????????????????-WU (E??p??)????4:1


These are the problems I have faced but these are NOT THE ONLY POSSIBLE problems
because current behavior is quite unstable and unpredictable.

And of course I would prefer to use simple solution from v1 patch version
but in this case we will face with someone complaining about "tty0".

So all comments and suggestions are more than welcome.

?
> > We retain previous behavior for tty0 console (if "stdout-path" used)
> > as a special case:
> > tty0 will be registered even if it was specified neither
> > in "bootargs" nor in "stdout-path".
> > We had to retain this behavior because a lot of ARM boards (and some
> > powerpc) rely on it.
>?
> My main concern is the exception for "tty". Yes, it was regiression
> reported in the commit c6c7d83b9c9e6a8b3e ("Revert "console: don't
> prefer first registered if DT specifies stdout-path""). But is this
> the only possible regression?
>?
>?
> All this is about the fallback code that tries to enable all
> consoles until a real one with tty binding (newcon->device)
> is enabled.
>?
> v1 version of you patch disabled this fallback code when a console
> was defined by stdout-path in the device tree. This emulates
> defining the console by console= parameter on the command line.
>?
> It might make sense until some complains that a console is not
> longer automatically enabled while it was before. But wait.
> Someone already complained about "tty0". We can solve this
> by adding an exception for "tty0". And if anyone else complains
> about another console, we might need more exceptions.
>
> We might endup with so many exceptions that the fallback code
> will be always used. But then we are back in the square
> and have the original behavior before your patch.
>?

Yes, I understand your concerns.

But I also have another concern: If we decide to left current behavior untouched
(like after reverting patch 05fd007e4629)
more and more boards and devices will use current broken stdout-path behavior in?
any form and in the results we will get the situation when we can't fix
stdout-path behavior at all - because every change will break something somewhere.

(05fd007e4629 patch do absolutely the same as v1 version of my patch)

> This is why I would like to know more info about your problem.
> We need to decide if it is more important than a regression.
> Or if it can be fixed another way.
>?
> Best Regards,
> Petr
--?
?Eugeniy Paltsev

Re: [PATCH v2 2/2] console: don't select first registered console if stdout-path used

[Eugeniy Paltsev]

Hi Petr,

On Tue, 2017-09-05 at 16:54 +0200, Petr Mladek wrote:
> On Mon 2017-08-28 19:58:07, Eugeniy Paltsev wrote:
> > In the current implementation we take the first console that
> > registers if we didn't select one.
> > 
> > But if we specify console via "stdout-path" property in device tree
> > we don't want first console that registers here to be selected.
> > Otherwise we may choose wrong console - for example if some console
> > is registered earlier than console is pointed in "stdout-path"
> > property because console pointed in "stdout-path" property can be add as
> > preferred quite late - when it's driver is probed.
> 
> register_console() is really twisted function. I would like to better
> understand your problems before we add yet another twist there.
> 
> Could you please be more specific about your problems?
> What was the output of "cat /proc/consoles" before and after the fix?
> What exactly started and stopped working?

Ok, I faced with several problems when I tried to use stdout-path and this
patch solves all of them.
There is the description of some of the problems:

-----------------------------------------------------------------------------------
Problem 1: choosing wrong serial console device

Context:
Serial console device specified via "stdout-path" property in device tree,
support for console on virtual terminal is disabled (CONFIG_VT_CONSOLE is
not selected, CONFIG_VT is selected)

In this case wrong console device can be selected.

Example:
Device tree:
-------------->8--------
chosen {
    bootargs = ""
    stdout-path = &serial_1;
};

serial_0: uart-0@... {} /* FAIL: serial_0 is used as console (ttyS0) as it is
                         * probed earlier */
serial_1: uart-1@... {}
-------------->8--------

# cat /proc/consoles
ttyS0                -W- (EC   a)    4:64    /* FAIL: ttyS0 is used instead of 
                                              * ttyS1 */


This FAIL happens because we take the first registered console if we didn't select
a console via "console=" option in bootargs.

After my patch-v2:
# cat /proc/consoles
ttyS1                -W- (EC p a)    4:67


-----------------------------------------------------------------------------------
Problem 2: printing early boot messages twice and pause in boot messages printing

Context:
We use early console. Serial console device (and early console device) specified
via "stdout-path" property in device tree. 
Support for console on virtual terminal is enabled (CONFIG_VT_CONSOLE=y)

In this case early boot messages will be printed twice - firstly by
bootconsole and after that by 'real' serial console.
Also we will get pause in boot messages printing - as bootconsole will be disabled
mush earlier than 'real' serial console is enabled.

Example:
-------------->8--------
chosen {
    bootargs = "earlycon"
    stdout-path = &serial_3;
};

serial_3: uart-3@... {} 
-------------->8--------

So output of serial console will be be like that:
-------------->8--------
XXX - early boot messages, printed by bootconsole
    - FAIL: pause in boot messages printing
XXX - FAIL: again early boot messages, printed by serial console
YYY - rest of boot messages, printed by serial console
-------------->8--------

So the order of enabling/disabling consoles will be like that:
-------------->8--------
bootconsole [uart0] enabled
console [tty0] enabled              /* As no console is select 'tty0' was taken */
bootconsole [uart0] disabled        /* As we have real (tty0) console we disable
                                     * all bootconsoles */
console [ttyS3] enabled             /* We take ttyS3 but don't reset its 
                                     * CON_PRINTBUFFER flag (as there is NO enabled
				     * bootconsoles) */
-------------->8--------


# cat /proc/consoles
ttyS3                -W- (EC p a)    4:67
tty0                 -WU (E     )    4:1

As you can see CON_PRINTBUFFER flag (p) set for ttyS3 - that is wrong.

After my patch-v2:
# cat /proc/consoles
ttyS3                -W- (EC   a)    4:67
tty0                 -WU (E  p  )    4:1


These are the problems I have faced but these are NOT THE ONLY POSSIBLE problems
because current behavior is quite unstable and unpredictable.

And of course I would prefer to use simple solution from v1 patch version
but in this case we will face with someone complaining about "tty0".

So all comments and suggestions are more than welcome.

 
> > We retain previous behavior for tty0 console (if "stdout-path" used)
> > as a special case:
> > tty0 will be registered even if it was specified neither
> > in "bootargs" nor in "stdout-path".
> > We had to retain this behavior because a lot of ARM boards (and some
> > powerpc) rely on it.
> 
> My main concern is the exception for "tty". Yes, it was regiression
> reported in the commit c6c7d83b9c9e6a8b3e ("Revert "console: don't
> prefer first registered if DT specifies stdout-path""). But is this
> the only possible regression?
> 
> 
> All this is about the fallback code that tries to enable all
> consoles until a real one with tty binding (newcon->device)
> is enabled.
> 
> v1 version of you patch disabled this fallback code when a console
> was defined by stdout-path in the device tree. This emulates
> defining the console by console= parameter on the command line.
> 
> It might make sense until some complains that a console is not
> longer automatically enabled while it was before. But wait.
> Someone already complained about "tty0". We can solve this
> by adding an exception for "tty0". And if anyone else complains
> about another console, we might need more exceptions.
>
> We might endup with so many exceptions that the fallback code
> will be always used. But then we are back in the square
> and have the original behavior before your patch.
> 

Yes, I understand your concerns.

But I also have another concern: If we decide to left current behavior untouched
(like after reverting patch 05fd007e4629)
more and more boards and devices will use current broken stdout-path behavior in 
any form and in the results we will get the situation when we can't fix
stdout-path behavior at all - because every change will break something somewhere.

(05fd007e4629 patch do absolutely the same as v1 version of my patch)

> This is why I would like to know more info about your problem.
> We need to decide if it is more important than a regression.
> Or if it can be fixed another way.
> 
> Best Regards,
> Petr
-- 
 Eugeniy Paltsev

[PATCH v2 2/2] console: don't select first registered console if stdout-path used

[Paul Burton]

Hi Eugeniy,

On Wednesday, 6 September 2017 10:57:18 PDT Eugeniy Paltsev wrote:
> > > We retain previous behavior for tty0 console (if "stdout-path" used)
> > > as a special case:
> > > tty0 will be registered even if it was specified neither
> > > in "bootargs" nor in "stdout-path".
> > > We had to retain this behavior because a lot of ARM boards (and some
> > > powerpc) rely on it.
> >
> > My main concern is the exception for "tty". Yes, it was regiression
> > reported in the commit c6c7d83b9c9e6a8b3e ("Revert "console: don't
> > prefer first registered if DT specifies stdout-path""). But is this
> > the only possible regression?
> >
> > All this is about the fallback code that tries to enable all
> > consoles until a real one with tty binding (newcon->device)
> > is enabled.
> >
> > v1 version of you patch disabled this fallback code when a console
> > was defined by stdout-path in the device tree. This emulates
> > defining the console by console= parameter on the command line.
> >
> > It might make sense until some complains that a console is not
> > longer automatically enabled while it was before. But wait.
> > Someone already complained about "tty0". We can solve this
> > by adding an exception for "tty0". And if anyone else complains
> > about another console, we might need more exceptions.
> >
> > We might endup with so many exceptions that the fallback code
> > will be always used. But then we are back in the square
> > and have the original behavior before your patch.
> 
> Yes, I understand your concerns.
> 
> But I also have another concern: If we decide to left current behavior
> untouched (like after reverting patch 05fd007e4629)
> more and more boards and devices will use current broken stdout-path
> behavior in any form and in the results we will get the situation when we
> can't fix stdout-path behavior at all - because every change will break
> something somewhere. 
> (05fd007e4629 patch do absolutely the same as v1 version of my patch)
> 
> > This is why I would like to know more info about your problem.
> > We need to decide if it is more important than a regression.
> > Or if it can be fixed another way.

After the troubles with commit 05fd007e4629 ("console: don't prefer first 
registered if DT specifies stdout-path") I took an alternate approach: rather 
than preventing the first console being registered, I instead prevent the 
bootconsole from being unregistered until we see the stdout-path console 
probed.

For my systems, where there are 3 consoles involved, this is how it goes:

 - The 8250 earlycon is our boot console.

 - tty0 (from CONFIG_VT) comes along & gets registered fairly early, and
   becomes the "proper" console. With current mainline this causes the boot
   console to be unregistered & lose output over the UART for a while.

 - Eventually ttyS0, the proper 8250 console, gets registered & we re-gain
   UART output. If the system died between the tty0 & ttyS0 registering, it
   dies silently.

With my current approach the difference is that the boot console sticks around 
until the last step where ttyS0 is registered (or until the late_initcall 
stage if ttyS0 never registers). This solves my problem - would it solve 
yours?

I haven't submitted this patch yet, but you can find it here in a v4.13 based 
downstream if you want to give it a try:

  https://git.linux-mips.org/cgit/linux-mti.git/commit/?
h=eng-201705051946&id=dd144b12c899b591c0370715328199bc958878fe

Thanks,
    Paul
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-snps-arc/attachments/20170906/5600b677/attachment.sig>

Re: [PATCH v2 2/2] console: don't select first registered console if stdout-path used

[Paul Burton]

[-- Attachment #1: Type: text/plain, Size: 3481 bytes --]

Hi Eugeniy,

On Wednesday, 6 September 2017 10:57:18 PDT Eugeniy Paltsev wrote:
> > > We retain previous behavior for tty0 console (if "stdout-path" used)
> > > as a special case:
> > > tty0 will be registered even if it was specified neither
> > > in "bootargs" nor in "stdout-path".
> > > We had to retain this behavior because a lot of ARM boards (and some
> > > powerpc) rely on it.
> >
> > My main concern is the exception for "tty". Yes, it was regiression
> > reported in the commit c6c7d83b9c9e6a8b3e ("Revert "console: don't
> > prefer first registered if DT specifies stdout-path""). But is this
> > the only possible regression?
> >
> > All this is about the fallback code that tries to enable all
> > consoles until a real one with tty binding (newcon->device)
> > is enabled.
> >
> > v1 version of you patch disabled this fallback code when a console
> > was defined by stdout-path in the device tree. This emulates
> > defining the console by console= parameter on the command line.
> >
> > It might make sense until some complains that a console is not
> > longer automatically enabled while it was before. But wait.
> > Someone already complained about "tty0". We can solve this
> > by adding an exception for "tty0". And if anyone else complains
> > about another console, we might need more exceptions.
> >
> > We might endup with so many exceptions that the fallback code
> > will be always used. But then we are back in the square
> > and have the original behavior before your patch.
> 
> Yes, I understand your concerns.
> 
> But I also have another concern: If we decide to left current behavior
> untouched (like after reverting patch 05fd007e4629)
> more and more boards and devices will use current broken stdout-path
> behavior in any form and in the results we will get the situation when we
> can't fix stdout-path behavior at all - because every change will break
> something somewhere. 
> (05fd007e4629 patch do absolutely the same as v1 version of my patch)
> 
> > This is why I would like to know more info about your problem.
> > We need to decide if it is more important than a regression.
> > Or if it can be fixed another way.

After the troubles with commit 05fd007e4629 ("console: don't prefer first 
registered if DT specifies stdout-path") I took an alternate approach: rather 
than preventing the first console being registered, I instead prevent the 
bootconsole from being unregistered until we see the stdout-path console 
probed.

For my systems, where there are 3 consoles involved, this is how it goes:

 - The 8250 earlycon is our boot console.

 - tty0 (from CONFIG_VT) comes along & gets registered fairly early, and
   becomes the "proper" console. With current mainline this causes the boot
   console to be unregistered & lose output over the UART for a while.

 - Eventually ttyS0, the proper 8250 console, gets registered & we re-gain
   UART output. If the system died between the tty0 & ttyS0 registering, it
   dies silently.

With my current approach the difference is that the boot console sticks around 
until the last step where ttyS0 is registered (or until the late_initcall 
stage if ttyS0 never registers). This solves my problem - would it solve 
yours?

I haven't submitted this patch yet, but you can find it here in a v4.13 based 
downstream if you want to give it a try:

  https://git.linux-mips.org/cgit/linux-mti.git/commit/?
h=eng-201705051946&id=dd144b12c899b591c0370715328199bc958878fe

Thanks,
    Paul

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/2] console: don't select first registered console if stdout-path used

[Petr Mladek]

Hi Eigeniy,

I am sorry for the late response. The problem is far from
trivial. I am getting snowed under many other tasks as well.


On Wed 2017-09-06 17:57:18, Eugeniy Paltsev wrote:
> Hi Petr,
> 
> On Tue, 2017-09-05@16:54 +0200, Petr Mladek wrote:
> > On Mon 2017-08-28 19:58:07, Eugeniy Paltsev wrote:
> > > In the current implementation we take the first console that
> > > registers if we didn't select one.
> > >?
> > > But if we specify console via "stdout-path" property in device tree
> > > we don't want first console that registers here to be selected.
> > > Otherwise we may choose wrong console - for example if some console
> > > is registered earlier than console is pointed in "stdout-path"
> > > property because console pointed in "stdout-path" property can be add as
> > > preferred quite late - when it's driver is probed.
> >?
> > register_console() is really twisted function. I would like to better
> > understand your problems before we add yet another twist there.
> >?
> > Could you please be more specific about your problems?
> > What was the output of "cat /proc/consoles" before and after the fix?
> > What exactly started and stopped working?
> 
> Ok, I faced with several problems when I tried to use stdout-path and this
> patch solves all of them.
> There is the description of some of the problems:
> 
> -----------------------------------------------------------------------------------
> Problem 1: choosing wrong serial console device
> 
> Context:
> Serial console device specified via "stdout-path" property in device tree,
> support for console on virtual terminal is disabled (CONFIG_VT_CONSOLE is
> not selected, CONFIG_VT is selected)
> 
> In this case wrong console device can be selected.
> 
> Example:
> Device tree:
> -------------->8--------
> chosen {
> ????bootargs = ""
> ????stdout-path = &serial_1;
> };
> 
> serial_0: uart-0 at ... {} /* FAIL: serial_0 is used as console (ttyS0) as it is
> ?????????????????????????* probed earlier */
> serial_1: uart-1 at ... {}
> -------------->8--------
>
> # cat /proc/consoles
> ttyS0????????????????-W- (EC???a)????4:64????/* FAIL: ttyS0 is used instead of?
> ??????????????????????????????????????????????* ttyS1 */

I guess that you know this. But let's be sure that we
understand the problem the same way.

The fact that ttyS0 was registered means that register_console()
was called for ttyS0 before __add_preferred_console() was called
for ttyS1 (defined as stdout-path).

__add_preferred_console() sets "preferred_console". This causes that
register_console() sets "has_preferred" and waits for the configured
console.

The preferred console from the device three seems to be add this way:

+ uart_add_one_port()
  + of_console_check()
    + add_preferred_console()
      + __add_preferred_console()


If I get this properly, uart_add_one_port() is called when
the serial port is probed. It calls add_preferred_console()
when the port really exists. IMHO, this is the root of
the problem. It is too late because register_console()
enables another console as a fallback in the meantime.

[Later realized that the commit 05fd007e46296afb24 ("console:
don't prefer first registered if DT specifies stdout-path")
basically confirmed this.].


BTW: Your solution with the check of "of_stdout" in
register_console() looks like a hack to me. A cleaner
solution would have been to call add_preferred_console()
earlier from the "of" code. For example, from
of_alias_scan() when "stdout-path" is analyzed and
"of_stdout" is set.

Note that similar solution is used for the console defined
via spcr. See add_preferred_console() in parse_spcr().


BTW2: Also note the following condition in of_console_check()

	if (!dn || dn != of_stdout || console_set_on_cmdline)
		return false;

It means that the console defined in the device three (stdout-path)
is ignored when there is console= defined on the command line.

Your patch did not break this logic but might have made wrong
assumptions. In this case, has_preferred should not be set
because of of_stdout.

This is another reason why a solution on the "of" code side
might have been cleaner.


> This FAIL happens because we take the first registered console if we didn't select
> a console via "console=" option in bootargs.
> 
> After my patch-v2:
> # cat /proc/consoles
> ttyS1????????????????-W- (EC p a)????4:67

> 
> -----------------------------------------------------------------------------------
> Problem 2: printing early boot messages twice and pause in boot messages printing
> 
> Context:
> We use early console. Serial console device (and early console device) specified
> via "stdout-path" property in device tree.?
> Support for console on virtual terminal is enabled (CONFIG_VT_CONSOLE=y)
> 
> In this case early boot messages will be printed twice - firstly by
> bootconsole and after that by 'real' serial console.
> Also we will get pause in boot messages printing - as bootconsole will be disabled
> mush earlier than 'real' serial console is enabled.
> 
> Example:
> -------------->8--------
> chosen {
> ????bootargs = "earlycon"
> ????stdout-path = &serial_3;
> };
> 
> serial_3: uart-3 at ... {}?
> -------------->8--------
> 
> So output of serial console will be be like that:
> -------------->8--------
> XXX - early boot messages, printed by bootconsole
> ????- FAIL: pause in boot messages printing
> XXX - FAIL: again early boot messages, printed by serial console
> YYY - rest of boot messages, printed by serial console
> -------------->8--------
> 
> So the order of enabling/disabling consoles will be like that:
> -------------->8--------
> bootconsole [uart0] enabled
> console [tty0] enabled??????????????/* As no console is select 'tty0' was taken */

There is a special hack in param_setup_earlycon(). It allows to
mention just "earlycon" in the device tree. The particular
early console is must be compatible with the one defined
as stdout-path.

Well, I am not sure why ttyS0 is used instead of ttyS3 as
the earlycon. One possibility is that ttyS0 passes
fdt_node_check_compatible() in early_init_dt_scan_chosen_stdout().
Another possibility is that CONFIG_ACPI_SPCR_TABLE was enabled,
earlycon_init_is_deferred was set, and the deferred handling
caused fallback to ttyS0.


> bootconsole [uart0] disabled????????/* As we have real (tty0) console we disable
> ?????????????????????????????????????* all bootconsoles */


This is one big and old problem of console registration code.
console->match() function has side-effects. Therefore we could
not easily match early consoles with the real consoles. Therefore
we do not know when it is the right time to disable the boot
console. The current code disables all boot consoles when
the real, so-called preferred, console is registered.

The preferred console is the last one on the command line.
In this case, it is ttyS0 because it thinks that there is
no preferred console.

A proper solution is to rework the console matching mechanism
and allow to match early and real consoles. It is a lot of work.


> console [ttyS3] enabled?????????????/* We take ttyS3 but don't reset its?
> ?????????????????????????????????????* CON_PRINTBUFFER flag (as there is NO enabled
> 				?????* bootconsoles) */

The "sad" thing is that the race with early console helped
to register the configured ttyS3 instead of ttyS0.

> -------------->8--------
> 
> 
> # cat /proc/consoles
> ttyS3????????????????-W- (EC p a)????4:67
> tty0?????????????????-WU (E?????)????4:1
> 
> As you can see CON_PRINTBUFFER flag (p) set for ttyS3 - that is wrong.

Well, is this really wrong? The early console was ttyS0 and the final
one ttyS3. These should be different devices. Therefore the messages
should not be duplicated.


> After my patch-v2:
> # cat /proc/consoles
> ttyS3????????????????-W- (EC???a)????4:67
> tty0?????????????????-WU (E??p??)????4:1

I think that this is actually worse. You will miss many messages
on the ttyS3 console.
 
> 
> These are the problems I have faced but these are NOT THE ONLY POSSIBLE problems
> because current behavior is quite unstable and unpredictable.

Yes, I know and we should do something about it. The problem is that
probably no-one really understand the code and historic aspects.
People just added hacks when they needed something. These were
rejected when they caused regressions.

I am trying to get the picture and eventually put the code into
a shape. But it seems to be a long term task.


> And of course I would prefer to use simple solution from v1 patch version
> but in this case we will face with someone complaining about "tty0".
> 
> So all comments and suggestions are more than welcome.
?
> > > We retain previous behavior for tty0 console (if "stdout-path" used)
> > > as a special case:
> > > tty0 will be registered even if it was specified neither
> > > in "bootargs" nor in "stdout-path".
> > > We had to retain this behavior because a lot of ARM boards (and some
> > > powerpc) rely on it.
> >?
> > My main concern is the exception for "tty". Yes, it was regiression
> > reported in the commit c6c7d83b9c9e6a8b3e ("Revert "console: don't
> > prefer first registered if DT specifies stdout-path""). But is this
> > the only possible regression?
> >?
> >?
> > All this is about the fallback code that tries to enable all
> > consoles until a real one with tty binding (newcon->device)
> > is enabled.
> >?
> > v1 version of you patch disabled this fallback code when a console
> > was defined by stdout-path in the device tree. This emulates
> > defining the console by console= parameter on the command line.
> >?
> > It might make sense until some complains that a console is not
> > longer automatically enabled while it was before. But wait.
> > Someone already complained about "tty0". We can solve this
> > by adding an exception for "tty0". And if anyone else complains
> > about another console, we might need more exceptions.
> >
> > We might endup with so many exceptions that the fallback code
> > will be always used. But then we are back in the square
> > and have the original behavior before your patch.
> >?
> 
> Yes, I understand your concerns.
> 
> But I also have another concern: If we decide to left current behavior untouched
> (like after reverting patch 05fd007e4629)
> more and more boards and devices will use current broken stdout-path behavior in?
> any form and in the results we will get the situation when we can't fix
> stdout-path behavior at all - because every change will break something somewhere.

I see the point. I am only afraid that it is already too late, see below.


> (05fd007e4629 patch do absolutely the same as v1 version of my patch)

It is clear that we have already been there and people complained.
The question is if there is another way out.

IMHO, the most important thing is to allow matching the various
aliases, early, and real consoles without side-effects.
This should allow to:

    + Disable boot console exactly when the related real console
      is registered. It would remove the problems with duplicated
      or missing messages.

    + Do more conservative changes in the fallback console
      registration. For example, allow to register ttyX as
      a fallback but wait for the right ttySY that is defined
      in stdout-path.

    + Make the console registration more predictable and reliable
      in general.


I am sorry that I do not have a better answer for you at the moment.
But I really do not like your patches. They are hacks (adding
exceptions) into already hacky code. The first version was
the same as an already reverted commit. The second version tried
to work around the regression but it seemed to change the behavior
as well.

A workaround should be to define the console on the command like.
This would disable the fallback registration.

Best Regards,
Petr

Re: [PATCH v2 2/2] console: don't select first registered console if stdout-path used

[Petr Mladek]

Hi Eigeniy,

I am sorry for the late response. The problem is far from
trivial. I am getting snowed under many other tasks as well.


On Wed 2017-09-06 17:57:18, Eugeniy Paltsev wrote:
> Hi Petr,
> 
> On Tue, 2017-09-05 at 16:54 +0200, Petr Mladek wrote:
> > On Mon 2017-08-28 19:58:07, Eugeniy Paltsev wrote:
> > > In the current implementation we take the first console that
> > > registers if we didn't select one.
> > > 
> > > But if we specify console via "stdout-path" property in device tree
> > > we don't want first console that registers here to be selected.
> > > Otherwise we may choose wrong console - for example if some console
> > > is registered earlier than console is pointed in "stdout-path"
> > > property because console pointed in "stdout-path" property can be add as
> > > preferred quite late - when it's driver is probed.
> > 
> > register_console() is really twisted function. I would like to better
> > understand your problems before we add yet another twist there.
> > 
> > Could you please be more specific about your problems?
> > What was the output of "cat /proc/consoles" before and after the fix?
> > What exactly started and stopped working?
> 
> Ok, I faced with several problems when I tried to use stdout-path and this
> patch solves all of them.
> There is the description of some of the problems:
> 
> -----------------------------------------------------------------------------------
> Problem 1: choosing wrong serial console device
> 
> Context:
> Serial console device specified via "stdout-path" property in device tree,
> support for console on virtual terminal is disabled (CONFIG_VT_CONSOLE is
> not selected, CONFIG_VT is selected)
> 
> In this case wrong console device can be selected.
> 
> Example:
> Device tree:
> -------------->8--------
> chosen {
>     bootargs = ""
>     stdout-path = &serial_1;
> };
> 
> serial_0: uart-0@... {} /* FAIL: serial_0 is used as console (ttyS0) as it is
>                          * probed earlier */
> serial_1: uart-1@... {}
> -------------->8--------
>
> # cat /proc/consoles
> ttyS0                -W- (EC   a)    4:64    /* FAIL: ttyS0 is used instead of 
>                                               * ttyS1 */

I guess that you know this. But let's be sure that we
understand the problem the same way.

The fact that ttyS0 was registered means that register_console()
was called for ttyS0 before __add_preferred_console() was called
for ttyS1 (defined as stdout-path).

__add_preferred_console() sets "preferred_console". This causes that
register_console() sets "has_preferred" and waits for the configured
console.

The preferred console from the device three seems to be add this way:

+ uart_add_one_port()
  + of_console_check()
    + add_preferred_console()
      + __add_preferred_console()


If I get this properly, uart_add_one_port() is called when
the serial port is probed. It calls add_preferred_console()
when the port really exists. IMHO, this is the root of
the problem. It is too late because register_console()
enables another console as a fallback in the meantime.

[Later realized that the commit 05fd007e46296afb24 ("console:
don't prefer first registered if DT specifies stdout-path")
basically confirmed this.].


BTW: Your solution with the check of "of_stdout" in
register_console() looks like a hack to me. A cleaner
solution would have been to call add_preferred_console()
earlier from the "of" code. For example, from
of_alias_scan() when "stdout-path" is analyzed and
"of_stdout" is set.

Note that similar solution is used for the console defined
via spcr. See add_preferred_console() in parse_spcr().


BTW2: Also note the following condition in of_console_check()

	if (!dn || dn != of_stdout || console_set_on_cmdline)
		return false;

It means that the console defined in the device three (stdout-path)
is ignored when there is console= defined on the command line.

Your patch did not break this logic but might have made wrong
assumptions. In this case, has_preferred should not be set
because of of_stdout.

This is another reason why a solution on the "of" code side
might have been cleaner.


> This FAIL happens because we take the first registered console if we didn't select
> a console via "console=" option in bootargs.
> 
> After my patch-v2:
> # cat /proc/consoles
> ttyS1                -W- (EC p a)    4:67

> 
> -----------------------------------------------------------------------------------
> Problem 2: printing early boot messages twice and pause in boot messages printing
> 
> Context:
> We use early console. Serial console device (and early console device) specified
> via "stdout-path" property in device tree. 
> Support for console on virtual terminal is enabled (CONFIG_VT_CONSOLE=y)
> 
> In this case early boot messages will be printed twice - firstly by
> bootconsole and after that by 'real' serial console.
> Also we will get pause in boot messages printing - as bootconsole will be disabled
> mush earlier than 'real' serial console is enabled.
> 
> Example:
> -------------->8--------
> chosen {
>     bootargs = "earlycon"
>     stdout-path = &serial_3;
> };
> 
> serial_3: uart-3@... {} 
> -------------->8--------
> 
> So output of serial console will be be like that:
> -------------->8--------
> XXX - early boot messages, printed by bootconsole
>     - FAIL: pause in boot messages printing
> XXX - FAIL: again early boot messages, printed by serial console
> YYY - rest of boot messages, printed by serial console
> -------------->8--------
> 
> So the order of enabling/disabling consoles will be like that:
> -------------->8--------
> bootconsole [uart0] enabled
> console [tty0] enabled              /* As no console is select 'tty0' was taken */

There is a special hack in param_setup_earlycon(). It allows to
mention just "earlycon" in the device tree. The particular
early console is must be compatible with the one defined
as stdout-path.

Well, I am not sure why ttyS0 is used instead of ttyS3 as
the earlycon. One possibility is that ttyS0 passes
fdt_node_check_compatible() in early_init_dt_scan_chosen_stdout().
Another possibility is that CONFIG_ACPI_SPCR_TABLE was enabled,
earlycon_init_is_deferred was set, and the deferred handling
caused fallback to ttyS0.


> bootconsole [uart0] disabled        /* As we have real (tty0) console we disable
>                                      * all bootconsoles */


This is one big and old problem of console registration code.
console->match() function has side-effects. Therefore we could
not easily match early consoles with the real consoles. Therefore
we do not know when it is the right time to disable the boot
console. The current code disables all boot consoles when
the real, so-called preferred, console is registered.

The preferred console is the last one on the command line.
In this case, it is ttyS0 because it thinks that there is
no preferred console.

A proper solution is to rework the console matching mechanism
and allow to match early and real consoles. It is a lot of work.


> console [ttyS3] enabled             /* We take ttyS3 but don't reset its 
>                                      * CON_PRINTBUFFER flag (as there is NO enabled
> 				     * bootconsoles) */

The "sad" thing is that the race with early console helped
to register the configured ttyS3 instead of ttyS0.

> -------------->8--------
> 
> 
> # cat /proc/consoles
> ttyS3                -W- (EC p a)    4:67
> tty0                 -WU (E     )    4:1
> 
> As you can see CON_PRINTBUFFER flag (p) set for ttyS3 - that is wrong.

Well, is this really wrong? The early console was ttyS0 and the final
one ttyS3. These should be different devices. Therefore the messages
should not be duplicated.


> After my patch-v2:
> # cat /proc/consoles
> ttyS3                -W- (EC   a)    4:67
> tty0                 -WU (E  p  )    4:1

I think that this is actually worse. You will miss many messages
on the ttyS3 console.
 
> 
> These are the problems I have faced but these are NOT THE ONLY POSSIBLE problems
> because current behavior is quite unstable and unpredictable.

Yes, I know and we should do something about it. The problem is that
probably no-one really understand the code and historic aspects.
People just added hacks when they needed something. These were
rejected when they caused regressions.

I am trying to get the picture and eventually put the code into
a shape. But it seems to be a long term task.


> And of course I would prefer to use simple solution from v1 patch version
> but in this case we will face with someone complaining about "tty0".
> 
> So all comments and suggestions are more than welcome.
 
> > > We retain previous behavior for tty0 console (if "stdout-path" used)
> > > as a special case:
> > > tty0 will be registered even if it was specified neither
> > > in "bootargs" nor in "stdout-path".
> > > We had to retain this behavior because a lot of ARM boards (and some
> > > powerpc) rely on it.
> > 
> > My main concern is the exception for "tty". Yes, it was regiression
> > reported in the commit c6c7d83b9c9e6a8b3e ("Revert "console: don't
> > prefer first registered if DT specifies stdout-path""). But is this
> > the only possible regression?
> > 
> > 
> > All this is about the fallback code that tries to enable all
> > consoles until a real one with tty binding (newcon->device)
> > is enabled.
> > 
> > v1 version of you patch disabled this fallback code when a console
> > was defined by stdout-path in the device tree. This emulates
> > defining the console by console= parameter on the command line.
> > 
> > It might make sense until some complains that a console is not
> > longer automatically enabled while it was before. But wait.
> > Someone already complained about "tty0". We can solve this
> > by adding an exception for "tty0". And if anyone else complains
> > about another console, we might need more exceptions.
> >
> > We might endup with so many exceptions that the fallback code
> > will be always used. But then we are back in the square
> > and have the original behavior before your patch.
> > 
> 
> Yes, I understand your concerns.
> 
> But I also have another concern: If we decide to left current behavior untouched
> (like after reverting patch 05fd007e4629)
> more and more boards and devices will use current broken stdout-path behavior in 
> any form and in the results we will get the situation when we can't fix
> stdout-path behavior at all - because every change will break something somewhere.

I see the point. I am only afraid that it is already too late, see below.


> (05fd007e4629 patch do absolutely the same as v1 version of my patch)

It is clear that we have already been there and people complained.
The question is if there is another way out.

IMHO, the most important thing is to allow matching the various
aliases, early, and real consoles without side-effects.
This should allow to:

    + Disable boot console exactly when the related real console
      is registered. It would remove the problems with duplicated
      or missing messages.

    + Do more conservative changes in the fallback console
      registration. For example, allow to register ttyX as
      a fallback but wait for the right ttySY that is defined
      in stdout-path.

    + Make the console registration more predictable and reliable
      in general.


I am sorry that I do not have a better answer for you at the moment.
But I really do not like your patches. They are hacks (adding
exceptions) into already hacky code. The first version was
the same as an already reverted commit. The second version tried
to work around the regression but it seemed to change the behavior
as well.

A workaround should be to define the console on the command like.
This would disable the fallback registration.

Best Regards,
Petr