diff mbox series

[printk,v2,23/38] tty: tty_io: document console_lock usage

Message ID	20221019145600.1282823-24-john.ogness@linutronix.de
State	New
Headers	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; From: John Ogness <john.ogness@linutronix.de> To: Petr Mladek <pmladek@suse.com> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>, Steven Rostedt <rostedt@goodmis.org>, Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org> Subject: [PATCH printk v2 23/38] tty: tty_io: document console_lock usage Date: Wed, 19 Oct 2022 17:01:45 +0206 Message-Id: <20221019145600.1282823-24-john.ogness@linutronix.de> In-Reply-To: <20221019145600.1282823-1-john.ogness@linutronix.de> References: <20221019145600.1282823-1-john.ogness@linutronix.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	reduce console_lock scope \| [printk,v2,00/38] reduce console_lock scope [printk,v2,01/38] serial: kgdboc: Lock console list in probe function [printk,v2,02/38] printk: Convert console_drivers list to hlist [printk,v2,03/38] printk: Prepare for SRCU console list protection [printk,v2,04/38] printk: introduce console_is_enabled() wrapper [printk,v2,05/38] printk: use console_is_enabled() [printk,v2,06/38] tty: nfcon: use console_is_enabled() [printk,v2,07/38] um: kmsg_dump: use console_is_enabled() [printk,v2,08/38] efi: earlycon: use console_is_enabled() [printk,v2,09/38] netconsole: use console_is_enabled() [printk,v2,10/38] tty: hvc: use console_is_enabled() [printk,v2,11/38] tty: serial: earlycon: use console_is_enabled() [printk,v2,12/38] tty: serial: kgdboc: use console_is_enabled() [printk,v2,13/38] tty: serial: pic32_uart: use console_is_enabled() [printk,v2,14/38] tty: serial: samsung_tty: use console_is_enabled() [printk,v2,15/38] tty: serial: serial_core: use console_is_enabled() [printk,v2,16/38] tty: serial: xilinx_uartps: use console_is_enabled() [printk,v2,17/38] tty: tty_io: use console_is_enabled() [printk,v2,18/38] usb: early: xhci-dbc: use console_is_enabled() [printk,v2,19/38] kdb: kdb_io: use console_is_enabled() [printk,v2,20/38] um: kmsg_dumper: use srcu console list iterator [printk,v2,21/38] serial: kgdboc: use srcu console list iterator [printk,v2,22/38] serial: kgdboc: document console_lock usage [printk,v2,23/38] tty: tty_io: document console_lock usage [printk,v2,24/38] xen: fbfront: use srcu console list iterator [printk,v2,25/38] proc: consoles: document console_lock usage [printk,v2,26/38] kdb: use srcu console list iterator [printk,v2,27/38] printk: console_flush_all: use srcu console list iterator [printk,v2,28/38] printk: console_unblank: use srcu console list iterator [printk,v2,29/38] printk: console_flush_on_panic: use srcu console list iterator [printk,v2,30/38] printk: console_device: use srcu console list iterator [printk,v2,31/38] printk: register_console: use srcu console list iterator [printk,v2,32/38] printk: __pr_flush: use srcu console list iterator [printk,v2,33/38] printk: introduce console_list_lock [printk,v2,34/38] serial: kgdboc: use console_list_lock instead of console_lock [printk,v2,35/38] tty: tty_io: use console_list_lock for list synchronization [printk,v2,36/38] proc: consoles: use console_list_lock for list iteration [printk,v2,37/38] printk: relieve console_lock of list synchronization duties [printk,v2,38/38] printk, xen: fbfront: create/use safe function for forcing preferred

Commit Message

John Ogness Oct. 19, 2022, 2:55 p.m. UTC

  show_cons_active() uses the console_lock to gather information
on registered consoles. Since the console_lock is being used for
multiple reasons, explicitly document these reasons. This will
be useful when the console_lock is split into fine-grained
locking.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/tty_io.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Greg KH Oct. 20, 2022, 7:43 a.m. UTC | #1

On Wed, Oct 19, 2022 at 05:01:45PM +0206, John Ogness wrote:
> show_cons_active() uses the console_lock to gather information
> on registered consoles. Since the console_lock is being used for
> multiple reasons, explicitly document these reasons. This will
> be useful when the console_lock is split into fine-grained
> locking.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/tty_io.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 2050e63963bb..333579bfa335 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3526,6 +3526,14 @@ static ssize_t show_cons_active(struct device *dev,
>  	struct console *c;
>  	ssize_t count = 0;
>  
> +	/*
> +	 * Hold the console_lock to guarantee that no consoles are
> +	 * unregistered until all console processing is complete.
> +	 * This also allows safe traversal of the console list.
> +	 *
> +	 * Stop console printing because the device() callback may
> +	 * assume the console is not within its write() callback.
> +	 */
>  	console_lock();
>  	for_each_console(c) {
>  		if (!c->device)
> -- 
> 2.30.2
> 

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Petr Mladek Oct. 25, 2022, 1:31 p.m. UTC | #2

On Wed 2022-10-19 17:01:45, John Ogness wrote:
> show_cons_active() uses the console_lock to gather information
> on registered consoles. Since the console_lock is being used for
> multiple reasons, explicitly document these reasons. This will
> be useful when the console_lock is split into fine-grained
> locking.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/tty_io.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 2050e63963bb..333579bfa335 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3526,6 +3526,14 @@ static ssize_t show_cons_active(struct device *dev,
>  	struct console *c;
>  	ssize_t count = 0;
>  
> +	/*
> +	 * Hold the console_lock to guarantee that no consoles are
> +	 * unregistered until all console processing is complete.
> +	 * This also allows safe traversal of the console list.

This is more or less clear. show_cons_active() reads a lot of
information from the registered consoles.

> +	 *
> +	 * Stop console printing because the device() callback may
> +	 * assume the console is not within its write() callback.

I wonder if this is based on some real example or if you just want
to stay on the safe side.

It is perfectly fine to stay on the safe side. But we should make
it clear if the dependency really exists or if it has to be
investigated later during the clean up.

> +	 */
>  	console_lock();
>  	for_each_console(c) {
>  		if (!c->device)

Anyway, thanks for adding the comment.

Best Regards,
Petr

diff mbox series

Patch

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 2050e63963bb..333579bfa335 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3526,6 +3526,14 @@  static ssize_t show_cons_active(struct device *dev,
 	struct console *c;
 	ssize_t count = 0;
 
+	/*
+	 * Hold the console_lock to guarantee that no consoles are
+	 * unregistered until all console processing is complete.
+	 * This also allows safe traversal of the console list.
+	 *
+	 * Stop console printing because the device() callback may
+	 * assume the console is not within its write() callback.
+	 */
 	console_lock();
 	for_each_console(c) {
 		if (!c->device)