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:   Thu, 27 Oct 2022 14:40:22 +0200
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 v2 37/38] printk: relieve console_lock of list
 synchronization duties
Message-ID: <Y1p8NrxqaGP+iGQU@alley>
References: <20221019145600.1282823-1-john.ogness@linutronix.de>
 <20221019145600.1282823-38-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221019145600.1282823-38-john.ogness@linutronix.de>
Precedence: bulk

On Wed 2022-10-19 17:01:59, John Ogness wrote:
> The console_list_lock provides synchronization for console list and
> console->flags updates. All call sites that were using the console_lock
> for this synchronization have either switched to use the
> console_list_lock or the SRCU list iterator.
> 
> Remove console_lock usage for console list updates and console->flags
> updates.
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 3615ee6d68fd..840d581c4b23 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3370,8 +3363,6 @@ static int unregister_console_locked(struct console *console)
>  	if (res > 0)
>  		return 0;
>  
> -	console_lock();
> -
>  	/* Disable it unconditionally */
>  	console->flags &= ~CON_ENABLED;
>  
> @@ -3394,8 +3385,6 @@ static int unregister_console_locked(struct console *console)
>  	if (!hlist_empty(&console_list) && console->flags & CON_CONSDEV)
>  		console_first()->flags |= CON_CONSDEV;
>  
> -	console_unlock();
> -
>  	/* Ensure that all SRCU list walks have completed */
>  	synchronize_srcu(&console_srcu);

In addition, in unregister_console_locked(), we need to remove goto out_unlock:

	if (hlist_unhashed(&console->node)) {
		res = -ENODEV;
		goto out_unlock;
	}

out_unlock:
	console_unlock();
	return res;


I did my best to check all users that either iterate over the list
and/or access console->flags. And I did not find any real problems.

With the removal of the above mentioned out_unlock:

Reviewed-by: Petr Mladek <pmladek@suse.com>


Best Regards,
Petr

PS: Two ideas for possible future improvement:

  1. It might make sense to synchronize also @console_cmdline using
     the console_list_lock. It is modified only in
     __add_preferred_console() and read only from register_console().

     It would remove another possible race. Well, it is probably hard
     to hit, especially when new entries are always added at the end.


  2. It might make sense to create an API to modify and check
     console->flags. It might print a warning when the flags
     are accessed without console_list_lock or console_srcu.

     These days, it is pretty hard to catch all users.