Re: [PATCH printk v2 21/38] serial: kgdboc: use srcu console list iterator

[Petr Mladek <pmladek@suse.com>]

On Wed 2022-10-19 17:01:43, John Ogness wrote:
> Use srcu console list iteration for safe console list traversal.
>
> Note that configure_kgdboc() still requires the console_lock in
> order to ensure that no console is in its write() callback when
> its direct() callback is called. Add comments to clarify this.

s/direct()/device()/

Do you know about such requirements or is it just
a conservative approach, please?

I ask because the comment in the code says "may assume".


Anyway, this would deserve a comment why the SRCU list iteration is
needed even when console_lock() is needed as well.

The reason is that further patches are going to synchronize
console_list manipulation with another lock and console_lock()
will be used only to serialize accessing con->write() callbacks.

Best Regards,
Petr

> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/kgdboc.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index b17aa7e49894..e9d3f8c6e3dc 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -171,6 +171,7 @@ static int configure_kgdboc(void)
>  	int err = -ENODEV;
>  	char *cptr = config;
>  	struct console *cons;
> +	int cookie;
>  
>  	if (!strlen(config) || isspace(config[0])) {
>  		err = 0;
> @@ -193,8 +194,14 @@ static int configure_kgdboc(void)
>  	if (!p)
>  		goto noconfig;
>  
> +	/*
> +	 * Stop console printing because the device() callback may
> +	 * assume the console is not within its write() callback.
> +	 */
>  	console_lock();
> -	for_each_console(cons) {
> +
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(cons) {
>  		int idx;
>  		if (cons->device && cons->device(cons, &idx) == p &&
>  		    idx == tty_line) {
> @@ -202,6 +209,8 @@ static int configure_kgdboc(void)
>  			break;
>  		}
>  	}
> +	console_srcu_read_unlock(cookie);
> +
>  	console_unlock();
>  
>  	kgdb_tty_driver = p;
> @@ -451,6 +460,7 @@ static void kgdboc_earlycon_pre_exp_handler(void)
>  {
>  	struct console *con;
>  	static bool already_warned;
> +	int cookie;
>  
>  	if (already_warned)
>  		return;
> @@ -463,9 +473,14 @@ static void kgdboc_earlycon_pre_exp_handler(void)
>  	 * serial drivers might be OK with this, print a warning once per
>  	 * boot if we detect this case.
>  	 */
> -	for_each_console(con)
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(con) {
>  		if (con == kgdboc_earlycon_io_ops.cons)
> -			return;
> +			break;
> +	}
> +	console_srcu_read_unlock(cookie);
> +	if (con)
> +		return;
>  
>  	already_warned = true;
>  	pr_warn("kgdboc_earlycon is still using bootconsole\n");
> -- 
> 2.30.2