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/[email protected]/
>
> + 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
