Message-ID: <aSgeqM3DWvR8-cMY@pathway.suse.cz>
Date: Thu, 27 Nov 2025 10:49:28 +0100
From: Petr Mladek <pmladek@...e.com>
To: Marcos Paulo de Souza <mpdesouza@...e.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Jason Wessel <jason.wessel@...driver.com>,
	Daniel Thompson <danielt@...nel.org>,
	Douglas Anderson <dianders@...omium.org>,
	Richard Weinberger <richard@....at>,
	Anton Ivanov <anton.ivanov@...bridgegreys.com>,
	Johannes Berg <johannes@...solutions.net>,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	kgdb-bugreport@...ts.sourceforge.net, linux-um@...ts.infradead.org
Subject: Re: [PATCH v2 4/4] printk: Make console_{suspend,resume} handle
 CON_SUSPENDED

On Fri 2025-11-21 15:50:36, 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.
> 
> To make the behavior consistent, change show_cons_active to look for
> consoles that are not suspended, instead of checking CON_ENABLED.
> 
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3554,7 +3554,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)

I believe that we could and should replace

		if (!(c->flags & CON_NBCON) && !c->write)
			continue;
		if (c->flags & CON_SUSPENDED)
			continue;

with

		if (!console_is_usable(c, c->flags, true) &&
		    !console_is_usable(c, c->flags, false))
			continue;

It would make the value compatible with all other callers/users of
the console drivers.

The variant using two console_is_usable() calls with "true/false"
parameters is inspited by __pr_flush().

>  			continue;
>  		cs[i++] = c;
>  		if (i >= ARRAY_SIZE(cs))
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index fed98a18e830..fe7c956f73bd 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3542,7 +3542,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);

This is the same flag which is set also by the console_suspend_all()
API. Now, as discussed at
https://lore.kernel.org/lkml/844j4lepak.fsf@jogness.linutronix.de/

   + console_suspend()/console_resume() API is used by few console
     drivers to suspend the console when the related HW device
     gets suspended.

   + console_suspend_all()/console_resume_all() is used by
     the power management subsystem to call down/up all consoles
     when the system is going down/up. It is a big hammer approach.

We need to distinguish the two APIs so that console drivers which were
suspended by both APIs stay suspended until they get resumed by both
APIs. I mean:

	// This should suspend all consoles unless it is not disabled
	// by "no_console_suspend" API.
	console_suspend_all();
	// This suspends @con even when "no_console_suspend" parameter
	// is used. It is needed because the HW is going to be suspended.
	// It has no effect when the consoles were already suspended
	// by the big hammer API.
	console_suspend(con);

	// This might resume the console when "no_console_suspend" option
	// is used. The driver should work because the HW was resumed.
	// But it should stay suspended when all consoles are supposed
	// to stay suspended because of the big hammer API.
	console_resume(con);
	// This should resume all consoles.
	console_resume_all();

Other behavior would be unexpected and untested. It might cause regression.

I see two solutions:

   + add another CON_SUSPENDED_ALL flag
   + add back "consoles_suspended" global variable

I prefer adding back the "consoles_suspended" global variable because
it is a global state...

The global state should be synchronized the same way as the current
per-console flag (write under console_list_lock, read under
console_srcu_read_lock()).

Also it should be checked by console_is_usable() API. Otherwise, we
would need to update all callers.

This brings a challenge how to make it safe and keep the API sane.
I propose to create:

  + __console_is_usable() where the "consoles_suspended" value will
    be passed as parameter. It might be used directly under
    console_list_lock().

  + console_is_usable() with the existing parameters. It will check
    the it was called under console_srcu_read_lock, read
    the global "consoles_suspend" and pass it to
    __console_is_usable().


>  	console_list_unlock();
>  
>  	/*

I played with the code to make sure that it looked sane
and I ended with the following changes on top of this patch.

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 1b2ce0f36010..fda4683d12f1 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3552,9 +3552,8 @@ static ssize_t show_cons_active(struct device *dev,
 	for_each_console(c) {
 		if (!c->device)
 			continue;
-		if (!(c->flags & CON_NBCON) && !c->write)
-			continue;
-		if (c->flags & CON_SUSPENDED)
+		if (!__console_is_usable(c, c->flags, consoles_suspended, true) &&
+		    !__console_is_usable(c, c->flags, consoles_suspended, false))
 			continue;
 		cs[i++] = c;
 		if (i >= ARRAY_SIZE(cs))
diff --git a/include/linux/console.h b/include/linux/console.h
index 5f17321ed962..090490ef570f 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -496,6 +496,7 @@ extern void console_list_lock(void) __acquires(console_mutex);
 extern void console_list_unlock(void) __releases(console_mutex);
 
 extern struct hlist_head console_list;
+extern bool consoles_suspended;
 
 /**
  * console_srcu_read_flags - Locklessly read flags of a possibly registered
@@ -548,6 +549,47 @@ static inline void console_srcu_write_flags(struct console *con, short flags)
 	WRITE_ONCE(con->flags, flags);
 }
 
+/**
+ * consoles_suspended_srcu_read - Locklessly read the global flag for
+ *				suspending all consoles.
+ *
+ * The global "consoles_suspended" flag is synchronized using console_list_lock
+ * and console_srcu_read_lock. It is the same approach as CON_SUSSPENDED flag.
+ * See console_srcu_read_flags() for more details.
+ *
+ * Context: Any context.
+ * Return: The current value of the global "consoles_suspended" flag.
+ */
+static inline short consoles_suspended_srcu_read(void)
+{
+	WARN_ON_ONCE(!console_srcu_read_lock_is_held());
+
+	/*
+	 * The READ_ONCE() matches the WRITE_ONCE() when "consoles_suspended"
+	 * is modified with consoles_suspended_srcu_write().
+	 */
+	return data_race(READ_ONCE(consoles_suspended));
+}
+
+/**
+ * consoles_suspended_srcu_write - Write the global flag for suspending
+ *			all consoles.
+ * @suspend:	new value to write
+ *
+ * The write must be done under the console_list_lock. The caller is responsible
+ * for calling synchronize_srcu() to make sure that all callers checking the
+ * usablility of registered consoles see the new state.
+ *
+ * Context: Any context.
+ */
+static inline void consoles_suspended_srcu_write(bool suspend)
+{
+	lockdep_assert_console_list_lock_held();
+
+	/* This matches the READ_ONCE() in consoles_suspended_srcu_read(). */
+	WRITE_ONCE(consoles_suspended, suspend);
+}
+
 /* Variant of console_is_registered() when the console_list_lock is held. */
 static inline bool console_is_registered_locked(const struct console *con)
 {
@@ -617,13 +659,15 @@ extern bool nbcon_kdb_try_acquire(struct console *con,
 extern void nbcon_kdb_release(struct nbcon_write_context *wctxt);
 
 /*
- * Check if the given console is currently capable and allowed to print
- * records. Note that this function does not consider the current context,
- * which can also play a role in deciding if @con can be used to print
- * records.
+ * This variant might be called under console_list_lock where both
+ * @flags and @all_suspended flags can be read directly.
  */
-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 all_suspended, bool use_atomic)
 {
+	if (all_suspended)
+		return false;
+
 	if (!(flags & CON_ENABLED))
 		return false;
 
@@ -666,6 +710,20 @@ static inline bool console_is_usable(struct console *con, short flags, bool use_
 	return true;
 }
 
+/*
+ * Check if the given console is currently capable and allowed to print
+ * records. Note that this function does not consider the current context,
+ * 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)
+{
+	bool all_suspended = consoles_suspended_srcu_read();
+
+	return __console_is_usable(con, flags, all_suspended, use_atomic);
+}
+
 #else
 static inline void nbcon_cpu_emergency_enter(void) { }
 static inline void nbcon_cpu_emergency_exit(void) { }
@@ -678,6 +736,8 @@ static inline void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
 static inline bool nbcon_kdb_try_acquire(struct console *con,
 					 struct nbcon_write_context *wctxt) { return false; }
 static inline void nbcon_kdb_release(struct nbcon_write_context *wctxt) { }
+static inline bool __console_is_usable(struct console *con, short flags,
+				       bool all_suspended, bool use_atomic) { return false; }
 static inline bool console_is_usable(struct console *con, short flags,
 				     bool use_atomic) { return false; }
 #endif
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 23a14e8c7a49..12247df07420 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -104,6 +104,13 @@ DEFINE_STATIC_SRCU(console_srcu);
  */
 int __read_mostly suppress_printk;
 
+/*
+ * Global flag for calling down all consoles during suspend.
+ * There is also a per-console flag which is used when the related
+ * device HW gets disabled, see CON_SUSPEND.
+ */
+bool consoles_suspended;
+
 #ifdef CONFIG_LOCKDEP
 static struct lockdep_map console_lock_dep_map = {
 	.name = "console_lock"
@@ -2731,8 +2738,6 @@ 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)
 		pr_info("Suspending console(s) (use no_console_suspend to debug)\n");
 
@@ -2749,8 +2754,7 @@ void console_suspend_all(void)
 		return;
 
 	console_list_lock();
-	for_each_console(con)
-		console_srcu_write_flags(con, con->flags | CON_SUSPENDED);
+	consoles_suspended_srcu_write(true);
 	console_list_unlock();
 
 	/*
@@ -2765,7 +2769,6 @@ void console_suspend_all(void)
 void console_resume_all(void)
 {
 	struct console_flush_type ft;
-	struct console *con;
 
 	/*
 	 * Allow queueing irq_work. After restoring console state, deferred
@@ -2776,8 +2779,7 @@ void console_resume_all(void)
 
 	if (console_suspend_enabled) {
 		console_list_lock();
-		for_each_console(con)
-			console_srcu_write_flags(con, con->flags & ~CON_SUSPENDED);
+		consoles_suspended_srcu_write(false);
 		console_list_unlock();
 
 		/*

Best Regards,
Petr

Powered by blists - more mailing lists