Message-ID: <136fd27eab418f1d4ad008d9591bcc00669e2f8d.camel@suse.com>
Date: Tue, 02 Dec 2025 09:38:12 -0300
From: Marcos Paulo de Souza <mpdesouza@...e.com>
To: Petr Mladek <pmladek@...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 Thu, 2025-11-27 at 10:49 +0100, Petr Mladek wrote:
> 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.

Thanks Petr. I already have a local branch that will reduce the
console_is_usable of usages like to be called just once, so I'll work
on this change on top of it.

> 
> 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().
> 
> 

Makes sense. Thanks a lot for the suggestion.

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

You did all the work :) Let pick your changes and prepare the new
patchset using it. Thanks a lot!

> 
> Best Regards,
> Petr

Powered by blists - more mailing lists