Re: [PATCH] nbcon/reboot: Flush nbcon consoles synchronously on reboot

[Petr Mladek <pmladek@suse.com>]

On Fri 2026-10-09 08:51:52, Sebastian Andrzej Siewior wrote:
> On 2026-10-08 17:08:52 [+0200], Petr Mladek wrote:
> > NBCON consoles emit messages in a dedicated kthreads when the system
> > is working properly. printk() tries to flush them synchronously in
> > explicitly marked emergency context and in panic().
> > 
> > Another situation where printk() could not rely on kthreads are the various
> > reboot and halt code paths. They can be detected by the `system_state`
> > variable.
> > 
> > Let's default to NBCON_PRIO_EMERGENCY for the post-running states.
> > printk() will automatically try flushing the consoles synchronously.
> > Also do not rely on printk() and explicitly flush the consoles
> > after these states are set.
> 
> while this seems okay, didn't we have pr_flush() to flush the output on
> shutdown/ reboot?

Good point! We should clean this up.

Anyway, IMHO, we should switch to NBCON_PRIO_EMERGENCY for the post-running
states and flush the pending messages immediately. It is more
reliable. It is easy to find the locations when system_state() is set.
And all follow-up printk() calls will try the direct flush so that
they won't rely on an explicit flush.

An ideal solution would be to add an wrapper, e.g.

	void set_syste_state(enum system_states state)
	{
		system_state = state;

		if (system_state > SYSTEM_RUNNING)
			pr_flush(0, true);
	}


Another thing is that printk_trigger_flush() is an overkill.
It tries to wake kthreads even via irq_work but we are
interested only in the direct flush.

I am not sure why neither me nor John used pr_flush().
It might be because it originally did not flush atomic consoles
directly. At least I had an outdated mental map.

Also the timeout should not be needed because all consoles should
be flushed directly. But it can be solved by using zero timeout.

In fact, we should block the kthreads to prevent seeing more
incomplete/interrupted messages, see the commit c41c0ebfa1e0eb
("printk/nbcon: Block printk kthreads when any CPU is in an emergency
context"). Something like:

diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index d8f8ec836eea..1c21c1e65b00 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1187,13 +1187,14 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex
 		return true;
 
 	/*
-	 * Block the kthread when the system is in an emergency or panic mode.
-	 * It increases the chance that these contexts would be able to show
-	 * the messages directly. And it reduces the risk of interrupted writes
-	 * where the context with a higher priority takes over the nbcon console
-	 * ownership in the middle of a message.
+	 * Block the kthread when the system is in an emergency, going down,
+	 * or panic mode. It increases the chance that these contexts would
+	 * be able to show the messages directly. And it reduces the risk of
+	 * interrupted writes where the context with a higher priority takes
+	 * over the nbcon console ownership in the middle of a message.
 	 */
 	if (unlikely(atomic_read(&nbcon_cpu_emergency_cnt)) ||
+	    unlikely(system_state > SYSTEM_RUNNING) ||
 	    unlikely(panic_in_progress()))
 		return false;
 
@@ -1249,10 +1250,12 @@ static int nbcon_kthread_func(void *__console)
 			return 0;
 
 		/*
-		 * Block the kthread when the system is in an emergency or panic
-		 * mode. See nbcon_kthread_should_wakeup() for more details.
+		 * Block the kthread when the system is in an emergency, going
+		 * down, or panic mode. See nbcon_kthread_should_wakeup() for
+		 * more details.
 		 */
 		if (unlikely(atomic_read(&nbcon_cpu_emergency_cnt)) ||
+		    unlikely(system_state > SYSTEM_RUNNING) ||
 		    unlikely(panic_in_progress()))
 			goto wait_for_event;
 

But wait, this might cause regression on netconsole which sets
CON_NBCON_ATOMIC_UNSAFE and is not able to flush the messages
directly a safe way.

A possibility would be to use con->write_thread() when pr_flush()
is called in task context. It should be safe in most shutdown
code paths except for the emergency_restart().

And we need an explicit pr_flush() even later in the halt
and maybe even some unsafe flush later in emergency_restart()
code path.

Sigh, this is getting complicated.

Best Regards,
Petr