Re: [PATCH printk v3 39/40] printk: relieve console_lock of list synchronization duties

[Petr Mladek]

On Mon 2022-11-07 17:36:48, John Ogness wrote:
> On 2022-11-07, John Ogness <john.ogness@xxxxxxxxxxxxx> wrote:
> > @@ -3344,7 +3340,6 @@ void register_console(struct console *newcon)
> >  	 * Put this console in the list - keep the
> >  	 * preferred driver at the head of the list.
> >  	 */
> > -	console_lock();
> >  	if (hlist_empty(&console_list)) {
> >  		/* Ensure CON_CONSDEV is always set for the head. */
> >  		newcon->flags |= CON_CONSDEV;
> > @@ -3358,7 +3353,6 @@ void register_console(struct console *newcon)
> >  	} else {
> >  		hlist_add_behind_rcu(&newcon->node, console_list.first);
> >  	}
> > -	console_unlock();
> >  
> >  	/*
> >  	 * No need to synchronize SRCU here! The caller does not rely
> 
> I just realized that because of the new @seq initialization (patch 5/40)
> that we cannot completely remove the console_lock from
> register_console(). It will still be needed for @seq synchronization
> when registering non-boot/non-printbuffer consoles. So something like
> the patch below will need to be folded into this one.

Great catch!

> I am not happy with this. If an enabled boot console is behind, the
> console_unlock() will probably catch it up and we will end up with some
> repeat messages. But maybe this is "good enough" until we implement some
> real coordination between boot console and normal console takeovers.

The same problem actually has been there even before. The new console
was added in console_list under console_lock(). console_unlock() was
called before the early consoles were unregistered.

A solution would be to call pr_flush() before. But it should be
done in a separate patch.

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 17765166ac42..bb119001df56 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3328,12 +3328,21 @@ void register_console(struct console *newcon)
>  		 * that message instead. That boot console will be
>  		 * unregistered shortly and may be the same device.
>  		 */
> +
> +		/*
> +		 * Hold the console_lock to guarantee safe access to
> +		 * console->seq.
> +		 */
> +		console_lock();
> +
>  		for_each_console(con) {
>  			if ((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED) &&
>  			    con->seq < newcon->seq) {
>  				newcon->seq = con->seq;
>  			}
>  		}
> +
> +		console_unlock();
>  	}

This should be added already into the 5th patch that added this cycle.
We just must keep it in this patch.

Best Regards,
Petr