[PATCH 0/7] printk cleanup - part 2

[PATCH 0/7] printk cleanup - part 2

[Marcos Paulo de Souza]

The first part can be found here[1]. The proposed changes do not
change the functionality of printk, but were suggestions made by
Petr Mladek. I already have more patches for a part 3 ,but I would like
to see these ones merged first.

I did the testing with VMs, checking suspend and resume cycles, and it worked
as expected.

Thanks for reviewing!

[1]: https://lore.kernel.org/lkml/20250226-printk-renaming-v1-0-0b878577f2e6@suse.com/

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
Marcos Paulo de Souza (7):
      printk: Make console_{suspend,resume} handle CON_SUSPENDED
      printk: Use consoles_suspended flag when suspending/resuming all consoles
      drivers: tty: Check CON_SUSPENDED instead of CON_ENABLED
      drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED
      arch: um: kmsg_dump: Don't check for CON_ENABLED
      debug: kgd_io: Don't check for CON_ENABLED
      printk: Don't check for CON_ENABLED on console_unblank

 arch/um/kernel/kmsg_dump.c  |  2 +-
 drivers/tty/serial/kgdboc.c |  3 ++-
 drivers/tty/tty_io.c        |  2 +-
 kernel/debug/kdb/kdb_io.c   |  2 +-
 kernel/printk/internal.h    |  7 ++++++-
 kernel/printk/nbcon.c       |  8 ++++----
 kernel/printk/printk.c      | 32 +++++++++++++++-----------------
 7 files changed, 30 insertions(+), 26 deletions(-)
---
base-commit: e728cdbeed0667b6261fceeddc91ef1420463ac7
change-id: 20250601-printk-cleanup-part2-38f8d5108099

Best regards,
-- 
Marcos Paulo de Souza <mpdesouza@suse.com>

[PATCH 1/7] printk: Make console_{suspend,resume} handle CON_SUSPENDED

[Marcos Paulo de Souza]

Since commit 9e70a5e109a4 ("printk: Add per-console suspended state") the
CON_SUSPENDED flag was introced, and this flag was being checked on
console_is_usable function, which returns false if the console is suspended.

No functional changes.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 kernel/printk/printk.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1eea80d0648ed3583375cce3dfe60407894d659c..6d3cf488f4261a3dfd8809a5ab7164b218238c13 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3530,7 +3530,7 @@ void console_suspend(struct console *console)
 {
 	__pr_flush(console, 1000, true);
 	console_list_lock();
-	console_srcu_write_flags(console, console->flags & ~CON_ENABLED);
+	console_srcu_write_flags(console, console->flags | CON_SUSPENDED);
 	console_list_unlock();
 
 	/*
@@ -3543,13 +3543,14 @@ void console_suspend(struct console *console)
 }
 EXPORT_SYMBOL(console_suspend);
 
+/* Unset CON_SUSPENDED flag so the console can start printing again. */
 void console_resume(struct console *console)
 {
 	struct console_flush_type ft;
 	bool is_nbcon;
 
 	console_list_lock();
-	console_srcu_write_flags(console, console->flags | CON_ENABLED);
+	console_srcu_write_flags(console, console->flags & ~CON_SUSPENDED);
 	is_nbcon = console->flags & CON_NBCON;
 	console_list_unlock();
 

-- 
2.49.0

Re: [PATCH 1/7] printk: Make console_{suspend,resume} handle CON_SUSPENDED

[Petr Mladek]

On Fri 2025-06-06 23:53:43, Marcos Paulo de Souza wrote:
> Since commit 9e70a5e109a4 ("printk: Add per-console suspended state") the
> CON_SUSPENDED flag was introced, and this flag was being checked on
> console_is_usable function, which returns false if the console is suspended.
> 
> No functional changes.

I double checked potential functional changes. In particular, I
checked where the CON_ENABLED and CON_SUSPENDED flags were used.

Both flags seems to have the same effect in most situations,
for example, in console_is_usable() or console_unblank().

But there seems to be two exceptions: kdb_msg_write() and
show_cons_active(). These two functions check only
the CON_ENABLED flag. And they think that the console is
usable when the flag is set.

The change in this patch would change the behavior of the two
functions during suspend. It is later fixed by the 3rd and 4th
patch. But it might cause regressions during bisections.

It is probably not a big deal because the system is not much
usable during the suspend anyway. But still, I would feel more
comfortable if we prevented the "temporary" regression.

I see two possibilities:

   1. Merge the 3rd and 4th patch into this one. It would change
      the semantic in a single patch.

   2. First update kdb_msg_write() and show_cons_active()
      to check both CON_ENABLE and CON_SUSPENDED flags.

The 1st solution probably makes more sense because we are going
to remove the CON_ENABLE flag in the end. And even the merged
patch is small enough.

Best Regards,
Petr

Re: [PATCH 1/7] printk: Make console_{suspend,resume} handle CON_SUSPENDED

[Marcos Paulo de Souza]

On Thu, 2025-06-12 at 13:46 +0200, Petr Mladek wrote:
> On Fri 2025-06-06 23:53:43, Marcos Paulo de Souza wrote:
> > Since commit 9e70a5e109a4 ("printk: Add per-console suspended
> > state") the
> > CON_SUSPENDED flag was introced, and this flag was being checked on
> > console_is_usable function, which returns false if the console is
> > suspended.
> > 
> > No functional changes.
> 
> I double checked potential functional changes. In particular, I
> checked where the CON_ENABLED and CON_SUSPENDED flags were used.
> 
> Both flags seems to have the same effect in most situations,
> for example, in console_is_usable() or console_unblank().
> 
> But there seems to be two exceptions: kdb_msg_write() and
> show_cons_active(). These two functions check only
> the CON_ENABLED flag. And they think that the console is
> usable when the flag is set.
> 
> The change in this patch would change the behavior of the two
> functions during suspend. It is later fixed by the 3rd and 4th
> patch. But it might cause regressions during bisections.
> 
> It is probably not a big deal because the system is not much
> usable during the suspend anyway. But still, I would feel more
> comfortable if we prevented the "temporary" regression.
> 
> I see two possibilities:
> 
>    1. Merge the 3rd and 4th patch into this one. It would change
>       the semantic in a single patch.

I liked the suggestion, I'll do it in the next revision. Thanks for
looking into it!

> 
>    2. First update kdb_msg_write() and show_cons_active()
>       to check both CON_ENABLE and CON_SUSPENDED flags.
> 
> The 1st solution probably makes more sense because we are going
> to remove the CON_ENABLE flag in the end. And even the merged
> patch is small enough.
> 
> Best Regards,
> Petr

[PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[Marcos Paulo de Souza]

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)) {
 		/* 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;
 			}
 

-- 
2.49.0

Re: [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[Petr Mladek]

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.

Re: [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[John Ogness]

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.

-- 
John Ogness
Linutronix GmbH | Bahnhofstraße 3 | D-88690 Uhldingen-Mühlhofen
Phone: +49 7556 25 999 20; Fax.: +49 7556 25 999 99

Hinweise zum Datenschutz finden Sie hier (Information on data privacy
can be found here): https://linutronix.de/legal/data-protection.php

Linutronix GmbH | Firmensitz (Registered Office): Uhldingen-Mühlhofen |
Registergericht (Registration Court): Amtsgericht Freiburg i.Br., HRB700
806 | Geschäftsführer (Managing Directors): Harry Demas, Heinz Egger,
Thomas Gleixner, Yin Sorrell, Jeffrey Schneiderman

Re: [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[Petr Mladek]

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

Re: [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[John Ogness]

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

Re: [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[Petr Mladek]

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

Re: [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[Marcos Paulo de Souza]

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.

[PATCH 3/7] drivers: tty: Check CON_SUSPENDED instead of CON_ENABLED

[Marcos Paulo de Souza]

All consoles found on for_each_console are registered, meaning that all of
them are CON_ENABLED. The code tries to find an active console, so check if the
console is not suspended instead.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 drivers/tty/tty_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index ca9b7d7bad2b6807b29d3768bb655528ea162816..42f81573d8dfc668b38cd0b1c14962a7370cd954 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3570,7 +3570,7 @@ static ssize_t show_cons_active(struct device *dev,
 			continue;
 		if (!(c->flags & CON_NBCON) && !c->write)
 			continue;
-		if ((c->flags & CON_ENABLED) == 0)
+		if (c->flags & CON_SUSPENDED)
 			continue;
 		cs[i++] = c;
 		if (i >= ARRAY_SIZE(cs))

-- 
2.49.0

Re: [PATCH 3/7] drivers: tty: Check CON_SUSPENDED instead of CON_ENABLED

[Petr Mladek]

On Fri 2025-06-06 23:53:45, Marcos Paulo de Souza wrote:
> All consoles found on for_each_console are registered, meaning that all of
> them are CON_ENABLED. The code tries to find an active console, so check if the
> console is not suspended instead.

This patch "fixes" a behavior change caused by the 1st patch. Please,
merge it into the 1st patch to avoid regressions when bisecting.

Best Regards,
Petr

[PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Marcos Paulo de Souza]

All consoles found on for_each_console are registered, meaning that all of
them are CON_ENABLED. The code tries to find an active console, so check if the
console is not suspended instead.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 drivers/tty/serial/kgdboc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b006b2923583a0d2 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char *opt)
 	console_list_lock();
 	for_each_console(con) {
 		if (con->write && con->read &&
-		    (con->flags & (CON_BOOT | CON_ENABLED)) &&
+		    (con->flags & CON_BOOT) &&
+		    ((con->flags & CON_SUSPENDED) == 0) &&
 		    (!opt || !opt[0] || strcmp(con->name, opt) == 0))
 			break;
 	}

-- 
2.49.0

Re: [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Doug Anderson]

Hi,

On Fri, Jun 6, 2025 at 7:54 PM Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
>
> All consoles found on for_each_console are registered, meaning that all of
> them are CON_ENABLED. The code tries to find an active console, so check if the
> console is not suspended instead.
>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  drivers/tty/serial/kgdboc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b006b2923583a0d2 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char *opt)
>         console_list_lock();
>         for_each_console(con) {
>                 if (con->write && con->read &&
> -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> +                   (con->flags & CON_BOOT) &&
> +                   ((con->flags & CON_SUSPENDED) == 0) &&

I haven't tried running the code, so I could easily be mistaken, but...

...the above doesn't seem like the correct conversion. The old expression was:

(con->flags & (CON_BOOT | CON_ENABLED))

That would evaluate to non-zero (true) if the console was _either_
"boot" or "enabled".

The new expression is is:

(con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0)

That's only true if the console is _both_ "boot" and "not suspended".

-Doug

Re: [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Marcos Paulo de Souza]

On Mon, 2025-06-09 at 13:13 -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Jun 6, 2025 at 7:54 PM Marcos Paulo de Souza
> <mpdesouza@suse.com> wrote:
> > 
> > All consoles found on for_each_console are registered, meaning that
> > all of
> > them are CON_ENABLED. The code tries to find an active console, so
> > check if the
> > console is not suspended instead.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> >  drivers/tty/serial/kgdboc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/kgdboc.c
> > b/drivers/tty/serial/kgdboc.c
> > index
> > 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b
> > 006b2923583a0d2 100644
> > --- a/drivers/tty/serial/kgdboc.c
> > +++ b/drivers/tty/serial/kgdboc.c
> > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char
> > *opt)
> >         console_list_lock();
> >         for_each_console(con) {
> >                 if (con->write && con->read &&
> > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > +                   (con->flags & CON_BOOT) &&
> > +                   ((con->flags & CON_SUSPENDED) == 0) &&
> 
> I haven't tried running the code, so I could easily be mistaken,
> but...
> 
> ...the above doesn't seem like the correct conversion. The old
> expression was:
> 
> (con->flags & (CON_BOOT | CON_ENABLED))
> 
> That would evaluate to non-zero (true) if the console was _either_
> "boot" or "enabled".
> 
> The new expression is is:
> 
> (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0)
> 
> That's only true if the console is _both_ "boot" and "not suspended".

My idea here was that the users of for_each_console would find the
first available console, and by available I would expect them to be
usable. In this case, is there any value for kgdboc to use a console
that is suspended? Would it work in this case?

I never really used kgdboc, but only checking if the console was
enabled (which it's always the case here) was something that needed to
be fixed.

Maybe I'm missing something here as well, so please let me know if I
should remove the new check.

> 
> -Doug

Re: [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Doug Anderson]

Hi,

On Tue, Jun 10, 2025 at 1:03 PM Marcos Paulo de Souza
<mpdesouza@suse.com> wrote:
>
> On Mon, 2025-06-09 at 13:13 -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Jun 6, 2025 at 7:54 PM Marcos Paulo de Souza
> > <mpdesouza@suse.com> wrote:
> > >
> > > All consoles found on for_each_console are registered, meaning that
> > > all of
> > > them are CON_ENABLED. The code tries to find an active console, so
> > > check if the
> > > console is not suspended instead.
> > >
> > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > > ---
> > >  drivers/tty/serial/kgdboc.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/kgdboc.c
> > > b/drivers/tty/serial/kgdboc.c
> > > index
> > > 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b
> > > 006b2923583a0d2 100644
> > > --- a/drivers/tty/serial/kgdboc.c
> > > +++ b/drivers/tty/serial/kgdboc.c
> > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char
> > > *opt)
> > >         console_list_lock();
> > >         for_each_console(con) {
> > >                 if (con->write && con->read &&
> > > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > > +                   (con->flags & CON_BOOT) &&
> > > +                   ((con->flags & CON_SUSPENDED) == 0) &&
> >
> > I haven't tried running the code, so I could easily be mistaken,
> > but...
> >
> > ...the above doesn't seem like the correct conversion. The old
> > expression was:
> >
> > (con->flags & (CON_BOOT | CON_ENABLED))
> >
> > That would evaluate to non-zero (true) if the console was _either_
> > "boot" or "enabled".
> >
> > The new expression is is:
> >
> > (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0)
> >
> > That's only true if the console is _both_ "boot" and "not suspended".
>
> My idea here was that the users of for_each_console would find the
> first available console, and by available I would expect them to be
> usable. In this case, is there any value for kgdboc to use a console
> that is suspended? Would it work in this case?
>
> I never really used kgdboc, but only checking if the console was
> enabled (which it's always the case here) was something that needed to
> be fixed.
>
> Maybe I'm missing something here as well, so please let me know if I
> should remove the new check.

So it's been 5 years since I wrote the code, but reading that I was
checking for:

  (con->flags & (CON_BOOT | CON_ENABLED))

Makes me believe that this was the case when I wrote the code:
1. Early boot consoles (earlycon) were not marked as CON_ENABLED but
were instead marked as CON_BOOT.
2. Once consoles became non-early they were moved to CON_ENABLED.

...and the code was basically looking for any consoles with a matching
name that were either boot consoles or normal/enabled consoles.

Is that a plausible theory? It's also possible that I just was
confused when I code things up and that I really meant to write:

((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED))

...AKA that I wanted consoles that were BOOT _and_ ENABLED.

In any case, I booted up the current mainline and I put a printout here. I saw:

[    0.000000] kgdboc: DOUG: console qcom_geni has flags 0x0000000f

So that means that both BOOT and ENABLED were set. That makes me feel
like it's plausible that I was confused and really meant BOOT _and_
ENABLED. I didn't spend time trying to figure out how to build/boot an
old kernel to check, though.

In any case, given my test then I think your change should be fine.
Given that it does change the boolean logic, it seems like that
deserves a mention in the commit message.

-Doug

Re: [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Petr Mladek]

On Tue 2025-06-10 16:18:22, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jun 10, 2025 at 1:03 PM Marcos Paulo de Souza
> <mpdesouza@suse.com> wrote:
> >
> > On Mon, 2025-06-09 at 13:13 -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Fri, Jun 6, 2025 at 7:54 PM Marcos Paulo de Souza
> > > <mpdesouza@suse.com> wrote:
> > > >
> > > > All consoles found on for_each_console are registered, meaning that
> > > > all of
> > > > them are CON_ENABLED. The code tries to find an active console, so
> > > > check if the
> > > > console is not suspended instead.
> > > >
> > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > > > ---
> > > >  drivers/tty/serial/kgdboc.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/tty/serial/kgdboc.c
> > > > b/drivers/tty/serial/kgdboc.c
> > > > index
> > > > 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b
> > > > 006b2923583a0d2 100644
> > > > --- a/drivers/tty/serial/kgdboc.c
> > > > +++ b/drivers/tty/serial/kgdboc.c
> > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char
> > > > *opt)
> > > >         console_list_lock();
> > > >         for_each_console(con) {
> > > >                 if (con->write && con->read &&
> > > > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > > > +                   (con->flags & CON_BOOT) &&
> > > > +                   ((con->flags & CON_SUSPENDED) == 0) &&
> > >
> > > I haven't tried running the code, so I could easily be mistaken,
> > > but...
> > >
> > > ...the above doesn't seem like the correct conversion. The old
> > > expression was:
> > >
> > > (con->flags & (CON_BOOT | CON_ENABLED))
> > >
> > > That would evaluate to non-zero (true) if the console was _either_
> > > "boot" or "enabled".
> > >
> > > The new expression is is:
> > >
> > > (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0)
> > >
> > > That's only true if the console is _both_ "boot" and "not suspended".
> >
> > My idea here was that the users of for_each_console would find the
> > first available console, and by available I would expect them to be
> > usable. In this case, is there any value for kgdboc to use a console
> > that is suspended? Would it work in this case?
> >
> > I never really used kgdboc, but only checking if the console was
> > enabled (which it's always the case here) was something that needed to
> > be fixed.
> >
> > Maybe I'm missing something here as well, so please let me know if I
> > should remove the new check.
> 
> So it's been 5 years since I wrote the code, but reading that I was
> checking for:
> 
>   (con->flags & (CON_BOOT | CON_ENABLED))
> 
> Makes me believe that this was the case when I wrote the code:
> 1. Early boot consoles (earlycon) were not marked as CON_ENABLED but
> were instead marked as CON_BOOT.
> 2. Once consoles became non-early they were moved to CON_ENABLED.
> 
> ...and the code was basically looking for any consoles with a matching
> name that were either boot consoles or normal/enabled consoles.
> 
> Is that a plausible theory? It's also possible that I just was
> confused when I code things up and that I really meant to write:

It is easy to get confused by the register_console() code. And
it has been even worse some years ago.

Anyway, the current code sets CON_ENABLED for all registered
consoles, including CON_BOOT consoles. The flag is cleared only
by console_suspend()[*] or unregister_console().

IMHO, kgdboc_earlycon_init() does not need to care about
console_suspend() because the kernel could not be suspended
during boot. Does this makes sense?

Also it does not need to care about unregister_console() because
the unregistered console won't be listed by for_each_console().

[*] The 1st patch in this patchset updates console_suspend()
    to set CON_SUSPENDED instead of clearing CON_ENABLED.
    It helps to make it clear that it is suspend-related.


Resume:

I would remove the check of CON_ENABLED or CON_SUSPENDED
from kgdboc_earlycon_init() completely.

IMHO, we should keep the check of CON_BOOT because we do not
want to register "normal" console drivers as kgdboc_earlycon_io_ops.
It is later removed by kgdboc_earlycon_deinit(). I guess
that the code is not ready to handle a takeover from normal
to normal (even the same) console driver.

To make it clear, I would use:

	for_each_console(con) {
		if (con->write && con->read &&
		    (con->flags & CON_BOOT) &&
		    (!opt || !opt[0] || strcmp(con->name, opt) == 0))
			break;
	}

And I would do this change as the 1st patch in the patchset
before we change the flag modified by console_suspend().

Best Regards,
Petr

Re: [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Doug Anderson]

Hi,

On Thu, Jun 12, 2025 at 6:57 AM Petr Mladek <pmladek@suse.com> wrote:
>
> > > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char
> > > > > *opt)
> > > > >         console_list_lock();
> > > > >         for_each_console(con) {
> > > > >                 if (con->write && con->read &&
> > > > > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > > > > +                   (con->flags & CON_BOOT) &&
> > > > > +                   ((con->flags & CON_SUSPENDED) == 0) &&
> > > >
> > > > I haven't tried running the code, so I could easily be mistaken,
> > > > but...
> > > >
> > > > ...the above doesn't seem like the correct conversion. The old
> > > > expression was:
> > > >
> > > > (con->flags & (CON_BOOT | CON_ENABLED))
> > > >
> > > > That would evaluate to non-zero (true) if the console was _either_
> > > > "boot" or "enabled".
> > > >
> > > > The new expression is is:
> > > >
> > > > (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0)
> > > >
> > > > That's only true if the console is _both_ "boot" and "not suspended".
> > >
> > > My idea here was that the users of for_each_console would find the
> > > first available console, and by available I would expect them to be
> > > usable. In this case, is there any value for kgdboc to use a console
> > > that is suspended? Would it work in this case?
> > >
> > > I never really used kgdboc, but only checking if the console was
> > > enabled (which it's always the case here) was something that needed to
> > > be fixed.
> > >
> > > Maybe I'm missing something here as well, so please let me know if I
> > > should remove the new check.
> >
> > So it's been 5 years since I wrote the code, but reading that I was
> > checking for:
> >
> >   (con->flags & (CON_BOOT | CON_ENABLED))
> >
> > Makes me believe that this was the case when I wrote the code:
> > 1. Early boot consoles (earlycon) were not marked as CON_ENABLED but
> > were instead marked as CON_BOOT.
> > 2. Once consoles became non-early they were moved to CON_ENABLED.
> >
> > ...and the code was basically looking for any consoles with a matching
> > name that were either boot consoles or normal/enabled consoles.
> >
> > Is that a plausible theory? It's also possible that I just was
> > confused when I code things up and that I really meant to write:
>
> It is easy to get confused by the register_console() code. And
> it has been even worse some years ago.
>
> Anyway, the current code sets CON_ENABLED for all registered
> consoles, including CON_BOOT consoles. The flag is cleared only
> by console_suspend()[*] or unregister_console().
>
> IMHO, kgdboc_earlycon_init() does not need to care about
> console_suspend() because the kernel could not be suspended
> during boot. Does this makes sense?

Yeah, makes sense to me.


> Also it does not need to care about unregister_console() because
> the unregistered console won't be listed by for_each_console().
>
> [*] The 1st patch in this patchset updates console_suspend()
>     to set CON_SUSPENDED instead of clearing CON_ENABLED.
>     It helps to make it clear that it is suspend-related.
>
>
> Resume:
>
> I would remove the check of CON_ENABLED or CON_SUSPENDED
> from kgdboc_earlycon_init() completely.
>
> IMHO, we should keep the check of CON_BOOT because we do not
> want to register "normal" console drivers as kgdboc_earlycon_io_ops.
> It is later removed by kgdboc_earlycon_deinit(). I guess
> that the code is not ready to handle a takeover from normal
> to normal (even the same) console driver.

I'm not sure I understand your last sentence there. In general the
code handling all of the possible handoff (or lack of handoff) cases
between kgdboc_earlycon and regular kgdboc is pretty wacky. At one
point I thought through it all and tried to test as many scenarios as
I could and I seem to remember trying to model some of the behavior on
how earlycon worked. If something looks broken here then let me know.


> To make it clear, I would use:
>
>         for_each_console(con) {
>                 if (con->write && con->read &&
>                     (con->flags & CON_BOOT) &&
>                     (!opt || !opt[0] || strcmp(con->name, opt) == 0))
>                         break;
>         }
>
> And I would do this change as the 1st patch in the patchset
> before we change the flag modified by console_suspend().

Seems OK to me.

-Doug

Re: [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Petr Mladek]

On Thu 2025-06-12 16:16:09, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 12, 2025 at 6:57 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > > > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char
> > > > > > *opt)
> > > > > >         console_list_lock();
> > > > > >         for_each_console(con) {
> > > > > >                 if (con->write && con->read &&
> > > > > > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > > > > > +                   (con->flags & CON_BOOT) &&
> > > > > > +                   ((con->flags & CON_SUSPENDED) == 0) &&
> > > > >
> > > > > I haven't tried running the code, so I could easily be mistaken,
> > > > > but...
> > > > >
> > > > > ...the above doesn't seem like the correct conversion. The old
> > > > > expression was:
> > > > >
> > > > > (con->flags & (CON_BOOT | CON_ENABLED))
> > > > >
> > It is easy to get confused by the register_console() code. And
> > it has been even worse some years ago.
> >
> > Anyway, the current code sets CON_ENABLED for all registered
> > consoles, including CON_BOOT consoles. The flag is cleared only
> > by console_suspend()[*] or unregister_console().
> >
> > IMHO, kgdboc_earlycon_init() does not need to care about
> > console_suspend() because the kernel could not be suspended
> > during boot. Does this makes sense?
> 
> Yeah, makes sense to me.
> 
> > Resume:
> >
> > I would remove the check of CON_ENABLED or CON_SUSPENDED
> > from kgdboc_earlycon_init() completely.
> >
> > IMHO, we should keep the check of CON_BOOT because we do not
> > want to register "normal" console drivers as kgdboc_earlycon_io_ops.
> > It is later removed by kgdboc_earlycon_deinit(). I guess
> > that the code is not ready to handle a takeover from normal
> > to normal (even the same) console driver.
> 
> I'm not sure I understand your last sentence there. In general the
> code handling all of the possible handoff (or lack of handoff) cases
> between kgdboc_earlycon and regular kgdboc is pretty wacky. At one
> point I thought through it all and tried to test as many scenarios as
> I could and I seem to remember trying to model some of the behavior on
> how earlycon worked. If something looks broken here then let me know.

Later update: The code is safe. The scenario below does not exist,
	      see the "WAIT:" section below.


I am not familiar with the kgdb init code. I thought about the
following scenario:

1. kgdboc_earlycon_init() registers some struct console via

	kgdboc_earlycon_io_ops.cons = con;
	pr_info("Going to register kgdb with earlycon '%s'\n", con->name);
	if (kgdb_register_io_module(&kgdboc_earlycon_io_ops) != 0) {

   and sets

		earlycon_orig_exit = con->exit;
		con->exit = kgdboc_earlycon_deferred_exit;


2. Later, configure_kgdboc() would find some "preferred" console
   and register it via

	for_each_console_srcu(cons) {
		int idx;
		if (cons->device && cons->device(cons, &idx) == p &&
		    idx == tty_line) {
			kgdboc_io_ops.cons = cons;
[...]
	err = kgdb_register_io_module(&kgdboc_io_ops);

   , where kgdb_register_io_module would call deinit for the
   previously registered kgdboc_earlycon_io_ops:

	if (old_dbg_io_ops) {
		old_dbg_io_ops->deinit();
		return 0;
	}

   , where kgdboc_earlycon_deinit() might call the .exit() callback

		kgdboc_earlycon_io_ops.cons->exit(kgdboc_earlycon_io_ops.cons);


BANG: If both "kgdboc_earlycon_io_ops" and "kgdboc_io_ops" pointed to
      the same struct console then this might call .exit() callback
      for a console which is still being used.

      But I am wrong, see below.

WAIT:

I have got all the pieces together when writing this mail).
I see that the code is safe, namely:

static void kgdboc_earlycon_deinit(void)
{
	if (!kgdboc_earlycon_io_ops.cons)
		return;

	if (kgdboc_earlycon_io_ops.cons->exit == kgdboc_earlycon_deferred_exit)
		/*
		 * kgdboc_earlycon is exiting but original boot console exit
		 * was never called (AKA kgdboc_earlycon_deferred_exit()
		 * didn't ever run).  Undo our trap.
		 */
		kgdboc_earlycon_io_ops.cons->exit = earlycon_orig_exit;
	else if (kgdboc_earlycon_io_ops.cons->exit)
		/*
		 * We skipped calling the exit() routine so we could try to
		 * keep using the boot console even after it went away.  We're
		 * finally done so call the function now.
		 */
		kgdboc_earlycon_io_ops.cons->exit(kgdboc_earlycon_io_ops.cons);

	kgdboc_earlycon_io_ops.cons = NULL;
}

It calls kgdboc_earlycon_io_ops.cons->exit() only when
unregister_console() tried to call the original con->exit()
which was reassigned to kgdboc_earlycon_deferred_exit()...

Updated resume:

It looks to me that even normal console can be used by
kgdboc_earlycon_io_ops and we could remove even the check
of the CON_BOOT flag.

My expectation:

If a "struct console" is registered then the driver is used
by printk() and it should be safe even for kgdboc_earlycon,
as long as it has both "con->write" and "con->read" callbacks.

So the check in kgdboc_earlycon_init() might be:

	for_each_console(con) {
		if (con->write && con->read &&
		    (!opt || !opt[0] || strcmp(con->name, opt) == 0))
			break;
	}

Heh, I hope that you were able to follow my thoughts and I did not
create even bigger confusion.

Best Regards,
Petr

Re: [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Doug Anderson]

Hi,

On Fri, Jun 13, 2025 at 3:52 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Thu 2025-06-12 16:16:09, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Jun 12, 2025 at 6:57 AM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > > > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char
> > > > > > > *opt)
> > > > > > >         console_list_lock();
> > > > > > >         for_each_console(con) {
> > > > > > >                 if (con->write && con->read &&
> > > > > > > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > > > > > > +                   (con->flags & CON_BOOT) &&
> > > > > > > +                   ((con->flags & CON_SUSPENDED) == 0) &&
> > > > > >
> > > > > > I haven't tried running the code, so I could easily be mistaken,
> > > > > > but...
> > > > > >
> > > > > > ...the above doesn't seem like the correct conversion. The old
> > > > > > expression was:
> > > > > >
> > > > > > (con->flags & (CON_BOOT | CON_ENABLED))
> > > > > >
> > > It is easy to get confused by the register_console() code. And
> > > it has been even worse some years ago.
> > >
> > > Anyway, the current code sets CON_ENABLED for all registered
> > > consoles, including CON_BOOT consoles. The flag is cleared only
> > > by console_suspend()[*] or unregister_console().
> > >
> > > IMHO, kgdboc_earlycon_init() does not need to care about
> > > console_suspend() because the kernel could not be suspended
> > > during boot. Does this makes sense?
> >
> > Yeah, makes sense to me.
> >
> > > Resume:
> > >
> > > I would remove the check of CON_ENABLED or CON_SUSPENDED
> > > from kgdboc_earlycon_init() completely.
> > >
> > > IMHO, we should keep the check of CON_BOOT because we do not
> > > want to register "normal" console drivers as kgdboc_earlycon_io_ops.
> > > It is later removed by kgdboc_earlycon_deinit(). I guess
> > > that the code is not ready to handle a takeover from normal
> > > to normal (even the same) console driver.
> >
> > I'm not sure I understand your last sentence there. In general the
> > code handling all of the possible handoff (or lack of handoff) cases
> > between kgdboc_earlycon and regular kgdboc is pretty wacky. At one
> > point I thought through it all and tried to test as many scenarios as
> > I could and I seem to remember trying to model some of the behavior on
> > how earlycon worked. If something looks broken here then let me know.
>
> Later update: The code is safe. The scenario below does not exist,
>               see the "WAIT:" section below.
>
>
> I am not familiar with the kgdb init code. I thought about the
> following scenario:
>
> 1. kgdboc_earlycon_init() registers some struct console via
>
>         kgdboc_earlycon_io_ops.cons = con;
>         pr_info("Going to register kgdb with earlycon '%s'\n", con->name);
>         if (kgdb_register_io_module(&kgdboc_earlycon_io_ops) != 0) {
>
>    and sets
>
>                 earlycon_orig_exit = con->exit;
>                 con->exit = kgdboc_earlycon_deferred_exit;
>
>
> 2. Later, configure_kgdboc() would find some "preferred" console
>    and register it via
>
>         for_each_console_srcu(cons) {
>                 int idx;
>                 if (cons->device && cons->device(cons, &idx) == p &&
>                     idx == tty_line) {
>                         kgdboc_io_ops.cons = cons;
> [...]
>         err = kgdb_register_io_module(&kgdboc_io_ops);
>
>    , where kgdb_register_io_module would call deinit for the
>    previously registered kgdboc_earlycon_io_ops:
>
>         if (old_dbg_io_ops) {
>                 old_dbg_io_ops->deinit();
>                 return 0;
>         }
>
>    , where kgdboc_earlycon_deinit() might call the .exit() callback
>
>                 kgdboc_earlycon_io_ops.cons->exit(kgdboc_earlycon_io_ops.cons);
>
>
> BANG: If both "kgdboc_earlycon_io_ops" and "kgdboc_io_ops" pointed to
>       the same struct console then this might call .exit() callback
>       for a console which is still being used.
>
>       But I am wrong, see below.
>
> WAIT:
>
> I have got all the pieces together when writing this mail).
> I see that the code is safe, namely:
>
> static void kgdboc_earlycon_deinit(void)
> {
>         if (!kgdboc_earlycon_io_ops.cons)
>                 return;
>
>         if (kgdboc_earlycon_io_ops.cons->exit == kgdboc_earlycon_deferred_exit)
>                 /*
>                  * kgdboc_earlycon is exiting but original boot console exit
>                  * was never called (AKA kgdboc_earlycon_deferred_exit()
>                  * didn't ever run).  Undo our trap.
>                  */
>                 kgdboc_earlycon_io_ops.cons->exit = earlycon_orig_exit;
>         else if (kgdboc_earlycon_io_ops.cons->exit)
>                 /*
>                  * We skipped calling the exit() routine so we could try to
>                  * keep using the boot console even after it went away.  We're
>                  * finally done so call the function now.
>                  */
>                 kgdboc_earlycon_io_ops.cons->exit(kgdboc_earlycon_io_ops.cons);
>
>         kgdboc_earlycon_io_ops.cons = NULL;
> }
>
> It calls kgdboc_earlycon_io_ops.cons->exit() only when
> unregister_console() tried to call the original con->exit()
> which was reassigned to kgdboc_earlycon_deferred_exit()...
>
> Updated resume:
>
> It looks to me that even normal console can be used by
> kgdboc_earlycon_io_ops and we could remove even the check
> of the CON_BOOT flag.
>
> My expectation:
>
> If a "struct console" is registered then the driver is used
> by printk() and it should be safe even for kgdboc_earlycon,
> as long as it has both "con->write" and "con->read" callbacks.
>
> So the check in kgdboc_earlycon_init() might be:
>
>         for_each_console(con) {
>                 if (con->write && con->read &&
>                     (!opt || !opt[0] || strcmp(con->name, opt) == 0))
>                         break;
>         }
>
> Heh, I hope that you were able to follow my thoughts and I did not
> create even bigger confusion.

I didn't go back to the code and trace through every step you made,
but it sounds like the code looks OK even if it's a bit convoluted
(sorry about that! at least it's commented!).

I agree with your final suggestion for the check. That has the side
effect of also being less of a change from what we had before. Because
of how the code was written before, it would have allowed any console
because it checked for "enabled or boot" and all consoles were
enabled. So just getting rid of the check for flags seems reasonable
to me.

-Doug

[PATCH 5/7] arch: um: kmsg_dump: Don't check for CON_ENABLED

[Marcos Paulo de Souza]

All consoles found on for_each_console are registered, meaning that all of
them are CON_ENABLED. The code tries to find an active console, so check if the
console is not suspended instead.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 arch/um/kernel/kmsg_dump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
index 4190211752726593dd2847f66efd9d3a61cea982..f3025b2a813453f479d720618c630bee135d4e08 100644
--- a/arch/um/kernel/kmsg_dump.c
+++ b/arch/um/kernel/kmsg_dump.c
@@ -31,7 +31,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
 		 * expected to output the crash information.
 		 */
 		if (strcmp(con->name, "ttynull") != 0 &&
-		    (console_srcu_read_flags(con) & CON_ENABLED)) {
+		    (console_srcu_read_flags(con) & CON_SUSPENDED) == 0) {
 			break;
 		}
 	}

-- 
2.49.0

Re: [PATCH 5/7] arch: um: kmsg_dump: Don't check for CON_ENABLED

[Petr Mladek]

On Fri 2025-06-06 23:53:47, Marcos Paulo de Souza wrote:
> All consoles found on for_each_console are registered, meaning that all of
> them are CON_ENABLED. The code tries to find an active console, so check if the
> console is not suspended instead.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  arch/um/kernel/kmsg_dump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
> index 4190211752726593dd2847f66efd9d3a61cea982..f3025b2a813453f479d720618c630bee135d4e08 100644
> --- a/arch/um/kernel/kmsg_dump.c
> +++ b/arch/um/kernel/kmsg_dump.c
> @@ -31,7 +31,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
>  		 * expected to output the crash information.
>  		 */
>  		if (strcmp(con->name, "ttynull") != 0 &&
> -		    (console_srcu_read_flags(con) & CON_ENABLED)) {
> +		    (console_srcu_read_flags(con) & CON_SUSPENDED) == 0) {
>  			break;

I think that we should actually replace the check of the
CON_ENABLE/CON_SUSPENDED flag with

		is_console_usable(con, console_srcu_read_flags(con), true)

And it should be done at the beginning of the patchset before
changing the semantic of the flags.

Motivation:

There is the following comment at the beginning of the function:

	/*
	 * If no consoles are available to output crash information, dump
	 * the kmsg buffer to stdout.
	 */

The if-condition checks for:

  + "ttynull" because this special console does not show any messages
    by definition

  + disabled/suspended consoles; note that this patchset is replacing
    CON_ENABLED with CON_SUSPENDED flag because it the state is
    changed during suspend.

But it should check also for:

  + whether the console is NBCON_console and does not have con->write_atomic
    because such a console would not be able to show the messages
    in panic().

And it should also check the global "consoles_suspended" flag. Because
consoles won't show anything when it is set.

And all these is already done by "is_console_usable()" except for
the check of "ttynull" which is very special.

How does the sound, please?

Best Regards,
Petr

>  		}
>  	}
> 
> -- 
> 2.49.0

Re: [PATCH 5/7] arch: um: kmsg_dump: Don't check for CON_ENABLED

[John Ogness]

On 2025-06-16, Petr Mladek <pmladek@suse.com> wrote:
> On Fri 2025-06-06 23:53:47, Marcos Paulo de Souza wrote:
>> All consoles found on for_each_console are registered, meaning that all of
>> them are CON_ENABLED. The code tries to find an active console, so check if the
>> console is not suspended instead.
>> 
>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
>> ---
>>  arch/um/kernel/kmsg_dump.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
>> index 4190211752726593dd2847f66efd9d3a61cea982..f3025b2a813453f479d720618c630bee135d4e08 100644
>> --- a/arch/um/kernel/kmsg_dump.c
>> +++ b/arch/um/kernel/kmsg_dump.c
>> @@ -31,7 +31,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
>>  		 * expected to output the crash information.
>>  		 */
>>  		if (strcmp(con->name, "ttynull") != 0 &&
>> -		    (console_srcu_read_flags(con) & CON_ENABLED)) {
>> +		    (console_srcu_read_flags(con) & CON_SUSPENDED) == 0) {
>>  			break;
>
> I think that we should actually replace the check of the
> CON_ENABLE/CON_SUSPENDED flag with
>
> 		is_console_usable(con, console_srcu_read_flags(con), true)
>
> And it should be done at the beginning of the patchset before
> changing the semantic of the flags.
>
> Motivation:
>
> There is the following comment at the beginning of the function:
>
> 	/*
> 	 * If no consoles are available to output crash information, dump
> 	 * the kmsg buffer to stdout.
> 	 */
>
> The if-condition checks for:
>
>   + "ttynull" because this special console does not show any messages
>     by definition
>
>   + disabled/suspended consoles; note that this patchset is replacing
>     CON_ENABLED with CON_SUSPENDED flag because it the state is
>     changed during suspend.
>
> But it should check also for:
>
>   + whether the console is NBCON_console and does not have con->write_atomic
>     because such a console would not be able to show the messages
>     in panic().
>
> And it should also check the global "consoles_suspended" flag. Because
> consoles won't show anything when it is set.
>
> And all these is already done by "is_console_usable()" except for
> the check of "ttynull" which is very special.
>
> How does the sound, please?

FWIW, I agree with all these points.

John

[PATCH 6/7] debug: kgd_io: Don't check for CON_ENABLED

[Marcos Paulo de Souza]

All consoles found on for_each_console_srcu are registered, meaning that all of
them are CON_ENABLED. The code tries to find an active console, so check if the
console is not suspended instead.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 kernel/debug/kdb/kdb_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 9b11b10b120cf07e451a7a4d92ce50f9a6c066b2..cdc1ee81d7332a9a00b967af719939f438f26cef 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -589,7 +589,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
 	 */
 	cookie = console_srcu_read_lock();
 	for_each_console_srcu(c) {
-		if (!(console_srcu_read_flags(c) & CON_ENABLED))
+		if (console_srcu_read_flags(c) & CON_SUSPENDED)
 			continue;
 		if (c == dbg_io_ops->cons)
 			continue;

-- 
2.49.0

Re: [PATCH 6/7] debug: kgd_io: Don't check for CON_ENABLED

[Petr Mladek]

On Fri 2025-06-06 23:53:48, Marcos Paulo de Souza wrote:
> All consoles found on for_each_console_srcu are registered, meaning that all of
> them are CON_ENABLED. The code tries to find an active console, so check if the
> console is not suspended instead.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  kernel/debug/kdb/kdb_io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 9b11b10b120cf07e451a7a4d92ce50f9a6c066b2..cdc1ee81d7332a9a00b967af719939f438f26cef 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -589,7 +589,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  	 */
>  	cookie = console_srcu_read_lock();
>  	for_each_console_srcu(c) {
> -		if (!(console_srcu_read_flags(c) & CON_ENABLED))
> +		if (console_srcu_read_flags(c) & CON_SUSPENDED)
>  			continue;

I think that this is similar to the 5th patch. We should check
here is_console_usable(con, console_srcu_read_flags(c), true)
because it checks more conditions:

     + the global console_suspended flag. The consoles drivers should
       not be used when it is set...

     + whether NBCON console driver has con->write_atomic

and we should also fix kdb_msg_write() to actually use
con->write_atomic() when it is a NBCON console driver.
There is hard-coded con->write() at the moment.

But it might get more complicated. It would be nice to do it correctly
and use con->write_atomit() only when nbcon_context_try_acquire()
succeeds. We probably should use a context with NBCON_PRIO_EMERGENCY.

And this should be fixed at the beginning of the patchset because
it actually fixes the support of the new NBCON console drivers.

Best Regards,
Petr

>  		if (c == dbg_io_ops->cons)
>  			continue;
> 
> -- 
> 2.49.0

Re: [PATCH 6/7] debug: kgd_io: Don't check for CON_ENABLED

[Marcos Paulo de Souza]

On Mon, 2025-06-16 at 15:56 +0200, Petr Mladek wrote:
> On Fri 2025-06-06 23:53:48, Marcos Paulo de Souza wrote:
> > All consoles found on for_each_console_srcu are registered, meaning
> > that all of
> > them are CON_ENABLED. The code tries to find an active console, so
> > check if the
> > console is not suspended instead.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> >  kernel/debug/kdb/kdb_io.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index
> > 9b11b10b120cf07e451a7a4d92ce50f9a6c066b2..cdc1ee81d7332a9a00b967af7
> > 19939f438f26cef 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -589,7 +589,7 @@ static void kdb_msg_write(const char *msg, int
> > msg_len)
> >  	 */
> >  	cookie = console_srcu_read_lock();
> >  	for_each_console_srcu(c) {
> > -		if (!(console_srcu_read_flags(c) & CON_ENABLED))
> > +		if (console_srcu_read_flags(c) & CON_SUSPENDED)
> >  			continue;
> 
> I think that this is similar to the 5th patch. We should check
> here is_console_usable(con, console_srcu_read_flags(c), true)
> because it checks more conditions:
> 
>      + the global console_suspended flag. The consoles drivers should
>        not be used when it is set...
> 
>      + whether NBCON console driver has con->write_atomic
> 

Makes sense, I'll work on it first then.

> and we should also fix kdb_msg_write() to actually use
> con->write_atomic() when it is a NBCON console driver.
> There is hard-coded con->write() at the moment.
> 
> But it might get more complicated. It would be nice to do it
> correctly
> and use con->write_atomit() only when nbcon_context_try_acquire()
> succeeds. We probably should use a context with NBCON_PRIO_EMERGENCY.
> 

I'll check how that can be done by looking at Docs.

> And this should be fixed at the beginning of the patchset because
> it actually fixes the support of the new NBCON console drivers.

I'll try to send patches for this case first, and then come back with
this series once this one is fixed, so I can start converting the other
places like you suggested later.

Thanks a lot for the suggestion!

> 
> Best Regards,
> Petr
> 
> >  		if (c == dbg_io_ops->cons)
> >  			continue;
> > 
> > -- 
> > 2.49.0

[PATCH 7/7] printk: Don't check for CON_ENABLED on console_unblank

[Marcos Paulo de Souza]

All consoles found on for_each_console_srcu are registered, meaning that all of
them are already CON_ENABLEDed.

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 kernel/printk/printk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 658acf92aa3d2a3d1e294b7e17e5ee96d8169afe..8074a0f73691cfc5f637361048097ace1545c7c0 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3360,7 +3360,7 @@ void console_unblank(void)
 		if (flags & CON_SUSPENDED)
 			continue;
 
-		if ((flags & CON_ENABLED) && c->unblank) {
+		if (c->unblank) {
 			found_unblank = true;
 			break;
 		}
@@ -3402,7 +3402,7 @@ void console_unblank(void)
 		if (flags & CON_SUSPENDED)
 			continue;
 
-		if ((flags & CON_ENABLED) && c->unblank)
+		if (c->unblank)
 			c->unblank();
 	}
 	console_srcu_read_unlock(cookie);

-- 
2.49.0

Re: [PATCH 7/7] printk: Don't check for CON_ENABLED on console_unblank

[Petr Mladek]

On Fri 2025-06-06 23:53:49, Marcos Paulo de Souza wrote:
> All consoles found on for_each_console_srcu are registered, meaning that all of
> them are already CON_ENABLEDed.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  kernel/printk/printk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 658acf92aa3d2a3d1e294b7e17e5ee96d8169afe..8074a0f73691cfc5f637361048097ace1545c7c0 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3360,7 +3360,7 @@ void console_unblank(void)
>  		if (flags & CON_SUSPENDED)
>  			continue;
>  
> -		if ((flags & CON_ENABLED) && c->unblank) {
> +		if (c->unblank) {

It might actually make sense to check is_console_usable() here.
The reason is similar as for the 5th and 6th patch, see
https://lore.kernel.org/r/aFAdGas9yGB4zhqc@pathway.suse.cz
https://lore.kernel.org/r/aFAiq3IEic8DuATR@pathway.suse.cz


>  			found_unblank = true;
>  			break;
>  		}
> @@ -3402,7 +3402,7 @@ void console_unblank(void)
>  		if (flags & CON_SUSPENDED)
>  			continue;
>  
> -		if ((flags & CON_ENABLED) && c->unblank)
> +		if (c->unblank)
>  			c->unblank();

Same here.

>  	}
>  	console_srcu_read_unlock(cookie);
> 
> -- 
> 2.49.0

Re: [PATCH 7/7] printk: Don't check for CON_ENABLED on console_unblank

[Geert Uytterhoeven]

Hi Petr,

On Mon, 16 Jun 2025 at 17:27, Petr Mladek <pmladek@suse.com> wrote:
> On Fri 2025-06-06 23:53:49, Marcos Paulo de Souza wrote:
> > All consoles found on for_each_console_srcu are registered, meaning that all of
> > them are already CON_ENABLEDed.
> >
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> >  kernel/printk/printk.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 658acf92aa3d2a3d1e294b7e17e5ee96d8169afe..8074a0f73691cfc5f637361048097ace1545c7c0 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3360,7 +3360,7 @@ void console_unblank(void)
> >               if (flags & CON_SUSPENDED)
> >                       continue;
> >
> > -             if ((flags & CON_ENABLED) && c->unblank) {
> > +             if (c->unblank) {
>
> It might actually make sense to check is_console_usable() here.

My first thought was: one more to convert to a better name!
But the actual function is already called console_is_usable() ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds