Re: [PATCH printk v2 05/38] printk: use console_is_enabled()

Petr Mladek
2022-10-21T09:25:49Z

On Wed 2022-10-19 17:01:27, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
> kernel/printk/printk.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index e8a56056cd50..7ff2fc75fc3b 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2658,7 +2658,7 @@ static bool abandon_console_lock_in_panic(void)
> */
> static inline bool console_is_usable(struct console *con)
> {
> - if (!(con->flags & CON_ENABLED))
> + if (!console_is_enabled(con))
> return false;

This allows to use console_is_usable() without synchronization against
modification of the state. I guess that it will be needed for the
printk kthreads. But it is not needed at the moment.

We should document why it is needed and why it is safe.

>
> if (!con->write)
> @@ -2944,7 +2944,7 @@ void console_unblank(void)
> console_locked = 1;
> console_may_schedule = 0;
> for_each_console(c)
> - if ((c->flags & CON_ENABLED) && c->unblank)
> + if (console_is_enabled(c) && c->unblank)
> c->unblank();
> console_unlock();

The reading of the flag is actually synchronized against modifications
here. I guess that we need this because c->unblank() probably is not safe
against other operations with the console, e.g. c->write().

Well, it probably does not matter if reading of CON_ENABLED flag is
serialized against modifications. So, it is perfectly fine to use
the "racy" function.

> @@ -3098,7 +3098,7 @@ static int try_enable_preferred_console(struct console *newcon,
> * without matching. Accept the pre-enabled consoles only when match()
> * and setup() had a chance to be called.
> */
> - if (newcon->flags & CON_ENABLED && c->user_specified == user_specified)
> + if (console_is_enabled(newcon) && (c->user_specified == user_specified))

This is not racy because newcon is not in console_list at this
point. So that the state can't be modified by another callers.

Anyway, it is not explicitly synchronized, so we need to use
console_is_enabled with data_race() annotation.

> return 0;
>
> return -ENOENT;

Best Regards,
Petr