Message-ID: <ZhAaiuR8fu6CQ_Sn@alley>
Date: Fri, 5 Apr 2024 17:37:38 +0200
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH printk v4 02/27] printk: Properly deal with nbcon
 consoles on seq init

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@...utronix.de>

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.


>From 2fcb73c98dde2c099fd5d5e4de9c0ffb449c7cc6 Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@...e.com>
Date: Fri, 5 Apr 2024 15:44:45 +0200
Subject: [PATCH] printk: Clean up code initializing the per-console sequence
 number

console_init_seq() reads either con->seq or con->nbcon_seq
depending on the value of CON_NBCON flag. But it always sets
newcon->seq even for nbcon consoles. The value is later
moved in register_console().

Rename console_init_seq() to get_init_console_seq() and just
return the value. Pass the value to nbcon_init() so that
it might be initialized there.

The cleaned design should make sure that the value stays
and is set before the kthread is created.

Signed-off-by: Petr Mladek <pmladek@...e.com>
---
 kernel/printk/internal.h |  2 +-
 kernel/printk/nbcon.c    |  5 +++--
 kernel/printk/printk.c   | 37 +++++++++++++++++--------------------
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index bcf2105a5c5c..2366303f31ae 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -84,7 +84,7 @@ int console_lock_spinning_disable_and_check(int cookie);
 u64 nbcon_seq_read(struct console *con);
 void nbcon_seq_force(struct console *con, u64 seq);
 bool nbcon_alloc(struct console *con);
-void nbcon_init(struct console *con);
+void nbcon_init(struct console *con, u64 init_seq);
 void nbcon_free(struct console *con);
 enum nbcon_prio nbcon_get_default_prio(void);
 void nbcon_atomic_flush_pending(void);
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 4c852c2e8d89..c57ad34a8d56 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1262,18 +1262,19 @@ bool nbcon_alloc(struct console *con)
 /**
  * nbcon_init - Initialize the nbcon console specific data
  * @con:	Console to initialize
+ * @init_seq	Sequence number of the first to-be-emitted record
  *
  * nbcon_alloc() *must* be called and succeed before this function
  * is called.
  */
-void nbcon_init(struct console *con)
+void nbcon_init(struct console *con, u64 init_seq)
 {
 	struct nbcon_state state = { };
 
 	/* nbcon_alloc() must have been called and successful! */
 	BUG_ON(!con->pbufs);
 
-	nbcon_seq_force(con, 0);
+	nbcon_seq_force(con, init_seq);
 	nbcon_state_set(con, &state);
 }
 
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9da70d7dda3d..9164b7f72e17 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3427,20 +3427,21 @@ 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)
+/* Return starting sequence number to be used by a newly registered console. */
+static u64 get_init_console_seq(struct console *newcon, bool bootcon_registered)
 {
 	struct console *con;
+	u64 init_seq;
 	bool handover;
 
 	if (newcon->flags & (CON_PRINTBUFFER | CON_BOOT)) {
 		/* Get a consistent copy of @syslog_seq. */
 		mutex_lock(&syslog_lock);
-		newcon->seq = syslog_seq;
+		init_seq = syslog_seq;
 		mutex_unlock(&syslog_lock);
 	} else {
 		/* Begin with next message added to ringbuffer. */
-		newcon->seq = prb_next_seq(prb);
+		init_seq = prb_next_seq(prb);
 
 		/*
 		 * If any enabled boot consoles are due to be unregistered
@@ -3461,7 +3462,7 @@ static void console_init_seq(struct console *newcon, bool bootcon_registered)
 			 * Flush all consoles and set the console to start at
 			 * the next unprinted sequence number.
 			 */
-			if (!console_flush_all(true, &newcon->seq, &handover)) {
+			if (!console_flush_all(true, &init_seq, &handover)) {
 				/*
 				 * Flushing failed. Just choose the lowest
 				 * sequence of the enabled boot consoles.
@@ -3474,12 +3475,12 @@ static void console_init_seq(struct console *newcon, bool bootcon_registered)
 				if (handover)
 					console_lock();
 
-				newcon->seq = prb_next_seq(prb);
+				init_seq = prb_next_seq(prb);
 				for_each_console(con) {
 					u64 seq;
 
-					if (!((con->flags & CON_BOOT) &&
-					      (con->flags & CON_ENABLED))) {
+					if (!(con->flags & CON_BOOT) ||
+					    !(con->flags & CON_ENABLED)) {
 						continue;
 					}
 
@@ -3488,14 +3489,16 @@ static void console_init_seq(struct console *newcon, bool bootcon_registered)
 					else
 						seq = con->seq;
 
-					if (seq < newcon->seq)
-						newcon->seq = seq;
+					if (seq < init_seq)
+						init_seq = seq;
 				}
 			}
 
 			console_unlock();
 		}
 	}
+
+	return init_seq;
 }
 
 #define console_first()				\
@@ -3528,6 +3531,7 @@ void register_console(struct console *newcon)
 	bool bootcon_registered = false;
 	bool realcon_registered = false;
 	unsigned long flags;
+	u64 init_seq;
 	int err;
 
 	console_list_lock();
@@ -3605,21 +3609,14 @@ void register_console(struct console *newcon)
 	}
 
 	newcon->dropped = 0;
-	console_init_seq(newcon, bootcon_registered);
+	init_seq = get_init_console_seq(newcon, have_boot_console);
 
 	if (newcon->flags & CON_NBCON) {
 		have_nbcon_console = true;
-		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;
+		nbcon_init(newcon, init_seq);
 	} else {
 		have_legacy_console = true;
+		newcon->seq = init_seq;
 	}
 
 	if (newcon->flags & CON_BOOT)
-- 
2.44.0

Powered by blists - more mailing lists