* [RFC/PATCH] printk: Fix preferred console selection with multiple matches @ 2019-12-10 0:57 Benjamin Herrenschmidt 2019-12-10 8:01 ` Sergey Senozhatsky 2019-12-10 9:15 ` Petr Mladek 0 siblings, 2 replies; 15+ messages in thread From: Benjamin Herrenschmidt @ 2019-12-10 0:57 UTC (permalink / raw) To: linux-kernel Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, AlekseyMakarov In the following circumstances, the rule of selecting the console corresponding to the last "console=" entry on the command line as the preferred console (CON_CONSDEV, ie, /dev/console) fails. This is a specific example, but it could happen with different consoles that have a similar name aliasing mechanism. - The kernel command line has both console=tty0 and console=ttyS0 in that order (the latter with speed etc... arguments). This is common with some cloud setups such as Amazon Linux. - add_preferred_console is called early to register "uart0". In our case that happens from acpi_parse_spcr() on arm64 since the "enable_console" argument is true on that architecture. This causes "uart0" to become entry 0 of the console_cmdline array. Now, because of the above, what happens is: - add_preferred_console is called by the cmdline parsing for tty0 and ttyS0 respectively, thus occupying entries 1 and 2 of the console_cmdline array (since this happens after ACPI SPCR parsing). At that point preferred_console is set to 2 as expected. - When the tty layer kicks in, it will call register_console for tty0. This will match entry 1 in console_cmdline array. It isn't our preferred console but because it's our only console at this point, it will end up "first" in the consoles list. - When 8250 probes the actual serial port later on, it calls register_console for ttyS0. At that point the loop in register_console tries to match it with the entries in the console_cmdline array. Ideally this should match ttyS0 in entry 2, which is preferred, causing it to be inserted first and to replace tty0 as CONSDEV. However, 8250 provides a "match" hook in its struct console, and that hook will match "uart" as an alias to "ttyS". So we match uart0 at entry 0 in the array which is not the preferred console and will not match entry 2 which is since we break out of the loop on the first match. As a result, we don't set CONSDEV and don't insert it first, but second in the console list. As a result, we end up with tty0 remaining first in the array, and thus /dev/console going there instead of the last user specified one which is ttyS0. This tentative fix changes the loop in register_console to continue matching with the array until the match is actually the preferred console. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- kernel/printk/printk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 5aa96098c64d..d36b9901c0e0 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -2646,8 +2646,8 @@ void register_console(struct console *newcon) if (i == preferred_console) { newcon->flags |= CON_CONSDEV; has_preferred = true; + break; } - break; } if (!(newcon->flags & CON_ENABLED)) -- 2.17.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-10 0:57 [RFC/PATCH] printk: Fix preferred console selection with multiple matches Benjamin Herrenschmidt @ 2019-12-10 8:01 ` Sergey Senozhatsky 2019-12-10 22:26 ` Benjamin Herrenschmidt 2019-12-10 9:15 ` Petr Mladek 1 sibling, 1 reply; 15+ messages in thread From: Sergey Senozhatsky @ 2019-12-10 8:01 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: linux-kernel, Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, AlekseyMakarov On (19/12/10 11:57), Benjamin Herrenschmidt wrote: [..] > - add_preferred_console is called early to register "uart0". In > our case that happens from acpi_parse_spcr() on arm64 since the > "enable_console" argument is true on that architecture. This causes > "uart0" to become entry 0 of the console_cmdline array. Hmm, two independent console list configuration sources. [..] > +++ b/kernel/printk/printk.c > @@ -2646,8 +2646,8 @@ void register_console(struct console *newcon) > if (i == preferred_console) { > newcon->flags |= CON_CONSDEV; > has_preferred = true; > + break; > } > - break; > } > > if (!(newcon->flags & CON_ENABLED)) Wouldn't this, basically, mean that we want to match only consoles, which were in the kernel's console= cmdline? IOW, ignore consoles that were placed into consoles list via alternative path - ACPI. Hmm. The patch may affect setups where alias matching is expected to happen. E.g.: console=uartFOO,BAR Is 8250 the only console that does alias matching? -ss ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-10 8:01 ` Sergey Senozhatsky @ 2019-12-10 22:26 ` Benjamin Herrenschmidt 2019-12-11 2:01 ` Sergey Senozhatsky 0 siblings, 1 reply; 15+ messages in thread From: Benjamin Herrenschmidt @ 2019-12-10 22:26 UTC (permalink / raw) To: Sergey Senozhatsky Cc: linux-kernel, Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, AlekseyMakarov On Tue, 2019-12-10 at 17:01 +0900, Sergey Senozhatsky wrote: > On (19/12/10 11:57), Benjamin Herrenschmidt wrote: > [..] > > - add_preferred_console is called early to register "uart0". In > > our case that happens from acpi_parse_spcr() on arm64 since the > > "enable_console" argument is true on that architecture. This causes > > "uart0" to become entry 0 of the console_cmdline array. > > Hmm, two independent console list configuration sources. Yes, we've had that for a while. So far it "worked" because the explicit calls to add_preferred_console() tends to happen before the parsing of the command line, and thus are "overriden" by the latter if any. > [..] > > +++ b/kernel/printk/printk.c > > @@ -2646,8 +2646,8 @@ void register_console(struct console *newcon) > > if (i == preferred_console) { > > newcon->flags |= CON_CONSDEV; > > has_preferred = true; > > + break; > > } > > - break; > > } > > > > if (!(newcon->flags & CON_ENABLED)) > > Wouldn't this, basically, mean that we want to match only consoles, > which were in the kernel's console= cmdline? IOW, ignore consoles > that were placed into consoles list via alternative path - ACPI. No not exactly. Architectures/platforms use add_preferred_console() (such as arm64 with ACPI but powerpc at least does it too) based on various factors to select a reasonable "default" for that specific platform. Without that the kernel will basically default to the first one to register which may not be what you want. The command line ones however want to override the defaults (provided they exist, ie, it's possible that whever is specified on the command line doesn't actually exist, and thus shall be ignored. That typically happens when there is either no match or ->setup fails). > Hmm. > > The patch may affect setups where alias matching is expected to > happen. E.g.: > > console=uartFOO,BAR > > Is 8250 the only console that does alias matching? Why would the patch affect this negatively ? Today we stop on the first match, mark the driver enabled, and make it preferred if the match index matches preferred_console. My patch makes us continue searching if it wasnt' the preferred console (but we still mark it enabled). Which means either of those two things happen: - No more match or another match that isn't the preferred_console, this won't change the existing behaviour. - Another match that is marked preferred_console, in which case in addition to being enabled, the newly registered console will also be made the default console (ie, first in the list with CONSDEV set). This is actually what we want ! IE. The console matches the last specified one on the command line. Cheers, Ben. > -ss ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-10 22:26 ` Benjamin Herrenschmidt @ 2019-12-11 2:01 ` Sergey Senozhatsky 2019-12-11 4:02 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 15+ messages in thread From: Sergey Senozhatsky @ 2019-12-11 2:01 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Sergey Senozhatsky, linux-kernel, Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds On (19/12/11 09:26), Benjamin Herrenschmidt wrote: [..] > No not exactly. Architectures/platforms use add_preferred_console() > (such as arm64 with ACPI but powerpc at least does it too) based on > various factors to select a reasonable "default" for that specific > platform. Without that the kernel will basically default to the first > one to register which may not be what you want. > > The command line ones however want to override the defaults (provided > they exist, ie, it's possible that whever is specified on the command > line doesn't actually exist, and thus shall be ignored. That typically > happens when there is either no match or ->setup fails). > > > Hmm. > > > > The patch may affect setups where alias matching is expected to > > happen. E.g.: > > > > console=uartFOO,BAR > > > > Is 8250 the only console that does alias matching? > > Why would the patch affect this negatively ? Today we stop on the first > match, mark the driver enabled, and make it preferred if the match > index matches preferred_console. As far as I know, ->match() does not only match but also does ->setup(). If we have two console list entries that match (one via aliasing and one via exact match) then the console driver is setup twice. Do all console drivers handle it? [double setup] If we could perform simple alias matching, without ->setup() call, and exact matching (strcmp()), and then, if newcon would match two entries, we would pick up the last matching entry and configure newcon only once. This changes the order, tho. [..] > - Another match that is marked preferred_console, in which case in > addition to being enabled, the newly registered console will also be > made the default console (ie, first in the list with CONSDEV set). This > is actually what we want ! IE. The console matches the last specified > one on the command line. Well, it still looks to me that what you want is to "ignore alias match and prefer exact match". -ss ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-11 2:01 ` Sergey Senozhatsky @ 2019-12-11 4:02 ` Benjamin Herrenschmidt 2019-12-11 5:35 ` Sergey Senozhatsky 2019-12-11 12:53 ` Petr Mladek 0 siblings, 2 replies; 15+ messages in thread From: Benjamin Herrenschmidt @ 2019-12-11 4:02 UTC (permalink / raw) To: Sergey Senozhatsky Cc: linux-kernel, Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds On Wed, 2019-12-11 at 11:01 +0900, Sergey Senozhatsky wrote: > On (19/12/11 09:26), Benjamin Herrenschmidt wrote: > > As far as I know, ->match() does not only match but also does ->setup(). > If we have two console list entries that match (one via aliasing and one > via exact match) then the console driver is setup twice. Do all console > drivers handle it? [double setup] I don't think it's an issue but I may be wrong. I had a quick look at some of the drivers and I don't really see why they would break but I couldn't look at them all and I might be mistaken. We could skip setup if the console is already enabled but I would advise against that since the two calls might have different options (the firmware baud rate could be different from the command line one for example) and we want the options for the last one. > If we could perform simple alias matching, without ->setup() call, and > exact matching (strcmp()), and then, if newcon would match two entries, > we would pick up the last matching entry and configure newcon only once. > > This changes the order, tho. Walking the array backwards might just be what we want actually for the case at hand, but of course if some platforms or driver call add_preferred_console *after* the command line parsing, then it will break those. Simple alias matching would require re-working all the match() callbacks. That said I think it was a mistake to begin with to have them include setup(). Those should have remained separate. What about a compromise here: Instead of walking the array and testing for preferred_console as we do so, we first test the array entry pointed to by preferred_console (doing both match & setup as today) and if that doesn't work, fallback to our existing mechanism ? It would be a first step. It wouldn't fix all cases but would be something reasonable to backport. Another approach woudl be to pass to add_preferred_console an argument "bool user_specified" which we would use to set a CON_USER flag. We could then do a two-pass lookup of the array where we first only match user specified entries, then match the rest. > [..] > > - Another match that is marked preferred_console, in which case in > > addition to being enabled, the newly registered console will also be > > made the default console (ie, first in the list with CONSDEV set). This > > is actually what we want ! IE. The console matches the last specified > > one on the command line. > > Well, it still looks to me that what you want is to "ignore alias > match and prefer exact match". We don't want to ignore the alias match. But we do want to prefer the exact match. We still want to keep the fallback to the alias match. Cheers, Ben. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-11 4:02 ` Benjamin Herrenschmidt @ 2019-12-11 5:35 ` Sergey Senozhatsky 2019-12-11 12:53 ` Petr Mladek 1 sibling, 0 replies; 15+ messages in thread From: Sergey Senozhatsky @ 2019-12-11 5:35 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Sergey Senozhatsky, linux-kernel, Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds On (19/12/11 15:02), Benjamin Herrenschmidt wrote: > On Wed, 2019-12-11 at 11:01 +0900, Sergey Senozhatsky wrote: > > On (19/12/11 09:26), Benjamin Herrenschmidt wrote: > > [..] > > If we could perform simple alias matching, without ->setup() call, and > > exact matching (strcmp()), and then, if newcon would match two entries, > > we would pick up the last matching entry and configure newcon only once. > > > > This changes the order, tho. > > Walking the array backwards might just be what we want actually for the > case at hand, but of course if some platforms or driver call > add_preferred_console *after* the command line parsing, then it will > break those. Reverse loop sounds like a nice idea. But yes I guess this can break things. > Simple alias matching would require re-working all the match() > callbacks. That said I think it was a mistake to begin with to have > them include setup(). Those should have remained separate. Agreed. strcmp(alias) and strcmp(exact name) are the same things. The latter does "match" and setup() as separate steps, but the former does both at once. > What about a compromise here: > > Instead of walking the array and testing for preferred_console as we do > so, we first test the array entry pointed to by preferred_console > (doing both match & setup as today) and if that doesn't work, fallback > to our existing mechanism ? This may do the trick. And perform preferred_console fast-path configuration only if `has_preferred' is false. > > Well, it still looks to me that what you want is to "ignore alias > > match and prefer exact match". > > We don't want to ignore the alias match. But we do want to prefer the > exact match. We still want to keep the fallback to the alias match. Yeah, "prefer" is the key word here. -ss ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-11 4:02 ` Benjamin Herrenschmidt 2019-12-11 5:35 ` Sergey Senozhatsky @ 2019-12-11 12:53 ` Petr Mladek 1 sibling, 0 replies; 15+ messages in thread From: Petr Mladek @ 2019-12-11 12:53 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Sergey Senozhatsky, linux-kernel, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds On Wed 2019-12-11 15:02:02, Benjamin Herrenschmidt wrote: > On Wed, 2019-12-11 at 11:01 +0900, Sergey Senozhatsky wrote: > > On (19/12/11 09:26), Benjamin Herrenschmidt wrote: > > > > > As far as I know, ->match() does not only match but also does ->setup(). > > If we have two console list entries that match (one via aliasing and one > > via exact match) then the console driver is setup twice. Do all console > > drivers handle it? [double setup] > > I don't think it's an issue but I may be wrong. I had a quick look > at some of the drivers and I don't really see why they would break but > I couldn't look at them all and I might be mistaken. > > We could skip setup if the console is already enabled but I would > advise against that since the two calls might have different options > (the firmware baud rate could be different from the command line one > for example) and we want the options for the last one. For example: + ip22zilog_console_setup() calls __ip22zilog_startup() that does some initialization of the device + pl011_console_setup() does some non-trivial things as well. Honestly, I am not familiar with these devices. I am not sure if it is dangerous or safe to call these functions twice. I am not sure if anybody would know this for sure. Therefore I suggest a conservative approach and avoid calling setup() twice(). I think that it is dangerous and error-prone design. The best solution would be to split setup() and match() functionality. Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-10 0:57 [RFC/PATCH] printk: Fix preferred console selection with multiple matches Benjamin Herrenschmidt 2019-12-10 8:01 ` Sergey Senozhatsky @ 2019-12-10 9:15 ` Petr Mladek 2019-12-10 22:39 ` Benjamin Herrenschmidt 2019-12-12 0:35 ` Benjamin Herrenschmidt 1 sibling, 2 replies; 15+ messages in thread From: Petr Mladek @ 2019-12-10 9:15 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: linux-kernel, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, AlekseyMakarov On Tue 2019-12-10 11:57:26, Benjamin Herrenschmidt wrote: > In the following circumstances, the rule of selecting the console > corresponding to the last "console=" entry on the command line as > the preferred console (CON_CONSDEV, ie, /dev/console) fails. This > is a specific example, but it could happen with different consoles > that have a similar name aliasing mechanism. > > This tentative fix changes the loop in register_console to continue > matching with the array until the match is actually the preferred > console. One problem with this is that con->match() callbacks might have side effects. If the console matches, the callback sometimes do some changes in the console driver because it expects that the console is going to be registered and used. I have solved the same problem some time ago and we use the following patch in SUSE kernels. Sigh, I have never send it upstream because it looked too complicated to me. I wanted to clean up the console registration code a bit first, see https://lkml.kernel.org/r/1497358444-30736-1-git-send-email-pmladek@suse.com But it was pretty complicated and it has somehow fallen into cracks. Anyway, here is the patch that we use. Could you please check if it works for you as well? Does it make sense, please? From: Petr Mladek <pmladek@suse.com> Date: Tue, 20 Jun 2017 14:40:34 +0200 Subject: printk/console: Correctly mark console that is used when opening /dev/console Patch-mainline: Never, an extensive clean up is being prepared for upstream References: bsc#1040020 showconsole tool shows the real name of tty device associated with /dev/console. It expects that the related console driver has set CON_CONSDEV flag. On the other hand, kernel ignores CON_CONSDEV flag when it looks for the right driver. Instead, it takes the first driver that has the tty binding (console->device). See tty_lookup_driver() and console_device(). All this works most of the time because kernel puts the driver with CON_CONSDEV flag first into the list. There is almost always registered a real (non-boot) console with this flag set. The real consoles mostly (always?) have tty binding. Boot consoles that might miss the tty binding are always removed unless keep_bootcon command line parameter is used. The problem is when some consoles are defined on the command line and the preferred one (last one) is not registered from some reason. Note that the consoles might be added to the command line also using ACPI SPCR or device tree. It might happen that, for example, SPCR code and user add the same console using two aliases. Then the first alias matches and we might miss that it matched also with the preferred console. There was one attempt to fix this by searching the command line from the end and match the preferred alias first. But it caused regressions. For example, ttyS* are taken as aliases as well and kernel messages can appear only on one serial port. The reversed matching caused that the logs suddenly appeared on another serial port. The right solution is to set CON_CONSDEV flag for the driver used by tty_lookup_driver() even when the preferred console is not registered. It is a bit complicated because register_console() code is tricky. It expects that only the preferred driver will have CON_CONSDEV flag set. Also it expects that a boot console will stay first in the list until the preferred console is registered. These information are used to make various decisions: + Use a fallback code when none console is configured on the command line. This code tries to enable any console until a real one is enabled. + Unregister all boot consoles when the real preferred one is registered. And do not reply the log on the real console to avoid duplicates. A rather invasive clean up is being prepared for upstream. This patch tries to be as minimalist and do not change the order of consoles as possible. It keeps the logic about having a boot console first until the real preferred console is registered. But it makes sure that the first console with tty binding (console->device) will have CON_CONSDEV flag set. Let's look at this in more details. The fallback code in console_register() already works as expected. It sets CON_CONSDEV flag for any console with tty binding. The code matching all consoles from the command line newly sets CON_CONSDEV flag also for the fist console with the tty binding. But it sets "consdev_fallback" to avoid putting this console first into the list. Remember that we want to keep the boot console first until the preferred is registered. The information about the fallback is used also to avoid doing other actions that need to wait for the preferred console. The code adding the console into the list of drivers must put non-preferred drivers with tty binding next to the console with CON_CONSDEV set. This is the only change that might change the order of console drivers in the list and eventually cause regressions. But it has an effect only when there are at least three drivers mentioned on the command line, a boot console is registered and the preferred driver is not registered. This should be a corner case. Finally, unregister_console() sets CON_CONSDEV to first console with tty binding instead of the first one in the list. Signed-off-by: Petr Mladek <pmladek@suse.com> --- kernel/printk/printk.c | 59 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 50 insertions(+), 9 deletions(-) diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 94ec1aacea64..b6bb4d362b22 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -2662,16 +2662,23 @@ void register_console(struct console *newcon) int i; unsigned long flags; struct console *bcon = NULL; + struct console *con_consdev = NULL; struct console_cmdline *c; static bool has_preferred; + bool consdev_fallback = false; - if (console_drivers) - for_each_console(bcon) + if (console_drivers) { + for_each_console(bcon) { if (WARN(bcon == newcon, "console '%s%d' already registered\n", bcon->name, bcon->index)) return; + if (bcon->flags & CON_CONSDEV && !con_consdev) + con_consdev = bcon; + } + } + /* * before we register a new CON_BOOT console, make sure we don't * already have a valid console @@ -2739,8 +2746,17 @@ void register_console(struct console *newcon) newcon->flags |= CON_ENABLED; if (i == preferred_console) { + /* This is the last console on the command line. */ newcon->flags |= CON_CONSDEV; has_preferred = true; + } else if (newcon->device && !con_consdev) { + /* + * This is the first console with tty binding. It will + * be used for /dev/console when the preferred one + * will not get registered for some reason. + */ + newcon->flags |= CON_CONSDEV; + consdev_fallback = true; } break; } @@ -2754,7 +2770,9 @@ void register_console(struct console *newcon) * the real console are the same physical device, it's annoying to * see the beginning boot messages twice */ - if (bcon && ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV)) + if (bcon && + ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) && + !consdev_fallback) newcon->flags &= ~CON_PRINTBUFFER; /* @@ -2762,12 +2780,28 @@ void register_console(struct console *newcon) * preferred driver at the head of the list. */ console_lock(); - if ((newcon->flags & CON_CONSDEV) || console_drivers == NULL) { + if ((newcon->flags & CON_CONSDEV && !consdev_fallback) || + console_drivers == NULL) { + /* Put the preferred or the first console at the head. */ newcon->next = console_drivers; console_drivers = newcon; - if (newcon->next) - newcon->next->flags &= ~CON_CONSDEV; + /* Only one console can have CON_CONSDEV flag set */ + if (con_consdev) + con_consdev->flags &= ~CON_CONSDEV; + } else if (newcon->device && con_consdev) { + /* + * Keep the driver associated with /dev/console. + * We are here only when the console was enabled by the cycle + * checking console_cmdline and this is neither preferred + * console nor the consdev fallback. + */ + newcon->next = con_consdev->next; + con_consdev->next = newcon; } else { + /* + * Keep a boot console first until the preferred real one + * is registered. + */ newcon->next = console_drivers->next; console_drivers->next = newcon; } @@ -2808,6 +2842,7 @@ void register_console(struct console *newcon) newcon->name, newcon->index); if (bcon && ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) && + !consdev_fallback && !keep_bootcon) { /* We need to iterate through all boot consoles, to make * sure we print everything out, before we unregister them. @@ -2853,10 +2888,16 @@ int unregister_console(struct console *console) /* * If this isn't the last console and it has CON_CONSDEV set, we - * need to set it on the next preferred console. + * need to set it on the first console with tty binding. */ - if (console_drivers != NULL && console->flags & CON_CONSDEV) - console_drivers->flags |= CON_CONSDEV; + if (console_drivers != NULL && console->flags & CON_CONSDEV) { + for_each_console(a) { + if (a->device) { + a->flags |= CON_CONSDEV; + break; + } + } + } console->flags &= ~CON_ENABLED; console_unlock(); -- 1.8.5.6 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-10 9:15 ` Petr Mladek @ 2019-12-10 22:39 ` Benjamin Herrenschmidt 2019-12-11 9:17 ` Petr Mladek 2019-12-12 0:35 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 15+ messages in thread From: Benjamin Herrenschmidt @ 2019-12-10 22:39 UTC (permalink / raw) To: Petr Mladek Cc: linux-kernel, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, AlekseyMakarov On Tue, 2019-12-10 at 10:15 +0100, Petr Mladek wrote: > On Tue 2019-12-10 11:57:26, Benjamin Herrenschmidt wrote: > > In the following circumstances, the rule of selecting the console > > corresponding to the last "console=" entry on the command line as > > the preferred console (CON_CONSDEV, ie, /dev/console) fails. This > > is a specific example, but it could happen with different consoles > > that have a similar name aliasing mechanism. > > > > This tentative fix changes the loop in register_console to continue > > matching with the array until the match is actually the preferred > > console. > > One problem with this is that con->match() callbacks might have > side effects. If the console matches, the callback sometimes > do some changes in the console driver because it expects > that the console is going to be registered and used. It will still be enabled. I am not changing that. The main issue would be if the match callback chokes on being called multiple times. IE. The only change in behaviour I think with my patch is who gets to be the default, ie who gets to be first in the list with CONSDEV set. There should be no change in who gets enabled. > I have solved the same problem some time ago and we use the following > patch in SUSE kernels. > > Sigh, I have never send it upstream because it looked too complicated > to me. I wanted to clean up the console registration code a bit first, > see https://lkml.kernel.org/r/1497358444-30736-1-git-send-email-pmladek@suse.com > But it was pretty complicated and it has somehow fallen into cracks. This looks indeed a lot more invasive. Is there any reason why what I propose wouldn't work as a first patch that can easily be backported ? I don't see how it would break anything but I haven't necessarily fully understood everything the driver match callbacks might be doing... We can continue cleaning up from there of course, but I'd be keen on having a minimal fix that can go back to stable first. > Anyway, here is the patch that we use. Could you please check if it > works for you as well? Does it make sense, please? I'll give it a spin. However I don't fully grasp why it's necessarily so complicated. Correct me if I'm wrong here but you are trying to address two issues in that patch: - The one I'm trying to address which is that we might "miss" the preferred console in the case of multiple matches. - The fact that when the preferred console isn't found, the one we default to (which ends up first in the list) is missing CONSDEV. Or am I missing something here ? Now couldnt we just use a combination of my patch and one that sets CONSDEV on the first enabled console if not set at the end of register_console ? If later on the preferred one comes in, it will be inserted first with CONSDEV and the flag will be removed from the previous first unless I misread the code. Cheers, Ben. > From: Petr Mladek <pmladek@suse.com> > Date: Tue, 20 Jun 2017 14:40:34 +0200 > Subject: printk/console: Correctly mark console that is used when opening /dev/console > Patch-mainline: Never, an extensive clean up is being prepared for upstream > References: bsc#1040020 > > showconsole tool shows the real name of tty device associated with > /dev/console. It expects that the related console driver has set > CON_CONSDEV flag. > > On the other hand, kernel ignores CON_CONSDEV flag when it looks > for the right driver. Instead, it takes the first driver that > has the tty binding (console->device). See tty_lookup_driver() > and console_device(). > > All this works most of the time because kernel puts the driver > with CON_CONSDEV flag first into the list. There is almost always > registered a real (non-boot) console with this flag set. The real > consoles mostly (always?) have tty binding. Boot consoles that > might miss the tty binding are always removed unless keep_bootcon > command line parameter is used. > > The problem is when some consoles are defined on the command line > and the preferred one (last one) is not registered from some reason. > Note that the consoles might be added to the command line also > using ACPI SPCR or device tree. It might happen that, for example, > SPCR code and user add the same console using two aliases. > Then the first alias matches and we might miss that it matched > also with the preferred console. > > There was one attempt to fix this by searching the command line > from the end and match the preferred alias first. But it caused > regressions. For example, ttyS* are taken as aliases as wellhttps://lkml.kernel.org/r/1497358444-30736-1-git-send-email-pmladek@suse.com > and kernel messages can appear only on one serial port. > The reversed matching caused that the logs suddenly appeared > on another serial port. > > The right solution is to set CON_CONSDEV flag for the driver > used by tty_lookup_driver() even when the preferred console > is not registered. > > It is a bit complicated because register_console() code is tricky. > It expects that only the preferred driver will have CON_CONSDEV > flag set. Also it expects that a boot console will stay firsthttps://lkml.kernel.org/r/1497358444-30736-1-git-send-email-pmladek@suse.com > in the list until the preferred console is registered. These > information are used to make various decisions: > > + Use a fallback code when none console is configured on > the command line. This code tries to enable any console > until a real one is enabled. > > + Unregister all boot consoles when the real preferred one > is registered. And do not reply the log on the real console > to avoid duplicates. > > A rather invasive clean up is being prepared for upstream. This > patch tries to be as minimalist and do not change the order > of consoles as possible. > > It keeps the logic about having a boot console first until > the real preferred console is registered. But it makes sure > that the first console with tty binding (console->device) will > have CON_CONSDEV flag set. Let's look at this in more details. > > The fallback code in console_register() already works as > expected. It sets CON_CONSDEV flag for any console with > tty binding. > > The code matching all consoles from the command line newly sets > CON_CONSDEV flag also for the fist console with the tty binding. > But it sets "consdev_fallback" to avoid putting this console > first into the list. Remember that we want to keep the boot > console first until the preferred is registered. The information > about the fallback is used also to avoid doing other actions > that need to wait for the preferred console. > > The code adding the console into the list of drivers must > put non-preferred drivers with tty binding next to the > console with CON_CONSDEV set. This is the only change that > might change the order of console drivers in the list > and eventually cause regressions. But it has an effect only > when there are at least three drivers mentioned on the command > line, a boot console is registered and the preferred driver > is not registered. This should be a corner case. > > Finally, unregister_console() sets CON_CONSDEV to first console > with tty binding instead of the first one in the list. > > Signed-off-by: Petr Mladek <pmladek@suse.com> > --- > kernel/printk/printk.c | 59 ++++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 50 insertions(+), 9 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 94ec1aacea64..b6bb4d362b22 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2662,16 +2662,23 @@ void register_console(struct console *newcon) > int i; > unsigned long flags; > struct console *bcon = NULL; > + struct console *con_consdev = NULL; > struct console_cmdline *c; > static bool has_preferred; > + bool consdev_fallback = false; > > - if (console_drivers) > - for_each_console(bcon) > + if (console_drivers) { > + for_each_console(bcon) { > if (WARN(bcon == newcon, > "console '%s%d' already registered\n", > bcon->name, bcon->index)) > return; > > + if (bcon->flags & CON_CONSDEV && !con_consdev) > + con_consdev = bcon; > + } > + } > + > /* > * before we register a new CON_BOOT console, make sure we don't > * already have a valid console > @@ -2739,8 +2746,17 @@ void register_console(struct console *newcon) > > newcon->flags |= CON_ENABLED; > if (i == preferred_console) { > + /* This is the last console on the command line. */ > newcon->flags |= CON_CONSDEV; > has_preferred = true; > + } else if (newcon->device && !con_consdev) { > + /* > + * This is the first console with tty binding. It will > + * be used for /dev/console when the preferred one > + * will not get registered for some reason. > + */ > + newcon->flags |= CON_CONSDEV; > + consdev_fallback = true; > } > break; > } > @@ -2754,7 +2770,9 @@ void register_console(struct console *newcon) > * the real console are the same physical device, it's annoying to > * see the beginning boot messages twice > */ > - if (bcon && ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV)) > + if (bcon && > + ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) && > + !consdev_fallback) > newcon->flags &= ~CON_PRINTBUFFER; > > /* > @@ -2762,12 +2780,28 @@ void register_console(struct console *newcon) > * preferred driver at the head of the list. > */ > console_lock(); > - if ((newcon->flags & CON_CONSDEV) || console_drivers == NULL) { > + if ((newcon->flags & CON_CONSDEV && !consdev_fallback) || > + console_drivers == NULL) { > + /* Put the preferred or the first console at the head. */ > newcon->next = console_drivers; > console_drivers = newcon; > - if (newcon->next) > see > https://lkml.kernel.org/r/1497358444-30736-1-git-send-email-pmladek@suse.com> > ; But it was pretty complicated and it has somehow fallen into > cracks. > This looks indeed a lot more invasive. Is there any reason why what I > propose wouldn't work as a first patch that can easily be backported > ? > I don't see how it would break anything but I haven't necessarily > fully understood everything the driver match callbacks might be > doing... > We can continue cleaning up from there of course, but I'd be keen on > having a minimal fix that can go back to stable first. > > Anyway, here is the patch that we use. Could you please check if > it> works for you as well? Does it make sense, please? > I'll give it a spin. Some comments on it at first look though:> > > - newcon->next->flags &= ~CON_CONSDEV; > + /* Only one console can have CON_CONSDEV flag set */ > + if (con_consdev) > + con_consdev->flags &= ~CON_CONSDEV; > + } else if (newcon->device && con_consdev) { > + /* > + * Keep the driver associated with /dev/console. > + * We are here only when the console was enabled by the cycle > + * checking console_cmdline and this is neither preferred > + * console nor the consdev fallback. > + */ > + newcon->next = con_consdev->next; > + con_consdev->next = newcon; > } else { > + /* > + * Keep a boot console first until the preferred real one > + * is registered. > + */ > newcon->next = console_drivers->next; > console_drivers->next = newcon; > } > @@ -2808,6 +2842,7 @@ void register_console(struct console *newcon) > newcon->name, newcon->index); > if (bcon && > ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) && > + !consdev_fallback && > !keep_bootcon) { > /* We need to iterate through all boot consoles, to make > * sure we print everything out, before we unregister them. > @@ -2853,10 +2888,16 @@ int unregister_console(struct console *console) > > /* > * If this isn't the last console and it has CON_CONSDEV set, we > - * need to set it on the next preferred console. > + * need to set it on the first console with tty binding. > */ > - if (console_drivers != NULL && console->flags & CON_CONSDEV) > - console_drivers->flags |= CON_CONSDEV; > + if (console_drivers != NULL && console->flags & CON_CONSDEV) { > + for_each_console(a) { > + if (a->device) { > + a->flags |= CON_CONSDEV; > + break; > + } > + } > + } > > console->flags &= ~CON_ENABLED; > console_unlock(); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-10 22:39 ` Benjamin Herrenschmidt @ 2019-12-11 9:17 ` Petr Mladek 2019-12-12 1:23 ` Sergey Senozhatsky 2019-12-16 0:09 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 15+ messages in thread From: Petr Mladek @ 2019-12-11 9:17 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: linux-kernel, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, AlekseyMakarov On Wed 2019-12-11 09:39:06, Benjamin Herrenschmidt wrote: > On Tue, 2019-12-10 at 10:15 +0100, Petr Mladek wrote: > > On Tue 2019-12-10 11:57:26, Benjamin Herrenschmidt wrote: > > > In the following circumstances, the rule of selecting the console > > > corresponding to the last "console=" entry on the command line as > > > the preferred console (CON_CONSDEV, ie, /dev/console) fails. This > > > is a specific example, but it could happen with different consoles > > > that have a similar name aliasing mechanism. > > > > > > This tentative fix changes the loop in register_console to continue > > > matching with the array until the match is actually the preferred > > > console. > > > > One problem with this is that con->match() callbacks might have > > side effects. If the console matches, the callback sometimes > > do some changes in the console driver because it expects > > that the console is going to be registered and used. > > It will still be enabled. I am not changing that. The main issue would > be if the match callback chokes on being called multiple times. And this would exactly happen as pointed out by Sergey. match() does also some setup operations. I would be scared to call them twice. > This looks indeed a lot more invasive. Is there any reason why what I > propose wouldn't work as a first patch that can easily be backported ? Your solution is not acceptable because it might cause calling match() more times. The reverse search of list of console does not work for ttySX consoles because the number is omitted when matching. And the messages will appear only on the first matched serial console. There is a paragraph about this in the commit message of my patch. > We can continue cleaning up from there of course, but I'd be keen on > having a minimal fix that can go back to stable first. I would be fine to take my patch as is and do the clean up later. It seems that more users are affected by the problem. The clean up might be complicated and prone to regressions. The question is how far back you would like to go. The changes in register_console() are rare. Everybody is scared to touch this mess ;-) And it is prone to regressions. > I'll give it a spin. However I don't fully grasp why it's necessarily > so complicated. Correct me if I'm wrong here but you are trying to > address two issues in that patch: > > - The one I'm trying to address which is that we might "miss" the > preferred console in the case of multiple matches. > > - The fact that when the preferred console isn't found, the one we > default to (which ends up first in the list) is missing CONSDEV. My patch is primary fixing this 2nd problem. It fixes showconsole tool to work correctly. But it covers also the 1st one. It might be possible to split it when needed. But I think that you could not easily fix the 1st problem without solving the 2nd one. The best solution would be to split the match() and setup functionality. It would be really appreciated. But it is a lot of work. And it might be hard to backport. Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-11 9:17 ` Petr Mladek @ 2019-12-12 1:23 ` Sergey Senozhatsky 2019-12-16 0:09 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 15+ messages in thread From: Sergey Senozhatsky @ 2019-12-12 1:23 UTC (permalink / raw) To: Petr Mladek Cc: Benjamin Herrenschmidt, linux-kernel, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, AlekseyMakarov On (19/12/11 10:17), Petr Mladek wrote: > And this would exactly happen as pointed out by Sergey. match() does > also some setup operations. I would be scared to call them twice. > > > This looks indeed a lot more invasive. Is there any reason why what I > > propose wouldn't work as a first patch that can easily be backported ? > > Your solution is not acceptable because it might cause calling > match() more times. > > The reverse search of list of console does not work for ttySX > consoles because the number is omitted when matching. And the messages > will appear only on the first matched serial console. There is > a paragraph about this in the commit message of my patch. A fast path for preferred_console match()/setup() sounds like something that may work. There won't be double setup()-s and we scan console list in the same direction. We just have separate match()/setup() for preferred_console. Do you think this won't do the trick? -ss ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-11 9:17 ` Petr Mladek 2019-12-12 1:23 ` Sergey Senozhatsky @ 2019-12-16 0:09 ` Benjamin Herrenschmidt 2019-12-19 9:50 ` Petr Mladek 1 sibling, 1 reply; 15+ messages in thread From: Benjamin Herrenschmidt @ 2019-12-16 0:09 UTC (permalink / raw) To: Petr Mladek Cc: linux-kernel, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, AlekseyMakarov On Wed, 2019-12-11 at 10:17 +0100, Petr Mladek wrote: > The reverse search of list of console does not work for ttySX > consoles because the number is omitted when matching. And the messages > will appear only on the first matched serial console. There is > a paragraph about this in the commit message of my patch. About that specific issue... I see indeed that 8250_core.c registers a "generic" console with index -1 which will match whetever we hit first in the array. This is actually wrong isn't it ? Without any change such as what we've been proposing, it means that an arch doing add_preferred_console of any ttyS* will override anything on the command line, and it also means that a command line with multiple ttyS entries will stop at the first one, not the last one. IE. In both case the code will select a console that isn't preferred_console... or am I missing something subtle ? So yes, fixing that will "regress" in the sense that it will change the behaviour, but to make it match what's documented... am I wrong ? The question then becomes what's the most broken ? Changing the behaviour that might have become expected or leaving the (alledgedly) broken behaviour in place ? Cheers, Ben. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-16 0:09 ` Benjamin Herrenschmidt @ 2019-12-19 9:50 ` Petr Mladek 0 siblings, 0 replies; 15+ messages in thread From: Petr Mladek @ 2019-12-19 9:50 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: linux-kernel, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, AlekseyMakarov On Mon 2019-12-16 11:09:33, Benjamin Herrenschmidt wrote: > On Wed, 2019-12-11 at 10:17 +0100, Petr Mladek wrote: > > The reverse search of list of console does not work for ttySX > > consoles because the number is omitted when matching. And the messages > > will appear only on the first matched serial console. There is > > a paragraph about this in the commit message of my patch. > > About that specific issue... > > I see indeed that 8250_core.c registers a "generic" console with index > -1 which will match whetever we hit first in the array. > > This is actually wrong isn't it ? Without any change such as what we've > been proposing, it means that an arch doing add_preferred_console of > any ttyS* will override anything on the command line, and it also means > that a command line with multiple ttyS entries will stop at the first > one, not the last one. > > IE. In both case the code will select a console that isn't > preferred_console... or am I missing something subtle ? > > So yes, fixing that will "regress" in the sense that it will change the > behaviour, but to make it match what's documented... am I wrong ? > > The question then becomes what's the most broken ? Changing the > behaviour that might have become expected or leaving the (alledgedly) > broken behaviour in place ? I though the same. But then I found the following in Documentation/admin-guide/serial-console.rst "If no console device is specified, the first device found capable of acting as a system console will be used. At this time, the system first looks for a VGA card and then for a serial port. So if you don't have a VGA card in your system the first serial port will automatically become the console." In addition, there is the "documentation" how systemd handles serial console and login prompts, see http://0pointer.de/blog/projects/serial-console.html I agree that the current behavior is wrong. I really would like to change it. But the result would be that some users will not get login prompt after a kernel update. I am afraid that we are blocked by the "do not break userspace" rule. Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-10 9:15 ` Petr Mladek 2019-12-10 22:39 ` Benjamin Herrenschmidt @ 2019-12-12 0:35 ` Benjamin Herrenschmidt 2019-12-12 9:09 ` Petr Mladek 1 sibling, 1 reply; 15+ messages in thread From: Benjamin Herrenschmidt @ 2019-12-12 0:35 UTC (permalink / raw) To: Petr Mladek Cc: linux-kernel, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, AlekseyMakarov On Tue, 2019-12-10 at 10:15 +0100, Petr Mladek wrote: > > Anyway, here is the patch that we use. Could you please check if it > works for you as well? Does it make sense, please? It doesn't fix my problem. tty0 remains the default console instead of ttyS0 with your patch applied. I suspect for the same reason, we match uart0 which isn't preferred, so we enable that but don't put it "first" in the list, and since we break out of the loop we never match ttyS0. I see 3 simple ways out of this that don't involve breaking up match() - Bite the bullet and use my patch assuming that calling setup() multiple times is safe. I had a look at the two you had concerns with, the zilog ones seems safe. pl1011 will leak a clk_prepare reference but I think that's a non-issue for a kernel console (I may be wrong) - Rework the loop to try matching against the array entry pointed by preferred_console first. - Rework the loop to try matching the entries from the command line before trying to match the entries added by the platform/arch. (Easily done by flagging them in the array, I can cook a patch). Let me know what you prefer or if you have a different idea and I'll try to whip up a patch tomorrow. Cheers, Ben. > From: Petr Mladek <pmladek@suse.com> > Date: Tue, 20 Jun 2017 14:40:34 +0200 > Subject: printk/console: Correctly mark console that is used when opening /dev/console > Patch-mainline: Never, an extensive clean up is being prepared for upstream > References: bsc#1040020 > > showconsole tool shows the real name of tty device associated with > /dev/console. It expects that the related console driver has set > CON_CONSDEV flag. > > On the other hand, kernel ignores CON_CONSDEV flag when it looks > for the right driver. Instead, it takes the first driver that > has the tty binding (console->device). See tty_lookup_driver() > and console_device(). > > All this works most of the time because kernel puts the driver > with CON_CONSDEV flag first into the list. There is almost always > registered a real (non-boot) console with this flag set. The real > consoles mostly (always?) have tty binding. Boot consoles that > might miss the tty binding are always removed unless keep_bootcon > command line parameter is used. > > The problem is when some consoles are defined on the command line > and the preferred one (last one) is not registered from some reason. > Note that the consoles might be added to the command line also > using ACPI SPCR or device tree. It might happen that, for example, > SPCR code and user add the same console using two aliases. > Then the first alias matches and we might miss that it matched > also with the preferred console. > > There was one attempt to fix this by searching the command line > from the end and match the preferred alias first. But it caused > regressions. For example, ttyS* are taken as aliases as well > and kernel messages can appear only on one serial port. > The reversed matching caused that the logs suddenly appeared > on another serial port. > > The right solution is to set CON_CONSDEV flag for the driver > used by tty_lookup_driver() even when the preferred console > is not registered. > > It is a bit complicated because register_console() code is tricky. > It expects that only the preferred driver will have CON_CONSDEV > flag set. Also it expects that a boot console will stay first > in the list until the preferred console is registered. These > information are used to make various decisions: > > + Use a fallback code when none console is configured on > the command line. This code tries to enable any console > until a real one is enabled. > > + Unregister all boot consoles when the real preferred one > is registered. And do not reply the log on the real console > to avoid duplicates. > > A rather invasive clean up is being prepared for upstream. This > patch tries to be as minimalist and do not change the order > of consoles as possible. > > It keeps the logic about having a boot console first until > the real preferred console is registered. But it makes sure > that the first console with tty binding (console->device) will > have CON_CONSDEV flag set. Let's look at this in more details. > > The fallback code in console_register() already works as > expected. It sets CON_CONSDEV flag for any console with > tty binding. > > The code matching all consoles from the command line newly sets > CON_CONSDEV flag also for the fist console with the tty binding. > But it sets "consdev_fallback" to avoid putting this console > first into the list. Remember that we want to keep the boot > console first until the preferred is registered. The information > about the fallback is used also to avoid doing other actions > that need to wait for the preferred console. > > The code adding the console into the list of drivers must > put non-preferred drivers with tty binding next to the > console with CON_CONSDEV set. This is the only change that > might change the order of console drivers in the list > and eventually cause regressions. But it has an effect only > when there are at least three drivers mentioned on the command > line, a boot console is registered and the preferred driver > is not registered. This should be a corner case. > > Finally, unregister_console() sets CON_CONSDEV to first console > with tty binding instead of the first one in the list. > > Signed-off-by: Petr Mladek <pmladek@suse.com> > --- > kernel/printk/printk.c | 59 ++++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 50 insertions(+), 9 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 94ec1aacea64..b6bb4d362b22 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2662,16 +2662,23 @@ void register_console(struct console *newcon) > int i; > unsigned long flags; > struct console *bcon = NULL; > + struct console *con_consdev = NULL; > struct console_cmdline *c; > static bool has_preferred; > + bool consdev_fallback = false; > > - if (console_drivers) > - for_each_console(bcon) > + if (console_drivers) { > + for_each_console(bcon) { > if (WARN(bcon == newcon, > "console '%s%d' already registered\n", > bcon->name, bcon->index)) > return; > > + if (bcon->flags & CON_CONSDEV && !con_consdev) > + con_consdev = bcon; > + } > + } > + > /* > * before we register a new CON_BOOT console, make sure we don't > * already have a valid console > @@ -2739,8 +2746,17 @@ void register_console(struct console *newcon) > > newcon->flags |= CON_ENABLED; > if (i == preferred_console) { > + /* This is the last console on the command line. */ > newcon->flags |= CON_CONSDEV; > has_preferred = true; > + } else if (newcon->device && !con_consdev) { > + /* > + * This is the first console with tty binding. It will > + * be used for /dev/console when the preferred one > + * will not get registered for some reason. > + */ > + newcon->flags |= CON_CONSDEV; > + consdev_fallback = true; > } > break; > } > @@ -2754,7 +2770,9 @@ void register_console(struct console *newcon) > * the real console are the same physical device, it's annoying to > * see the beginning boot messages twice > */ > - if (bcon && ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV)) > + if (bcon && > + ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) && > + !consdev_fallback) > newcon->flags &= ~CON_PRINTBUFFER; > > /* > @@ -2762,12 +2780,28 @@ void register_console(struct console *newcon) > * preferred driver at the head of the list. > */ > console_lock(); > - if ((newcon->flags & CON_CONSDEV) || console_drivers == NULL) { > + if ((newcon->flags & CON_CONSDEV && !consdev_fallback) || > + console_drivers == NULL) { > + /* Put the preferred or the first console at the head. */ > newcon->next = console_drivers; > console_drivers = newcon; > - if (newcon->next) > - newcon->next->flags &= ~CON_CONSDEV; > + /* Only one console can have CON_CONSDEV flag set */ > + if (con_consdev) > + con_consdev->flags &= ~CON_CONSDEV; > + } else if (newcon->device && con_consdev) { > + /* > + * Keep the driver associated with /dev/console. > + * We are here only when the console was enabled by the cycle > + * checking console_cmdline and this is neither preferred > + * console nor the consdev fallback. > + */ > + newcon->next = con_consdev->next; > + con_consdev->next = newcon; > } else { > + /* > + * Keep a boot console first until the preferred real one > + * is registered. > + */ > newcon->next = console_drivers->next; > console_drivers->next = newcon; > } > @@ -2808,6 +2842,7 @@ void register_console(struct console *newcon) > newcon->name, newcon->index); > if (bcon && > ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) && > + !consdev_fallback && > !keep_bootcon) { > /* We need to iterate through all boot consoles, to make > * sure we print everything out, before we unregister them. > @@ -2853,10 +2888,16 @@ int unregister_console(struct console *console) > > /* > * If this isn't the last console and it has CON_CONSDEV set, we > - * need to set it on the next preferred console. > + * need to set it on the first console with tty binding. > */ > - if (console_drivers != NULL && console->flags & CON_CONSDEV) > - console_drivers->flags |= CON_CONSDEV; > + if (console_drivers != NULL && console->flags & CON_CONSDEV) { > + for_each_console(a) { > + if (a->device) { > + a->flags |= CON_CONSDEV; > + break; > + } > + } > + } > > console->flags &= ~CON_ENABLED; > console_unlock(); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches 2019-12-12 0:35 ` Benjamin Herrenschmidt @ 2019-12-12 9:09 ` Petr Mladek 0 siblings, 0 replies; 15+ messages in thread From: Petr Mladek @ 2019-12-12 9:09 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: linux-kernel, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, AlekseyMakarov On Thu 2019-12-12 11:35:23, Benjamin Herrenschmidt wrote: > On Tue, 2019-12-10 at 10:15 +0100, Petr Mladek wrote: > > > > Anyway, here is the patch that we use. Could you please check if it > > works for you as well? Does it make sense, please? > > It doesn't fix my problem. tty0 remains the default console instead > of ttyS0 with your patch applied. Sigh, I see. > I suspect for the same reason, we match uart0 which isn't preferred, > so we enable that but don't put it "first" in the list, and since > we break out of the loop we never match ttyS0. Yeah. > I see 3 simple ways out of this that don't involve breaking up match() > > - Bite the bullet and use my patch assuming that calling setup() > multiple times is safe. I had a look at the two you had concerns > with, the zilog ones seems safe. pl1011 will leak a clk_prepare > reference but I think that's a non-issue for a kernel console > (I may be wrong) This does not sound much convincing to me. Leaking reference is an issue, definitely. > - Rework the loop to try matching against the array entry pointed > by preferred_console first. IMHO, in principle, we are trying to solve the same problem as the commit cf39bf58afdaabc0b86f141 ("printk: fix double printing with earlycon"). And it was reverted because it broke some setups, see the commit dac8bbbae1d0ccba96402d25d ("Revert "printk: fix double printing with earlycon"). Trying only the preferred console first is less invasive but it might cause exactly the same regression. > - Rework the loop to try matching the entries from the command line > before trying to match the entries added by the platform/arch. > (Easily done by flagging them in the array, I can cook a patch). This makes some sense. It would allow user to override the fallback defined by platform/arch. IMHO, it would solve all the problems that motivated people working on this. And it should not cause regression that forced us to revert the backward search. There is still some risk of regressions. But I would give it a try. Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-12-19 9:50 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-10 0:57 [RFC/PATCH] printk: Fix preferred console selection with multiple matches Benjamin Herrenschmidt 2019-12-10 8:01 ` Sergey Senozhatsky 2019-12-10 22:26 ` Benjamin Herrenschmidt 2019-12-11 2:01 ` Sergey Senozhatsky 2019-12-11 4:02 ` Benjamin Herrenschmidt 2019-12-11 5:35 ` Sergey Senozhatsky 2019-12-11 12:53 ` Petr Mladek 2019-12-10 9:15 ` Petr Mladek 2019-12-10 22:39 ` Benjamin Herrenschmidt 2019-12-11 9:17 ` Petr Mladek 2019-12-12 1:23 ` Sergey Senozhatsky 2019-12-16 0:09 ` Benjamin Herrenschmidt 2019-12-19 9:50 ` Petr Mladek 2019-12-12 0:35 ` Benjamin Herrenschmidt 2019-12-12 9:09 ` Petr Mladek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).