Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 8 Nov 2022 18:40:30 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk v3 08/40] printk: use console_is_enabled()
Message-ID: <Y2qUjiBStRbVRGZ9@alley>
References: <20221107141638.3790965-1-john.ogness@linutronix.de>
 <20221107141638.3790965-9-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221107141638.3790965-9-john.ogness@linutronix.de>
Precedence: bulk

On Mon 2022-11-07 15:22:06, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled()
> if it involves a data race. Otherwise add comments mentioning why the
> wrapper is not used.

The wrapper will be used/needed only on few locations. There are many
more locations where the console flags are modified without any
locking.

Note that it is not only about CON_ENABLE flag. If we started playing
the game with WRITE_ONCE()/READ_ONCE() then we would need to consider
all locations where the flags are modified.

In the end, it might be easier to use the proposed console_set_flag(),
console_clear_flag(), console_check_flag(), and
console_check_flag_unsafe(), instead of documenting all the locations.

Also it is more important to document why it is acceptable to use
the racy variant. The wrappers would make sure that all the other
accesses are safe.


> Note that this is a preparatory change for when console_lock no longer
> provides synchronization for console->flags.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  kernel/printk/printk.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 79811984da34..f243bb56a3ba 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2660,7 +2660,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))

I agree that it makes sense to do this now. console_is_usable() is
used in two cycles. They will get switched to the srcu walk one by one.

Just please document that this allows to use console_is_usable() under
console_srcu_read_lock. And that in this case, the value of the flag might
get cleared at any time but the console still might be used
as long as the console_srcu_read_lock is held.

We should actually add a check into console_is_enabled() that either
console_lock or console_srcu_read_lock is held. The console_lock
should later be changed to console_list_lock.


>  		return false;
>  
>  	if (!con->write)
> @@ -2946,7 +2946,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)

I would prefer to do this change together with switching to
for_each_console_srcu(). It would be more clear why this change
is needed.

>  			c->unblank();
>  	console_unlock();
>  
> @@ -3104,8 +3104,11 @@ static int try_enable_preferred_console(struct console *newcon,
>  	 * Some consoles, such as pstore and netconsole, can be enabled even
>  	 * without matching. Accept the pre-enabled consoles only when match()
>  	 * and setup() had a chance to be called.
> +	 *
> +	 * Note that reading @flags is race-free because the console is not
> +	 * yet added to the console list.

Nit: This is not completely true. We just know that it will not get
     modified by the printk/console framework because the console is not
     registered yet.

Well, I could live with it. The comment is good enough. I am still
more concerned about how to distinguish when READ_ONCE()/WRITE_ONCE()
is needed. And it would affect all accesses to the flags.

>  	 */
> -	if (newcon->flags & CON_ENABLED && c->user_specified ==	user_specified)
> +	if ((newcon->flags & CON_ENABLED) && (c->user_specified == user_specified))
>  		return 0;
>  
>  	return -ENOENT;

Best Regards,
Petr