| Message ID | 20250606-printk-cleanup-part2-v1-2-f427c743dda0@suse.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | printk cleanup - part 2 | expand |
On Fri 2025-06-06 23:53:44, Marcos Paulo de Souza wrote: > Instead of update a per-console CON_SUSPENDED flag, use the console_list > locks to protect this flag. This is also applied to console_is_usable > functions, which now also checks if consoles_suspend is set. > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > --- > kernel/printk/internal.h | 7 ++++++- > kernel/printk/nbcon.c | 8 ++++---- > kernel/printk/printk.c | 23 ++++++++++------------- > 3 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h > index 48a24e7b309db20fdd7419f7aeda68ea7c79fd80..752101904f44b13059b6a922519d88e24c9f32c0 100644 > --- a/kernel/printk/internal.h > +++ b/kernel/printk/internal.h > @@ -118,8 +118,12 @@ void nbcon_kthreads_wake(void); > * which can also play a role in deciding if @con can be used to print > * records. > */ > -static inline bool console_is_usable(struct console *con, short flags, bool use_atomic) > +static inline bool console_is_usable(struct console *con, short flags, > + bool use_atomic, bool consoles_suspended) > { > + if (consoles_suspended) > + return false; > + > if (!(flags & CON_ENABLED)) > return false; > > @@ -212,6 +216,7 @@ extern bool have_boot_console; > extern bool have_nbcon_console; > extern bool have_legacy_console; > extern bool legacy_allow_panic_sync; > +extern bool consoles_suspended; > > /** > * struct console_flush_type - Define available console flush methods > diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c > index fd12efcc4aeda8883773d9807bc215f6e5cdf71a..72de12396e6f1bc5234acfdf6dcc393acf88d216 100644 > --- a/kernel/printk/nbcon.c > +++ b/kernel/printk/nbcon.c > @@ -1147,7 +1147,7 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex > cookie = console_srcu_read_lock(); > > flags = console_srcu_read_flags(con); > - if (console_is_usable(con, flags, false)) { > + if (console_is_usable(con, flags, false, consoles_suspended)) { The new global console_suspended value has the be synchronized the same way as the current CON_SUSPENDED per-console flag. It means that the value must be: + updated only under console_list_lock together with synchronize_rcu(). + read using READ_ONCE() under console_srcu_read_lock() I am going to propose more solutions because no one is obviously the best one. Variant A: ========= Create a helper functions, similar to console_srcu_read_flags() and console_srcu_write_flags(): Something like: static inline bool console_srcu_read_consoles_suspended() { WARN_ON_ONCE(!console_srcu_read_lock_is_held()); /* * The READ_ONCE() matches the WRITE_ONCE() when the value * is modified console_srcu_write_consoles_suspended(). */ return data_race(READ_ONCE(consoles_suspended)); } static inline void console_srcu_write_consoles_suspended(bool suspended) { lockdep_assert_console_list_lock_held(); /* This matches the READ_ONCE() in console_srcu_read_consoles_suspended(). */ WRITE_ONCE(consoles_suspended, suspended); } This has the drawback that most console_is_usable() callers would need to get and pass both variables, for example: --- a/kernel/printk/nbcon.c +++ b/kernel/printk/nbcon.c @@ -1137,6 +1137,7 @@ static bool nbcon_emit_one(struct nbcon_write_context *wctxt, bool use_atomic) */ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_context *ctxt) { + bool cons_suspended; bool ret = false; short flags; int cookie; @@ -1147,7 +1148,8 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex cookie = console_srcu_read_lock(); flags = console_srcu_read_flags(con); - if (console_is_usable(con, flags, false)) { + cons_suspended = console_srcu_read_consoles_suspended(); + if (console_is_usable(con, flags, false, cons_suspended)) { /* Bring the sequence in @ctxt up to date */ ctxt->seq = nbcon_seq_read(con); Pros: + always correct Cons: + not user friendly Variant B: ========== Do not pass @consoles_suspended as a parameter. Instead, read it in console_us_usable() directly. I do not like this because it is not consistent with the con->flags handling and it is not clear why. Variant C: ========== Remove even @flags parameter from console_is_usable() and read both values there directly. Many callers read @flags only because they call console_is_usable(). The change would simplify the code. But there are few exceptions: 1. __nbcon_atomic_flush_pending(), console_flush_all(), and legacy_kthread_should_wakeup() pass @flags to console_is_usable() and also check CON_NBCON flag. But CON_NBCON flag is special. It is statically initialized and never set/cleared at runtime. It can be checked without READ_ONCE(). Well, we still might want to be sure that the struct console can't disappear. IMHO, this can be solved by a helper function: /** * console_srcu_is_nbcon - Locklessly check whether the console is nbcon * @con: struct console pointer of console to check * * Requires console_srcu_read_lock to be held, which implies that @con might * be a registered console. The purpose of holding console_srcu_read_lock is * to guarantee that no exit/cleanup routines will run if the console * is currently undergoing unregistration. * * If the caller is holding the console_list_lock or it is _certain_ that * @con is not and will not become registered, the caller may read * @con->flags directly instead. * * Context: Any context. * Return: True when CON_NBCON flag is set. */ static inline bool console_is_nbcon(const struct console *con) { WARN_ON_ONCE(!console_srcu_read_lock_is_held()); /* * The CON_NBCON flag is statically initialized and is never * set or cleared at runtime. return data_race(con->flags & CON_NBCON); } 2. Another exception is __pr_flush() where console_is_usable() is called twice with @use_atomic set "true" and "false". We would want to read "con->flags" only once here. A solution would be to add a parameter to check both con->write_atomic and con->write_thread in a single call. But it might actually be enough to check is with the "false" value because "con->write_thread()" is mandatory for nbcon consoles. And legacy consoles do not distinguish atomic mode. Variant D: ========== We need to distinguish the global and per-console "suspended" flag because they might be nested. But we could use a separate flag for the global setting. I mean that: + console_suspend() would set CON_SUSPENDED flag + console_suspend_all() would set CON_SUSPENDED_ALL flag They both will be in con->flags. Pros: + It is easy to implement. Cons: + It feels a bit ugly. My opinion: =========== I personally prefer the variant C because: + Removes one parameter from console_is_usable(). + The lockless synchronization of both global and per-console flags is hidden in console_is_usable(). + The global console_suspended flag will be stored in global variable (in compare with variant D). What do you think, please? Best Regards, Petr PS: The commit message and the cover letter should better explain the background of this change. It would be great if the cover letter described the bigger picture, especially the history of the console_suspended, CON_SUSPENDED, and CON_ENABLED flags. It might use info from https://lore.kernel.org/lkml/ZyoNZfLT6tlVAWjO@pathway.suse.cz/ and maybe even this link. Also this commit message should mention that it partly reverts the commit 9e70a5e109a4a233678 ("printk: Add per-console suspended state"). But it is not simple revert because we need to preserve the synchronization using the console_list_lock for writing and SRCU for reading.
On 2025-06-13, Petr Mladek <pmladek@suse.com> wrote: >> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c >> index fd12efcc4aeda8883773d9807bc215f6e5cdf71a..72de12396e6f1bc5234acfdf6dcc393acf88d216 100644 >> --- a/kernel/printk/nbcon.c >> +++ b/kernel/printk/nbcon.c >> @@ -1147,7 +1147,7 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex >> cookie = console_srcu_read_lock(); >> >> flags = console_srcu_read_flags(con); >> - if (console_is_usable(con, flags, false)) { >> + if (console_is_usable(con, flags, false, consoles_suspended)) { > > The new global console_suspended value has the be synchronized the > same way as the current CON_SUSPENDED per-console flag. > It means that the value must be: > > + updated only under console_list_lock together with > synchronize_rcu(). > > + read using READ_ONCE() under console_srcu_read_lock() Yes. > I am going to propose more solutions because no one is obviously > the best one. [...] > Variant C: > ========== > > Remove even @flags parameter from console_is_usable() and read both > values there directly. > > Many callers read @flags only because they call console_is_usable(). > The change would simplify the code. > > But there are few exceptions: > > 1. __nbcon_atomic_flush_pending(), console_flush_all(), > and legacy_kthread_should_wakeup() pass @flags to > console_is_usable() and also check CON_NBCON flag. > > But CON_NBCON flag is special. It is statically initialized > and never set/cleared at runtime. It can be checked without > READ_ONCE(). Well, we still might want to be sure that > the struct console can't disappear. > > IMHO, this can be solved by a helper function: > > /** > * console_srcu_is_nbcon - Locklessly check whether the console is nbcon > * @con: struct console pointer of console to check > * > * Requires console_srcu_read_lock to be held, which implies that @con might > * be a registered console. The purpose of holding console_srcu_read_lock is > * to guarantee that no exit/cleanup routines will run if the console > * is currently undergoing unregistration. > * > * If the caller is holding the console_list_lock or it is _certain_ that > * @con is not and will not become registered, the caller may read > * @con->flags directly instead. > * > * Context: Any context. > * Return: True when CON_NBCON flag is set. > */ > static inline bool console_is_nbcon(const struct console *con) > { > WARN_ON_ONCE(!console_srcu_read_lock_is_held()); > > /* > * The CON_NBCON flag is statically initialized and is never > * set or cleared at runtime. > return data_race(con->flags & CON_NBCON); > } Agreed. > 2. Another exception is __pr_flush() where console_is_usable() is > called twice with @use_atomic set "true" and "false". > > We would want to read "con->flags" only once here. A solution > would be to add a parameter to check both con->write_atomic > and con->write_thread in a single call. Or it could become a bitmask of printing types to check: #define ATOMIC_PRINTING 0x1 #define NONATOMIC_PRINTING 0x2 and then __pr_flush() looks like: if (!console_is_usable(c, flags, ATOMIC_PRINTING|NONATOMIC_PRINTING) > But it might actually be enough to check is with the "false" > value because "con->write_thread()" is mandatory for nbcon > consoles. And legacy consoles do not distinguish atomic mode. A bit tricky, but you are right. > > > Variant D: > ========== > > We need to distinguish the global and per-console "suspended" flag > because they might be nested. But we could use a separate flag > for the global setting. > > I mean that: > > + console_suspend() would set CON_SUSPENDED flag > + console_suspend_all() would set CON_SUSPENDED_ALL flag > > They both will be in con->flags. > > Pros: > > + It is easy to implement. > > Cons: > > + It feels a bit ugly. > > > My opinion: > =========== > > I personally prefer the variant C because: > > + Removes one parameter from console_is_usable(). > > + The lockless synchronization of both global and per-console > flags is hidden in console_is_usable(). > > + The global console_suspended flag will be stored in global > variable (in compare with variant D). > > What do you think, please? > > Best Regards, > Petr > > > PS: The commit message and the cover letter should better explain > the background of this change. > > It would be great if the cover letter described the bigger > picture, especially the history of the console_suspended, > CON_SUSPENDED, and CON_ENABLED flags. It might use info > from > https://lore.kernel.org/lkml/ZyoNZfLT6tlVAWjO@pathway.suse.cz/ > and maybe even this link. > > Also this commit message should mention that it partly reverts > the commit 9e70a5e109a4a233678 ("printk: Add per-console > suspended state"). But it is not simple revert because > we need to preserve the synchronization using > the console_list_lock for writing and SRCU for reading.
On Fri, 2025-06-13 at 17:20 +0200, Petr Mladek wrote: > On Fri 2025-06-06 23:53:44, Marcos Paulo de Souza wrote: > > > Variant C: > ========== > > Remove even @flags parameter from console_is_usable() and read both > values there directly. > > Many callers read @flags only because they call console_is_usable(). > The change would simplify the code. > > But there are few exceptions: > > 1. __nbcon_atomic_flush_pending(), console_flush_all(), > and legacy_kthread_should_wakeup() pass @flags to > console_is_usable() and also check CON_NBCON flag. > > But CON_NBCON flag is special. It is statically initialized > and never set/cleared at runtime. It can be checked without > READ_ONCE(). Well, we still might want to be sure that > the struct console can't disappear. > > IMHO, this can be solved by a helper function: > > /** > * console_srcu_is_nbcon - Locklessly check whether the > console is nbcon > * @con: struct console pointer of console to check > * > * Requires console_srcu_read_lock to be held, which implies > that @con might > * be a registered console. The purpose of holding > console_srcu_read_lock is > * to guarantee that no exit/cleanup routines will run if > the console > * is currently undergoing unregistration. > * > * If the caller is holding the console_list_lock or it is > _certain_ that > * @con is not and will not become registered, the caller > may read > * @con->flags directly instead. > * > * Context: Any context. > * Return: True when CON_NBCON flag is set. > */ > static inline bool console_is_nbcon(const struct console > *con) > { > WARN_ON_ONCE(!console_srcu_read_lock_is_held()); > > /* > * The CON_NBCON flag is statically initialized and > is never > * set or cleared at runtime. > return data_race(con->flags & CON_NBCON); > } > > > 2. Another exception is __pr_flush() where console_is_usable() is > called twice with @use_atomic set "true" and "false". > > We would want to read "con->flags" only once here. A solution > would be to add a parameter to check both con->write_atomic > and con->write_thread in a single call. > > But it might actually be enough to check is with the "false" > value because "con->write_thread()" is mandatory for nbcon > consoles. And legacy consoles do not distinguish atomic mode. > I like this idea. Also, thanks a lot for explaining why the current version won't work. I also liked John's proposal to use a a bitmask on console_is_usable, but I'll think a little on it once I restart working on it this week. > > My opinion: > =========== > > I personally prefer the variant C because: > > + Removes one parameter from console_is_usable(). > > + The lockless synchronization of both global and per-console > flags is hidden in console_is_usable(). > > + The global console_suspended flag will be stored in global > variable (in compare with variant D). > > What do you think, please? Much better, I'll adapt the code as you suggested. > > Best Regards, > Petr > > > PS: The commit message and the cover letter should better explain > the background of this change. > > It would be great if the cover letter described the bigger > picture, especially the history of the console_suspended, > CON_SUSPENDED, and CON_ENABLED flags. It might use info > from > https://lore.kernel.org/lkml/ZyoNZfLT6tlVAWjO@pathway.suse.cz/ > and maybe even this link. > > Also this commit message should mention that it partly reverts > the commit 9e70a5e109a4a233678 ("printk: Add per-console > suspended state"). But it is not simple revert because > we need to preserve the synchronization using > the console_list_lock for writing and SRCU for reading. I agree, such a context would even help the reviewers that would spend some time reading the code and thinking themselves that some code is being readded for some reason.
On Fri 2025-06-20 16:49:07, John Ogness wrote: > On 2025-06-13, Petr Mladek <pmladek@suse.com> wrote: > >> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c > >> index fd12efcc4aeda8883773d9807bc215f6e5cdf71a..72de12396e6f1bc5234acfdf6dcc393acf88d216 100644 > >> --- a/kernel/printk/nbcon.c > >> +++ b/kernel/printk/nbcon.c > >> @@ -1147,7 +1147,7 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex > >> cookie = console_srcu_read_lock(); > >> > >> flags = console_srcu_read_flags(con); > >> - if (console_is_usable(con, flags, false)) { > >> + if (console_is_usable(con, flags, false, consoles_suspended)) { > > > > The new global console_suspended value has the be synchronized the > > same way as the current CON_SUSPENDED per-console flag. > > It means that the value must be: > > > > + updated only under console_list_lock together with > > synchronize_rcu(). > > > > + read using READ_ONCE() under console_srcu_read_lock() > > Yes. > > > I am going to propose more solutions because no one is obviously > > the best one. > > [...] > > > Variant C: > > ========== > > > > Remove even @flags parameter from console_is_usable() and read both > > values there directly. > > > > Many callers read @flags only because they call console_is_usable(). > > The change would simplify the code. > > > > But there are few exceptions: > > > > 2. Another exception is __pr_flush() where console_is_usable() is > > called twice with @use_atomic set "true" and "false". > > > > We would want to read "con->flags" only once here. A solution > > would be to add a parameter to check both con->write_atomic > > and con->write_thread in a single call. > > Or it could become a bitmask of printing types to check: > > #define ATOMIC_PRINTING 0x1 > #define NONATOMIC_PRINTING 0x2 > > and then __pr_flush() looks like: > > if (!console_is_usable(c, flags, ATOMIC_PRINTING|NONATOMIC_PRINTING) I like this. It will help even in all other cases when one mode is needed. I mean that, for example: console_is_usable(c, flags, ATOMIC_PRINTING) is more self-explaining than console_is_usable(c, flags, true) Best Regards, Petr
On 2025-06-24, Petr Mladek <pmladek@suse.com> wrote: >> > Variant C: >> > ========== >> > >> > Remove even @flags parameter from console_is_usable() and read both >> > values there directly. >> > >> > Many callers read @flags only because they call console_is_usable(). >> > The change would simplify the code. >> > >> > But there are few exceptions: >> > >> > 2. Another exception is __pr_flush() where console_is_usable() is >> > called twice with @use_atomic set "true" and "false". >> > >> > We would want to read "con->flags" only once here. A solution >> > would be to add a parameter to check both con->write_atomic >> > and con->write_thread in a single call. >> >> Or it could become a bitmask of printing types to check: >> >> #define ATOMIC_PRINTING 0x1 >> #define NONATOMIC_PRINTING 0x2 >> >> and then __pr_flush() looks like: >> >> if (!console_is_usable(c, flags, ATOMIC_PRINTING|NONATOMIC_PRINTING) > > I like this. It will help even in all other cases when one mode is needed. > I mean that, for example: > > console_is_usable(c, flags, ATOMIC_PRINTING) > > is more self-explaining than > > console_is_usable(c, flags, true) After I wrote that suggestion, I decided that the naming is not good. There is always confusion about what "atomic printing" means. For that reason the parameter was changed to "use_atomic". Basically we are specifying which callback to use and not the purpose. It is a bit tricky because legacy consoles do not have an atomic callback, i.e. the parameter only has meaning for nbcon consoles. Perhaps these macros would be more suitable: #define NBCON_USE_ATOMIC 0x1 #define NBCON_USE_THREAD 0x2 or #define NBCON_USE_WRITE_ATOMIC 0x1 #define NBCON_USE_WRITE_THREAD 0x2 or #define NBCON_ATOMIC_CB 0x1 #define NBCON_THREAD_CB 0x2 or #define NBCON_ATOMIC_FUNC 0x1 #define NBCON_THREAD_FUNC 0x2 Hopefully that gives Petr enough ideas that he can come up with good naming. ;-) John
On Tue 2025-06-24 13:10:25, John Ogness wrote: > On 2025-06-24, Petr Mladek <pmladek@suse.com> wrote: > >> > Variant C: > >> > ========== > >> > > >> > Remove even @flags parameter from console_is_usable() and read both > >> > values there directly. > >> > > >> > Many callers read @flags only because they call console_is_usable(). > >> > The change would simplify the code. > >> > > >> > But there are few exceptions: > >> > > >> > 2. Another exception is __pr_flush() where console_is_usable() is > >> > called twice with @use_atomic set "true" and "false". > >> > > >> > We would want to read "con->flags" only once here. A solution > >> > would be to add a parameter to check both con->write_atomic > >> > and con->write_thread in a single call. > >> > >> Or it could become a bitmask of printing types to check: > >> > >> #define ATOMIC_PRINTING 0x1 > >> #define NONATOMIC_PRINTING 0x2 > >> > >> and then __pr_flush() looks like: > >> > >> if (!console_is_usable(c, flags, ATOMIC_PRINTING|NONATOMIC_PRINTING) > > > > I like this. It will help even in all other cases when one mode is needed. > > I mean that, for example: > > > > console_is_usable(c, flags, ATOMIC_PRINTING) > > > > is more self-explaining than > > > > console_is_usable(c, flags, true) > > After I wrote that suggestion, I decided that the naming is not > good. There is always confusion about what "atomic printing" means. For > that reason the parameter was changed to "use_atomic". Basically we are > specifying which callback to use and not the purpose. It is a bit tricky > because legacy consoles do not have an atomic callback, i.e. the > parameter only has meaning for nbcon consoles. > > Perhaps these macros would be more suitable: > > #define NBCON_USE_ATOMIC 0x1 > #define NBCON_USE_THREAD 0x2 I personally prefer this variant. > or > > #define NBCON_USE_WRITE_ATOMIC 0x1 > #define NBCON_USE_WRITE_THREAD 0x2 This one sounds more precise but it very long. > or > > #define NBCON_ATOMIC_CB 0x1 > #define NBCON_THREAD_CB 0x2 > > or > > #define NBCON_ATOMIC_FUNC 0x1 > #define NBCON_THREAD_FUNC 0x2 > > Hopefully that gives Petr enough ideas that he can come up with good > naming. ;-) I thought about better names yesterday but I somehow did not have inspiration ;-) Thanks for coming with the variants. Best Regards, Petr
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h index 48a24e7b309db20fdd7419f7aeda68ea7c79fd80..752101904f44b13059b6a922519d88e24c9f32c0 100644 --- a/kernel/printk/internal.h +++ b/kernel/printk/internal.h @@ -118,8 +118,12 @@ void nbcon_kthreads_wake(void); * which can also play a role in deciding if @con can be used to print * records. */ -static inline bool console_is_usable(struct console *con, short flags, bool use_atomic) +static inline bool console_is_usable(struct console *con, short flags, + bool use_atomic, bool consoles_suspended) { + if (consoles_suspended) + return false; + if (!(flags & CON_ENABLED)) return false; @@ -212,6 +216,7 @@ extern bool have_boot_console; extern bool have_nbcon_console; extern bool have_legacy_console; extern bool legacy_allow_panic_sync; +extern bool consoles_suspended; /** * struct console_flush_type - Define available console flush methods diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c index fd12efcc4aeda8883773d9807bc215f6e5cdf71a..72de12396e6f1bc5234acfdf6dcc393acf88d216 100644 --- a/kernel/printk/nbcon.c +++ b/kernel/printk/nbcon.c @@ -1147,7 +1147,7 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex cookie = console_srcu_read_lock(); flags = console_srcu_read_flags(con); - if (console_is_usable(con, flags, false)) { + if (console_is_usable(con, flags, false, consoles_suspended)) { /* Bring the sequence in @ctxt up to date */ ctxt->seq = nbcon_seq_read(con); @@ -1206,7 +1206,7 @@ static int nbcon_kthread_func(void *__console) con_flags = console_srcu_read_flags(con); - if (console_is_usable(con, con_flags, false)) + if (console_is_usable(con, con_flags, false, consoles_suspended)) backlog = nbcon_emit_one(&wctxt, false); console_srcu_read_unlock(cookie); @@ -1584,7 +1584,7 @@ static void __nbcon_atomic_flush_pending(u64 stop_seq, bool allow_unsafe_takeove if (!(flags & CON_NBCON)) continue; - if (!console_is_usable(con, flags, true)) + if (!console_is_usable(con, flags, true, consoles_suspended)) continue; if (nbcon_seq_read(con) >= stop_seq) @@ -1795,7 +1795,7 @@ void nbcon_device_release(struct console *con) */ cookie = console_srcu_read_lock(); printk_get_console_flush_type(&ft); - if (console_is_usable(con, console_srcu_read_flags(con), true) && + if (console_is_usable(con, console_srcu_read_flags(con), true, consoles_suspended) && !ft.nbcon_offload && prb_read_valid(prb, nbcon_seq_read(con), NULL)) { /* diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 6d3cf488f4261a3dfd8809a5ab7164b218238c13..658acf92aa3d2a3d1e294b7e17e5ee96d8169afe 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -241,7 +241,7 @@ int devkmsg_sysctl_set_loglvl(const struct ctl_table *table, int write, /** * console_list_lock - Lock the console list * - * For console list or console->flags updates + * For console list, console->flags and consoles_suspended updates */ void console_list_lock(void) { @@ -383,6 +383,8 @@ bool other_cpu_in_panic(void) */ static int console_locked; +bool consoles_suspended; + /* * Array of consoles built from command line options (console=) */ @@ -2755,16 +2757,13 @@ MODULE_PARM_DESC(console_no_auto_verbose, "Disable console loglevel raise to hig */ void console_suspend_all(void) { - struct console *con; - if (!console_suspend_enabled) return; pr_info("Suspending console(s) (use no_console_suspend to debug)\n"); pr_flush(1000, true); console_list_lock(); - for_each_console(con) - console_srcu_write_flags(con, con->flags | CON_SUSPENDED); + consoles_suspended = true; console_list_unlock(); /* @@ -2779,14 +2778,12 @@ void console_suspend_all(void) void console_resume_all(void) { struct console_flush_type ft; - struct console *con; if (!console_suspend_enabled) return; console_list_lock(); - for_each_console(con) - console_srcu_write_flags(con, con->flags & ~CON_SUSPENDED); + consoles_suspended = false; console_list_unlock(); /* @@ -3214,7 +3211,7 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove if ((flags & CON_NBCON) && (ft.nbcon_atomic || ft.nbcon_offload)) continue; - if (!console_is_usable(con, flags, !do_cond_resched)) + if (!console_is_usable(con, flags, !do_cond_resched, consoles_suspended)) continue; any_usable = true; @@ -3604,7 +3601,7 @@ static bool legacy_kthread_should_wakeup(void) if ((flags & CON_NBCON) && (ft.nbcon_atomic || ft.nbcon_offload)) continue; - if (!console_is_usable(con, flags, false)) + if (!console_is_usable(con, flags, false, consoles_suspended)) continue; if (flags & CON_NBCON) { @@ -4165,7 +4162,7 @@ static int unregister_console_locked(struct console *console) if (!console_is_registered_locked(console)) res = -ENODEV; - else if (console_is_usable(console, console->flags, true)) + else if (console_is_usable(console, console->flags, true, consoles_suspended)) __pr_flush(console, 1000, true); /* Disable it unconditionally */ @@ -4445,8 +4442,8 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre * that they make forward progress, so only increment * @diff for usable consoles. */ - if (!console_is_usable(c, flags, true) && - !console_is_usable(c, flags, false)) { + if (!console_is_usable(c, flags, true, consoles_suspended) && + !console_is_usable(c, flags, false, consoles_suspended)) { continue; }
Instead of update a per-console CON_SUSPENDED flag, use the console_list locks to protect this flag. This is also applied to console_is_usable functions, which now also checks if consoles_suspend is set. Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> --- kernel/printk/internal.h | 7 ++++++- kernel/printk/nbcon.c | 8 ++++---- kernel/printk/printk.c | 23 ++++++++++------------- 3 files changed, 20 insertions(+), 18 deletions(-)