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 = _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 

Re: [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=dd144b12c899b591c0370715328199bc958878fe

Thanks,
Paul

signature.asc
Description: This is a digitally signed message part.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

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 = _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 = _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?

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

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

[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 
---
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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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


___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc