| Message ID | 20200430161438.17640-2-alpernebiyasak@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prefer working VT console over SPCR and device-tree chosen stdout-path | expand |
On Thu, Apr 30, 2020 at 07:14:35PM +0300, Alper Nebi Yasak wrote: > Currently, add_preferred_console sets a preferred console, but doesn't > actually change /dev/console to match it. That part is handled within > register_device, where a newly registered console driver will be set as > /dev/console if it matches the preferred console. > > However, if the relevant driver is already registered, the only way to > set it as /dev/console is by un-registering and re-registering it. An > example is the xenfb_make_preferred_console() function: > > console_lock(); > for_each_console(c) { > if (!strcmp(c->name, "tty") && c->index == 0) > break; > } > console_unlock(); > if (c) { > unregister_console(c); > c->flags |= CON_CONSDEV; > c->flags &= ~CON_PRINTBUFFER; /* don't print again */ > register_console(c); > } > > The code above was introduced in commit 9e124fe16ff2 ("xen: Enable > console tty by default in domU if it's not a dummy"). In short, it's aim > is to set VT as the preferred console only after a working framebuffer > is registered and thus VT is not the dummy device. > > This patch introduces an update_console_to_preferred function that > handles the necessary /dev/console change. With this change, the example > above can be replaced with: > > console_lock(); > add_preferred_console("tty", 0, NULL); > update_console_to_preferred(); > console_unlock(); > > More importantly, these two calls can be moved to vt.c in order to bump > its priority when a non-dummy backend for it is introduced, solving that > problem in general. Even w/o looking into the code I believe it breaks more platforms than fixes anything. It was not first time one tries to do something about preferred consoles and it appeared to break working configurations. I would wait for PRINTK maintainers to tell, but to me it sounds like papering over the real issue which you don't understand (yet).
On (20/04/30 19:14), Alper Nebi Yasak wrote: > Currently, add_preferred_console sets a preferred console, but doesn't > actually change /dev/console to match it. That part is handled within > register_device, where a newly registered console driver will be set as > /dev/console if it matches the preferred console. > > However, if the relevant driver is already registered, the only way to > set it as /dev/console is by un-registering and re-registering it. Hmm. Preferred console selection is very fragile, there are too many setups and workarounds that even minor tweaks introduce regressions oftentimes. We have, by the way, a pending patchset which changes the same are - preferred console selection. git://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git for-5.7-preferred-console [..] > An example is the xenfb_make_preferred_console() function: > > console_lock(); > for_each_console(c) { > if (!strcmp(c->name, "tty") && c->index == 0) > break; > } > console_unlock(); > if (c) { > unregister_console(c); > c->flags |= CON_CONSDEV; > c->flags &= ~CON_PRINTBUFFER; /* don't print again */ > register_console(c); > } I didn't know about this code. > The code above was introduced in commit 9e124fe16ff2 ("xen: Enable > console tty by default in domU if it's not a dummy"). In short, it's aim > is to set VT as the preferred console only after a working framebuffer > is registered and thus VT is not the dummy device. > > This patch introduces an update_console_to_preferred function that > handles the necessary /dev/console change. With this change, the example > above can be replaced with: > > console_lock(); > add_preferred_console("tty", 0, NULL); > update_console_to_preferred(); > console_unlock(); > > More importantly, these two calls can be moved to vt.c in order to bump > its priority when a non-dummy backend for it is introduced, solving that > problem in general. Let me take a look over the weekend. -ss
On 01/05/2020 04:44, Sergey Senozhatsky wrote: > Hmm. Preferred console selection is very fragile, there are too many > setups and workarounds that even minor tweaks introduce regressions > oftentimes. All this would only execute on #ifdef VT_CONSOLE right now and I think they can be gated further by a new Kconfig like VT_CONSOLE_PREFERRED, if that'd make it better. > We have, by the way, a pending patchset which changes the same > are - preferred console selection. > > git://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git for-5.7-preferred-console I was working on next-20200430 where that patchset is already included, can confirm it doesn't solve this problem. I hope I've correctly avoided breaking it by setting its "has_preferred_console" variable (thus this patchset somewhat depends on that). > Let me take a look over the weekend. Thanks in advance.
On (20/04/30 19:14), Alper Nebi Yasak wrote: [..] > +int update_console_to_preferred(void) > +{ > + struct console_cmdline *c = NULL; > + struct console *con = NULL; > + struct console *tmp = NULL; > + > + if (preferred_console >= 0) > + c = &console_cmdline[preferred_console]; > + > + if (!c || !c->name[0]) > + return 0; > + > + for_each_console(con) { > + if (!con->next || !(con->next->flags & CON_ENABLED)) > + continue; > + if (strcmp(c->name, con->next->name) != 0) > + continue; This matches the consoles by exact name. Consoles can have aliases, but matching by alias is rather complex and it has some side effects. Let me Cc more people on this. VT has a console takeover logic, I wonder if we can extend the takeover code somehow. Daniel, any thoughts? https://lore.kernel.org/lkml/20200430161438.17640-1-alpernebiyasak@gmail.com -ss
On Wed, May 13, 2020 at 7:35 AM Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote: > > On (20/04/30 19:14), Alper Nebi Yasak wrote: > [..] > > +int update_console_to_preferred(void) > > +{ > > + struct console_cmdline *c = NULL; > > + struct console *con = NULL; > > + struct console *tmp = NULL; > > + > > + if (preferred_console >= 0) > > + c = &console_cmdline[preferred_console]; > > + > > + if (!c || !c->name[0]) > > + return 0; > > + > > + for_each_console(con) { > > + if (!con->next || !(con->next->flags & CON_ENABLED)) > > + continue; > > + if (strcmp(c->name, con->next->name) != 0) > > + continue; > > This matches the consoles by exact name. Consoles can have aliases, > but matching by alias is rather complex and it has some side effects. > > Let me Cc more people on this. VT has a console takeover logic, > I wonder if we can extend the takeover code somehow. > > Daniel, any thoughts? Apologies for late reply, but nope, no thoughts. I have some ideas for the locking in the console subsystem, but that's just to untangle it from gpu drivers as much as possible. Otherwise I'm trying to stay away from it as far as I can :-) Cheers, Daniel > > https://lore.kernel.org/lkml/20200430161438.17640-1-alpernebiyasak@gmail.com > > -ss
diff --git a/include/linux/console.h b/include/linux/console.h index 75dd20650fbe..4b3fa34be245 100644 --- a/include/linux/console.h +++ b/include/linux/console.h @@ -172,6 +172,7 @@ enum con_flush_mode { }; extern int add_preferred_console(char *name, int idx, char *options); +extern int update_console_to_preferred(void); extern void register_console(struct console *); extern int unregister_console(struct console *); extern struct console *console_drivers; diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 6ede4a7222e6..efda422203e4 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -2240,12 +2240,68 @@ __setup("console=", console_setup); * be used by arch-specific code either to override the user or more * commonly to provide a default console (ie from PROM variables) when * the user has not supplied one. + * + * Preferences set by this function don't take effect until the next + * time a matching driver for the preferred console is registered. If a + * matching driver was already registered, @update_console_to_preferred + * function can be used to set that as the preferred console driver. */ int add_preferred_console(char *name, int idx, char *options) { return __add_preferred_console(name, idx, options, NULL, false); } +/** + * update_console_to_preferred - set console to the preferred console's driver. + * + * Updates console_drivers and CON_CONSDEV flags so that an already + * registered and enabled console driver matching the preferred console + * is used as /dev/console. + * + * Must be called within console_lock();. + */ +int update_console_to_preferred(void) +{ + struct console_cmdline *c = NULL; + struct console *con = NULL; + struct console *tmp = NULL; + + if (preferred_console >= 0) + c = &console_cmdline[preferred_console]; + + if (!c || !c->name[0]) + return 0; + + for_each_console(con) { + if (!con->next || !(con->next->flags & CON_ENABLED)) + continue; + if (strcmp(c->name, con->next->name) != 0) + continue; + if (con->next->index >= 0 && + con->next->index != c->index) + continue; + break; + } + + if (!con) + return -ENODEV; + + pr_info("switching to console [%s%d]\n", + con->next->name, con->next->index); + + tmp = con->next; + con->next = con->next->next; + tmp->next = console_drivers; + console_drivers = tmp; + + if (console_drivers->next) + console_drivers->next->flags &= ~CON_CONSDEV; + console_drivers->flags |= CON_CONSDEV; + has_preferred_console = true; + + return 0; +} + bool console_suspend_enabled = true; EXPORT_SYMBOL(console_suspend_enabled);
Currently, add_preferred_console sets a preferred console, but doesn't actually change /dev/console to match it. That part is handled within register_device, where a newly registered console driver will be set as /dev/console if it matches the preferred console. However, if the relevant driver is already registered, the only way to set it as /dev/console is by un-registering and re-registering it. An example is the xenfb_make_preferred_console() function: console_lock(); for_each_console(c) { if (!strcmp(c->name, "tty") && c->index == 0) break; } console_unlock(); if (c) { unregister_console(c); c->flags |= CON_CONSDEV; c->flags &= ~CON_PRINTBUFFER; /* don't print again */ register_console(c); } The code above was introduced in commit 9e124fe16ff2 ("xen: Enable console tty by default in domU if it's not a dummy"). In short, it's aim is to set VT as the preferred console only after a working framebuffer is registered and thus VT is not the dummy device. This patch introduces an update_console_to_preferred function that handles the necessary /dev/console change. With this change, the example above can be replaced with: console_lock(); add_preferred_console("tty", 0, NULL); update_console_to_preferred(); console_unlock(); More importantly, these two calls can be moved to vt.c in order to bump its priority when a non-dummy backend for it is introduced, solving that problem in general. Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com> --- Changes in v2: - Use the correct format when referencing a commit include/linux/console.h | 1 + kernel/printk/printk.c | 56 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+)