Re: [PATCH printk v4 02/27] printk: Properly deal with nbcon consoles on seq init

[Petr Mladek]

On Wed 2024-04-03 00:17:04, John Ogness wrote:
> If a non-boot console is registering and boot consoles exist, the
> consoles are flushed before being unregistered. This allows the
> non-boot console to continue where the boot console left off.
> 
> If for whatever reason flushing fails, the lowest seq found from
> any of the enabled boot consoles is used. Until now con->seq was
> checked. However, if it is an nbcon boot console, the function
> nbcon_seq_read() must be used to read seq because con->seq is
> not updated for nbcon consoles.
> 
> Check if it is an nbcon boot console and if so call
> nbcon_seq_read() to read seq.
> 
> Also setup the nbcon sequence number and reset the legacy
> sequence number from register_console() (rather than in
> nbcon_init() and nbcon_seq_force()). This removes all legacy
> sequence handling from nbcon.c so the code is easier to follow
> and maintain.
> 
> Signed-off-by: John Ogness <john.ogness@xxxxxxxxxxxxx>

Sigh, I wanted to wave this over. But then I ended in some
doubts, see below.


> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -974,7 +969,7 @@ void nbcon_init(struct console *con)
>  	/* nbcon_alloc() must have been called and successful! */
>  	BUG_ON(!con->pbufs);
>  
> -	nbcon_seq_force(con, con->seq);
> +	nbcon_seq_force(con, 0);
>  	nbcon_state_set(con, &state);
>  }
>  
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index c7c0ee2b47eb..b7e52b3f3e96 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3348,6 +3348,7 @@ static void try_enable_default_console(struct console *newcon)
>  		newcon->flags |= CON_CONSDEV;
>  }
>  
> +/* Set @newcon->seq to the first record this console should print. */
>  static void console_init_seq(struct console *newcon, bool bootcon_registered)
>  {
>  	struct console *con;
> @@ -3396,11 +3397,20 @@ static void console_init_seq(struct console *newcon, bool bootcon_registered)
>  
>  				newcon->seq = prb_next_seq(prb);
>  				for_each_console(con) {
> -					if ((con->flags & CON_BOOT) &&
> -					    (con->flags & CON_ENABLED) &&
> -					    con->seq < newcon->seq) {
> -						newcon->seq = con->seq;
> +					u64 seq;
> +
> +					if (!((con->flags & CON_BOOT) &&
> +					      (con->flags & CON_ENABLED))) {
> +						continue;
>  					}
> +
> +					if (con->flags & CON_NBCON)
> +						seq = nbcon_seq_read(con);
> +					else
> +						seq = con->seq;
> +
> +					if (seq < newcon->seq)
> +						newcon->seq = seq;
>  				}
>  			}
>  
> @@ -3517,9 +3527,18 @@ void register_console(struct console *newcon)
>  	newcon->dropped = 0;
>  	console_init_seq(newcon, bootcon_registered);
>  
> -	if (newcon->flags & CON_NBCON)
> +	if (newcon->flags & CON_NBCON) {
>  		nbcon_init(newcon);
>  
> +		/*
> +		 * nbcon consoles have their own sequence counter. The legacy
> +		 * sequence counter is reset so that it is clear it is not
> +		 * being used.
> +		 */
> +		nbcon_seq_force(newcon, newcon->seq);
> +		newcon->seq = 0;

I have tried whether we could get rid of this hack by assigning the
value correctly already in console_init_seq().

But I ended with a checken-and-egg problem whether to call
nbcon_init() before console_init_seq() or vice versa.
No solution was ideal.

Then I realized that the full solution in RT tree starts the kthread
in nbcon_init() => con->nbcon_seq must be initialized earlier
=> the current code is buggy.

BTW: I wonder if it is sane to start the kthread in the middle of
     struct console initialization. Especially when the full
     initialization is needed for a correct serialization against
     uart_port_lock().

     It might be better to create the kthread in a separate function
     called later. But this will be another patchset...


For now, I suggest to make the seq initialization cleaner. Here is
my proposal. The patch can be applied on top of this patchset.
It is only compile tested.