| Message ID | 20170828165807.8408-3-Eugeniy.Paltsev@synopsys.com |
|---|---|
| State | New |
| Headers | show |
| Series | console: don't select first registered console if stdout-path used | expand |
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
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
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
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
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
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(-)