Received-SPF: pass (google.com: domain of linux-kernel+bounces-133264-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 5 Apr 2024 17:37:38 +0200
From: Petr Mladek <pmladek@suse.com>
To: John Ogness <john.ogness@linutronix.de>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk v4 02/27] printk: Properly deal with nbcon
 consoles on seq init
Message-ID: <ZhAaiuR8fu6CQ_Sn@alley>
References: <20240402221129.2613843-1-john.ogness@linutronix.de>
 <20240402221129.2613843-3-john.ogness@linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240402221129.2613843-3-john.ogness@linutronix.de>

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@linutronix.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@suse.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@suse.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