[PATCH printk v2 00/26] wire up write_atomic() printing

[PATCH printk v2 00/26] wire up write_atomic() printing

[John Ogness]

Hi,

This is v3 of a series to wire up the nbcon consoles so that
they actually perform printing using their write_atomic()
callback. v2 is here [0]. For information about the motivation
of the atomic consoles, please read the cover letter of v1 [1].

The main focus of this series:

- For nbcon consoles, always call write_atomic() directly from
  printk() caller context for the panic CPU.

- For nbcon consoles, call write_atomic() when unlocking the
  console lock.

- Only perform the console_lock()/_unlock() dance if legacy or
  boot consoles are registered.

- For legacy consoles, if nbcon consoles are registered, do not
  attempt to print from printk() caller context for the panic
  CPU until nbcon consoles have had a chance to print the most
  significant messages.

- Mark emergency sections. In these sections printk() calls
  will only store the messages. Upon exiting the emergency
  section, console flushing is triggered via irq_work.

This series does _not_ include threaded printing or nbcon
drivers. Those features will be added in separate follow-up
series.

Note1: With this series, a system with _only_ nbcon consoles
       registered will not have any console printing except
       on panic. This is on purpose. When nbcon kthreads are
       introduced, they will fill this gap.

Note2: Patches 1-3 are already mainline, but not yet in the
       printk/for-next tree. They are included for
       completeness, but are not actually part of this series.

The changes since v2:

- Eliminate CPU states (normal, emergency, panic) and instead
  just track per-cpu emergency nesting.

- Instead of talking about "atomic mode", talk about "using the
  write_atomic() callback". This avoids confusion about what
  "atomic mode" means (i.e. "atomic" always means the
  write_atomic() callback is used).

- Rename atomic_enter()/_exit() to
  cpu_emergency_enter()/_exit().

- When entering emergency mode for a CPU, disable preemption
  rather than just migration to allow the warning to be
  completely handled before permitting rescheduling.

- Implement nbcon locking within the uart port lock wrappers.
  This provides synchronization between write_atomic() and
  non-printing driver activities (such as changing the baud
  rate.)

- Implement a one-way trigger printk_legacy_allow_panic_sync()
  to allow legacy consoles to print from the printk() caller
  context for the panic CPU. This allows the safe nbcon
  consoles to print before falling back to legacy consoles.
  Note that if no nbcon consoles are registered, legacy
  consoles are always allowed to print from the printk() caller
  context.

- Perform unsafe nbcon flushing at the very end of panic before
  going into the infinite loop.

- Add nbcon_get_default_prio() helper to return the appropriate
  prio for the current CPU.

- Do not assume that if write_atomic() returns false that the
  console has been released.

- For nbcon_atomic_emit_one(), rely on @ctxt->backlog rather
  than trying to read the next record.

- Rename nbcon_console_emit_next_record() to
  nbcon_legacy_emit_next_record() and have it use the same
  procedure as console_emit_next_record() (enter printk_safe,
  enable console_lock spinning, stop critical timings).

- Add nbcon_atomic_flush_unsafe() to allow flushing nbcon
  consoles in an unsafe manner.

- For nbcon flushing, add @stop_seq argument limit how much to
  print. This avoids a CPU getting stuck printing endlessly.

- For nbcon flushing, disable irqs to avoid an interrupt
  possibly calling into console code and deadlocking on nbcon
  ownership.

- The rules for allowing printing from the printk() caller
  context are getting quite complex. Move all this logic into
  vprintk_emit().

- For console_init_seq(), also consider nbcon consoles.

- For __pr_flush(), only take the console_lock if legacy or
  boot consoles are registered.

- For printk_trigger_flush(), do not flush nbcon consoles
  directly.

- For defer_console_output(), only trigger
  console_lock()/unlock() if legacy or boot consoles are
  registered.

- Add detailed kerneldoc for the write_atomic() callback.

- Fix kerneldoc for enum types (cons_flags, nbcon_prio).

- Add extra check to printk_deferred_enter()/_exit() to ensure
  it is called with migration disabled.

[0] https://lore.kernel.org/lkml/20230919230856.661435-1-john.ogness@linutronix.de
[1] https://lore.kernel.org/lkml/20230302195618.156940-1-john.ogness@linutronix.de

John Ogness (19):
  printk: Consider nbcon boot consoles on seq init
  printk: Add notation to console_srcu locking
  printk: nbcon: Ensure ownership release on failed emit
  printk: nbcon: Implement processing in port->lock wrapper
  printk: nbcon: Add detailed doc for write_atomic()
  printk: nbcon: Fix kerneldoc for enums
  printk: Make console_is_usable() available to nbcon
  printk: Let console_is_usable() handle nbcon
  printk: Add @flags argument for console_is_usable()
  printk: Track registered boot consoles
  printk: nbcon: Use nbcon consoles in console_flush_all()
  printk: nbcon: Assign priority based on CPU state
  printk: nbcon: Add unsafe flushing on panic
  printk: Avoid console_lock dance if no legacy or boot consoles
  printk: Track nbcon consoles
  printk: Coordinate direct printing in panic
  panic: Mark emergency section in oops
  rcu: Mark emergency section in rcu stalls
  lockdep: Mark emergency section in lockdep splats

Randy Dunlap (1):
  serial: core: fix kernel-doc for uart_port_unlock_irqrestore()

Sebastian Andrzej Siewior (1):
  printk: Check printk_deferred_enter()/_exit() usage

Thomas Gleixner (5):
  serial: core: Provide port lock wrappers
  serial: core: Use lock wrappers
  printk: nbcon: Provide function to flush using write_atomic()
  printk: nbcon: Implement emergency sections
  panic: Mark emergency section in warn

 drivers/tty/serial/8250/8250_port.c |   1 +
 include/linux/console.h             |  42 +++-
 include/linux/printk.h              |  30 ++-
 include/linux/serial_core.h         | 106 +++++++-
 kernel/locking/lockdep.c            |   5 +
 kernel/panic.c                      |   9 +
 kernel/printk/internal.h            |  57 +++++
 kernel/printk/nbcon.c               | 362 +++++++++++++++++++++++++++-
 kernel/printk/printk.c              | 248 +++++++++++++------
 kernel/printk/printk_safe.c         |  12 +
 kernel/rcu/tree_stall.h             |   5 +
 11 files changed, 784 insertions(+), 93 deletions(-)


base-commit: e7081d5a9d976b84f61f497316d7c940a4a2e67a
-- 
2.39.2

[PATCH printk v2 01/26] serial: core: Provide port lock wrappers

[John Ogness]

From: Thomas Gleixner <tglx@linutronix.de>

mainline commit: b0af4bcb49464c221ad5f95d40f2b1b252ceedcc

When a serial port is used for kernel console output, then all
modifications to the UART registers which are done from other contexts,
e.g. getty, termios, are interference points for the kernel console.

So far this has been ignored and the printk output is based on the
principle of hope. The rework of the console infrastructure which aims to
support threaded and atomic consoles, requires to mark sections which
modify the UART registers as unsafe. This allows the atomic write function
to make informed decisions and eventually to restore operational state. It
also allows to prevent the regular UART code from modifying UART registers
while printk output is in progress.

All modifications of UART registers are guarded by the UART port lock,
which provides an obvious synchronization point with the console
infrastructure.

Provide wrapper functions for spin_[un]lock*(port->lock) invocations so
that the console mechanics can be applied later on at a single place and
does not require to copy the same logic all over the drivers.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Link: https://lore.kernel.org/r/20230914183831.587273-2-john.ogness@linutronix.de
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/serial_core.h | 79 +++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index bb6f073bc159..f1d5c0d1568c 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -588,6 +588,85 @@ struct uart_port {
 	void			*private_data;		/* generic platform data pointer */
 };
 
+/**
+ * uart_port_lock - Lock the UART port
+ * @up:		Pointer to UART port structure
+ */
+static inline void uart_port_lock(struct uart_port *up)
+{
+	spin_lock(&up->lock);
+}
+
+/**
+ * uart_port_lock_irq - Lock the UART port and disable interrupts
+ * @up:		Pointer to UART port structure
+ */
+static inline void uart_port_lock_irq(struct uart_port *up)
+{
+	spin_lock_irq(&up->lock);
+}
+
+/**
+ * uart_port_lock_irqsave - Lock the UART port, save and disable interrupts
+ * @up:		Pointer to UART port structure
+ * @flags:	Pointer to interrupt flags storage
+ */
+static inline void uart_port_lock_irqsave(struct uart_port *up, unsigned long *flags)
+{
+	spin_lock_irqsave(&up->lock, *flags);
+}
+
+/**
+ * uart_port_trylock - Try to lock the UART port
+ * @up:		Pointer to UART port structure
+ *
+ * Returns: True if lock was acquired, false otherwise
+ */
+static inline bool uart_port_trylock(struct uart_port *up)
+{
+	return spin_trylock(&up->lock);
+}
+
+/**
+ * uart_port_trylock_irqsave - Try to lock the UART port, save and disable interrupts
+ * @up:		Pointer to UART port structure
+ * @flags:	Pointer to interrupt flags storage
+ *
+ * Returns: True if lock was acquired, false otherwise
+ */
+static inline bool uart_port_trylock_irqsave(struct uart_port *up, unsigned long *flags)
+{
+	return spin_trylock_irqsave(&up->lock, *flags);
+}
+
+/**
+ * uart_port_unlock - Unlock the UART port
+ * @up:		Pointer to UART port structure
+ */
+static inline void uart_port_unlock(struct uart_port *up)
+{
+	spin_unlock(&up->lock);
+}
+
+/**
+ * uart_port_unlock_irq - Unlock the UART port and re-enable interrupts
+ * @up:		Pointer to UART port structure
+ */
+static inline void uart_port_unlock_irq(struct uart_port *up)
+{
+	spin_unlock_irq(&up->lock);
+}
+
+/**
+ * uart_port_lock_irqrestore - Unlock the UART port, restore interrupts
+ * @up:		Pointer to UART port structure
+ * @flags:	The saved interrupt flags for restore
+ */
+static inline void uart_port_unlock_irqrestore(struct uart_port *up, unsigned long flags)
+{
+	spin_unlock_irqrestore(&up->lock, flags);
+}
+
 static inline int serial_port_in(struct uart_port *up, int offset)
 {
 	return up->serial_in(up, offset);
-- 
2.39.2

[PATCH printk v2 02/26] serial: core: Use lock wrappers

[John Ogness]

From: Thomas Gleixner <tglx@linutronix.de>

mainline commit: c5cbdb76e8e33ce90fec2946e8eee7d71d68e57a

When a serial port is used for kernel console output, then all
modifications to the UART registers which are done from other contexts,
e.g. getty, termios, are interference points for the kernel console.

So far this has been ignored and the printk output is based on the
principle of hope. The rework of the console infrastructure which aims to
support threaded and atomic consoles, requires to mark sections which
modify the UART registers as unsafe. This allows the atomic write function
to make informed decisions and eventually to restore operational state. It
also allows to prevent the regular UART code from modifying UART registers
while printk output is in progress.

All modifications of UART registers are guarded by the UART port lock,
which provides an obvious synchronization point with the console
infrastructure.

To avoid adding this functionality to all UART drivers, wrap the
spin_[un]lock*() invocations for uart_port::lock into helper functions
which just contain the spin_[un]lock*() invocations for now. In a
subsequent step these helpers will gain the console synchronization
mechanisms.

Converted with coccinelle. No functional change.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Link: https://lore.kernel.org/r/20230914183831.587273-3-john.ogness@linutronix.de
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/serial_core.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index f1d5c0d1568c..3091c62ec37b 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -1035,14 +1035,14 @@ static inline void uart_unlock_and_check_sysrq(struct uart_port *port)
 	u8 sysrq_ch;
 
 	if (!port->has_sysrq) {
-		spin_unlock(&port->lock);
+		uart_port_unlock(port);
 		return;
 	}
 
 	sysrq_ch = port->sysrq_ch;
 	port->sysrq_ch = 0;
 
-	spin_unlock(&port->lock);
+	uart_port_unlock(port);
 
 	if (sysrq_ch)
 		handle_sysrq(sysrq_ch);
@@ -1054,14 +1054,14 @@ static inline void uart_unlock_and_check_sysrq_irqrestore(struct uart_port *port
 	u8 sysrq_ch;
 
 	if (!port->has_sysrq) {
-		spin_unlock_irqrestore(&port->lock, flags);
+		uart_port_unlock_irqrestore(port, flags);
 		return;
 	}
 
 	sysrq_ch = port->sysrq_ch;
 	port->sysrq_ch = 0;
 
-	spin_unlock_irqrestore(&port->lock, flags);
+	uart_port_unlock_irqrestore(port, flags);
 
 	if (sysrq_ch)
 		handle_sysrq(sysrq_ch);
@@ -1077,12 +1077,12 @@ static inline int uart_prepare_sysrq_char(struct uart_port *port, u8 ch)
 }
 static inline void uart_unlock_and_check_sysrq(struct uart_port *port)
 {
-	spin_unlock(&port->lock);
+	uart_port_unlock(port);
 }
 static inline void uart_unlock_and_check_sysrq_irqrestore(struct uart_port *port,
 		unsigned long flags)
 {
-	spin_unlock_irqrestore(&port->lock, flags);
+	uart_port_unlock_irqrestore(port, flags);
 }
 #endif	/* CONFIG_MAGIC_SYSRQ_SERIAL */
 
-- 
2.39.2

[PATCH printk v2 03/26] serial: core: fix kernel-doc for uart_port_unlock_irqrestore()

[John Ogness]

From: Randy Dunlap <rdunlap@infradead.org>

mainline commit: 29bff582b74ed0bdb7e6986482ad9e6799ea4d2f

Fix the function name to avoid a kernel-doc warning:

include/linux/serial_core.h:666: warning: expecting prototype for uart_port_lock_irqrestore(). Prototype was for uart_port_unlock_irqrestore() instead

Fixes: b0af4bcb4946 ("serial: core: Provide port lock wrappers")
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: linux-serial@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
Link: https://lore.kernel.org/r/20230927044128.4748-1-rdunlap@infradead.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/serial_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 3091c62ec37b..89f7b6c63598 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -658,7 +658,7 @@ static inline void uart_port_unlock_irq(struct uart_port *up)
 }
 
 /**
- * uart_port_lock_irqrestore - Unlock the UART port, restore interrupts
+ * uart_port_unlock_irqrestore - Unlock the UART port, restore interrupts
  * @up:		Pointer to UART port structure
  * @flags:	The saved interrupt flags for restore
  */
-- 
2.39.2

[PATCH printk v2 04/26] printk: Consider nbcon boot consoles on seq init

[John Ogness]

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
always 0.

Check if it is an nbcon boot console and if so call
nbcon_seq_read() to read seq.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1685a71f3f71..696a9d76c09c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3391,11 +3391,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;
 				}
 			}
 
-- 
2.39.2

[PATCH printk v2 05/26] printk: Add notation to console_srcu locking

[John Ogness]

kernel/printk/printk.c:284:5: sparse: sparse: context imbalance in
'console_srcu_read_lock' - wrong count at exit
include/linux/srcu.h:301:9: sparse: sparse: context imbalance in
'console_srcu_read_unlock' - unexpected unlock

Reported-by: kernel test robot <lkp@intel.com>
Fixes: 6c4afa79147e ("printk: Prepare for SRCU console list protection")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 696a9d76c09c..b75ca383683d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -282,6 +282,7 @@ EXPORT_SYMBOL(console_list_unlock);
  * Return: A cookie to pass to console_srcu_read_unlock().
  */
 int console_srcu_read_lock(void)
+	__acquires(&console_srcu)
 {
 	return srcu_read_lock_nmisafe(&console_srcu);
 }
@@ -295,6 +296,7 @@ EXPORT_SYMBOL(console_srcu_read_lock);
  * Counterpart to console_srcu_read_lock()
  */
 void console_srcu_read_unlock(int cookie)
+	__releases(&console_srcu)
 {
 	srcu_read_unlock_nmisafe(&console_srcu, cookie);
 }
-- 
2.39.2

[PATCH printk v2 06/26] printk: nbcon: Ensure ownership release on failed emit

[John Ogness]

Until now it was assumed that ownership has been lost when the
write_atomic() callback fails. And nbcon_emit_next_record()
directly returned false. However, if nbcon_emit_next_record()
returns false, the context must no longer have ownership.

The semantics for the callbacks could be specified such that
if they return false, they must have released ownership. But
in practice those semantics seem odd since the ownership was
acquired outside of the callback.

Ensure ownership has been released before reporting failure by
explicitly attempting a release. If the current context is not
the owner, the release has no effect.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/nbcon.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index c8093bcc01fe..8ecd76aa22e6 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -852,7 +852,7 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
 	unsigned long con_dropped;
 	struct nbcon_state cur;
 	unsigned long dropped;
-	bool done;
+	bool done = false;
 
 	/*
 	 * The printk buffers are filled within an unsafe section. This
@@ -891,17 +891,18 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
 	nbcon_state_read(con, &cur);
 	wctxt->unsafe_takeover = cur.unsafe_takeover;
 
-	if (con->write_atomic) {
+	if (con->write_atomic)
 		done = con->write_atomic(con, wctxt);
-	} else {
-		nbcon_context_release(ctxt);
-		WARN_ON_ONCE(1);
-		done = false;
-	}
 
-	/* If not done, the emit was aborted. */
-	if (!done)
+	if (!done) {
+		/*
+		 * The emit was aborted, probably due to a loss of ownership.
+		 * Ensure ownership was lost or released before reporting the
+		 * loss.
+		 */
+		nbcon_context_release(ctxt);
 		return false;
+	}
 
 	/*
 	 * Since any dropped message was successfully output, reset the
-- 
2.39.2

[PATCH printk v2 07/26] printk: Check printk_deferred_enter()/_exit() usage

[John Ogness]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Add validation that printk_deferred_enter()/_exit() are called in
non-migration contexts.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/printk.h      |  7 +++++--
 kernel/printk/printk_safe.c | 12 ++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 955e31860095..8d5c5588eec9 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -159,13 +159,16 @@ __printf(1, 2) __cold int _printk_deferred(const char *fmt, ...);
 
 extern void __printk_safe_enter(void);
 extern void __printk_safe_exit(void);
+extern void __printk_deferred_enter(void);
+extern void __printk_deferred_exit(void);
+
 /*
  * The printk_deferred_enter/exit macros are available only as a hack for
  * some code paths that need to defer all printk console printing. Interrupts
  * must be disabled for the deferred duration.
  */
-#define printk_deferred_enter __printk_safe_enter
-#define printk_deferred_exit __printk_safe_exit
+#define printk_deferred_enter() __printk_deferred_enter()
+#define printk_deferred_exit() __printk_deferred_exit()
 
 /*
  * Please don't use printk_ratelimit(), because it shares ratelimiting state
diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index 6d10927a07d8..8d9408d653de 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -26,6 +26,18 @@ void __printk_safe_exit(void)
 	this_cpu_dec(printk_context);
 }
 
+void __printk_deferred_enter(void)
+{
+	cant_migrate();
+	this_cpu_inc(printk_context);
+}
+
+void __printk_deferred_exit(void)
+{
+	cant_migrate();
+	this_cpu_dec(printk_context);
+}
+
 asmlinkage int vprintk(const char *fmt, va_list args)
 {
 #ifdef CONFIG_KGDB_KDB
-- 
2.39.2

[PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[John Ogness]

Currently the port->lock wrappers uart_port_lock(),
uart_port_unlock() (and their variants) only lock/unlock
the spin_lock.

If the port is an nbcon console, the wrappers must also
acquire/release the console and mark the region as unsafe. This
allows general port->lock synchronization to be synchronized
with the nbcon console ownership.

Add a flag to struct uart_port to track nbcon console ownership.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_port.c |  1 +
 include/linux/printk.h              | 13 +++++
 include/linux/serial_core.h         | 19 ++++++-
 kernel/printk/nbcon.c               | 77 +++++++++++++++++++++++++++++
 4 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 141627370aab..16e2705b4867 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3284,6 +3284,7 @@ void serial8250_init_port(struct uart_8250_port *up)
 	struct uart_port *port = &up->port;
 
 	spin_lock_init(&port->lock);
+	port->nbcon_locked_port = false;
 	port->ctrl_id = 0;
 	port->pm = NULL;
 	port->ops = &serial8250_pops;
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 8d5c5588eec9..ef57a4d93ae2 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -9,6 +9,8 @@
 #include <linux/ratelimit_types.h>
 #include <linux/once_lite.h>
 
+struct uart_port;
+
 extern const char linux_banner[];
 extern const char linux_proc_banner[];
 
@@ -195,6 +197,8 @@ void show_regs_print_info(const char *log_lvl);
 extern asmlinkage void dump_stack_lvl(const char *log_lvl) __cold;
 extern asmlinkage void dump_stack(void) __cold;
 void printk_trigger_flush(void);
+extern void uart_nbcon_acquire(struct uart_port *up);
+extern void uart_nbcon_release(struct uart_port *up);
 #else
 static inline __printf(1, 0)
 int vprintk(const char *s, va_list args)
@@ -274,6 +278,15 @@ static inline void dump_stack(void)
 static inline void printk_trigger_flush(void)
 {
 }
+
+static inline void uart_nbcon_acquire(struct uart_port *up)
+{
+}
+
+static inline void uart_nbcon_release(struct uart_port *up)
+{
+}
+
 #endif
 
 bool this_cpu_in_panic(void);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 89f7b6c63598..d4b93d721715 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -488,6 +488,7 @@ struct uart_port {
 	struct uart_icount	icount;			/* statistics */
 
 	struct console		*cons;			/* struct console, if any */
+	bool			nbcon_locked_port;	/* True, if the port is locked by nbcon */
 	/* flags must be updated while holding port mutex */
 	upf_t			flags;
 
@@ -595,6 +596,7 @@ struct uart_port {
 static inline void uart_port_lock(struct uart_port *up)
 {
 	spin_lock(&up->lock);
+	uart_nbcon_acquire(up);
 }
 
 /**
@@ -604,6 +606,7 @@ static inline void uart_port_lock(struct uart_port *up)
 static inline void uart_port_lock_irq(struct uart_port *up)
 {
 	spin_lock_irq(&up->lock);
+	uart_nbcon_acquire(up);
 }
 
 /**
@@ -614,6 +617,7 @@ static inline void uart_port_lock_irq(struct uart_port *up)
 static inline void uart_port_lock_irqsave(struct uart_port *up, unsigned long *flags)
 {
 	spin_lock_irqsave(&up->lock, *flags);
+	uart_nbcon_acquire(up);
 }
 
 /**
@@ -624,7 +628,11 @@ static inline void uart_port_lock_irqsave(struct uart_port *up, unsigned long *f
  */
 static inline bool uart_port_trylock(struct uart_port *up)
 {
-	return spin_trylock(&up->lock);
+	if (!spin_trylock(&up->lock))
+		return false;
+
+	uart_nbcon_acquire(up);
+	return true;
 }
 
 /**
@@ -636,7 +644,11 @@ static inline bool uart_port_trylock(struct uart_port *up)
  */
 static inline bool uart_port_trylock_irqsave(struct uart_port *up, unsigned long *flags)
 {
-	return spin_trylock_irqsave(&up->lock, *flags);
+	if (!spin_trylock_irqsave(&up->lock, *flags))
+		return false;
+
+	uart_nbcon_acquire(up);
+	return true;
 }
 
 /**
@@ -645,6 +657,7 @@ static inline bool uart_port_trylock_irqsave(struct uart_port *up, unsigned long
  */
 static inline void uart_port_unlock(struct uart_port *up)
 {
+	uart_nbcon_release(up);
 	spin_unlock(&up->lock);
 }
 
@@ -654,6 +667,7 @@ static inline void uart_port_unlock(struct uart_port *up)
  */
 static inline void uart_port_unlock_irq(struct uart_port *up)
 {
+	uart_nbcon_release(up);
 	spin_unlock_irq(&up->lock);
 }
 
@@ -664,6 +678,7 @@ static inline void uart_port_unlock_irq(struct uart_port *up)
  */
 static inline void uart_port_unlock_irqrestore(struct uart_port *up, unsigned long flags)
 {
+	uart_nbcon_release(up);
 	spin_unlock_irqrestore(&up->lock, flags);
 }
 
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 8ecd76aa22e6..02e8fdc1ea43 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -6,6 +6,7 @@
 #include <linux/console.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <linux/serial_core.h>
 #include "internal.h"
 /*
  * Printk console printing implementation for consoles which does not depend
@@ -995,3 +996,79 @@ void nbcon_free(struct console *con)
 
 	con->pbufs = NULL;
 }
+
+static inline bool uart_is_nbcon(struct uart_port *up)
+{
+	int cookie;
+	bool ret;
+
+	if (!uart_console(up))
+		return false;
+
+	cookie = console_srcu_read_lock();
+	ret = (console_srcu_read_flags(up->cons) & CON_NBCON);
+	console_srcu_read_unlock(cookie);
+	return ret;
+}
+
+/**
+ * uart_nbcon_acquire - The second half of the port locking wrapper
+ * @up:		The uart port whose @lock was locked
+ *
+ * The uart_port_lock() wrappers will first lock the spin_lock @up->lock.
+ * Then this function is called to implement nbcon-specific processing.
+ *
+ * If @up is an nbcon console, this console will be acquired and marked as
+ * unsafe. Otherwise this function does nothing.
+ */
+void uart_nbcon_acquire(struct uart_port *up)
+{
+	struct console *con = up->cons;
+	struct nbcon_context ctxt;
+
+	if (!uart_is_nbcon(up))
+		return;
+
+	WARN_ON_ONCE(up->nbcon_locked_port);
+
+	do {
+		do {
+			memset(&ctxt, 0, sizeof(ctxt));
+			ctxt.console	= con;
+			ctxt.prio	= NBCON_PRIO_NORMAL;
+		} while (!nbcon_context_try_acquire(&ctxt));
+
+	} while (!nbcon_context_enter_unsafe(&ctxt));
+
+	up->nbcon_locked_port = true;
+}
+EXPORT_SYMBOL_GPL(uart_nbcon_acquire);
+
+/**
+ * uart_nbcon_release - The first half of the port unlocking wrapper
+ * @up:		The uart port whose @lock is about to be unlocked
+ *
+ * The uart_port_unlock() wrappers will first call this function to implement
+ * nbcon-specific processing. Then afterwards the uart_port_unlock() wrappers
+ * will unlock the spin_lock @up->lock.
+ *
+ * If @up is an nbcon console, the console will be marked as safe and
+ * released. Otherwise this function does nothing.
+ */
+void uart_nbcon_release(struct uart_port *up)
+{
+	struct console *con = up->cons;
+	struct nbcon_context ctxt = {
+		.console	= con,
+		.prio		= NBCON_PRIO_NORMAL,
+	};
+
+	if (!up->nbcon_locked_port)
+		return;
+
+	if (nbcon_context_exit_unsafe(&ctxt))
+		nbcon_context_release(&ctxt);
+
+	up->nbcon_locked_port = false;
+}
+EXPORT_SYMBOL_GPL(uart_nbcon_release);
-- 
2.39.2

[PATCH printk v2 09/26] printk: nbcon: Add detailed doc for write_atomic()

[John Ogness]

The write_atomic() callback has special requirements and is
allowed to use special helper functions. Provide detailed
documentation of the callback so that a developer has a
chance of implementing it correctly.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/console.h | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index e4fc6f7c1496..5c55faa013e8 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -278,7 +278,7 @@ struct nbcon_write_context {
 /**
  * struct console - The console descriptor structure
  * @name:		The name of the console driver
- * @write:		Write callback to output messages (Optional)
+ * @write:		Legacy write callback to output messages (Optional)
  * @read:		Read callback for console input (Optional)
  * @device:		The underlying TTY device driver (Optional)
  * @unblank:		Callback to unblank the console (Optional)
@@ -295,7 +295,6 @@ struct nbcon_write_context {
  * @data:		Driver private data
  * @node:		hlist node for the console list
  *
- * @write_atomic:	Write callback for atomic context
  * @nbcon_state:	State for nbcon consoles
  * @nbcon_seq:		Sequence number of the next record for nbcon to print
  * @pbufs:		Pointer to nbcon private buffer
@@ -320,8 +319,35 @@ struct console {
 	struct hlist_node	node;
 
 	/* nbcon console specific members */
-	bool			(*write_atomic)(struct console *con,
-						struct nbcon_write_context *wctxt);
+
+	/**
+	 * @write_atomic:
+	 *
+	 * NBCON callback to write out text in any context. (Optional)
+	 *
+	 * This callback is called with the console already acquired. The
+	 * callback can use nbcon_can_proceed() at any time to verify that
+	 * it is still the owner of the console. In the case that it has
+	 * lost ownership, it is no longer allowed to go forward. In this
+	 * case it must back out immediately and carefully. The buffer
+	 * content is also no longer trusted since it no longer belongs to
+	 * the context.
+	 *
+	 * If the callback needs to perform actions where ownership is not
+	 * allowed to be taken over, nbcon_enter_unsafe() and
+	 * nbcon_exit_unsafe() can be used to mark such sections. These
+	 * functions are also points of possible ownership transfer. If
+	 * either function returns false, ownership has been lost.
+	 *
+	 * This callback can be called from any context (including NMI).
+	 * Therefore it must avoid usage of any locking and instead rely
+	 * on the console ownership for synchronization.
+	 *
+	 * Returns true if all text was successfully written out and
+	 * ownership was never lost, otherwise false.
+	 */
+	bool (*write_atomic)(struct console *con, struct nbcon_write_context *wctxt);
+
 	atomic_t		__private nbcon_state;
 	atomic_long_t		__private nbcon_seq;
 	struct printk_buffers	*pbufs;
-- 
2.39.2

[PATCH printk v2 10/26] printk: nbcon: Fix kerneldoc for enums

[John Ogness]

Kerneldoc requires enums to be specified as such. Otherwise it is
interpreted as function documentation.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/console.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index 5c55faa013e8..d8922282efa1 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -130,7 +130,7 @@ static inline int con_debug_leave(void)
  */
 
 /**
- * cons_flags - General console flags
+ * enum cons_flags - General console flags
  * @CON_PRINTBUFFER:	Used by newly registered consoles to avoid duplicate
  *			output of messages that were already shown by boot
  *			consoles or read by userspace via syslog() syscall.
@@ -211,7 +211,7 @@ struct nbcon_state {
 static_assert(sizeof(struct nbcon_state) <= sizeof(int));
 
 /**
- * nbcon_prio - console owner priority for nbcon consoles
+ * enum nbcon_prio - console owner priority for nbcon consoles
  * @NBCON_PRIO_NONE:		Unused
  * @NBCON_PRIO_NORMAL:		Normal (non-emergency) usage
  * @NBCON_PRIO_EMERGENCY:	Emergency output (WARN/OOPS...)
-- 
2.39.2

[PATCH printk v2 11/26] printk: Make console_is_usable() available to nbcon

[John Ogness]

Move console_is_usable() as-is into internal.h so that it can
be used by nbcon printing functions as well.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/internal.h | 32 ++++++++++++++++++++++++++++++++
 kernel/printk/printk.c   | 30 ------------------------------
 2 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 6c2afee5ef62..6e8c1b02adae 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -78,6 +78,36 @@ bool nbcon_alloc(struct console *con);
 void nbcon_init(struct console *con);
 void nbcon_free(struct console *con);
 
+/*
+ * Check if the given console is currently capable and allowed to print
+ * records.
+ *
+ * Requires the console_srcu_read_lock.
+ */
+static inline bool console_is_usable(struct console *con)
+{
+	short flags = console_srcu_read_flags(con);
+
+	if (!(flags & CON_ENABLED))
+		return false;
+
+	if ((flags & CON_SUSPENDED))
+		return false;
+
+	if (!con->write)
+		return false;
+
+	/*
+	 * Console drivers may assume that per-cpu resources have been
+	 * allocated. So unless they're explicitly marked as being able to
+	 * cope (CON_ANYTIME) don't call them until this CPU is officially up.
+	 */
+	if (!cpu_online(raw_smp_processor_id()) && !(flags & CON_ANYTIME))
+		return false;
+
+	return true;
+}
+
 #else
 
 #define PRINTK_PREFIX_MAX	0
@@ -99,6 +129,8 @@ static inline bool nbcon_alloc(struct console *con) { return false; }
 static inline void nbcon_init(struct console *con) { }
 static inline void nbcon_free(struct console *con) { }
 
+static inline bool console_is_usable(struct console *con) { return false; }
+
 #endif /* CONFIG_PRINTK */
 
 extern struct printk_buffers printk_shared_pbufs;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b75ca383683d..ecb50590815c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2694,36 +2694,6 @@ int is_console_locked(void)
 }
 EXPORT_SYMBOL(is_console_locked);
 
-/*
- * Check if the given console is currently capable and allowed to print
- * records.
- *
- * Requires the console_srcu_read_lock.
- */
-static inline bool console_is_usable(struct console *con)
-{
-	short flags = console_srcu_read_flags(con);
-
-	if (!(flags & CON_ENABLED))
-		return false;
-
-	if ((flags & CON_SUSPENDED))
-		return false;
-
-	if (!con->write)
-		return false;
-
-	/*
-	 * Console drivers may assume that per-cpu resources have been
-	 * allocated. So unless they're explicitly marked as being able to
-	 * cope (CON_ANYTIME) don't call them until this CPU is officially up.
-	 */
-	if (!cpu_online(raw_smp_processor_id()) && !(flags & CON_ANYTIME))
-		return false;
-
-	return true;
-}
-
 static void __console_unlock(void)
 {
 	console_locked = 0;
-- 
2.39.2

[PATCH printk v2 12/26] printk: Let console_is_usable() handle nbcon

[John Ogness]

The nbcon consoles use a different printing callback. For nbcon
consoles, check for the write_atomic() callback instead of
write().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/internal.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 6e8c1b02adae..69c8861be92c 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -80,6 +80,8 @@ void nbcon_free(struct console *con);
 
 /*
  * Check if the given console is currently capable and allowed to print
+ * records. Note that this function does not consider the current context,
+ * which can also play a role in deciding if @con can be used to print
  * records.
  *
  * Requires the console_srcu_read_lock.
@@ -94,8 +96,13 @@ static inline bool console_is_usable(struct console *con)
 	if ((flags & CON_SUSPENDED))
 		return false;
 
-	if (!con->write)
-		return false;
+	if (flags & CON_NBCON) {
+		if (!con->write_atomic)
+			return false;
+	} else {
+		if (!con->write)
+			return false;
+	}
 
 	/*
 	 * Console drivers may assume that per-cpu resources have been
-- 
2.39.2

[PATCH printk v2 13/26] printk: Add @flags argument for console_is_usable()

[John Ogness]

The caller of console_is_usable() usually needs @console->flags
for its own checks. Rather than having console_is_usable() read
its own copy, make the caller pass in the @flags. This also
ensures that the caller saw the same @flags value.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/internal.h | 8 ++------
 kernel/printk/printk.c   | 5 +++--
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 69c8861be92c..6780911fa8f2 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -83,13 +83,9 @@ void nbcon_free(struct console *con);
  * records. Note that this function does not consider the current context,
  * which can also play a role in deciding if @con can be used to print
  * records.
- *
- * Requires the console_srcu_read_lock.
  */
-static inline bool console_is_usable(struct console *con)
+static inline bool console_is_usable(struct console *con, short flags)
 {
-	short flags = console_srcu_read_flags(con);
-
 	if (!(flags & CON_ENABLED))
 		return false;
 
@@ -136,7 +132,7 @@ static inline bool nbcon_alloc(struct console *con) { return false; }
 static inline void nbcon_init(struct console *con) { }
 static inline void nbcon_free(struct console *con) { }
 
-static inline bool console_is_usable(struct console *con) { return false; }
+static inline bool console_is_usable(struct console *con, short flags) { return false; }
 
 #endif /* CONFIG_PRINTK */
 
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ecb50590815c..9d56ce5837f9 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2939,9 +2939,10 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 
 		cookie = console_srcu_read_lock();
 		for_each_console_srcu(con) {
+			short flags = console_srcu_read_flags(con);
 			bool progress;
 
-			if (!console_is_usable(con))
+			if (!console_is_usable(con, flags))
 				continue;
 			any_usable = true;
 
@@ -3783,7 +3784,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
 			 * that they make forward progress, so only increment
 			 * @diff for usable consoles.
 			 */
-			if (!console_is_usable(c))
+			if (!console_is_usable(c, flags))
 				continue;
 
 			if (flags & CON_NBCON) {
-- 
2.39.2

[PATCH printk v2 14/26] printk: nbcon: Provide function to flush using write_atomic()

[John Ogness]

From: Thomas Gleixner <tglx@linutronix.de>

Provide nbcon_atomic_flush_all() to perform flushing of all
registered nbcon consoles using their write_atomic() callback.
Like with legacy consoles, the nbcon consoles are flushed one
record per console. This allows all nbcon consoles to print
lines pseudo-simultaneously, rather than one console waiting
for the full ringbuffer to dump to another console before
printing anything.

Unlike console_flush_all(), nbcon_atomic_flush_all() will only
flush up through the newest record at the time of the call.
This prevents a CPU from printing unbounded when other CPUs are
adding records.

Perform nbcon console atomic flushing in
console_flush_on_panic(). This function is not only used in
panic() but also other locations where there may be stored
messages that need to be flushed.

Co-developed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
 kernel/printk/internal.h |   2 +
 kernel/printk/nbcon.c    | 100 ++++++++++++++++++++++++++++++++++++++-
 kernel/printk/printk.c   |   2 +
 3 files changed, 102 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 6780911fa8f2..d9a5205692fc 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -77,6 +77,7 @@ void nbcon_seq_force(struct console *con, u64 seq);
 bool nbcon_alloc(struct console *con);
 void nbcon_init(struct console *con);
 void nbcon_free(struct console *con);
+void nbcon_atomic_flush_all(void);
 
 /*
  * Check if the given console is currently capable and allowed to print
@@ -131,6 +132,7 @@ static inline void nbcon_seq_force(struct console *con, u64 seq) { }
 static inline bool nbcon_alloc(struct console *con) { return false; }
 static inline void nbcon_init(struct console *con) { }
 static inline void nbcon_free(struct console *con) { }
+static inline void nbcon_atomic_flush_all(void) { }
 
 static inline bool console_is_usable(struct console *con, short flags) { return false; }
 
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 02e8fdc1ea43..2eb2929c1027 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -539,7 +539,6 @@ static struct printk_buffers panic_nbcon_pbufs;
  * in an unsafe state. Otherwise, on success the caller may assume
  * the console is not in an unsafe state.
  */
-__maybe_unused
 static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
 {
 	unsigned int cpu = smp_processor_id();
@@ -841,7 +840,6 @@ EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
  * When true is returned, @wctxt->ctxt.backlog indicates whether there are
  * still records pending in the ringbuffer,
  */
-__maybe_unused
 static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
 {
 	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
@@ -930,6 +928,104 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
 	return nbcon_context_exit_unsafe(ctxt);
 }
 
+/**
+ * nbcon_atomic_emit_one - Print one record for an nbcon console using the
+ *				write_atomic() callback
+ * @wctxt:	An initialized write context struct to use
+ *		for this context
+ *
+ * Return:	False if the given console could not print a record or there
+ *		are no more records to print, otherwise true.
+ *
+ * This is an internal helper to handle the locking of the console before
+ * calling nbcon_emit_next_record().
+ */
+static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
+{
+	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
+
+	if (!nbcon_context_try_acquire(ctxt))
+		return false;
+
+	/*
+	 * nbcon_emit_next_record() returns false when the console was
+	 * handed over or taken over. In both cases the context is no
+	 * longer valid.
+	 */
+	if (!nbcon_emit_next_record(wctxt))
+		return false;
+
+	nbcon_context_release(ctxt);
+
+	return ctxt->backlog;
+}
+
+/**
+ * __nbcon_atomic_flush_all - Flush all nbcon consoles using their
+ *					write_atomic() callback
+ * @stop_seq:			Flush up until this record
+ */
+static void __nbcon_atomic_flush_all(u64 stop_seq)
+{
+	struct nbcon_write_context wctxt = { };
+	struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
+	struct console *con;
+	bool any_progress;
+	int cookie;
+
+	do {
+		any_progress = false;
+
+		cookie = console_srcu_read_lock();
+		for_each_console_srcu(con) {
+			short flags = console_srcu_read_flags(con);
+			unsigned long irq_flags;
+
+			if (!(flags & CON_NBCON))
+				continue;
+
+			if (!console_is_usable(con, flags))
+				continue;
+
+			if (nbcon_seq_read(con) >= stop_seq)
+				continue;
+
+			memset(ctxt, 0, sizeof(*ctxt));
+			ctxt->console			= con;
+			ctxt->spinwait_max_us		= 2000;
+			ctxt->prio			= NBCON_PRIO_NORMAL;
+
+			/*
+			 * Atomic flushing does not use console driver
+			 * synchronization (i.e. it does not hold the port
+			 * lock for uart consoles). Therefore IRQs must be
+			 * disabled to avoid being interrupted and then
+			 * calling into a driver that will deadlock trying
+			 * acquire console ownership.
+			 */
+			local_irq_save(irq_flags);
+
+			any_progress |= nbcon_atomic_emit_one(&wctxt);
+
+			local_irq_restore(irq_flags);
+		}
+		console_srcu_read_unlock(cookie);
+	} while (any_progress);
+}
+
+/**
+ * nbcon_atomic_flush_all - Flush all nbcon consoles using their
+ *				write_atomic() callback
+ *
+ * Flush the backlog up through the currently newest record. Any new
+ * records added while flushing will not be flushed. This is to avoid
+ * one CPU printing unbounded because other CPUs continue to add records.
+ */
+void nbcon_atomic_flush_all(void)
+{
+	__nbcon_atomic_flush_all(prb_next_reserve_seq(prb));
+}
+
 /**
  * nbcon_alloc - Allocate buffers needed by the nbcon console
  * @con:	Console to allocate buffers for
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9d56ce5837f9..ea170ade4d42 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3169,6 +3169,8 @@ void console_flush_on_panic(enum con_flush_mode mode)
 		console_srcu_read_unlock(cookie);
 	}
 
+	nbcon_atomic_flush_all();
+
 	console_flush_all(false, &next_seq, &handover);
 }
 
-- 
2.39.2

[PATCH printk v2 15/26] printk: Track registered boot consoles

[John Ogness]

Unfortunately it is not known if a boot console and a regular
(legacy or nbcon) console use the same hardware. For this reason
they must not be allowed to print simultaneously.

For legacy consoles this is not an issue because they are
already synchronized with the boot consoles using the console
lock. However nbcon consoles can be triggered separately.

Add a global flag @have_boot_console to identify if any boot
consoles are registered. This will be used in follow-up commits
to ensure that boot consoles and nbcon consoles cannot print
simultaneously.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ea170ade4d42..1b14159990ba 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -463,6 +463,14 @@ static int console_msg_format = MSG_FORMAT_DEFAULT;
 /* syslog_lock protects syslog_* variables and write access to clear_seq. */
 static DEFINE_MUTEX(syslog_lock);
 
+/*
+ * Specifies if a boot console is registered. If boot consoles are present,
+ * nbcon consoles cannot print simultaneously and must be synchronized by
+ * the console lock. This is because boot consoles and nbcon consoles may
+ * have mapped the same hardware.
+ */
+bool have_boot_console;
+
 #ifdef CONFIG_PRINTK
 DECLARE_WAIT_QUEUE_HEAD(log_wait);
 /* All 3 protected by @syslog_lock. */
@@ -3499,6 +3507,9 @@ void register_console(struct console *newcon)
 	if (newcon->flags & CON_NBCON)
 		nbcon_init(newcon);
 
+	if (newcon->flags & CON_BOOT)
+		have_boot_console = true;
+
 	/*
 	 * Put this console in the list - keep the
 	 * preferred driver at the head of the list.
@@ -3551,6 +3562,8 @@ EXPORT_SYMBOL(register_console);
 /* Must be called under console_list_lock(). */
 static int unregister_console_locked(struct console *console)
 {
+	bool found_boot_con = false;
+	struct console *c;
 	int res;
 
 	lockdep_assert_console_list_lock_held();
@@ -3598,6 +3611,17 @@ static int unregister_console_locked(struct console *console)
 	if (console->exit)
 		res = console->exit(console);
 
+	/*
+	 * With this console gone, the global flags tracking registered
+	 * console types may have changed. Update them.
+	 */
+	for_each_console(c) {
+		if (c->flags & CON_BOOT)
+			found_boot_con = true;
+	}
+	if (!found_boot_con)
+		have_boot_console = false;
+
 	return res;
 }
 
-- 
2.39.2

[PATCH printk v2 16/26] printk: nbcon: Use nbcon consoles in console_flush_all()

[John Ogness]

Allow nbcon consoles to print messages in the legacy printk()
caller context (printing via unlock) by integrating them into
console_flush_all(). The write_atomic() callback is used for
printing.

Provide nbcon_legacy_emit_next_record(), which acts as the
nbcon variant of console_emit_next_record(). Call this variant
within console_flush_all() for nbcon consoles. Since nbcon
consoles use their own @nbcon_seq variable to track the next
record to print, this also must be appropriately handled.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/internal.h |  6 ++++++
 kernel/printk/nbcon.c    | 45 ++++++++++++++++++++++++++++++++++++++++
 kernel/printk/printk.c   | 19 ++++++++++++-----
 3 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index d9a5205692fc..b34847ec6b0d 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -71,6 +71,8 @@ void defer_console_output(void);
 
 u16 printk_parse_prefix(const char *text, int *level,
 			enum printk_info_flags *flags);
+void console_lock_spinning_enable(void);
+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);
@@ -78,6 +80,8 @@ bool nbcon_alloc(struct console *con);
 void nbcon_init(struct console *con);
 void nbcon_free(struct console *con);
 void nbcon_atomic_flush_all(void);
+bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
+				   int cookie);
 
 /*
  * Check if the given console is currently capable and allowed to print
@@ -133,6 +137,8 @@ static inline bool nbcon_alloc(struct console *con) { return false; }
 static inline void nbcon_init(struct console *con) { }
 static inline void nbcon_free(struct console *con) { }
 static inline void nbcon_atomic_flush_all(void) { }
+static inline bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
+						 int cookie) { return false; }
 
 static inline bool console_is_usable(struct console *con, short flags) { return false; }
 
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 2eb2929c1027..747f5cbfe5ee 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -532,6 +532,7 @@ static struct printk_buffers panic_nbcon_pbufs;
  * nbcon_context_try_acquire - Try to acquire nbcon console
  * @ctxt:	The context of the caller
  *
+ * Context:	Any context which could not be migrated to another CPU.
  * Return:	True if the console was acquired. False otherwise.
  *
  * If the caller allowed an unsafe hostile takeover, on success the
@@ -960,6 +961,50 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
 	return ctxt->backlog;
 }
 
+/**
+ * nbcon_legacy_emit_next_record - Print one record for an nbcon console
+ *					in legacy contexts
+ * @con:	The console to print on
+ * @handover:	Will be set to true if a printk waiter has taken over the
+ *		console_lock, in which case the caller is no longer holding
+ *		both the console_lock and the SRCU read lock. Otherwise it
+ *		is set to false.
+ * @cookie:	The cookie from the SRCU read lock.
+ *
+ * Context:	Any context which could not be migrated to another CPU.
+ * Return:	True if a record could be printed, otherwise false.
+ *
+ * This function is meant to be called by console_flush_all() to print records
+ * on nbcon consoles from legacy context (printing via console unlocking).
+ * Essentially it is the nbcon version of console_emit_next_record().
+ */
+bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
+				   int cookie)
+{
+	struct nbcon_write_context wctxt = { };
+	struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
+	bool progress = false;
+	unsigned long flags;
+
+	*handover = false;
+
+	/* Use the same procedure as console_emit_next_record(). */
+	printk_safe_enter_irqsave(flags);
+	console_lock_spinning_enable();
+	stop_critical_timings();
+
+	ctxt->console	= con;
+	ctxt->prio	= NBCON_PRIO_NORMAL;
+
+	progress = nbcon_atomic_emit_one(&wctxt);
+
+	start_critical_timings();
+	*handover = console_lock_spinning_disable_and_check(cookie);
+	printk_safe_exit_irqrestore(flags);
+
+	return progress;
+}
+
 /**
  * __nbcon_atomic_flush_all - Flush all nbcon consoles using their
  *					write_atomic() callback
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1b14159990ba..d91771fb306e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1871,7 +1871,7 @@ static bool console_waiter;
  * there may be a waiter spinning (like a spinlock). Also it must be
  * ready to hand over the lock at the end of the section.
  */
-static void console_lock_spinning_enable(void)
+void console_lock_spinning_enable(void)
 {
 	/*
 	 * Do not use spinning in panic(). The panic CPU wants to keep the lock.
@@ -1910,7 +1910,7 @@ static void console_lock_spinning_enable(void)
  *
  * Return: 1 if the lock rights were passed, 0 otherwise.
  */
-static int console_lock_spinning_disable_and_check(int cookie)
+int console_lock_spinning_disable_and_check(int cookie)
 {
 	int waiter;
 
@@ -2948,13 +2948,22 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 		cookie = console_srcu_read_lock();
 		for_each_console_srcu(con) {
 			short flags = console_srcu_read_flags(con);
+			u64 printk_seq;
 			bool progress;
 
 			if (!console_is_usable(con, flags))
 				continue;
 			any_usable = true;
 
-			progress = console_emit_next_record(con, handover, cookie);
+			if (flags & CON_NBCON) {
+				progress = nbcon_legacy_emit_next_record(con, handover, cookie);
+
+				printk_seq = nbcon_seq_read(con);
+			} else {
+				progress = console_emit_next_record(con, handover, cookie);
+
+				printk_seq = con->seq;
+			}
 
 			/*
 			 * If a handover has occurred, the SRCU read lock
@@ -2964,8 +2973,8 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 				return false;
 
 			/* Track the next of the highest seq flushed. */
-			if (con->seq > *next_seq)
-				*next_seq = con->seq;
+			if (printk_seq > *next_seq)
+				*next_seq = printk_seq;
 
 			if (!progress)
 				continue;
-- 
2.39.2

[PATCH printk v2 17/26] printk: nbcon: Assign priority based on CPU state

[John Ogness]

Use the current state of the CPU to determine which priority to
assign to the printing context.

Note: The uart_port wrapper, which is responsible for non-console-
      printing activities, will always use NORMAL priority.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/internal.h |  2 ++
 kernel/printk/nbcon.c    | 30 ++++++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index b34847ec6b0d..70e9a1ea75be 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -79,6 +79,7 @@ void nbcon_seq_force(struct console *con, u64 seq);
 bool nbcon_alloc(struct console *con);
 void nbcon_init(struct console *con);
 void nbcon_free(struct console *con);
+enum nbcon_prio nbcon_get_default_prio(void);
 void nbcon_atomic_flush_all(void);
 bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
 				   int cookie);
@@ -136,6 +137,7 @@ static inline void nbcon_seq_force(struct console *con, u64 seq) { }
 static inline bool nbcon_alloc(struct console *con) { return false; }
 static inline void nbcon_init(struct console *con) { }
 static inline void nbcon_free(struct console *con) { }
+static inline enum nbcon_prio nbcon_get_default_prio(void) { return NBCON_PRIO_NONE; }
 static inline void nbcon_atomic_flush_all(void) { }
 static inline bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
 						 int cookie) { return false; }
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 747f5cbfe5ee..1a18549e43b8 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -961,6 +961,22 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
 	return ctxt->backlog;
 }
 
+/**
+ * nbcon_get_default_prio - The appropriate nbcon priority to use for nbcon
+ *				printing on the current CPU
+ *
+ * Context:	Any context which could not be migrated to another CPU.
+ * Return:	The nbcon_prio to use for acquiring an nbcon console in this
+ *		context for printing.
+ */
+enum nbcon_prio nbcon_get_default_prio(void)
+{
+	if (this_cpu_in_panic())
+		return NBCON_PRIO_PANIC;
+
+	return NBCON_PRIO_NORMAL;
+}
+
 /**
  * nbcon_legacy_emit_next_record - Print one record for an nbcon console
  *					in legacy contexts
@@ -994,7 +1010,7 @@ bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
 	stop_critical_timings();
 
 	ctxt->console	= con;
-	ctxt->prio	= NBCON_PRIO_NORMAL;
+	ctxt->prio	= nbcon_get_default_prio();
 
 	progress = nbcon_atomic_emit_one(&wctxt);
 
@@ -1038,7 +1054,6 @@ static void __nbcon_atomic_flush_all(u64 stop_seq)
 			memset(ctxt, 0, sizeof(*ctxt));
 			ctxt->console			= con;
 			ctxt->spinwait_max_us		= 2000;
-			ctxt->prio			= NBCON_PRIO_NORMAL;
 
 			/*
 			 * Atomic flushing does not use console driver
@@ -1047,9 +1062,14 @@ static void __nbcon_atomic_flush_all(u64 stop_seq)
 			 * disabled to avoid being interrupted and then
 			 * calling into a driver that will deadlock trying
 			 * acquire console ownership.
+			 *
+			 * This also disables migration in order to get the
+			 * current CPU priority.
 			 */
 			local_irq_save(irq_flags);
 
+			ctxt->prio = nbcon_get_default_prio();
+
 			any_progress |= nbcon_atomic_emit_one(&wctxt);
 
 			local_irq_restore(irq_flags);
@@ -1161,6 +1181,9 @@ static inline bool uart_is_nbcon(struct uart_port *up)
  *
  * If @up is an nbcon console, this console will be acquired and marked as
  * unsafe. Otherwise this function does nothing.
+ *
+ * nbcon consoles acquired via the port lock wrapper always use priority
+ * NBCON_PRIO_NORMAL.
  */
 void uart_nbcon_acquire(struct uart_port *up)
 {
@@ -1195,6 +1218,9 @@ EXPORT_SYMBOL_GPL(uart_nbcon_acquire);
  *
  * If @up is an nbcon console, the console will be marked as safe and
  * released. Otherwise this function does nothing.
+ *
+ * nbcon consoles acquired via the port lock wrapper always use priority
+ * NBCON_PRIO_NORMAL.
  */
 void uart_nbcon_release(struct uart_port *up)
 {
-- 
2.39.2

[PATCH printk v2 18/26] printk: nbcon: Add unsafe flushing on panic

[John Ogness]

Add nbcon_atomic_flush_unsafe() to flush all nbcon consoles
using the write_atomic() callback and allowing unsafe hostile
takeovers. Call this at the end of panic() as a final attempt
to flush any pending messages.

Note that legacy consoles use unsafe methods for flushing
from the beginning of panic (see bust_spinlocks()). Therefore,
systems using both legacy and nbcon consoles may still fail to
see panic messages due to unsafe legacy console usage.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/printk.h |  5 +++++
 kernel/panic.c         |  1 +
 kernel/printk/nbcon.c  | 18 ++++++++++++++++--
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index ef57a4d93ae2..fe37395af9e2 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -199,6 +199,7 @@ extern asmlinkage void dump_stack(void) __cold;
 void printk_trigger_flush(void);
 extern void uart_nbcon_acquire(struct uart_port *up);
 extern void uart_nbcon_release(struct uart_port *up);
+void nbcon_atomic_flush_unsafe(void);
 #else
 static inline __printf(1, 0)
 int vprintk(const char *s, va_list args)
@@ -287,6 +288,10 @@ static inline void uart_nbcon_release(struct uart_port *up)
 {
 }
 
+static inline void nbcon_atomic_flush_unsafe(void)
+{
+}
+
 #endif
 
 bool this_cpu_in_panic(void);
diff --git a/kernel/panic.c b/kernel/panic.c
index f22d8f33ea14..c039f8e1ddae 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -453,6 +453,7 @@ void panic(const char *fmt, ...)
 	 * Explicitly flush the kernel log buffer one last time.
 	 */
 	console_flush_on_panic(CONSOLE_FLUSH_PENDING);
+	nbcon_atomic_flush_unsafe();
 
 	local_irq_enable();
 	for (i = 0; ; i += PANIC_TIMER_STEP) {
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 1a18549e43b8..64dedd79e880 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1025,8 +1025,9 @@ bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
  * __nbcon_atomic_flush_all - Flush all nbcon consoles using their
  *					write_atomic() callback
  * @stop_seq:			Flush up until this record
+ * @allow_unsafe_takeover:	True, to allow unsafe hostile takeovers
  */
-static void __nbcon_atomic_flush_all(u64 stop_seq)
+static void __nbcon_atomic_flush_all(u64 stop_seq, bool allow_unsafe_takeover)
 {
 	struct nbcon_write_context wctxt = { };
 	struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
@@ -1054,6 +1055,7 @@ static void __nbcon_atomic_flush_all(u64 stop_seq)
 			memset(ctxt, 0, sizeof(*ctxt));
 			ctxt->console			= con;
 			ctxt->spinwait_max_us		= 2000;
+			ctxt->allow_unsafe_takeover	= allow_unsafe_takeover;
 
 			/*
 			 * Atomic flushing does not use console driver
@@ -1088,7 +1090,19 @@ static void __nbcon_atomic_flush_all(u64 stop_seq)
  */
 void nbcon_atomic_flush_all(void)
 {
-	__nbcon_atomic_flush_all(prb_next_reserve_seq(prb));
+	__nbcon_atomic_flush_all(prb_next_reserve_seq(prb), false);
+}
+
+/**
+ * nbcon_atomic_flush_unsafe - Flush all nbcon consoles using their
+ *	write_atomic() callback and allowing unsafe hostile takeovers
+ *
+ * Flush the backlog up through the currently newest record. Unsafe hostile
+ * takeovers will be performed, if necessary.
+ */
+void nbcon_atomic_flush_unsafe(void)
+{
+	__nbcon_atomic_flush_all(prb_next_reserve_seq(prb), true);
 }
 
 /**
-- 
2.39.2

[PATCH printk v2 19/26] printk: Avoid console_lock dance if no legacy or boot consoles

[John Ogness]

Currently the console lock is used to attempt legacy-type
printing even if there are no legacy or boot consoles registered.
If no such consoles are registered, the console lock does not
need to be taken.

Add tracking of legacy console registration and use it with
boot console tracking to avoid unnecessary code paths, i.e.
do not use the console lock if there are no boot consoles
and no legacy consoles.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/internal.h | 12 ++++++++
 kernel/printk/printk.c   | 59 ++++++++++++++++++++++++++++++----------
 2 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 70e9a1ea75be..74181c71776f 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -44,6 +44,16 @@ enum printk_info_flags {
 };
 
 extern struct printk_ringbuffer *prb;
+extern bool have_legacy_console;
+extern bool have_boot_console;
+
+/*
+ * Specifies if the console lock/unlock dance is needed for console
+ * printing. If @have_boot_console is true, the nbcon consoles will
+ * be printed serially along with the legacy consoles because nbcon
+ * consoles cannot print simultaneously with boot consoles.
+ */
+#define printing_via_unlock (have_legacy_console || have_boot_console)
 
 __printf(4, 0)
 int vprintk_store(int facility, int level,
@@ -123,6 +133,8 @@ static inline bool console_is_usable(struct console *con, short flags)
 #define PRINTK_MESSAGE_MAX	0
 #define PRINTKRB_RECORD_MAX	0
 
+#define printing_via_unlock (false)
+
 /*
  * In !PRINTK builds we still export console_sem
  * semaphore and some of console functions (console_unlock()/etc.), so
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d91771fb306e..336dad179b99 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -463,6 +463,13 @@ static int console_msg_format = MSG_FORMAT_DEFAULT;
 /* syslog_lock protects syslog_* variables and write access to clear_seq. */
 static DEFINE_MUTEX(syslog_lock);
 
+/*
+ * Specifies if a legacy console is registered. If legacy consoles are
+ * present, it is necessary to perform the console_lock/console_unlock dance
+ * whenever console flushing should occur.
+ */
+bool have_legacy_console;
+
 /*
  * Specifies if a boot console is registered. If boot consoles are present,
  * nbcon consoles cannot print simultaneously and must be synchronized by
@@ -2344,7 +2351,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 	printed_len = vprintk_store(facility, level, dev_info, fmt, args);
 
 	/* If called from the scheduler, we can not call up(). */
-	if (!in_sched) {
+	if (!in_sched && printing_via_unlock) {
 		/*
 		 * The caller may be holding system-critical or
 		 * timing-sensitive locks. Disable preemption during
@@ -2645,7 +2652,7 @@ void resume_console(void)
  */
 static int console_cpu_notify(unsigned int cpu)
 {
-	if (!cpuhp_tasks_frozen) {
+	if (!cpuhp_tasks_frozen && printing_via_unlock) {
 		/* If trylock fails, someone else is doing the printing */
 		if (console_trylock())
 			console_unlock();
@@ -3188,7 +3195,8 @@ void console_flush_on_panic(enum con_flush_mode mode)
 
 	nbcon_atomic_flush_all();
 
-	console_flush_all(false, &next_seq, &handover);
+	if (printing_via_unlock)
+		console_flush_all(false, &next_seq, &handover);
 }
 
 /*
@@ -3513,8 +3521,11 @@ 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);
+	} else {
+		have_legacy_console = true;
+	}
 
 	if (newcon->flags & CON_BOOT)
 		have_boot_console = true;
@@ -3571,6 +3582,7 @@ EXPORT_SYMBOL(register_console);
 /* Must be called under console_list_lock(). */
 static int unregister_console_locked(struct console *console)
 {
+	bool found_legacy_con = false;
 	bool found_boot_con = false;
 	struct console *c;
 	int res;
@@ -3627,9 +3639,13 @@ static int unregister_console_locked(struct console *console)
 	for_each_console(c) {
 		if (c->flags & CON_BOOT)
 			found_boot_con = true;
+		if (!(c->flags & CON_NBCON))
+			found_legacy_con = true;
 	}
 	if (!found_boot_con)
 		have_boot_console = false;
+	if (!found_legacy_con)
+		have_legacy_console = false;
 
 	return res;
 }
@@ -3781,6 +3797,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
 	u64 last_diff = 0;
 	u64 printk_seq;
 	short flags;
+	bool locked;
 	int cookie;
 	u64 diff;
 	u64 seq;
@@ -3790,22 +3807,28 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
 	seq = prb_next_reserve_seq(prb);
 
 	/* Flush the consoles so that records up to @seq are printed. */
-	console_lock();
-	console_unlock();
+	if (printing_via_unlock) {
+		console_lock();
+		console_unlock();
+	}
 
 	for (;;) {
 		unsigned long begin_jiffies;
 		unsigned long slept_jiffies;
 
+		locked = false;
 		diff = 0;
 
-		/*
-		 * Hold the console_lock to guarantee safe access to
-		 * console->seq. Releasing console_lock flushes more
-		 * records in case @seq is still not printed on all
-		 * usable consoles.
-		 */
-		console_lock();
+		if (printing_via_unlock) {
+			/*
+			 * Hold the console_lock to guarantee safe access to
+			 * console->seq. Releasing console_lock flushes more
+			 * records in case @seq is still not printed on all
+			 * usable consoles.
+			 */
+			console_lock();
+			locked = true;
+		}
 
 		cookie = console_srcu_read_lock();
 		for_each_console_srcu(c) {
@@ -3825,6 +3848,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
 			if (flags & CON_NBCON) {
 				printk_seq = nbcon_seq_read(c);
 			} else {
+				WARN_ON_ONCE(!locked);
 				printk_seq = c->seq;
 			}
 
@@ -3836,7 +3860,8 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
 		if (diff != last_diff && reset_on_progress)
 			remaining_jiffies = timeout_jiffies;
 
-		console_unlock();
+		if (locked)
+			console_unlock();
 
 		/* Note: @diff is 0 if there are no usable consoles. */
 		if (diff == 0 || remaining_jiffies == 0)
@@ -3958,7 +3983,11 @@ void defer_console_output(void)
 	 * New messages may have been added directly to the ringbuffer
 	 * using vprintk_store(), so wake any waiters as well.
 	 */
-	__wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT);
+	int val = PRINTK_PENDING_WAKEUP;
+
+	if (printing_via_unlock)
+		val |= PRINTK_PENDING_OUTPUT;
+	__wake_up_klogd(val);
 }
 
 void printk_trigger_flush(void)
-- 
2.39.2

[PATCH printk v2 20/26] printk: Track nbcon consoles

[John Ogness]

Add a global flag @have_nbcon_console to identify if any nbcon
consoles are registered. This will be used in follow-up commits
to preserve legacy behavior when no nbcon consoles are registered.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 336dad179b99..2c4ab717c110 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -470,6 +470,13 @@ static DEFINE_MUTEX(syslog_lock);
  */
 bool have_legacy_console;
 
+/*
+ * Specifies if an nbcon console is registered. If nbcon consoles are present,
+ * synchronous printing of legacy consoles will not occur during panic until
+ * the backtrace has been stored to the ringbuffer.
+ */
+static bool have_nbcon_console;
+
 /*
  * Specifies if a boot console is registered. If boot consoles are present,
  * nbcon consoles cannot print simultaneously and must be synchronized by
@@ -3522,6 +3529,7 @@ void register_console(struct console *newcon)
 	console_init_seq(newcon, bootcon_registered);
 
 	if (newcon->flags & CON_NBCON) {
+		have_nbcon_console = true;
 		nbcon_init(newcon);
 	} else {
 		have_legacy_console = true;
@@ -3583,6 +3591,7 @@ EXPORT_SYMBOL(register_console);
 static int unregister_console_locked(struct console *console)
 {
 	bool found_legacy_con = false;
+	bool found_nbcon_con = false;
 	bool found_boot_con = false;
 	struct console *c;
 	int res;
@@ -3639,13 +3648,18 @@ static int unregister_console_locked(struct console *console)
 	for_each_console(c) {
 		if (c->flags & CON_BOOT)
 			found_boot_con = true;
-		if (!(c->flags & CON_NBCON))
+
+		if (c->flags & CON_NBCON)
+			found_nbcon_con = true;
+		else
 			found_legacy_con = true;
 	}
 	if (!found_boot_con)
 		have_boot_console = false;
 	if (!found_legacy_con)
 		have_legacy_console = false;
+	if (!found_nbcon_con)
+		have_nbcon_console = false;
 
 	return res;
 }
-- 
2.39.2

[PATCH printk v2 21/26] printk: Coordinate direct printing in panic

[John Ogness]

Perform printing by nbcon consoles on the panic CPU from the
printk() caller context in order to get panic messages printed
as soon as possible.

If legacy and nbcon consoles are registered, the legacy consoles
will no longer perform direct printing on the panic CPU until
after the backtrace has been stored. This will give the safe
nbcon consoles a chance to print the panic messages before
allowing the unsafe legacy consoles to print.

If no nbcon consoles are registered, there is no change in
behavior (i.e. legacy consoles will always attempt to print
from the printk() caller context).

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/printk.h |  5 ++++
 kernel/panic.c         |  2 ++
 kernel/printk/printk.c | 53 ++++++++++++++++++++++++++++++++++++------
 3 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index fe37395af9e2..a2d40a637226 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -197,6 +197,7 @@ void show_regs_print_info(const char *log_lvl);
 extern asmlinkage void dump_stack_lvl(const char *log_lvl) __cold;
 extern asmlinkage void dump_stack(void) __cold;
 void printk_trigger_flush(void);
+void printk_legacy_allow_panic_sync(void);
 extern void uart_nbcon_acquire(struct uart_port *up);
 extern void uart_nbcon_release(struct uart_port *up);
 void nbcon_atomic_flush_unsafe(void);
@@ -280,6 +281,10 @@ static inline void printk_trigger_flush(void)
 {
 }
 
+static inline void printk_legacy_allow_panic_sync(void)
+{
+}
+
 static inline void uart_nbcon_acquire(struct uart_port *up)
 {
 }
diff --git a/kernel/panic.c b/kernel/panic.c
index c039f8e1ddae..86813305510f 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -370,6 +370,8 @@ void panic(const char *fmt, ...)
 	 */
 	atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
 
+	printk_legacy_allow_panic_sync();
+
 	panic_print_sys_info(false);
 
 	kmsg_dump(KMSG_DUMP_PANIC);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 2c4ab717c110..109cfdd988aa 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2329,12 +2329,23 @@ int vprintk_store(int facility, int level,
 	return ret;
 }
 
+static bool legacy_allow_panic_sync;
+
+/*
+ * This acts as a one-way switch to allow legacy consoles to print from
+ * the printk() caller context on a panic CPU.
+ */
+void printk_legacy_allow_panic_sync(void)
+{
+	legacy_allow_panic_sync = true;
+}
+
 asmlinkage int vprintk_emit(int facility, int level,
 			    const struct dev_printk_info *dev_info,
 			    const char *fmt, va_list args)
 {
+	bool do_trylock_unlock = printing_via_unlock;
 	int printed_len;
-	bool in_sched = false;
 
 	/* Suppress unimportant messages after panic happens */
 	if (unlikely(suppress_printk))
@@ -2350,15 +2361,43 @@ asmlinkage int vprintk_emit(int facility, int level,
 
 	if (level == LOGLEVEL_SCHED) {
 		level = LOGLEVEL_DEFAULT;
-		in_sched = true;
+		/* If called from the scheduler, we can not call up(). */
+		do_trylock_unlock = false;
 	}
 
 	printk_delay(level);
 
 	printed_len = vprintk_store(facility, level, dev_info, fmt, args);
 
-	/* If called from the scheduler, we can not call up(). */
-	if (!in_sched && printing_via_unlock) {
+	if (!have_boot_console && have_nbcon_console) {
+		bool is_panic_context = this_cpu_in_panic();
+
+		/*
+		 * In panic, the legacy consoles are not allowed to print from
+		 * the printk calling context unless explicitly allowed. This
+		 * gives the safe nbcon consoles a chance to print out all the
+		 * panic messages first. This restriction only applies if
+		 * there are nbcon consoles registered.
+		 */
+		if (is_panic_context)
+			do_trylock_unlock &= legacy_allow_panic_sync;
+
+		/*
+		 * There are situations where nbcon atomic printing should
+		 * happen in the printk() caller context:
+		 *
+		 * - When this CPU is in panic.
+		 *
+		 * Note that if boot consoles are registered, the
+		 * console_lock/console_unlock dance must be relied upon
+		 * instead because nbcon consoles cannot print simultaneously
+		 * with boot consoles.
+		 */
+		if (is_panic_context)
+			nbcon_atomic_flush_all();
+	}
+
+	if (do_trylock_unlock) {
 		/*
 		 * The caller may be holding system-critical or
 		 * timing-sensitive locks. Disable preemption during
@@ -2378,10 +2417,10 @@ asmlinkage int vprintk_emit(int facility, int level,
 		preempt_enable();
 	}
 
-	if (in_sched)
-		defer_console_output();
-	else
+	if (do_trylock_unlock)
 		wake_up_klogd();
+	else
+		defer_console_output();
 
 	return printed_len;
 }
-- 
2.39.2

[PATCH printk v2 22/26] printk: nbcon: Implement emergency sections

[John Ogness]

From: Thomas Gleixner <tglx@linutronix.de>

In emergency situations (something has gone wrong but the
system continues to operate), usually important information
(such as a backtrace) is generated via printk(). Each
individual printk record has little meaning. It is the
collection of printk messages that is most often needed by
developers and users.

In order to help ensure that the collection of printk messages
in an emergency situation are all stored to the ringbuffer as
quickly as possible, disable console output for that CPU while
it is in the emergency situation. When exiting the emergency
situation, trigger the consoles to be flushed.

Add per-CPU emergency nesting tracking because an emergency
can arise while in an emergency situation.

Add functions to mark the beginning and end of emergency
sections where the urgent messages are generated.

Do not print if the current CPU is in an emergency state.

Trigger console flushing when exiting all emergency nesting.

Note that the emergency state is not system-wide. While one CPU
is in an emergency state, another CPU may continue to print
console messages.

Co-developed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
 include/linux/console.h |  4 ++
 kernel/printk/nbcon.c   | 81 +++++++++++++++++++++++++++++++++++++++++
 kernel/printk/printk.c  | 25 ++++++++++---
 3 files changed, 104 insertions(+), 6 deletions(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index d8922282efa1..b1c870898181 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -478,10 +478,14 @@ static inline bool console_is_registered(const struct console *con)
 	hlist_for_each_entry(con, &console_list, node)
 
 #ifdef CONFIG_PRINTK
+extern void nbcon_cpu_emergency_enter(void);
+extern void nbcon_cpu_emergency_exit(void);
 extern bool nbcon_can_proceed(struct nbcon_write_context *wctxt);
 extern bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt);
 extern bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt);
 #else
+static inline void nbcon_cpu_emergency_enter(void) { }
+static inline void nbcon_cpu_emergency_exit(void) { }
 static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return false; }
 static inline bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt) { return false; }
 static inline bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt) { return false; }
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 64dedd79e880..ce556c1f4dc5 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -929,6 +929,29 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
 	return nbcon_context_exit_unsafe(ctxt);
 }
 
+/* Track the nbcon emergency nesting per CPU. */
+static DEFINE_PER_CPU(unsigned int, nbcon_pcpu_emergency_nesting);
+static unsigned int early_nbcon_pcpu_emergency_nesting __initdata;
+
+/**
+ * nbcon_get_cpu_emergency_nesting - Get the per CPU emergency nesting pointer
+ *
+ * Return:	Either a pointer to the per CPU emergency nesting counter of
+ *		the current CPU or to the init data during early boot.
+ */
+static __ref unsigned int *nbcon_get_cpu_emergency_nesting(void)
+{
+	/*
+	 * The value of __printk_percpu_data_ready gets set in normal
+	 * context and before SMP initialization. As a result it could
+	 * never change while inside an nbcon emergency section.
+	 */
+	if (!printk_percpu_data_ready())
+		return &early_nbcon_pcpu_emergency_nesting;
+
+	return this_cpu_ptr(&nbcon_pcpu_emergency_nesting);
+}
+
 /**
  * nbcon_atomic_emit_one - Print one record for an nbcon console using the
  *				write_atomic() callback
@@ -971,9 +994,15 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
  */
 enum nbcon_prio nbcon_get_default_prio(void)
 {
+	unsigned int *cpu_emergency_nesting;
+
 	if (this_cpu_in_panic())
 		return NBCON_PRIO_PANIC;
 
+	cpu_emergency_nesting = nbcon_get_cpu_emergency_nesting();
+	if (*cpu_emergency_nesting)
+		return NBCON_PRIO_EMERGENCY;
+
 	return NBCON_PRIO_NORMAL;
 }
 
@@ -1105,6 +1134,58 @@ void nbcon_atomic_flush_unsafe(void)
 	__nbcon_atomic_flush_all(prb_next_reserve_seq(prb), true);
 }
 
+/**
+ * nbcon_cpu_emergency_enter - Enter an emergency section where printk()
+ *	messages for that CPU are only stored
+ *
+ * Upon exiting the emergency section, all stored messages are flushed.
+ *
+ * Context:	Any context. Disables preemption.
+ *
+ * When within an emergency section, no printing occurs on that CPU. This
+ * is to allow all emergency messages to be dumped into the ringbuffer before
+ * flushing the ringbuffer. The actual printing occurs when exiting the
+ * outermost emergency section.
+ */
+void nbcon_cpu_emergency_enter(void)
+{
+	unsigned int *cpu_emergency_nesting;
+
+	preempt_disable();
+
+	cpu_emergency_nesting = nbcon_get_cpu_emergency_nesting();
+	(*cpu_emergency_nesting)++;
+}
+
+/**
+ * nbcon_cpu_emergency_exit - Exit an emergency section and flush the
+ *	stored messages
+ *
+ * Flushing only occurs when exiting all nesting for the CPU.
+ *
+ * Context:	Any context. Enables preemption.
+ */
+void nbcon_cpu_emergency_exit(void)
+{
+	unsigned int *cpu_emergency_nesting;
+	bool do_trigger_flush = false;
+
+	cpu_emergency_nesting = nbcon_get_cpu_emergency_nesting();
+
+	WARN_ON_ONCE(*cpu_emergency_nesting == 0);
+
+	if (*cpu_emergency_nesting == 1)
+		do_trigger_flush = true;
+
+	/* Undo the nesting count of nbcon_cpu_emergency_enter(). */
+	(*cpu_emergency_nesting)--;
+
+	preempt_enable();
+
+	if (do_trigger_flush)
+		printk_trigger_flush();
+}
+
 /**
  * nbcon_alloc - Allocate buffers needed by the nbcon console
  * @con:	Console to allocate buffers for
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 109cfdd988aa..c8ad2b6ffe63 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2404,16 +2404,29 @@ asmlinkage int vprintk_emit(int facility, int level,
 		 * printing of all remaining records to all consoles so that
 		 * this context can return as soon as possible. Hopefully
 		 * another printk() caller will take over the printing.
+		 *
+		 * Also, nbcon_get_default_prio() requires migration disabled.
 		 */
 		preempt_disable();
+
 		/*
-		 * Try to acquire and then immediately release the console
-		 * semaphore. The release will print out buffers. With the
-		 * spinning variant, this context tries to take over the
-		 * printing from another printing context.
+		 * Do not emit for EMERGENCY priority. The console will be
+		 * explicitly flushed when exiting the emergency section.
 		 */
-		if (console_trylock_spinning())
-			console_unlock();
+		if (nbcon_get_default_prio() == NBCON_PRIO_EMERGENCY) {
+			do_trylock_unlock = false;
+		} else {
+			/*
+			 * Try to acquire and then immediately release the
+			 * console semaphore. The release will print out
+			 * buffers. With the spinning variant, this context
+			 * tries to take over the printing from another
+			 * printing context.
+			 */
+			if (console_trylock_spinning())
+				console_unlock();
+		}
+
 		preempt_enable();
 	}
 
-- 
2.39.2

[PATCH printk v2 23/26] panic: Mark emergency section in warn

[John Ogness]

From: Thomas Gleixner <tglx@linutronix.de>

Mark the full contents of __warn() as an emergency section. In
this section, the CPU will not perform console output for the
printk() calls. Instead, a flushing of the console output is
triggered when exiting the emergency section.

Co-developed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
 kernel/panic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/panic.c b/kernel/panic.c
index 86813305510f..d30d261f9246 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -667,6 +667,8 @@ struct warn_args {
 void __warn(const char *file, int line, void *caller, unsigned taint,
 	    struct pt_regs *regs, struct warn_args *args)
 {
+	nbcon_cpu_emergency_enter();
+
 	disable_trace_on_warning();
 
 	if (file)
@@ -697,6 +699,8 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 
 	/* Just a warning, don't kill lockdep. */
 	add_taint(taint, LOCKDEP_STILL_OK);
+
+	nbcon_cpu_emergency_exit();
 }
 
 #ifdef CONFIG_BUG
-- 
2.39.2

[PATCH printk v2 24/26] panic: Mark emergency section in oops

[John Ogness]

Mark an emergency section beginning with oops_enter() until the
end of oops_exit(). In this section, the CPU will not perform
console output for the printk() calls. Instead, a flushing of the
console output is triggered when exiting the emergency section.

The very end of oops_exit() performs a kmsg_dump(). This is not
included in the emergency section because it is another
flushing mechanism that should occur after the consoles have
been triggered to flush.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/panic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/panic.c b/kernel/panic.c
index d30d261f9246..9fa44bc38f46 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -634,6 +634,7 @@ bool oops_may_print(void)
  */
 void oops_enter(void)
 {
+	nbcon_cpu_emergency_enter();
 	tracing_off();
 	/* can't trust the integrity of the kernel anymore: */
 	debug_locks_off();
@@ -656,6 +657,7 @@ void oops_exit(void)
 {
 	do_oops_enter_exit();
 	print_oops_end_marker();
+	nbcon_cpu_emergency_exit();
 	kmsg_dump(KMSG_DUMP_OOPS);
 }
 
-- 
2.39.2

[PATCH printk v2 25/26] rcu: Mark emergency section in rcu stalls

[John Ogness]

Mark an emergency section within print_other_cpu_stall(), where
RCU stall information is printed. In this section, the CPU will
not perform console output for the printk() calls. Instead, a
flushing of the console output is triggered when exiting the
emergency section.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/rcu/tree_stall.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
index ac8e86babe44..efb2be8939a2 100644
--- a/kernel/rcu/tree_stall.h
+++ b/kernel/rcu/tree_stall.h
@@ -9,6 +9,7 @@
 
 #include <linux/kvm_para.h>
 #include <linux/rcu_notifier.h>
+#include <linux/console.h>
 
 //////////////////////////////////////////////////////////////////////////////
 //
@@ -604,6 +605,8 @@ static void print_other_cpu_stall(unsigned long gp_seq, unsigned long gps)
 	if (rcu_stall_is_suppressed())
 		return;
 
+	nbcon_cpu_emergency_enter();
+
 	/*
 	 * OK, time to rat on our buddy...
 	 * See Documentation/RCU/stallwarn.rst for info on how to debug
@@ -658,6 +661,8 @@ static void print_other_cpu_stall(unsigned long gp_seq, unsigned long gps)
 	panic_on_rcu_stall();
 
 	rcu_force_quiescent_state();  /* Kick them all. */
+
+	nbcon_cpu_emergency_exit();
 }
 
 static void print_cpu_stall(unsigned long gps)
-- 
2.39.2

[PATCH printk v2 26/26] lockdep: Mark emergency section in lockdep splats

[John Ogness]

Mark an emergency section within print_usage_bug(), where
lockdep bugs are printed. In this section, the CPU will not
perform console output for the printk() calls. Instead, a
flushing of the console output is triggered when exiting
the emergency section.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/locking/lockdep.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index e85b5ad3e206..00465373d358 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -56,6 +56,7 @@
 #include <linux/kprobes.h>
 #include <linux/lockdep.h>
 #include <linux/context_tracking.h>
+#include <linux/console.h>
 
 #include <asm/sections.h>
 
@@ -3970,6 +3971,8 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this,
 	if (!debug_locks_off() || debug_locks_silent)
 		return;
 
+	nbcon_cpu_emergency_enter();
+
 	pr_warn("\n");
 	pr_warn("================================\n");
 	pr_warn("WARNING: inconsistent lock state\n");
@@ -3998,6 +4001,8 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this,
 
 	pr_warn("\nstack backtrace:\n");
 	dump_stack();
+
+	nbcon_cpu_emergency_exit();
 }
 
 /*
-- 
2.39.2

Re: [PATCH printk v2 10/26] printk: nbcon: Fix kerneldoc for enums

[Randy Dunlap]

On 2/18/24 10:57, John Ogness wrote:
> Kerneldoc requires enums to be specified as such. Otherwise it is
> interpreted as function documentation.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Randy Dunlap <rdunlap@infradead.org>

Thanks.

> ---
>  include/linux/console.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 5c55faa013e8..d8922282efa1 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -130,7 +130,7 @@ static inline int con_debug_leave(void)
>   */
>  
>  /**
> - * cons_flags - General console flags
> + * enum cons_flags - General console flags
>   * @CON_PRINTBUFFER:	Used by newly registered consoles to avoid duplicate
>   *			output of messages that were already shown by boot
>   *			consoles or read by userspace via syslog() syscall.
> @@ -211,7 +211,7 @@ struct nbcon_state {
>  static_assert(sizeof(struct nbcon_state) <= sizeof(int));
>  
>  /**
> - * nbcon_prio - console owner priority for nbcon consoles
> + * enum nbcon_prio - console owner priority for nbcon consoles
>   * @NBCON_PRIO_NONE:		Unused
>   * @NBCON_PRIO_NORMAL:		Normal (non-emergency) usage
>   * @NBCON_PRIO_EMERGENCY:	Emergency output (WARN/OOPS...)

-- 
#Randy

Re: [PATCH printk v2 26/26] lockdep: Mark emergency section in lockdep splats

[Waiman Long]

On 2/18/24 13:57, John Ogness wrote:
> Mark an emergency section within print_usage_bug(), where
> lockdep bugs are printed. In this section, the CPU will not
> perform console output for the printk() calls. Instead, a
> flushing of the console output is triggered when exiting
> the emergency section.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>   kernel/locking/lockdep.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index e85b5ad3e206..00465373d358 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -56,6 +56,7 @@
>   #include <linux/kprobes.h>
>   #include <linux/lockdep.h>
>   #include <linux/context_tracking.h>
> +#include <linux/console.h>
>   
>   #include <asm/sections.h>
>   
> @@ -3970,6 +3971,8 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this,
>   	if (!debug_locks_off() || debug_locks_silent)
>   		return;
>   
> +	nbcon_cpu_emergency_enter();
> +
>   	pr_warn("\n");
>   	pr_warn("================================\n");
>   	pr_warn("WARNING: inconsistent lock state\n");
> @@ -3998,6 +4001,8 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this,
>   
>   	pr_warn("\nstack backtrace:\n");
>   	dump_stack();
> +
> +	nbcon_cpu_emergency_exit();
>   }
>   
>   /*

lockdep.c has multiple functions that print stuff to the console, like

  - print_circular_bug_header()
  - print_bad_irq_dependency()
  - print_deadlock_bug()
  - print_collision()
  - print_usage_bug()
  - print_irq_inversion_bug()
  - print_lock_invalid_wait_context()
  - print_lock_nested_lock_not_held()
  - print_unlock_imbalance_bug()
  - print_lock_contention_bug()
  - print_freed_lock_bug()
  - print_held_locks_bug()
  - lockdep_rcu_suspicious()

So what is special about print_usage_bug() that it needs this emergency 
treatment but not the other ones?

Cheers,
Longman

Re: [PATCH printk v2 26/26] lockdep: Mark emergency section in lockdep splats

[John Ogness]

On 2024-02-18, Waiman Long <longman@redhat.com> wrote:
> On 2/18/24 13:57, John Ogness wrote:
> lockdep.c has multiple functions that print stuff to the console, like
>
>   - print_circular_bug_header()
>   - print_bad_irq_dependency()
>   - print_deadlock_bug()
>   - print_collision()
>   - print_usage_bug()
>   - print_irq_inversion_bug()
>   - print_lock_invalid_wait_context()
>   - print_lock_nested_lock_not_held()
>   - print_unlock_imbalance_bug()
>   - print_lock_contention_bug()
>   - print_freed_lock_bug()
>   - print_held_locks_bug()
>   - lockdep_rcu_suspicious()
>
> So what is special about print_usage_bug() that it needs this
> emergency treatment but not the other ones?

I do not expect to be able to identify all "emergency printing" paths in
the kernel from the beginning. This series initially marks some sections
that are IMHO interesting for the feature.

As you are implying, for lockdep probably all printing should be
considered emergency. Is it preferred to place the markers outside the
high-level print functions, for example:

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 00465373d358..7a4e4f4a9156 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -2182,10 +2182,12 @@ check_noncircular(struct held_lock *src, struct held_lock *target,
 			*trace = save_trace();
 		}
 
+		nbcon_cpu_emergency_enter();
 		if (src->class_idx == target->class_idx)
 			print_deadlock_bug(current, src, target);
 		else
 			print_circular_bug(&src_entry, target_entry, src, target);
+		nbcon_cpu_emergency_exit();
 	}
 
 	return ret;

Or is it preferred to put them directly around the various pr_warn()
blocks (as the patch in this series is doing)?

John Ogness

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[Andy Shevchenko]

On Sun, Feb 18, 2024 at 08:03:08PM +0106, John Ogness wrote:
> Currently the port->lock wrappers uart_port_lock(),
> uart_port_unlock() (and their variants) only lock/unlock
> the spin_lock.
> 
> If the port is an nbcon console, the wrappers must also
> acquire/release the console and mark the region as unsafe. This
> allows general port->lock synchronization to be synchronized
> with the nbcon console ownership.
> 
> Add a flag to struct uart_port to track nbcon console ownership.

...

> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -6,6 +6,7 @@
>  #include <linux/console.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
> +#include <linux/serial_core.h>

The headers in this file is a mess. But here you can at least keep the piece
ordered, can you?

...

> +static inline bool uart_is_nbcon(struct uart_port *up)
> +{
> +	int cookie;
> +	bool ret;
> +
> +	if (!uart_console(up))
> +		return false;
> +
> +	cookie = console_srcu_read_lock();
> +	ret = (console_srcu_read_flags(up->cons) & CON_NBCON);

The outer parentheses are redundant.

> +	console_srcu_read_unlock(cookie);
> +	return ret;
> +}

...

> +void uart_nbcon_acquire(struct uart_port *up)
> +{
> +	struct console *con = up->cons;
> +	struct nbcon_context ctxt;
> +
> +	if (!uart_is_nbcon(up))
> +		return;

> +	WARN_ON_ONCE(up->nbcon_locked_port);

+ include linux/bug.h

> +	do {
> +		do {
> +			memset(&ctxt, 0, sizeof(ctxt));

+ include linux/string.h

> +			ctxt.console	= con;
> +			ctxt.prio	= NBCON_PRIO_NORMAL;
> +		} while (!nbcon_context_try_acquire(&ctxt));
> +
> +	} while (!nbcon_context_enter_unsafe(&ctxt));
> +
> +	up->nbcon_locked_port = true;
> +}
> +EXPORT_SYMBOL_GPL(uart_nbcon_acquire);

+ include linux/export.h

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[John Ogness]

On 2024-02-19, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> --- a/kernel/printk/nbcon.c
>> +++ b/kernel/printk/nbcon.c
>> @@ -6,6 +6,7 @@
>>  #include <linux/console.h>
>>  #include <linux/delay.h>
>>  #include <linux/slab.h>
>> +#include <linux/serial_core.h>
>
> The headers in this file is a mess. But here you can at least keep the
> piece ordered, can you?

Just to clarify, you would like to see this ordering and inclusion?

#include <linux/bug.h>
#include <linux/console.h>
#include <linux/delay.h>
#include <linux/export.h>
#include <linux/kernel.h>
#include <linux/serial_core.h>
#include <linux/slab.h>
#include <linux/string.h>
#include "internal.h"

>> +	ret = (console_srcu_read_flags(up->cons) & CON_NBCON);
>
> The outer parentheses are redundant.

Ack.

Thanks.

John

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[Andy Shevchenko]

On Mon, Feb 19, 2024 at 03:24:47PM +0106, John Ogness wrote:
> On 2024-02-19, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> >>  #include <linux/console.h>
> >>  #include <linux/delay.h>
> >>  #include <linux/slab.h>
> >> +#include <linux/serial_core.h>
> >
> > The headers in this file is a mess. But here you can at least keep the
> > piece ordered, can you?
> 
> Just to clarify, you would like to see this ordering and inclusion?

Roughly, yes. Ideally it is quite likely that kernel.h is being used as
a 'proxy' header. Nowadays, it's rare the code needs kernel.h.

> #include <linux/bug.h>
> #include <linux/console.h>
> #include <linux/delay.h>
> #include <linux/export.h>
> #include <linux/kernel.h>
> #include <linux/serial_core.h>
> #include <linux/slab.h>
> #include <linux/string.h>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH printk v2 26/26] lockdep: Mark emergency section in lockdep splats

[Waiman Long]

On 2/19/24 06:11, John Ogness wrote:
> On 2024-02-18, Waiman Long <longman@redhat.com> wrote:
>> On 2/18/24 13:57, John Ogness wrote:
>> lockdep.c has multiple functions that print stuff to the console, like
>>
>>    - print_circular_bug_header()
>>    - print_bad_irq_dependency()
>>    - print_deadlock_bug()
>>    - print_collision()
>>    - print_usage_bug()
>>    - print_irq_inversion_bug()
>>    - print_lock_invalid_wait_context()
>>    - print_lock_nested_lock_not_held()
>>    - print_unlock_imbalance_bug()
>>    - print_lock_contention_bug()
>>    - print_freed_lock_bug()
>>    - print_held_locks_bug()
>>    - lockdep_rcu_suspicious()
>>
>> So what is special about print_usage_bug() that it needs this
>> emergency treatment but not the other ones?
> I do not expect to be able to identify all "emergency printing" paths in
> the kernel from the beginning. This series initially marks some sections
> that are IMHO interesting for the feature.
That is what I like to see in the changelog. I am aware that this patch 
is probably not complete, but you need to set the right expectation that 
similar changes will have to be done elsewhere in lockdep to complete 
the change. We can make the other necessary changes after this patch 
series have been merged. It also helps if you can document what 
undesirable thing may happen if printk() is called without setting the 
emergency mode.
>   
>
> As you are implying, for lockdep probably all printing should be
> considered emergency. Is it preferred to place the markers outside the
> high-level print functions, for example:
>
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 00465373d358..7a4e4f4a9156 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -2182,10 +2182,12 @@ check_noncircular(struct held_lock *src, struct held_lock *target,
>   			*trace = save_trace();
>   		}
>   
> +		nbcon_cpu_emergency_enter();
>   		if (src->class_idx == target->class_idx)
>   			print_deadlock_bug(current, src, target);
>   		else
>   			print_circular_bug(&src_entry, target_entry, src, target);
> +		nbcon_cpu_emergency_exit();
>   	}
>   
>   	return ret;
>
> Or is it preferred to put them directly around the various pr_warn()
> blocks (as the patch in this series is doing)?

There are pros and cons for both. It will depend on how expensive is the 
nbcon_cpu_emergency_{enter|exit}() call as printing won't happen if 
lockdep is turned off somehow. Since lockdep is for debugging and 
efficiency isn't that important, putting the emergency enter/exit 
markers outside the high level print functions will make it a bit easier 
to read.

My 2 cents.

Cheers,
Longman


>
> John Ogness
>

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[John Ogness]

On 2024-02-19, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>>> The headers in this file is a mess. But here you can at least keep the
>>> piece ordered, can you?
>> 
>> Just to clarify, you would like to see this ordering and inclusion?
>
> Roughly, yes. Ideally it is quite likely that kernel.h is being used as
> a 'proxy' header. Nowadays, it's rare the code needs kernel.h.

So I took the time to painfully discover every header that is required
for nbcon.c without any proxy usage. It came down to this:

#include <linux/atomic.h>
#include <linux/bug.h>
#include <linux/compiler.h>
#include <linux/console.h>
#include <linux/delay.h>
#include <linux/errno.h>
#include <linux/export.h>
#include <linux/init.h>
#include <linux/irqflags.h>
#include <linux/minmax.h>
#include <linux/percpu-defs.h>
#include <linux/preempt.h>
#include <linux/serial_core.h>
#include <linux/slab.h>
#include <linux/smp.h>
#include <linux/stddef.h>
#include <linux/string.h>
#include <linux/types.h>
#include "internal.h"

For the next version of this series I will only add the includes you
suggested, but will follow-up with a patch that fixes all proxy headers
for nbcon.c. As a separate patch it will help with bisecting in case the
ordering causes an explosion on some config/architecture.

John

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[Andy Shevchenko]

On Mon, Feb 19, 2024 at 05:58:41PM +0106, John Ogness wrote:
> On 2024-02-19, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >>> The headers in this file is a mess. But here you can at least keep the
> >>> piece ordered, can you?
> >> 
> >> Just to clarify, you would like to see this ordering and inclusion?
> >
> > Roughly, yes. Ideally it is quite likely that kernel.h is being used as
> > a 'proxy' header. Nowadays, it's rare the code needs kernel.h.
> 
> So I took the time to painfully discover every header that is required
> for nbcon.c without any proxy usage. It came down to this:
> 
> #include <linux/atomic.h>
> #include <linux/bug.h>

> #include <linux/compiler.h>

This is guaranteed to be included by types.h, can be dropped.

> #include <linux/console.h>
> #include <linux/delay.h>
> #include <linux/errno.h>
> #include <linux/export.h>
> #include <linux/init.h>
> #include <linux/irqflags.h>
> #include <linux/minmax.h>

> #include <linux/percpu-defs.h>

This...

> #include <linux/preempt.h>
> #include <linux/serial_core.h>
> #include <linux/slab.h>

> #include <linux/smp.h>

...and this I believe can be represented by percpu.h as most likely that is the
"main" library you are using.

> #include <linux/stddef.h>
> #include <linux/string.h>
> #include <linux/types.h>
> #include "internal.h"
> 
> For the next version of this series I will only add the includes you
> suggested, but will follow-up with a patch that fixes all proxy headers
> for nbcon.c. As a separate patch it will help with bisecting in case the
> ordering causes an explosion on some config/architecture.

Sure, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH printk v2 04/26] printk: Consider nbcon boot consoles on seq init

[Petr Mladek]

On Sun 2024-02-18 20:03: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
> always 0.

This says that con->seq is always 0.

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3391,11 +3391,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;

But this sets con->seq to some value even for nbcon consoles.

It would make more sense to use nbcon_seq_force() for nbcon
consoles here and remove the copying from nbcon_init().

Otherwise, it looks good.

Best Regards,
Petr

Re: [PATCH printk v2 05/26] printk: Add notation to console_srcu locking

[Petr Mladek]

On Sun 2024-02-18 20:03:05, John Ogness wrote:
> kernel/printk/printk.c:284:5: sparse: sparse: context imbalance in
> 'console_srcu_read_lock' - wrong count at exit
> include/linux/srcu.h:301:9: sparse: sparse: context imbalance in
> 'console_srcu_read_unlock' - unexpected unlock
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: 6c4afa79147e ("printk: Prepare for SRCU console list protection")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 06/26] printk: nbcon: Ensure ownership release on failed emit

[Petr Mladek]

On Sun 2024-02-18 20:03:06, John Ogness wrote:
> Until now it was assumed that ownership has been lost when the
> write_atomic() callback fails. And nbcon_emit_next_record()
> directly returned false. However, if nbcon_emit_next_record()
> returns false, the context must no longer have ownership.
> 
> The semantics for the callbacks could be specified such that
> if they return false, they must have released ownership. But
> in practice those semantics seem odd since the ownership was
> acquired outside of the callback.
> 
> Ensure ownership has been released before reporting failure by
> explicitly attempting a release. If the current context is not
> the owner, the release has no effect.

Hmm, the new semantic is not ideal either. And I think that it is
even worse. The function still releases the owership even though
it has been acquired by the caller. In addition, it causes
a double unlock in a valid case. I know that the 2nd
nbcon_context_release() is a NOP but...

I would personally solve this by adding a comment into the code
and moving the check, see below.

> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -891,17 +891,18 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
>  	nbcon_state_read(con, &cur);
>  	wctxt->unsafe_takeover = cur.unsafe_takeover;
>  
> -	if (con->write_atomic) {
> +	if (con->write_atomic)
>  		done = con->write_atomic(con, wctxt);
> -	} else {

	This code path does not create a bad semantic. The semantic is
	as it is because the context might lose the ownership in "any"
	nested function.

	Well, it might deserve a comment, something like:

		/*
		 * nbcon_emit_next_record() should never be called for legacy
		 * consoles. Handle it as if write_atomic() have lost
		 * the ownership and try to continue.
		 */
> -		nbcon_context_release(ctxt);
> -		WARN_ON_ONCE(1);
> -		done = false;
> -	}
>  
> -	/* If not done, the emit was aborted. */
> -	if (!done)
> +	if (!done) {
> +		/*
> +		 * The emit was aborted, probably due to a loss of ownership.
> +		 * Ensure ownership was lost or released before reporting the
> +		 * loss.
> +		 */

Is there a valid reason when con->write_atomic() would return false
and still own the context?

If not, then this would hide bugs and cause double unlock in
the valid case.

> +		nbcon_context_release(ctxt);
>  		return false;

Even better solution might be to do the check at the beginning of
the function. It might look like:

	  if (WARN_ON_ONCE(!con->write_atomic)) {
		/*
		 * This function should never be called for legacy consoles.
		 * Handle it as if write_atomic() have lost the ownership
		 * and try to continue.
		 */
		nbcon_context_release(ctxt);
		return false;
	}


Best Regards,
Petr

Re: [PATCH printk v2 06/26] printk: nbcon: Ensure ownership release on failed emit

[John Ogness]

On 2024-02-20, Petr Mladek <pmladek@suse.com> wrote:
>> Until now it was assumed that ownership has been lost when the
>> write_atomic() callback fails. And nbcon_emit_next_record()
>> directly returned false. However, if nbcon_emit_next_record()
>> returns false, the context must no longer have ownership.
>> 
>> The semantics for the callbacks could be specified such that
>> if they return false, they must have released ownership. But
>> in practice those semantics seem odd since the ownership was
>> acquired outside of the callback.
>> 
>> Ensure ownership has been released before reporting failure by
>> explicitly attempting a release. If the current context is not
>> the owner, the release has no effect.
>
> Hmm, the new semantic is not ideal either. And I think that it is
> even worse. The function still releases the owership even though
> it has been acquired by the caller. In addition, it causes
> a double unlock in a valid case. I know that the 2nd
> nbcon_context_release() is a NOP but...
>
> I would personally solve this by adding a comment into the code
> and moving the check, see below.
>
>> --- a/kernel/printk/nbcon.c
>> +++ b/kernel/printk/nbcon.c
>> @@ -891,17 +891,18 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
>>  	nbcon_state_read(con, &cur);
>>  	wctxt->unsafe_takeover = cur.unsafe_takeover;
>>  
>> -	if (con->write_atomic) {
>> +	if (con->write_atomic)
>>  		done = con->write_atomic(con, wctxt);
>> -	} else {
>
> 	This code path does not create a bad semantic. The semantic is
> 	as it is because the context might lose the ownership in "any"
> 	nested function.
>
> 	Well, it might deserve a comment, something like:
>
> 		/*
> 		 * nbcon_emit_next_record() should never be called for legacy
> 		 * consoles. Handle it as if write_atomic() have lost
> 		 * the ownership and try to continue.
> 		 */
>> -		nbcon_context_release(ctxt);
>> -		WARN_ON_ONCE(1);
>> -		done = false;
>> -	}
>>  
>> -	/* If not done, the emit was aborted. */
>> -	if (!done)
>> +	if (!done) {
>> +		/*
>> +		 * The emit was aborted, probably due to a loss of ownership.
>> +		 * Ensure ownership was lost or released before reporting the
>> +		 * loss.
>> +		 */
>
> Is there a valid reason when con->write_atomic() would return false
> and still own the context?

This is driver code, so you must use your imagination. But I thought
maybe there might be some reason why the driver cannot print the message
(due to other driver-internal reasons). In this case, it would return
false even though it never lost ownership.

> If not, then this would hide bugs and cause double unlock in
> the valid case.

Even if true is returned, that does not mean that there is still
ownership (because it can be lost at any time). And even if we hit the
WARN because there is no callback, ownership may have been lost. My
point is that there is _always_ a chance that nbcon_context_release()
will be called when ownership was already lost.

nbcon_context_release() was purposely implemented with the idea that it
may be called by a context that has lost ownership. So why not leverage
this here? It is _critical_ that if _this_ function returns false, the
context no longer has ownership.

We could add a nbcon_can_proceed() in front of the release, but
nbcon_context_release() already does that internally.

>> +		nbcon_context_release(ctxt);
>>  		return false;
>
> Even better solution might be to do the check at the beginning of
> the function. It might look like:
>
> 	  if (WARN_ON_ONCE(!con->write_atomic)) {
> 		/*
> 		 * This function should never be called for legacy consoles.
> 		 * Handle it as if write_atomic() have lost the ownership
> 		 * and try to continue.
> 		 */
> 		nbcon_context_release(ctxt);
> 		return false;
> 	}

In the future, con->write_thread() is added. So the missing callback
check will end up in a final else branch anyway.

John

Re: [PATCH printk v2 07/26] printk: Check printk_deferred_enter()/_exit() usage

[Petr Mladek]

On Sun 2024-02-18 20:03:07, John Ogness wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Add validation that printk_deferred_enter()/_exit() are called in
> non-migration contexts.
> 
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -159,13 +159,16 @@ __printf(1, 2) __cold int _printk_deferred(const char *fmt, ...);
>  
>  extern void __printk_safe_enter(void);
>  extern void __printk_safe_exit(void);

It seems that these two does not longer need to stay in
the global linux/printk.h. The declaration can be
moved to kernel/printk/internal.h (with 'extern' removed).

> +extern void __printk_deferred_enter(void);
> +extern void __printk_deferred_exit(void);
> +
>  /*
>   * The printk_deferred_enter/exit macros are available only as a hack for
>   * some code paths that need to defer all printk console printing. Interrupts
>   * must be disabled for the deferred duration.
>   */
> -#define printk_deferred_enter __printk_safe_enter
> -#define printk_deferred_exit __printk_safe_exit
> +#define printk_deferred_enter() __printk_deferred_enter()
> +#define printk_deferred_exit() __printk_deferred_exit()
>  
>  /*
>   * Please don't use printk_ratelimit(), because it shares ratelimiting state
> diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
> index 6d10927a07d8..8d9408d653de 100644
> --- a/kernel/printk/printk_safe.c
> +++ b/kernel/printk/printk_safe.c
> @@ -26,6 +26,18 @@ void __printk_safe_exit(void)
>  	this_cpu_dec(printk_context);
>  }
>  
> +void __printk_deferred_enter(void)
> +{
> +	cant_migrate();
> +	this_cpu_inc(printk_context);

I would prefer to call __printk_safe_enter() here to make
it clear that they update printk_context the same way
and have the same effect.

Let's make compiler to do the inlining.

> +}
> +
> +void __printk_deferred_exit(void)
> +{
> +	cant_migrate();
> +	this_cpu_dec(printk_context);

ditto

> +}
> +
>  asmlinkage int vprintk(const char *fmt, va_list args)
>  {
>  #ifdef CONFIG_KGDB_KDB

With the two changes:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 06/26] printk: nbcon: Ensure ownership release on failed emit

[John Ogness]

On 2024-02-20, John Ogness <john.ogness@linutronix.de> wrote:
>> Is there a valid reason when con->write_atomic() would return false
>> and still own the context?
>
> This is driver code, so you must use your imagination. But I thought
> maybe there might be some reason why the driver cannot print the
> message (due to other driver-internal reasons). In this case, it would
> return false even though it never lost ownership.

I have been thinking about this. I think there is nothing useful that
write_atomic() can return. I suggest making it a void return. Then the
driver must print the message if ownership was not lost. This is already
how write() works and I think it is fine.

This simplifies nbcon_emit_next_record() because it can assume
write_atomic() was successful and try to enter the unsafe section for
the @seq update. If ownership was lost, it will be detected here. If
not, the message will be considered handled and @seq is updated.

>> 	  if (WARN_ON_ONCE(!con->write_atomic)) {
>> 		/*
>> 		 * This function should never be called for legacy consoles.
>> 		 * Handle it as if write_atomic() have lost the ownership
>> 		 * and try to continue.
>> 		 */
>> 		nbcon_context_release(ctxt);
>> 		return false;
>> 	}

I will keep the WARN with a comment similar to your suggestion.

John

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[Petr Mladek]

On Sun 2024-02-18 20:03:08, John Ogness wrote:
> Currently the port->lock wrappers uart_port_lock(),
> uart_port_unlock() (and their variants) only lock/unlock
> the spin_lock.
> 
> If the port is an nbcon console, the wrappers must also
> acquire/release the console and mark the region as unsafe. This
> allows general port->lock synchronization to be synchronized
> with the nbcon console ownership.
> 
> Add a flag to struct uart_port to track nbcon console ownership.

The patch makes sense.

My main (only) concern was the synchronization of the various accessed
variables, especially, port->cons.

Note: I am not completely sure how the early and valid console drivers
      share the same struct uart_port. So, maybe I miss some important
      guarantee.

Anyway. synchronization of port->cons looks like a shade area.
IMHO, the existing code expects that it is used _only when the console
is registered_. But this patch wants to access it _even before
the console is registered_!

For example, it took me quite a lot of time to shake my head around:

#define uart_console(port) \
	((port)->cons && (port)->cons->index == (port)->line)

  + port->cons and port->line are updated in the uart code.
    It seems that the update is not serialized by port->lock.
    Something might be done under port->mutex.

  + cons->index is updated in register_console() under
    console_list_lock.

Spoiler: I propose a solution which does not use uart_console().

See below for more details.

> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3284,6 +3284,7 @@ void serial8250_init_port(struct uart_8250_port *up)
>  	struct uart_port *port = &up->port;
>  
>  	spin_lock_init(&port->lock);
> +	port->nbcon_locked_port = false;

I am not sure if the variable really helps anything:

   + nbcon_context release() must handle the case when it
     lost the ownership anyway.

     It is the same as if it did not acquire the context
     in the first place [*].


   + nbcon_acquire() is called under port->lock. It means that
     nbcon_release() should always be called when the acquire
     succeeded earlier.

We just need to make sure that port->cons can be cleared only
under port->lock. But this would be required even with
port->nbcon_locked_port.

[*] I am not super-happy with this semantic because it prevents
    catching bugs by lockdep. But it is how nbcon_acquire/release
    work and it is important part of the design.


Apology: It is possible that I suggested to add this variable. I just
	see now that it does not really help much. It rather makes
	a false feeling about that nbcon acquire/release are always
	paired.

>  	port->ctrl_id = 0;
>  	port->pm = NULL;
>  	port->ops = &serial8250_pops;
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -995,3 +996,79 @@ void nbcon_free(struct console *con)
>  
>  	con->pbufs = NULL;
>  }
> +
> +static inline bool uart_is_nbcon(struct uart_port *up)
> +{
> +	int cookie;
> +	bool ret;
> +
> +	if (!uart_console(up))

This function accesses up->cons. I am not completely sure how
this value is synchronized, for example:

  + serial_core_add_one_port() sets uport->cons under port->mutex.
    The function is called uart_add_one_port() in various probe()
    or init() calls.

  + univ8250_console_setup() sets and clears port->cons with
    no explicit synchronization. The function is called from
    try_enable_preferred_console() under console_list_lock.

IMHO, the assignment is done when the drivers are being initialized.
It is done when the port might already be used by early consoles.

Especially, according to my understanding, newcon->setup() callbacks
are responsible for using the same port by early and real console drivers.

I guess that uart_port_lock() API is used by early consoles as well.
It means that they might access up->cons here while it is being
manipulated by the proper driver.

A minimal solution would be access/modify port->cons using
READ_ONCE()/WRITE_ONCE().

But I think that we do not need to call uart_console() at all.
We do not really need to synchronize the consistency of
con->index and port->line.

Instead, we could read up->cons using READ_ONCE() and
check if it is defined. Or even better would be to always
set/read port->cons under the port->lock.


> +		return false;
> +
> +	cookie = console_srcu_read_lock();
> +	ret = (console_srcu_read_flags(up->cons) & CON_NBCON);
> +	console_srcu_read_unlock(cookie);

Hmm, console_srcu_read_flags(con) is called under
console_srcu_read_lock() to make sure that it does not
disappear. It makes sense when it is used by registered consoles.

But uart_port_lock() might be called even when the console
is not registered.

I suggest to remove the SRCU lock here. In this code path,
it does not guarantee anything and is rather misleading.

I would use a READ_ONCE(), for example by splitting:

/*
 * Locklessly reading console->flags provides a consistent
 * read value because there is at most one CPU modifying
 * console->flags and that CPU is using only read-modify-write
 * operations to do so.
 *
 * The caller must make sure that @con does not disappear.
 * It can be done by console_srcu_read_lock() when used
 * only for registered consoles.
 */
static inline short console_read_flags(const struct console *con)
{
	return data_race(READ_ONCE(con->flags));
}

/* Locklessly reading console->flags for registered consoles */
static inline short console_srcu_read_flags(const struct console *con)
{
	WARN_ON_ONCE(!console_srcu_read_lock_is_held());

	console_read_flags();
}

> +	return ret;
> +}
> +
> +/**
> + * uart_nbcon_acquire - The second half of the port locking wrapper
> + * @up:		The uart port whose @lock was locked
> + *
> + * The uart_port_lock() wrappers will first lock the spin_lock @up->lock.
> + * Then this function is called to implement nbcon-specific processing.
> + *
> + * If @up is an nbcon console, this console will be acquired and marked as
> + * unsafe. Otherwise this function does nothing.
> + */
> +void uart_nbcon_acquire(struct uart_port *up)
> +{
> +	struct console *con = up->cons;
> +	struct nbcon_context ctxt;

I would add:

	lockdep_assert_held(&up->lock);
> +
> +	if (!uart_is_nbcon(up))
> +		return;
> +
> +	WARN_ON_ONCE(up->nbcon_locked_port);
> +
> +	do {
> +		do {
> +			memset(&ctxt, 0, sizeof(ctxt));
> +			ctxt.console	= con;
> +			ctxt.prio	= NBCON_PRIO_NORMAL;
> +		} while (!nbcon_context_try_acquire(&ctxt));
> +
> +	} while (!nbcon_context_enter_unsafe(&ctxt));
> +
> +	up->nbcon_locked_port = true;
> +}
> +EXPORT_SYMBOL_GPL(uart_nbcon_acquire);

I would prefer to split the uart and nbcon specific code, for example:

/**
 * nbcon_normal_context_acquire - Acquire a generic context with
 *	the normal priority for nbcon console
 * @con:	nbcon console
 *
 * Context:	Any context which could not be migrated to another CPU.
 *
 * Acquire a generic context with the normal priority for the given console.
 * Prevent the release by entering the unsafe state.
 *
 * Note: The console might still loose the ownership by a hostile takeover.
 *	 But it can be done only by the final flush in panic() when other
 *	 CPUs should be stopped and other contexts interrupted.
 */
static void nbcon_normal_context_acquire(struct console *con)
{
	struct nbcon_context ctxt;

	do {
		do {
			memset(&ctxt, 0, sizeof(ctxt));
			ctxt.console	= con;
			ctxt.prio	= NBCON_PRIO_NORMAL;
		} while (!nbcon_context_try_acquire(&ctxt));

	} while (!nbcon_context_enter_unsafe(&ctxt));
}

/**
 * uart_nbcon_acquire - Acquire nbcon console associated with the gived port.
 * @up:		uart port
 *
 * Context:	Must be called under up->lock to prevent manipulating
 *		up->cons and migrating to another CPU.
 *
 * Note: The console might still loose the ownership by a hostile takeover.
 *	 But it can be done only by the final flush in panic() when other
 *	 CPUs should be stopped and other contexts interrupted.
 */
void uart_nbcon_acquire(struct uart_port *up)
{
	struct console *con; = up->cons;

	lockdep_assert_held(&up->lock);

	/*
	 * FIXME: This would require adding WRITE_ONCE()
	 * on the write part.
	 *
	 * Or even better, the value should be modified under
	 * port->lock so that simple read would be enough here.
	 */
	con = data_race(READ_ONCE(up->cons));

	if (!con)
		return;

	if (!console_read_flags(con) & CON_NBCON)
		return;

	nbcon_normal_context_acquire(con);
}

Note that I did not use up->nbcon_locked_port as explained above.


> +
> +/**
> + * uart_nbcon_release - The first half of the port unlocking wrapper
> + * @up:		The uart port whose @lock is about to be unlocked
> + *
> + * The uart_port_unlock() wrappers will first call this function to implement
> + * nbcon-specific processing. Then afterwards the uart_port_unlock() wrappers
> + * will unlock the spin_lock @up->lock.
> + *
> + * If @up is an nbcon console, the console will be marked as safe and
> + * released. Otherwise this function does nothing.
> + */
> +void uart_nbcon_release(struct uart_port *up)
> +{
> +	struct console *con = up->cons;
> +	struct nbcon_context ctxt = {
> +		.console	= con,
> +		.prio		= NBCON_PRIO_NORMAL,
> +	};
> +

I would add here as well.

	lockdep_assert_held(&up->lock);


This deserves a comment why we do not complain when this function
is called for nbcon and it is not locked. Something like:

	/*
	 * Even port used by nbcon console might be seen unlocked
	 * when it was locked and the console has been registered
	 * at the same time.
	 */
> +	if (!up->nbcon_locked_port)
> +		return;

Wait, if up->cons might be set asynchronously then it might also
disapper assynchronously. Which would be really bad because we would
not be able to release the normal context.

IMHO, we really need to synchronize up->cons acceess using up->lock.

> +
> +	if (nbcon_context_exit_unsafe(&ctxt))
> +		nbcon_context_release(&ctxt);
> +
> +	up->nbcon_locked_port = false;
> +}

Again I would better split the nbcon and uart part and create:

/**
 * nbcon_normal_context_release - Release a generic context with
 *	the normal priority.
 * @con:	nbcon console
 *
 * Context:	Any context which could not be migrated to another CPU.
 *
 * Release a generic context with the normal priority for the given console.
 * Prevent the release by entering the unsafe state.
 *
 * Note: The console might have lost the ownership by a hostile takeover.
 *	 But it should not happen in reality because the hostile
 *       takeover is allowed only for the final flush in panic()
 *	 when other CPUs should be stopped and other contexts interrupted.
 */
static void nbcon_normal_context_release(struct console *con)
{
	struct nbcon_context ctxt = {
		.console	= con,
		.prio		= NBCON_PRIO_NORMAL,
	};

	if (nbcon_context_exit_unsafe(&ctxt))
		nbcon_context_release(&ctxt);
}

/**
 * uart_nbcon_acquire - Release nbcon console associated with the given port.
 * @up:		uart port
 *
 * Context:	Must be called under up->lock to prevent manipulating
 *		up->cons and migrating to another CPU.
 */
void uart_nbcon_release(struct uart_port *up)
{
	struct console *con;

	lockdep_assert_held(&up->lock);

	/*
	 * FIXME: This would require adding WRITE_ONCE()
	 * on the write part.
	 *
	 * Or even better, the value should be modified under
	 * port->lock so that simple read would be enough here.
	 */
	con = data_race(READ_ONCE(up->cons));

	if (!con)
		return;

	if (!console_read_flags(con) & CON_NBCON)
		return;

	nbcon_normal_context_release(con);
}

Best Regards,
Petr

Re: [PATCH printk v2 09/26] printk: nbcon: Add detailed doc for write_atomic()

[Petr Mladek]

On Sun 2024-02-18 20:03:09, John Ogness wrote:
> The write_atomic() callback has special requirements and is
> allowed to use special helper functions. Provide detailed
> documentation of the callback so that a developer has a
> chance of implementing it correctly.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 10/26] printk: nbcon: Fix kerneldoc for enums

[Petr Mladek]

On Sun 2024-02-18 20:03:10, John Ogness wrote:
> Kerneldoc requires enums to be specified as such. Otherwise it is
> interpreted as function documentation.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Good to know. I havn't been aware of it.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 14/26] printk: nbcon: Provide function to flush using write_atomic()

[Petr Mladek]

On Sun 2024-02-18 20:03:14, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Provide nbcon_atomic_flush_all() to perform flushing of all
> registered nbcon consoles using their write_atomic() callback.
> Like with legacy consoles, the nbcon consoles are flushed one
> record per console. This allows all nbcon consoles to print
> lines pseudo-simultaneously, rather than one console waiting
> for the full ringbuffer to dump to another console before
> printing anything.
> 
> Unlike console_flush_all(), nbcon_atomic_flush_all() will only
> flush up through the newest record at the time of the call.
> This prevents a CPU from printing unbounded when other CPUs are
> adding records.

I think about using slightly different name to make the difference
more clear, for example nbcon_atomic_flush_pending() or so.

But I do not have a strong opinion.

> Perform nbcon console atomic flushing in
> console_flush_on_panic(). This function is not only used in
> panic() but also other locations where there may be stored
> messages that need to be flushed.

The above paragraph is a bit misleading. console_flush_on_panic()
is used only in panic(). I guess that you wanted to say something
like:

<proposal>
nbcon_atomic_flush_all() is safe in any context because it uses
write_atomic() and unsafe_takeover is disabled.

Use it in console_flush_on_panic() before flushing legacy consoles.
The legacy write() callbacks are not fully safe when oops_in_progress
is set.
</proposal>


> Co-developed-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>

The code looks good. After updating the commit message,
and eventually the function name, feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 15/26] printk: Track registered boot consoles

[Petr Mladek]

On Sun 2024-02-18 20:03:15, John Ogness wrote:
> Unfortunately it is not known if a boot console and a regular
> (legacy or nbcon) console use the same hardware. For this reason
> they must not be allowed to print simultaneously.
> 
> For legacy consoles this is not an issue because they are
> already synchronized with the boot consoles using the console
> lock. However nbcon consoles can be triggered separately.
> 
> Add a global flag @have_boot_console to identify if any boot
> consoles are registered. This will be used in follow-up commits
> to ensure that boot consoles and nbcon consoles cannot print
> simultaneously.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Just a nit below.

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3598,6 +3611,17 @@ static int unregister_console_locked(struct console *console)
>  	if (console->exit)
>  		res = console->exit(console);
>  
> +	/*
> +	 * With this console gone, the global flags tracking registered
> +	 * console types may have changed. Update them.
> +	 */
> +	for_each_console(c) {
> +		if (c->flags & CON_BOOT)
> +			found_boot_con = true;
> +	}
> +	if (!found_boot_con)
> +		have_boot_console = false;

I would use:

	have_boot_console = found_boot_console;

But I do not have strong opinion. Maybe, you wanted to make
it obvious that the function only clears the flag.

Best Regards,
Petr

Re: [PATCH printk v2 16/26] printk: nbcon: Use nbcon consoles in console_flush_all()

[Petr Mladek]

On Sun 2024-02-18 20:03:16, John Ogness wrote:
> Allow nbcon consoles to print messages in the legacy printk()
> caller context (printing via unlock) by integrating them into
> console_flush_all(). The write_atomic() callback is used for
> printing.
> 
> Provide nbcon_legacy_emit_next_record(), which acts as the
> nbcon variant of console_emit_next_record(). Call this variant
> within console_flush_all() for nbcon consoles. Since nbcon
> consoles use their own @nbcon_seq variable to track the next
> record to print, this also must be appropriately handled.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -960,6 +961,50 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
>  	return ctxt->backlog;
>  }
>  
> +/**
> + * nbcon_legacy_emit_next_record - Print one record for an nbcon console
> + *					in legacy contexts
> + * @con:	The console to print on
> + * @handover:	Will be set to true if a printk waiter has taken over the
> + *		console_lock, in which case the caller is no longer holding
> + *		both the console_lock and the SRCU read lock. Otherwise it
> + *		is set to false.
> + * @cookie:	The cookie from the SRCU read lock.
> + *
> + * Context:	Any context which could not be migrated to another CPU.

This seems to be a relic after shufling the code. IMHO, it was meant
for nbcon_atomic_emit_one(). This function disables interrupts
to prevent CPU migration when calling nbcon_atomic_emit_one().

> + * Return:	True if a record could be printed, otherwise false.

A more precise might be to say "has been printed" instead of
"could be printed".

It is more problematic here. It could return false also when it was
not able to acquire nbcon console.

While console_emit_next_record() has a bit different semantic.
It returns false only when there is no new record.

Thinking loudly:

It probably is not a bit deal. console_flush_all() does not guarantee
that it flushed all messages. It might return early when
the console_lock was handovered. It means there there is
another legacy context flushing the messages.

And nbcon console could not be acquired here also when
there is another nbcon context already flushing the nbcon.

Another question is whether console_flush_all() should return "false"
when nbcon consoles were not flushed because they could not be acquired.
It might be needed so that console_unlock() does not wait until
the other context flushes the nbcon console.

> + *
> + * This function is meant to be called by console_flush_all() to print records
> + * on nbcon consoles from legacy context (printing via console unlocking).
> + * Essentially it is the nbcon version of console_emit_next_record().
> + */
> +bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
> +				   int cookie)
> +{
> +	struct nbcon_write_context wctxt = { };
> +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
> +	bool progress = false;
> +	unsigned long flags;
> +
> +	*handover = false;
> +
> +	/* Use the same procedure as console_emit_next_record(). */
> +	printk_safe_enter_irqsave(flags);
> +	console_lock_spinning_enable();
> +	stop_critical_timings();
> +
> +	ctxt->console	= con;
> +	ctxt->prio	= NBCON_PRIO_NORMAL;
> +
> +	progress = nbcon_atomic_emit_one(&wctxt);
> +
> +	start_critical_timings();
> +	*handover = console_lock_spinning_disable_and_check(cookie);
> +	printk_safe_exit_irqrestore(flags);
> +
> +	return progress;
> +}
> +
>  /**
>   * __nbcon_atomic_flush_all - Flush all nbcon consoles using their
>   *					write_atomic() callback
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2948,13 +2948,22 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
>  		cookie = console_srcu_read_lock();
>  		for_each_console_srcu(con) {
>  			short flags = console_srcu_read_flags(con);
> +			u64 printk_seq;
>  			bool progress;
>  
>  			if (!console_is_usable(con, flags))
>  				continue;
>  			any_usable = true;
>  
> -			progress = console_emit_next_record(con, handover, cookie);
> +			if (flags & CON_NBCON) {
> +				progress = nbcon_legacy_emit_next_record(con, handover, cookie);
> +
> +				printk_seq = nbcon_seq_read(con);

It might looks better without the empty line. But it is just my view.
Feel free to keep it as is.

> +			} else {
> +				progress = console_emit_next_record(con, handover, cookie);
> +
> +				printk_seq = con->seq;
> +			}
>  
>  			/*
>  			 * If a handover has occurred, the SRCU read lock

Best Regards,
Petr

Re: [PATCH printk v2 17/26] printk: nbcon: Assign priority based on CPU state

[Petr Mladek]

On Sun 2024-02-18 20:03:17, John Ogness wrote:
> Use the current state of the CPU to determine which priority to
> assign to the printing context.
> 
> Note: The uart_port wrapper, which is responsible for non-console-
>       printing activities, will always use NORMAL priority.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -961,6 +961,22 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
>  	return ctxt->backlog;
>  }
>  
> +/**
> + * nbcon_get_default_prio - The appropriate nbcon priority to use for nbcon
> + *				printing on the current CPU
> + *
> + * Context:	Any context which could not be migrated to another CPU.

Strictly speaking, this variant does not need to be called with
disabled CPU migration. The panic-CPU could not be migrated.

It would be nice to mention in the commit message, something like:

</proposal>
The emergency context handling is going to be added in a separate
patch. It will use a per-CPU variable.
</proposal>

It would also explain why this context is not handled in this patch.

> + * Return:	The nbcon_prio to use for acquiring an nbcon console in this
> + *		context for printing.
> + */
> +enum nbcon_prio nbcon_get_default_prio(void)
> +{
> +	if (this_cpu_in_panic())
> +		return NBCON_PRIO_PANIC;
> +
> +	return NBCON_PRIO_NORMAL;
> +}
> +
>  /**
>   * nbcon_legacy_emit_next_record - Print one record for an nbcon console
>   *					in legacy contexts

Otherwise, it looks good. With the added commit message:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 18/26] printk: nbcon: Add unsafe flushing on panic

[Petr Mladek]

On Sun 2024-02-18 20:03:18, John Ogness wrote:
> Add nbcon_atomic_flush_unsafe() to flush all nbcon consoles
> using the write_atomic() callback and allowing unsafe hostile
> takeovers. Call this at the end of panic() as a final attempt
> to flush any pending messages.
> 
> Note that legacy consoles use unsafe methods for flushing
> from the beginning of panic (see bust_spinlocks()). Therefore,
> systems using both legacy and nbcon consoles may still fail to
> see panic messages due to unsafe legacy console usage.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 19/26] printk: Avoid console_lock dance if no legacy or boot consoles

[Petr Mladek]

On Sun 2024-02-18 20:03:19, John Ogness wrote:
> Currently the console lock is used to attempt legacy-type
> printing even if there are no legacy or boot consoles registered.
> If no such consoles are registered, the console lock does not
> need to be taken.
> 
> Add tracking of legacy console registration and use it with
> boot console tracking to avoid unnecessary code paths, i.e.
> do not use the console lock if there are no boot consoles
> and no legacy consoles.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3958,7 +3983,11 @@ void defer_console_output(void)
>  	 * New messages may have been added directly to the ringbuffer
>  	 * using vprintk_store(), so wake any waiters as well.
>  	 */
> -	__wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT);
> +	int val = PRINTK_PENDING_WAKEUP;
> +
> +	if (printing_via_unlock)
> +		val |= PRINTK_PENDING_OUTPUT;
> +	__wake_up_klogd(val);

I was thinking about handling this in wake_up_klogd_work_func().
But then I saw that __wake_up_klogd() already handled
PRINTK_PENDING_WAKEUP a similar way. And it even did not
queue the work when there was nothing to do.

It would be nice to handle both on the same place.
It would even untangle the condition in __wake_up_klogd().
Something like:

static void __wake_up_klogd(int val)
{
	if (!printk_percpu_data_ready())
		return;

	preempt_disable();

	/*
	 * Guarantee any new records can be seen by tasks preparing to wait
	 * before this context checks if the wait queue is empty.
	 *
	 * The full memory barrier within wq_has_sleeper() pairs with the full
	 * memory barrier within set_current_state() of
	 * prepare_to_wait_event(), which is called after ___wait_event() adds
	 * the waiter but before it has checked the wait condition.
	 *
	 * This pairs with devkmsg_read:A and syslog_print:A.
	 */
	if (!wq_has_sleeper(&log_wait))		/* LMM(__wake_up_klogd:A) */
		val &= ~PRINTK_PENDING_WAKEUP;

	/*
	 * Simple read is safe. register_console() would flush a newly
	 * registered legacy console when writing the message about
	 * that it has been enabled.
	 */
	if (!printing_via_unlock)
		val &= ~PRINTK_PENDING_OUTPUT;


	if (val) {
		this_cpu_or(printk_pending, val);
		irq_work_queue(this_cpu_ptr(&wake_up_klogd_work));
	}

	preempt_enable();
}

Otherwise, it looks good.

Best Regards,
Petr

READ_ONCE: was: Re: [PATCH printk v2 19/26] printk: Avoid console_lock dance if no legacy or boot consoles

[Petr Mladek]

Adding Paul into Cc.

On Sun 2024-02-18 20:03:19, John Ogness wrote:
> Currently the console lock is used to attempt legacy-type
> printing even if there are no legacy or boot consoles registered.
> If no such consoles are registered, the console lock does not
> need to be taken.
> 
> Add tracking of legacy console registration and use it with
> boot console tracking to avoid unnecessary code paths, i.e.
> do not use the console lock if there are no boot consoles
> and no legacy consoles.
> 
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -44,6 +44,16 @@ enum printk_info_flags {
>  };
>  
>  extern struct printk_ringbuffer *prb;
> +extern bool have_legacy_console;
> +extern bool have_boot_console;
> +
> +/*
> + * Specifies if the console lock/unlock dance is needed for console
> + * printing. If @have_boot_console is true, the nbcon consoles will
> + * be printed serially along with the legacy consoles because nbcon
> + * consoles cannot print simultaneously with boot consoles.
> + */
> +#define printing_via_unlock (have_legacy_console || have_boot_console)
>  
>  __printf(4, 0)
>  int vprintk_store(int facility, int level,
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -463,6 +463,13 @@ static int console_msg_format = MSG_FORMAT_DEFAULT;
>  /* syslog_lock protects syslog_* variables and write access to clear_seq. */
>  static DEFINE_MUTEX(syslog_lock);
>  
> +/*
> + * Specifies if a legacy console is registered. If legacy consoles are
> + * present, it is necessary to perform the console_lock/console_unlock dance
> + * whenever console flushing should occur.
> + */
> +bool have_legacy_console;
> +
>  /*
>   * Specifies if a boot console is registered. If boot consoles are present,
>   * nbcon consoles cannot print simultaneously and must be synchronized by
> @@ -3790,22 +3807,28 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
>  	seq = prb_next_reserve_seq(prb);
>  
>  	/* Flush the consoles so that records up to @seq are printed. */
> -	console_lock();
> -	console_unlock();
> +	if (printing_via_unlock) {
> +		console_lock();
> +		console_unlock();
> +	}
>  
>  	for (;;) {
>  		unsigned long begin_jiffies;
>  		unsigned long slept_jiffies;
>  
> +		locked = false;
>  		diff = 0;
>  
> -		/*
> -		 * Hold the console_lock to guarantee safe access to
> -		 * console->seq. Releasing console_lock flushes more
> -		 * records in case @seq is still not printed on all
> -		 * usable consoles.
> -		 */
> -		console_lock();
> +		if (printing_via_unlock) {
> +			/*
> +			 * Hold the console_lock to guarantee safe access to
> +			 * console->seq. Releasing console_lock flushes more
> +			 * records in case @seq is still not printed on all
> +			 * usable consoles.
> +			 */
> +			console_lock();
> +			locked = true;
> +		}
>  
>  		cookie = console_srcu_read_lock();
>  		for_each_console_srcu(c) {
> @@ -3836,7 +3860,8 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
>  		if (diff != last_diff && reset_on_progress)
>  			remaining_jiffies = timeout_jiffies;
>  
> -		console_unlock();
> +		if (locked)
> +			console_unlock();

Is this actually safe?

What prevents the compiler from optimizing out the "locked" variable
and reading "printing_via_unlock" once again here?

It is not exactly the same but it is similar to "invented loads"
described at
https://lwn.net/Articles/793253/#Invented%20Loads

The writes affecting printing_via_unlock are not synchronized
by console_lock().

Should we do the following?

/*
 * Specifies if the console lock/unlock dance is needed for console
 * printing. If @have_boot_console is true, the nbcon consoles will
 * be printed serially along with the legacy consoles because nbcon
 * consoles cannot print simultaneously with boot consoles.
 *
 * Prevent compiler speculations when checking the values.
 */
#define printing_via_unlock (READ_ONCE(have_legacy_console) ||     \
			     READ_ONCE(have_boot_console))


or

		if (printing_via_unlock) {
		[...]
			WRITE_ONCE(locked, true);
		}

Or am I too paranoid?

Best Regards,
Petr

Re: READ_ONCE: was: Re: [PATCH printk v2 19/26] printk: Avoid console_lock dance if no legacy or boot consoles

[Paul E. McKenney]

On Thu, Feb 29, 2024 at 05:19:04PM +0100, Petr Mladek wrote:
> Adding Paul into Cc.
> 
> On Sun 2024-02-18 20:03:19, John Ogness wrote:
> > Currently the console lock is used to attempt legacy-type
> > printing even if there are no legacy or boot consoles registered.
> > If no such consoles are registered, the console lock does not
> > need to be taken.
> > 
> > Add tracking of legacy console registration and use it with
> > boot console tracking to avoid unnecessary code paths, i.e.
> > do not use the console lock if there are no boot consoles
> > and no legacy consoles.
> > 
> > --- a/kernel/printk/internal.h
> > +++ b/kernel/printk/internal.h
> > @@ -44,6 +44,16 @@ enum printk_info_flags {
> >  };
> >  
> >  extern struct printk_ringbuffer *prb;
> > +extern bool have_legacy_console;
> > +extern bool have_boot_console;
> > +
> > +/*
> > + * Specifies if the console lock/unlock dance is needed for console
> > + * printing. If @have_boot_console is true, the nbcon consoles will
> > + * be printed serially along with the legacy consoles because nbcon
> > + * consoles cannot print simultaneously with boot consoles.
> > + */
> > +#define printing_via_unlock (have_legacy_console || have_boot_console)
> >  
> >  __printf(4, 0)
> >  int vprintk_store(int facility, int level,
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -463,6 +463,13 @@ static int console_msg_format = MSG_FORMAT_DEFAULT;
> >  /* syslog_lock protects syslog_* variables and write access to clear_seq. */
> >  static DEFINE_MUTEX(syslog_lock);
> >  
> > +/*
> > + * Specifies if a legacy console is registered. If legacy consoles are
> > + * present, it is necessary to perform the console_lock/console_unlock dance
> > + * whenever console flushing should occur.
> > + */
> > +bool have_legacy_console;
> > +
> >  /*
> >   * Specifies if a boot console is registered. If boot consoles are present,
> >   * nbcon consoles cannot print simultaneously and must be synchronized by
> > @@ -3790,22 +3807,28 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> >  	seq = prb_next_reserve_seq(prb);
> >  
> >  	/* Flush the consoles so that records up to @seq are printed. */
> > -	console_lock();
> > -	console_unlock();
> > +	if (printing_via_unlock) {
> > +		console_lock();
> > +		console_unlock();
> > +	}
> >  
> >  	for (;;) {
> >  		unsigned long begin_jiffies;
> >  		unsigned long slept_jiffies;
> >  
> > +		locked = false;
> >  		diff = 0;
> >  
> > -		/*
> > -		 * Hold the console_lock to guarantee safe access to
> > -		 * console->seq. Releasing console_lock flushes more
> > -		 * records in case @seq is still not printed on all
> > -		 * usable consoles.
> > -		 */
> > -		console_lock();
> > +		if (printing_via_unlock) {
> > +			/*
> > +			 * Hold the console_lock to guarantee safe access to
> > +			 * console->seq. Releasing console_lock flushes more
> > +			 * records in case @seq is still not printed on all
> > +			 * usable consoles.
> > +			 */
> > +			console_lock();
> > +			locked = true;
> > +		}
> >  
> >  		cookie = console_srcu_read_lock();
> >  		for_each_console_srcu(c) {
> > @@ -3836,7 +3860,8 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> >  		if (diff != last_diff && reset_on_progress)
> >  			remaining_jiffies = timeout_jiffies;
> >  
> > -		console_unlock();
> > +		if (locked)
> > +			console_unlock();
> 
> Is this actually safe?
> 
> What prevents the compiler from optimizing out the "locked" variable
> and reading "printing_via_unlock" once again here?
> 
> It is not exactly the same but it is similar to "invented loads"
> described at
> https://lwn.net/Articles/793253/#Invented%20Loads
> 
> The writes affecting printing_via_unlock are not synchronized
> by console_lock().

If there are no compiler barriers between those two regions of code,
that could happen.  Compiler barriers are of course barrier(), but also
almost all atomic and locking operations.

But without comments, any intervening compiler barriers might well be
removed in some future bug-fix effort.  Let's face it, that could happen
even with comments.

> Should we do the following?
> 
> /*
>  * Specifies if the console lock/unlock dance is needed for console
>  * printing. If @have_boot_console is true, the nbcon consoles will
>  * be printed serially along with the legacy consoles because nbcon
>  * consoles cannot print simultaneously with boot consoles.
>  *
>  * Prevent compiler speculations when checking the values.
>  */
> #define printing_via_unlock (READ_ONCE(have_legacy_console) ||     \
> 			     READ_ONCE(have_boot_console))
> 
> 
> or
> 
> 		if (printing_via_unlock) {
> 		[...]
> 			WRITE_ONCE(locked, true);
> 		}
> 
> Or am I too paranoid?

Not by my standards, not that this would be saying much.  ;-)

I prefer the approach adding the READ_ONCE(), especially if "locked"
happens to be a local variable.

							Thanx, Paul

Re: [PATCH printk v2 20/26] printk: Track nbcon consoles

[Petr Mladek]

On Sun 2024-02-18 20:03:20, John Ogness wrote:
> Add a global flag @have_nbcon_console to identify if any nbcon
> consoles are registered. This will be used in follow-up commits
> to preserve legacy behavior when no nbcon consoles are registered.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 21/26] printk: Coordinate direct printing in panic

[Petr Mladek]

On Sun 2024-02-18 20:03:21, John Ogness wrote:
> Perform printing by nbcon consoles on the panic CPU from the
> printk() caller context in order to get panic messages printed
> as soon as possible.
> 
> If legacy and nbcon consoles are registered, the legacy consoles
> will no longer perform direct printing on the panic CPU until
> after the backtrace has been stored. This will give the safe
> nbcon consoles a chance to print the panic messages before
> allowing the unsafe legacy consoles to print.
> 
> If no nbcon consoles are registered, there is no change in
> behavior (i.e. legacy consoles will always attempt to print
> from the printk() caller context).

> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -370,6 +370,8 @@ void panic(const char *fmt, ...)
>  	 */
>  	atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
>  
> +	printk_legacy_allow_panic_sync();

I would call this before the panic notifiers. They are known
to cause problems.

It was the reason to introduce "crash_kexec_post_notifiers" parameter.
Also there is a patchset which tries to somehow split them
by purpose, see
https://lore.kernel.org/all/20220427224924.592546-23-gpiccoli@igalia.com/

It brings another question whether to try flushing the legacy consoles
before calling the notifiers.

>  	panic_print_sys_info(false);
>  
>  	kmsg_dump(KMSG_DUMP_PANIC);
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2329,12 +2329,23 @@ int vprintk_store(int facility, int level,
>  	return ret;
>  }
>  
> +static bool legacy_allow_panic_sync;
> +
> +/*
> + * This acts as a one-way switch to allow legacy consoles to print from
> + * the printk() caller context on a panic CPU.
> + */
> +void printk_legacy_allow_panic_sync(void)
> +{
> +	legacy_allow_panic_sync = true;

I would flush the legacy consoles here. Otherwise it might be done
by another random printk() from the notifiers or by the even more
unsafe printk_console_flush_in_panic().

I mean to do something like:

	if (printing_via_unlock && console_trylock)
		console_unlock();

> +}
> +
>  asmlinkage int vprintk_emit(int facility, int level,
>  			    const struct dev_printk_info *dev_info,
>  			    const char *fmt, va_list args)
>  {
> +	bool do_trylock_unlock = printing_via_unlock;
>  	int printed_len;
> -	bool in_sched = false;
>  
>  	/* Suppress unimportant messages after panic happens */
>  	if (unlikely(suppress_printk))
> @@ -2350,15 +2361,43 @@ asmlinkage int vprintk_emit(int facility, int level,
>  
>  	if (level == LOGLEVEL_SCHED) {
>  		level = LOGLEVEL_DEFAULT;
> -		in_sched = true;
> +		/* If called from the scheduler, we can not call up(). */
> +		do_trylock_unlock = false;
>  	}
>  
>  	printk_delay(level);
>  
>  	printed_len = vprintk_store(facility, level, dev_info, fmt, args);
>  
> -	/* If called from the scheduler, we can not call up(). */
> -	if (!in_sched && printing_via_unlock) {
> +	if (!have_boot_console && have_nbcon_console) {

Nit: The opposite order is more logic ;-)

	if (have_nbcon_console && !have_boot_console) {

> +		bool is_panic_context = this_cpu_in_panic();
> +
> +		/*
> +		 * In panic, the legacy consoles are not allowed to print from
> +		 * the printk calling context unless explicitly allowed. This
> +		 * gives the safe nbcon consoles a chance to print out all the
> +		 * panic messages first. This restriction only applies if
> +		 * there are nbcon consoles registered.
> +		 */
> +		if (is_panic_context)
> +			do_trylock_unlock &= legacy_allow_panic_sync;
> +
> +		/*
> +		 * There are situations where nbcon atomic printing should
> +		 * happen in the printk() caller context:
> +		 *
> +		 * - When this CPU is in panic.
> +		 *
> +		 * Note that if boot consoles are registered, the
> +		 * console_lock/console_unlock dance must be relied upon
> +		 * instead because nbcon consoles cannot print simultaneously
> +		 * with boot consoles.
> +		 */
> +		if (is_panic_context)
> +			nbcon_atomic_flush_all();
> +	}
> +
> +	if (do_trylock_unlock) {
>  		/*
>  		 * The caller may be holding system-critical or
>  		 * timing-sensitive locks. Disable preemption during

Otherwise, it looks good.

Best Regards,
Petr

Re: [PATCH printk v2 22/26] printk: nbcon: Implement emergency sections

[Petr Mladek]

On Sun 2024-02-18 20:03:22, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> In emergency situations (something has gone wrong but the
> system continues to operate), usually important information
> (such as a backtrace) is generated via printk(). Each
> individual printk record has little meaning. It is the
> collection of printk messages that is most often needed by
> developers and users.
> 
> In order to help ensure that the collection of printk messages
> in an emergency situation are all stored to the ringbuffer as
> quickly as possible, disable console output for that CPU while
> it is in the emergency situation. When exiting the emergency
> situation, trigger the consoles to be flushed.
> 
> Add per-CPU emergency nesting tracking because an emergency
> can arise while in an emergency situation.
> 
> Add functions to mark the beginning and end of emergency
> sections where the urgent messages are generated.
> 
> Do not print if the current CPU is in an emergency state.
> 
> Trigger console flushing when exiting all emergency nesting.
> 
> Note that the emergency state is not system-wide. While one CPU
> is in an emergency state, another CPU may continue to print
> console messages.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2404,16 +2404,29 @@ asmlinkage int vprintk_emit(int facility, int level,
>  		 * printing of all remaining records to all consoles so that
>  		 * this context can return as soon as possible. Hopefully
>  		 * another printk() caller will take over the printing.
> +		 *
> +		 * Also, nbcon_get_default_prio() requires migration disabled.
>  		 */
>  		preempt_disable();
> +
>  		/*
> -		 * Try to acquire and then immediately release the console
> -		 * semaphore. The release will print out buffers. With the
> -		 * spinning variant, this context tries to take over the
> -		 * printing from another printing context.
> +		 * Do not emit for EMERGENCY priority. The console will be
> +		 * explicitly flushed when exiting the emergency section.
>  		 */
> -		if (console_trylock_spinning())
> -			console_unlock();
> +		if (nbcon_get_default_prio() == NBCON_PRIO_EMERGENCY) {
> +			do_trylock_unlock = false;

This would cause calling defer_console_output() in this printk().
I think that we do not want it. It is done later by
nbcon_cpu_emergency_exit(). I think that we want something like:


		/*
		 * Try to acquire and then immediately release the
		 * console semaphore. The release will print out
		 * buffers. With the spinning variant, this context
		 * tries to take over the printing from another
		 * printing context.
		 *
		 * Skip it in EMERGENCY priority. The console will be
		 * explicitly flushed when exiting the emergency section.
		 */
		if (nbcon_get_default_prio() != NBCON_PRIO_EMERGENCY) {
			if (console_trylock_spinning())
				console_unlock();
		}


> +		} else {
> +			/*
> +			 * Try to acquire and then immediately release the
> +			 * console semaphore. The release will print out
> +			 * buffers. With the spinning variant, this context
> +			 * tries to take over the printing from another
> +			 * printing context.
> +			 */
> +			if (console_trylock_spinning())
> +				console_unlock();
> +		}
> +
>  		preempt_enable();
>  	}

Otherwise, it looks good.

Best Regards,
Petr

Re: [PATCH printk v2 23/26] panic: Mark emergency section in warn

[Petr Mladek]

On Sun 2024-02-18 20:03:23, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Mark the full contents of __warn() as an emergency section. In
> this section, the CPU will not perform console output for the
> printk() calls. Instead, a flushing of the console output is
> triggered when exiting the emergency section.
> 
> Co-developed-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>

It will change the behavior immediately even before having
any nbcon console. But it makes sense to be consistent.
Let's try it ;-)

Reviewed-by: Petr Mladek <pmladek@suse.com>

Some comments below just to document my thoughts.

> ---
>  kernel/panic.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 86813305510f..d30d261f9246 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -667,6 +667,8 @@ struct warn_args {
>  void __warn(const char *file, int line, void *caller, unsigned taint,
>  	    struct pt_regs *regs, struct warn_args *args)
>  {
> +	nbcon_cpu_emergency_enter();

This will disable preemtion. But it should not cause any big problems.
The messages should be stored quickly when the consoles are not called.

>  	disable_trace_on_warning();
>  
>  	if (file)
> @@ -697,6 +699,8 @@ void __warn(const char *file, int line, void *caller, unsigned taint,

There is called check_panic_on_warn() in this context. It might call
panic(). I first thought that printk() would still defer the consoles.
But it actually won't because vprintk_emit() checks NBCON_PRIO_EMERGENCY.

All is good in the end. I just feel that the rules, when vprintk_emit()
flushes the consoles, are pretty complicated. I am a bit nervous that
anyone could break it in the future. Well, I can't think of any
approach which would make it more regression-proof.

The only way might be to do not be so perfect and simplify the logic.
Well, the current logic makes perfect sense. Let's try it.
Maybe, I am just tired today ;-)

>  
>  	/* Just a warning, don't kill lockdep. */
>  	add_taint(taint, LOCKDEP_STILL_OK);
> +
> +	nbcon_cpu_emergency_exit();
>  }
>  

Best Regards,
Petr

Re: [PATCH printk v2 24/26] panic: Mark emergency section in oops

[Petr Mladek]

On Sun 2024-02-18 20:03:24, John Ogness wrote:
> Mark an emergency section beginning with oops_enter() until the
> end of oops_exit(). In this section, the CPU will not perform
> console output for the printk() calls. Instead, a flushing of the
> console output is triggered when exiting the emergency section.
> 
> The very end of oops_exit() performs a kmsg_dump(). This is not
> included in the emergency section because it is another
> flushing mechanism that should occur after the consoles have
> been triggered to flush.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  kernel/panic.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index d30d261f9246..9fa44bc38f46 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -634,6 +634,7 @@ bool oops_may_print(void)
>   */
>  void oops_enter(void)
>  {
> +	nbcon_cpu_emergency_enter();
>  	tracing_off();
>  	/* can't trust the integrity of the kernel anymore: */
>  	debug_locks_off();
> @@ -656,6 +657,7 @@ void oops_exit(void)
>  {
>  	do_oops_enter_exit();

The comment above oops_enter() function says:

/*
 * Called when the architecture enters its oops handler, before it prints
 * anything.  If this is the first CPU to oops, and it's oopsing the first
 * time then let it proceed.
 *
 * This is all enabled by the pause_on_oops kernel boot option.  We do all
 * this to ensure that oopses don't scroll off the screen.  It has the
 * side-effect of preventing later-oopsing CPUs from mucking up the display,
 * too.
 *
 * It turns out that the CPU which is allowed to print ends up pausing for
 * the right duration, whereas all the other CPUs pause for twice as long:
 * once in oops_enter(), once in oops_exit().
 */

and indeed do_oops_enter_exit(); does the waiting.

IMHO, we should enter() the emergency context after waiting in
oops_enter(). And exit() it before waiting in oops_exit(). Aka


 void oops_enter(void)
 {
 	tracing_off();
 	/* can't trust the integrity of the kernel anymore: */
 	debug_locks_off();
 	do_oops_enter_exit();
+ 	nbcon_cpu_emergency_enter();
 
 	if (sysctl_oops_all_cpu_backtrace)
 		trigger_all_cpu_backtrace();
 }

 void oops_exit(void)
 {
+	nbcon_cpu_emergency_exit();
 	do_oops_enter_exit();
 	print_oops_end_marker();
 	kmsg_dump(KMSG_DUMP_OOPS);
 }


>  	print_oops_end_marker();
> +	nbcon_cpu_emergency_exit();
>  	kmsg_dump(KMSG_DUMP_OOPS);
>  }

Otherwise, it looks good.

Best Regards,
Petr

Re: [PATCH printk v2 25/26] rcu: Mark emergency section in rcu stalls

[Petr Mladek]

On Sun 2024-02-18 20:03:25, John Ogness wrote:
> Mark an emergency section within print_other_cpu_stall(), where
> RCU stall information is printed. In this section, the CPU will
> not perform console output for the printk() calls. Instead, a
> flushing of the console output is triggered when exiting the
> emergency section.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

I was just curious about one thing. But it seems to work well.

print_other_cpu_stall() print backtraces on other CPUs via NMI.
The other CPUs would not see the emergency context. They would
call defer_console_output() because they are in NMI. As a result:

  + Legacy consoles might be flushed on other CPUs even before
    nbcon_cpu_emergency_exit() gets called.

  + nbcon consoles might still be flushed by the printk kthread
    until all messages get flushed directly by nbcon_cpu_emergency_exit()

As I wrote. The behavior is corrent. It was just not obvious to me.


> --- a/kernel/rcu/tree_stall.h
> +++ b/kernel/rcu/tree_stall.h
> @@ -9,6 +9,7 @@
>  
>  #include <linux/kvm_para.h>
>  #include <linux/rcu_notifier.h>
> +#include <linux/console.h>
>  
>  //////////////////////////////////////////////////////////////////////////////
>  //
> @@ -604,6 +605,8 @@ static void print_other_cpu_stall(unsigned long gp_seq, unsigned long gps)
>  	if (rcu_stall_is_suppressed())
>  		return;
>  
> +	nbcon_cpu_emergency_enter();
> +
>  	/*
>  	 * OK, time to rat on our buddy...
>  	 * See Documentation/RCU/stallwarn.rst for info on how to debug
> @@ -658,6 +661,8 @@ static void print_other_cpu_stall(unsigned long gp_seq, unsigned long gps)
>  	panic_on_rcu_stall();
>  
>  	rcu_force_quiescent_state();  /* Kick them all. */
> +
> +	nbcon_cpu_emergency_exit();
>  }
>  
>  static void print_cpu_stall(unsigned long gps)

Best Regards,
Petr

Re: [PATCH printk v2 26/26] lockdep: Mark emergency section in lockdep splats

[Petr Mladek]

On Sun 2024-02-18 20:03:26, John Ogness wrote:
> Mark an emergency section within print_usage_bug(), where
> lockdep bugs are printed. In this section, the CPU will not
> perform console output for the printk() calls. Instead, a
> flushing of the console output is triggered when exiting
> the emergency section.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

The patch looks fine from my POV. Well, I expect that you will send
another version addressing Waiman's concerns.

Best Regards,
Petr

flush was: Re: [PATCH printk v2 22/26] printk: nbcon: Implement emergency sections

[Petr Mladek]

On Sun 2024-02-18 20:03:22, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> In emergency situations (something has gone wrong but the
> system continues to operate), usually important information
> (such as a backtrace) is generated via printk(). Each
> individual printk record has little meaning. It is the
> collection of printk messages that is most often needed by
> developers and users.
> 
> In order to help ensure that the collection of printk messages
> in an emergency situation are all stored to the ringbuffer as
> quickly as possible, disable console output for that CPU while
> it is in the emergency situation. When exiting the emergency
> situation, trigger the consoles to be flushed.
> 
> Add per-CPU emergency nesting tracking because an emergency
> can arise while in an emergency situation.
> 
> Add functions to mark the beginning and end of emergency
> sections where the urgent messages are generated.
> 
> Do not print if the current CPU is in an emergency state.
> 
> Trigger console flushing when exiting all emergency nesting.
> 
> Note that the emergency state is not system-wide. While one CPU
> is in an emergency state, another CPU may continue to print
> console messages.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1105,6 +1134,58 @@ void nbcon_atomic_flush_unsafe(void)
>  	__nbcon_atomic_flush_all(prb_next_reserve_seq(prb), true);
>  }
>  
> +/**
> + * nbcon_cpu_emergency_enter - Enter an emergency section where printk()
> + *	messages for that CPU are only stored
> + *
> + * Upon exiting the emergency section, all stored messages are flushed.
> + *
> + * Context:	Any context. Disables preemption.
> + *
> + * When within an emergency section, no printing occurs on that CPU. This
> + * is to allow all emergency messages to be dumped into the ringbuffer before
> + * flushing the ringbuffer. The actual printing occurs when exiting the
> + * outermost emergency section.
> + */
> +void nbcon_cpu_emergency_enter(void)
> +{
> +	unsigned int *cpu_emergency_nesting;
> +
> +	preempt_disable();
> +
> +	cpu_emergency_nesting = nbcon_get_cpu_emergency_nesting();
> +	(*cpu_emergency_nesting)++;
> +}
> +
> +/**
> + * nbcon_cpu_emergency_exit - Exit an emergency section and flush the
> + *	stored messages
> + *
> + * Flushing only occurs when exiting all nesting for the CPU.
> + *
> + * Context:	Any context. Enables preemption.
> + */
> +void nbcon_cpu_emergency_exit(void)
> +{
> +	unsigned int *cpu_emergency_nesting;
> +	bool do_trigger_flush = false;
> +
> +	cpu_emergency_nesting = nbcon_get_cpu_emergency_nesting();
> +
> +	WARN_ON_ONCE(*cpu_emergency_nesting == 0);
> +
> +	if (*cpu_emergency_nesting == 1)
> +		do_trigger_flush = true;
> +
> +	/* Undo the nesting count of nbcon_cpu_emergency_enter(). */
> +	(*cpu_emergency_nesting)--;
> +
> +	preempt_enable();
> +
> +	if (do_trigger_flush)
> +		printk_trigger_flush();

Just an idea. printk_trigger_flush() calls defer_console_output().
It always moves the flushing into IRQ context.

It might make sense to add a flush() function which would try
to flush the messages directly from this context and
queue the IRQ work only when it fails. Something like:

void printk_emergency_flush()
{
	/* nbcon consoles could be flushed in any context. */
	if (have_nbcon_console)
		nbcon_flush_all();

	if (have_legacy_console) {
		if (console_trylock())
			console_unlock();
		else
			defer_console_output();
	}
}

But wait, nbcon_flush_all() might have troubles to get the per-console
lock because it would be called with NBCON_PRIO_NORMAL.


Wait, wait, wait.

defer_console_output() schedules wake_up_klogd_work_func(). It flushes
only legacy consoles. It means that even emergency messages would
need to wait for the printk kthread.

By other words, it seems that the emergency context does not have any
effect for nbcon consoles.

It brings a question if any code would need to acquire the emergency
context at all.

I quess that this does not work as expected.

Or do I miss anything, please?

Best Regards,
Petr

Re: flush was: Re: [PATCH printk v2 22/26] printk: nbcon: Implement emergency sections

[Petr Mladek]

On Fri 2024-03-01 16:49:54, Petr Mladek wrote:
> On Sun 2024-02-18 20:03:22, John Ogness wrote:
> > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > In emergency situations (something has gone wrong but the
> > system continues to operate), usually important information
> > (such as a backtrace) is generated via printk(). Each
> > individual printk record has little meaning. It is the
> > collection of printk messages that is most often needed by
> > developers and users.
> > 
> > In order to help ensure that the collection of printk messages
> > in an emergency situation are all stored to the ringbuffer as
> > quickly as possible, disable console output for that CPU while
> > it is in the emergency situation. When exiting the emergency
> > situation, trigger the consoles to be flushed.
> > 
> > Add per-CPU emergency nesting tracking because an emergency
> > can arise while in an emergency situation.
> > 
> > Add functions to mark the beginning and end of emergency
> > sections where the urgent messages are generated.
> > 
> > Do not print if the current CPU is in an emergency state.
> > 
> > Trigger console flushing when exiting all emergency nesting.
> > 
> > Note that the emergency state is not system-wide. While one CPU
> > is in an emergency state, another CPU may continue to print
> > console messages.
> > 
> > --- a/kernel/printk/nbcon.c
> > +++ b/kernel/printk/nbcon.c
> > @@ -1105,6 +1134,58 @@ void nbcon_atomic_flush_unsafe(void)
> >  	__nbcon_atomic_flush_all(prb_next_reserve_seq(prb), true);
> >  }
> >  
> > +/**
> > + * nbcon_cpu_emergency_enter - Enter an emergency section where printk()
> > + *	messages for that CPU are only stored
> > + *
> > + * Upon exiting the emergency section, all stored messages are flushed.
> > + *
> > + * Context:	Any context. Disables preemption.
> > + *
> > + * When within an emergency section, no printing occurs on that CPU. This
> > + * is to allow all emergency messages to be dumped into the ringbuffer before
> > + * flushing the ringbuffer. The actual printing occurs when exiting the
> > + * outermost emergency section.
> > + */
> > +void nbcon_cpu_emergency_enter(void)
> > +{
> > +	unsigned int *cpu_emergency_nesting;
> > +
> > +	preempt_disable();
> > +
> > +	cpu_emergency_nesting = nbcon_get_cpu_emergency_nesting();
> > +	(*cpu_emergency_nesting)++;
> > +}
> > +
> > +/**
> > + * nbcon_cpu_emergency_exit - Exit an emergency section and flush the
> > + *	stored messages
> > + *
> > + * Flushing only occurs when exiting all nesting for the CPU.
> > + *
> > + * Context:	Any context. Enables preemption.
> > + */
> > +void nbcon_cpu_emergency_exit(void)
> > +{
> > +	unsigned int *cpu_emergency_nesting;
> > +	bool do_trigger_flush = false;
> > +
> > +	cpu_emergency_nesting = nbcon_get_cpu_emergency_nesting();
> > +
> > +	WARN_ON_ONCE(*cpu_emergency_nesting == 0);
> > +
> > +	if (*cpu_emergency_nesting == 1)
> > +		do_trigger_flush = true;
> > +
> > +	/* Undo the nesting count of nbcon_cpu_emergency_enter(). */
> > +	(*cpu_emergency_nesting)--;
> > +
> > +	preempt_enable();
> > +
> > +	if (do_trigger_flush)
> > +		printk_trigger_flush();
> 
> Just an idea. printk_trigger_flush() calls defer_console_output().
> It always moves the flushing into IRQ context.
> 
> It might make sense to add a flush() function which would try
> to flush the messages directly from this context and
> queue the IRQ work only when it fails. Something like:
> 
> void printk_emergency_flush()
> {
> 	/* nbcon consoles could be flushed in any context. */
> 	if (have_nbcon_console)
> 		nbcon_flush_all();

It seems that the previous version of the patchset called this
in nbcon_cpu_emergency_exit() before decrementing the cpu state.

Sigh, it was me who said that it was useless, see
https://lore.kernel.org/all/ZQ3R4Lz1LHQYsylw@alley/

I am sorry for this.

To my defense. It was a generic function added early in the patchset.
And it was used by both emergency and panic contexts. It is possible
that I did not know about the emergency context behavior at
the time of review.


> 	if (have_legacy_console) {
> 		if (console_trylock())
> 			console_unlock();
> 		else
> 			defer_console_output();
> 	}
> }
> 
> But wait, nbcon_flush_all() might have troubles to get the per-console
> lock because it would be called with NBCON_PRIO_NORMAL.
> 
> 
> Wait, wait, wait.
> 
> defer_console_output() schedules wake_up_klogd_work_func(). It flushes
> only legacy consoles. It means that even emergency messages would
> need to wait for the printk kthread.
> 
> By other words, it seems that the emergency context does not have any
> effect for nbcon consoles.

It should get fixed by calling nbcon_flush_all() before decrementing
cpu_emergency_nesting counter.

Best Regards,
Petr

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[John Ogness]

Hi Petr,

Thanks for the detailed feedback. Here is a lengthy response. I hope it
clarifies the uart port and console fields. And I think you identified a
bug relating to the setup() callback.

On 2024-02-23, Petr Mladek <pmladek@suse.com> wrote:
> My main (only) concern was the synchronization of the various accessed
> variables, especially, port->cons.

The only issue is if port->cons disappears between lock and unlock. I
see there is code setting port->cons to NULL, although I do not know
why. Once port->cons is set, there is never a reason to unset it.

Regardless, I will add port->lock synchronization when modifying
port->cons. There are only a few code sites and they are all during
driver setup.

> Note: I am not completely sure how the early and valid console drivers
>       share the same struct uart_port. So, maybe I miss some important
>       guarantee.

The struct uart_port is _not_ shared between the early and normal
consoles. However, the struct console is shared for normal consoles
amoung various ports of a particular driver.

> Anyway. synchronization of port->cons looks like a shade area.
> IMHO, the existing code expects that it is used _only when the console
> is registered_. But this patch wants to access it _even before
> the console is registered_!

Indeed. It is not enough for uart_is_nbcon() to check if it is an
NBCON. It must also check if it is registered, locklessly:

    hlist_unhashed_lockless(&con->node);

Most importantly to be sure that nbcon_init() has already been called.
register_console() clears the nbcon state after cons->index has been
set, but before the console has been added to the list.

> For example, it took me quite a lot of time to shake my head around:
>
> #define uart_console(port) \
> 	((port)->cons && (port)->cons->index == (port)->line)
>
>   + port->cons and port->line are updated in the uart code.
>     It seems that the update is not serialized by port->lock.
>     Something might be done under port->mutex.
>
>   + cons->index is updated in register_console() under
>     console_list_lock.
>
> Spoiler: I propose a solution which does not use uart_console().

This check is necessary because multiple ports of a driver will set and
share the same port->cons value, even if they are not really the
console. I looked into enforcing that port->cons is NULL if it is not a
registered console, but this is tricky. port->cons is driver-internal
and hidden from printk. The driver will set port->cons in case it
becomes the console and printk will set cons->index once it has chosen
which port will be the actual console. But there is no way to unset
port->cons if a port was not chosen by printk.

The various fields have the following meaning (AFAICT):

port->line: An identifier to represent a particular port supported by a
driver.

port->cons: The struct console to use if this port is chosen to be a
console.

port->console: Boolean, true if this port was chosen to be a
console. (Used only by the tty layer.)

cons->index: The port chosen by printk to be a console.

None of those fields specify if the port is currently registered as a
console. For that you would need to check if port->cons->node is hashed
and then verify that port->line matches port->cons->index. This is what
uart_nbcon_acquire() needs to do (as well as check if it is an nbcon
console).

>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -3284,6 +3284,7 @@ void serial8250_init_port(struct uart_8250_port *up)
>>  	struct uart_port *port = &up->port;
>>  
>>  	spin_lock_init(&port->lock);
>> +	port->nbcon_locked_port = false;
>
> I am not sure if the variable really helps anything:
>
>    + nbcon_context release() must handle the case when it
>      lost the ownership anyway.

uart_nbcon_release() must first check if the provided port is a
registered nbcon console. A simple boolean check is certainly faster
than the 4 checks mentioned above. After all, if it was never locked,
there is no reason to unlock it.

>    + nbcon_acquire() is called under port->lock. It means that
>      nbcon_release() should always be called when the acquire
>      succeeded earlier.

Same answer as above.

> We just need to make sure that port->cons can be cleared only
> under port->lock. But this would be required even with
> port->nbcon_locked_port.

Agreed. I will add this.

>> --- a/kernel/printk/nbcon.c
>> +++ b/kernel/printk/nbcon.c
>> @@ -995,3 +996,79 @@ void nbcon_free(struct console *con)
>>  
>>  	con->pbufs = NULL;
>>  }
>> +
>> +static inline bool uart_is_nbcon(struct uart_port *up)
>> +{
>> +	int cookie;
>> +	bool ret;
>> +
>> +	if (!uart_console(up))
>
> This function accesses up->cons. I am not completely sure how
> this value is synchronized, for example:
>
>   + serial_core_add_one_port() sets uport->cons under port->mutex.
>     The function is called uart_add_one_port() in various probe()
>     or init() calls.

I will add port->lock synchronization.

>   + univ8250_console_setup() sets and clears port->cons with
>     no explicit synchronization. The function is called from
>     try_enable_preferred_console() under console_list_lock.

I will add port->lock synchronization.

> IMHO, the assignment is done when the drivers are being initialized.
> It is done when the port might already be used by early consoles.
>
> Especially, according to my understanding, newcon->setup() callbacks
> are responsible for using the same port by early and real console drivers.
>
> I guess that uart_port_lock() API is used by early consoles as well.
> It means that they might access up->cons here while it is being
> manipulated by the proper driver.

Note that port->lock does not synchronize early and normal
consoles. Only the console lock can do that. But you bring up a very
good point with setup(). serial8250_console_setup() can call
probe_baud(), which will write to the hardware.

I think that con->setup() needs to be called under the console lock
(just as already with unblank() and device()).

>> +		return false;
>> +
>> +	cookie = console_srcu_read_lock();
>> +	ret = (console_srcu_read_flags(up->cons) & CON_NBCON);
>> +	console_srcu_read_unlock(cookie);
>
> Hmm, console_srcu_read_flags(con) is called under
> console_srcu_read_lock() to make sure that it does not
> disappear. It makes sense when it is used by registered consoles.
>
> But uart_port_lock() might be called even when the console
> is not registered.
>
> I suggest to remove the SRCU lock here. In this code path,
> it does not guarantee anything and is rather misleading.

Agreed.

> I would use a READ_ONCE(), for example by splitting:
>
> /*
>  * Locklessly reading console->flags provides a consistent
>  * read value because there is at most one CPU modifying
>  * console->flags and that CPU is using only read-modify-write
>  * operations to do so.
>  *
>  * The caller must make sure that @con does not disappear.
>  * It can be done by console_srcu_read_lock() when used
>  * only for registered consoles.
>  */
> static inline short console_read_flags(const struct console *con)
> {
> 	return data_race(READ_ONCE(con->flags));
> }
>
> /* Locklessly reading console->flags for registered consoles */
> static inline short console_srcu_read_flags(const struct console *con)
> {
> 	WARN_ON_ONCE(!console_srcu_read_lock_is_held());
>
> 	console_read_flags();
> }

OK.

>> +void uart_nbcon_acquire(struct uart_port *up)
>> +{
>> +	struct console *con = up->cons;
>> +	struct nbcon_context ctxt;
>
> I would add:
>
> 	lockdep_assert_held(&up->lock);

OK.

>> +
>> +	if (!uart_is_nbcon(up))
>> +		return;
>> +
>> +	WARN_ON_ONCE(up->nbcon_locked_port);
>> +
>> +	do {
>> +		do {
>> +			memset(&ctxt, 0, sizeof(ctxt));
>> +			ctxt.console	= con;
>> +			ctxt.prio	= NBCON_PRIO_NORMAL;
>> +		} while (!nbcon_context_try_acquire(&ctxt));
>> +
>> +	} while (!nbcon_context_enter_unsafe(&ctxt));
>> +
>> +	up->nbcon_locked_port = true;
>> +}
>> +EXPORT_SYMBOL_GPL(uart_nbcon_acquire);
>
> I would prefer to split the uart and nbcon specific code, for example:

Can you explain why? This code will not be used anywhere else.

> /**
>  * nbcon_normal_context_acquire - Acquire a generic context with
>  *	the normal priority for nbcon console
>  * @con:	nbcon console
>  *
>  * Context:	Any context which could not be migrated to another CPU.
>  *
>  * Acquire a generic context with the normal priority for the given console.
>  * Prevent the release by entering the unsafe state.
>  *
>  * Note: The console might still loose the ownership by a hostile takeover.
>  *	 But it can be done only by the final flush in panic() when other
>  *	 CPUs should be stopped and other contexts interrupted.
>  */
> static void nbcon_normal_context_acquire(struct console *con)
> {
> 	struct nbcon_context ctxt;
>
> 	do {
> 		do {
> 			memset(&ctxt, 0, sizeof(ctxt));
> 			ctxt.console	= con;
> 			ctxt.prio	= NBCON_PRIO_NORMAL;
> 		} while (!nbcon_context_try_acquire(&ctxt));
>
> 	} while (!nbcon_context_enter_unsafe(&ctxt));
> }
>
> /**
>  * uart_nbcon_acquire - Acquire nbcon console associated with the gived port.
>  * @up:		uart port
>  *
>  * Context:	Must be called under up->lock to prevent manipulating
>  *		up->cons and migrating to another CPU.
>  *
>  * Note: The console might still loose the ownership by a hostile takeover.
>  *	 But it can be done only by the final flush in panic() when other
>  *	 CPUs should be stopped and other contexts interrupted.
>  */
> void uart_nbcon_acquire(struct uart_port *up)
> {
> 	struct console *con; = up->cons;
>
> 	lockdep_assert_held(&up->lock);
>
> 	/*
> 	 * FIXME: This would require adding WRITE_ONCE()
> 	 * on the write part.
> 	 *
> 	 * Or even better, the value should be modified under
> 	 * port->lock so that simple read would be enough here.
> 	 */
> 	con = data_race(READ_ONCE(up->cons));
>
> 	if (!con)
> 		return;
>
> 	if (!console_read_flags(con) & CON_NBCON)
> 		return;
>
> 	nbcon_normal_context_acquire(con);
> }
>
> Note that I did not use up->nbcon_locked_port as explained above.

Note that it will not work because other ports will share the same
up->cons value even though they are not consoles. up->cons only
specifies which struct console to use _if_ printk chooses that port as a
console. It does _not_ mean that printk has chosen that port.

>> +void uart_nbcon_release(struct uart_port *up)
>> +{
>> +	struct console *con = up->cons;
>> +	struct nbcon_context ctxt = {
>> +		.console	= con,
>> +		.prio		= NBCON_PRIO_NORMAL,
>> +	};
>> +
>
> I would add here as well.
>
> 	lockdep_assert_held(&up->lock);

OK.

> This deserves a comment why we do not complain when this function
> is called for nbcon and it is not locked. Something like:
>
> 	/*
> 	 * Even port used by nbcon console might be seen unlocked
> 	 * when it was locked and the console has been registered
> 	 * at the same time.
> 	 */

I think a more appropriate comment would be:

	/*
	 * This function is called for ports that are not consoles
	 * and for ports that may be consoles but are not nbcon
	 * consoles. In those the cases the nbcon console was
	 * never locked and this context must not unlock.
	 */

>> +	if (!up->nbcon_locked_port)
>> +		return;
>> +
>> +	if (nbcon_context_exit_unsafe(&ctxt))
>> +		nbcon_context_release(&ctxt);
>> +
>> +	up->nbcon_locked_port = false;
>> +}
>
> Again I would better split the nbcon and uart part and create:

I can do the split, but I do not see the reason for it.

John

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[John Ogness]

On 2024-03-11, John Ogness <john.ogness@linutronix.de> wrote:
> And I think you identified a bug relating to the setup() callback.

Actually this bug was recently fixed by Peter Collingbourne:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/kernel/printk/printk.c?h=next-20240313&id=801410b26a0e8b8a16f7915b2b55c9528b69ca87

One nice thing that has risen from this is we are starting to see
exactly what the console lock is needed for. At this point I would say
its main function is synchronizing boot consoles with real
drivers. Which means we will not be able to remove the console lock
until we find a real solution to match boot consoles (which circumvent
the Linux driver model) with the real drivers.

John

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[John Ogness]

On 2024-03-11, John Ogness <john.ogness@linutronix.de> wrote:
>>> +	if (!uart_is_nbcon(up))
>>> +		return;
>>> +
>>> +	WARN_ON_ONCE(up->nbcon_locked_port);
>>> +
>>> +	do {
>>> +		do {
>>> +			memset(&ctxt, 0, sizeof(ctxt));
>>> +			ctxt.console	= con;
>>> +			ctxt.prio	= NBCON_PRIO_NORMAL;
>>> +		} while (!nbcon_context_try_acquire(&ctxt));
>>> +
>>> +	} while (!nbcon_context_enter_unsafe(&ctxt));
>>> +
>>> +	up->nbcon_locked_port = true;
>>> +}
>>> +EXPORT_SYMBOL_GPL(uart_nbcon_acquire);
>>
>> I would prefer to split the uart and nbcon specific code, for
>> example:
>
> Can you explain why? This code will not be used anywhere else.

No need to respond to this point. The v3 will be quite different here,
but will follow your suggestion. I am splitting the uart-specific code
into serial_core.h and calling a generic nbcon function for the nbcon
locking.

John

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[Petr Mladek]

On Mon 2024-03-11 18:14:19, John Ogness wrote:
> Hi Petr,
> 
> Thanks for the detailed feedback. Here is a lengthy response. I hope it
> clarifies the uart port and console fields. And I think you identified a
> bug relating to the setup() callback.
> 
> On 2024-02-23, Petr Mladek <pmladek@suse.com> wrote:
> > My main (only) concern was the synchronization of the various accessed
> > variables, especially, port->cons.
> 
> The only issue is if port->cons disappears between lock and unlock. I
> see there is code setting port->cons to NULL, although I do not know
> why. Once port->cons is set, there is never a reason to unset it.

I wonder if it might be needed for hotplugging of the device
or the driver. Well, I would expect that the structures won't exist
when the driver is not loaded and/or the device providing
the port does not exist.

But maybe, it is just a defensive programming style where unused
pointers are cleared.

> Regardless, I will add port->lock synchronization when modifying
> port->cons. There are only a few code sites and they are all during
> driver setup.
> 
> > Note: I am not completely sure how the early and valid console drivers
> >       share the same struct uart_port. So, maybe I miss some important
> >       guarantee.
> 
> The struct uart_port is _not_ shared between the early and normal
> consoles. However, the struct console is shared for normal consoles
> amoung various ports of a particular driver.

I see.

> > Anyway. synchronization of port->cons looks like a shade area.
> > IMHO, the existing code expects that it is used _only when the console
> > is registered_. But this patch wants to access it _even before
> > the console is registered_!
> 
> Indeed. It is not enough for uart_is_nbcon() to check if it is an
> NBCON. It must also check if it is registered, locklessly:
> 
>     hlist_unhashed_lockless(&con->node);
> 
> Most importantly to be sure that nbcon_init() has already been called.
> register_console() clears the nbcon state after cons->index has been
> set, but before the console has been added to the list.

Makes sense.

Well, it brings another question. Does this allow to have
the following situation?

CPU0				CPU1

  some_function()
    uart_port_lock()
      // locked just with up->lock
      // doing something with the port

				register_console()
				  // add struct console using the same
				  // port as CPU0
				  printk()
				    console_try_lock()
				    console_unlock()
				      console_flush_all()
					// acquire context for the newly
					// registered nbcon
					nbcon_context_try_acquire(ctxt)
					  con->write()

BANG: Both CPU0 and CPU1 are writing to the same port.

Reason: CPU0 locked only via port->lock.
	CPU1 locked only by acquiring nbcon context.

Maybe, this is not possible because the console is registered when
the struct uart_port is being initialized and nobody could
use the same port in parallel, except for the early console.
Where the early console is serialized using the console_lock().

Hmm, if the parallel use of struct port_lock is not possible
during register_console then we probably do not even need
to serialize setting and clearing of port->cons.

By other words, I wonder if printk() is the only nasty user of
the uart ports. By "nasty user" I mean:

  + Using the same port even without struct uart_port by
    early console.

  + Using the port via struct uart_port even before the device
    is completely initialized. I assume that register_console()
    is called from the driver init call.

For example, the device/port gets visible in sysfs. I wonder
if anyone could trigger an operation on the port which
it is being registered as a console.

Sigh, if we agree that the above race is possible then I can't think
of any elegant solution.

Well, it might be better to be on the safe side:

One solution would be to add nbcon consoles into the console_list
under uart_port_lock().

Another solution would be to make sure that any code serialized
by uart_port_lock() will be already synchronized by nbcon context
while the nbcon is added into the console_list. Maybe, we
could do this in con->setup() callback. Something like:

 * @need_nbcon_context: true when uart_port_lock() has to acquire
	nbcon context as well

struct uart_port {
	bool need_nbcon_context;

int uart_console_setup(struct console *con, char *options)
{
	struct uart_8250_port *up = &serial8250_ports[co->index];

	spin_lock_irq(&up->lock);
	up->need_nbcon_context=true;
	spin_unlock_irq(&up->lock);

	// do whatever the original uart_console_setup did
}

int uart_console_exit(struct console *con, char *options)
{
	struct uart_8250_port *up = &serial8250_ports[co->index];

	// do whatever the original uart_console_exit did

	spin_lock_irq(&up->lock);
	up->need_nbcon_context=false;
	spin_unlock_irq(&up->lock);
}

We might even use this variable in uart_nbcon_acquire() to
decide whether we need to acquire the context or not.


> > For example, it took me quite a lot of time to shake my head around:
> >
> > #define uart_console(port) \
> > 	((port)->cons && (port)->cons->index == (port)->line)
> >
> >   + port->cons and port->line are updated in the uart code.
> >     It seems that the update is not serialized by port->lock.
> >     Something might be done under port->mutex.
> >
> >   + cons->index is updated in register_console() under
> >     console_list_lock.
> >
> > Spoiler: I propose a solution which does not use uart_console().
> 
> This check is necessary because multiple ports of a driver will set and
> share the same port->cons value, even if they are not really the
> console. I looked into enforcing that port->cons is NULL if it is not a
> registered console, but this is tricky. port->cons is driver-internal
> and hidden from printk. The driver will set port->cons in case it
> becomes the console and printk will set cons->index once it has chosen
> which port will be the actual console. But there is no way to unset
> port->cons if a port was not chosen by printk.
> 
> The various fields have the following meaning (AFAICT):
> 
> port->line: An identifier to represent a particular port supported by a
> driver.
> 
> port->cons: The struct console to use if this port is chosen to be a
> console.
> 
> port->console: Boolean, true if this port was chosen to be a
> console. (Used only by the tty layer.)
> 
> cons->index: The port chosen by printk to be a console.
> 
> None of those fields specify if the port is currently registered as a
> console. For that you would need to check if port->cons->node is hashed
> and then verify that port->line matches port->cons->index. This is what
> uart_nbcon_acquire() needs to do (as well as check if it is an nbcon
> console).

This is a great description. It would be great to have it somewhere in
the sources. Maybe, above the locking/acquire functions.

> >> --- a/drivers/tty/serial/8250/8250_port.c
> >> +++ b/drivers/tty/serial/8250/8250_port.c
> >> @@ -3284,6 +3284,7 @@ void serial8250_init_port(struct uart_8250_port *up)
> >>  	struct uart_port *port = &up->port;
> >>  
> >>  	spin_lock_init(&port->lock);
> >> +	port->nbcon_locked_port = false;
> >
> > I am not sure if the variable really helps anything:
> >
> >    + nbcon_context release() must handle the case when it
> >      lost the ownership anyway.
> 
> uart_nbcon_release() must first check if the provided port is a
> registered nbcon console. A simple boolean check is certainly faster
> than the 4 checks mentioned above. After all, if it was never locked,
> there is no reason to unlock it.

Fair enough.

> > We just need to make sure that port->cons can be cleared only
> > under port->lock. But this would be required even with
> > port->nbcon_locked_port.
> 
> Agreed. I will add this.
> 
> >> --- a/kernel/printk/nbcon.c
> >> +++ b/kernel/printk/nbcon.c
> >> +void uart_nbcon_acquire(struct uart_port *up)
> >> +{
> >> +	struct console *con = up->cons;
> >> +	struct nbcon_context ctxt;
> >
> > I would add:
> >
> > 	lockdep_assert_held(&up->lock);
> 
> OK.
> 
> >> +
> >> +	if (!uart_is_nbcon(up))
> >> +		return;
> >> +
> >> +	WARN_ON_ONCE(up->nbcon_locked_port);
> >> +
> >> +	do {
> >> +		do {
> >> +			memset(&ctxt, 0, sizeof(ctxt));
> >> +			ctxt.console	= con;
> >> +			ctxt.prio	= NBCON_PRIO_NORMAL;
> >> +		} while (!nbcon_context_try_acquire(&ctxt));
> >> +
> >> +	} while (!nbcon_context_enter_unsafe(&ctxt));
> >> +
> >> +	up->nbcon_locked_port = true;
> >> +}
> >> +EXPORT_SYMBOL_GPL(uart_nbcon_acquire);
> >
> > I would prefer to split the uart and nbcon specific code, for example:
> 
> Can you explain why? This code will not be used anywhere else.

It would have helped me with the review. The function is short
but it touches internals from both uart_port and nbcon words:

  + Implements new variant of nbcon_ctxt_acquire() API (busy loop, no timeout)

  + Checks and modifies struct uart_port details which affect
    uart_port_lock() API.

IMHO, there is too much to think about in a single function ;-)


Best Regards,
Petr

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[John Ogness]

On 2024-03-14, Petr Mladek <pmladek@suse.com> wrote:
> Well, it brings another question. Does this allow to have
> the following situation?
>
> CPU0				CPU1
>
>   some_function()
>     uart_port_lock()
>       // locked just with up->lock
>       // doing something with the port
>
> 				register_console()
> 				  // add struct console using the same
> 				  // port as CPU0
> 				  printk()
> 				    console_try_lock()
> 				    console_unlock()
> 				      console_flush_all()
> 					// acquire context for the newly
> 					// registered nbcon
> 					nbcon_context_try_acquire(ctxt)
> 					  con->write()
>
> BANG: Both CPU0 and CPU1 are writing to the same port.
>
> Reason: CPU0 locked only via port->lock.
> 	CPU1 locked only by acquiring nbcon context.

Great catch! Yes, this is possible. :-/

When the kthread series part is introduced, there will be additional
callbacks that nbcon consoles must implement
(driver_enter()/driver_exit()). These provide driver-level
synchronization. In the case of serial uarts, the callbacks map to
locking/unlocking the port lock.

If I were to introduce those callbacks in _this_ series, they can be
used when adding a console to the list in register_console(). This
changes your example to:

CPU0				CPU1

  some_function()
    uart_port_lock()
      // locked just with up->lock
      // doing something with the port

				register_console()
				  // add struct console using the same
				  // port as CPU0
				  newcon->driver_enter()
				    spin_lock(port_lock)
				    // spin on CPU0
    uart_port_unlock()
				  // add new console to console list
				  newcon->driver_exit()
				    spin_unlock(port_lock)
				  ...

If any other CPUs come in and call uart_port_lock(), they will see the
console as registered and will acquire the nbcon to avoid the BANG.

> Maybe, this is not possible because the console is registered when
> the struct uart_port is being initialized and nobody could
> use the same port in parallel, except for the early console.
> Where the early console is serialized using the console_lock().

Yes, it is possible. Just check out:

    find /sys/ -name console -type f

If you echo 'Y' or 'N' into any of those files, you can dynamically
register and unregister those consoles, respectively.

I just ran some tests to verify this and was even able to trigger a
mainline bug because probe_baud() of the 8250 driver is not called under
the port lock. This is essentially the same scenario you
illustrated. But the 8250 probe_baud() issue is a driver bug and not
related to this series.

Getting back to this series, my proposal would change register_console()
like this:

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 68657d4d6649..25a0a81e8397 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3733,6 +3733,7 @@ void register_console(struct console *newcon)
 	struct console *con;
 	bool bootcon_registered = false;
 	bool realcon_registered = false;
+	unsigned long flags;
 	int err;
 
 	console_list_lock();
@@ -3831,6 +3832,19 @@ void register_console(struct console *newcon)
 	if (newcon->flags & CON_BOOT)
 		have_boot_console = true;
 
+	/*
+	 * If another context is actively using the hardware of this new
+	 * console, it will not be aware of the nbcon synchronization. This
+	 * is a risk that two contexts could access the hardware
+	 * simultaneously if this new console is used for atomic printing
+	 * and the other context is still using the hardware.
+	 * 
+	 * Use the driver synchronization to ensure that the hardware is not
+	 * in use while this new console transitions to being registered.
+	 */
+	if ((newcon->flags & CON_NBCON) && newcon->write_atomic)
+		newcon->driver_enter(newcon, &flags);
+
 	/*
 	 * Put this console in the list - keep the
 	 * preferred driver at the head of the list.
@@ -3855,6 +3869,10 @@ void register_console(struct console *newcon)
 	 * register_console() completes.
 	 */
 
+	/* This new console is now registered. */
+	if ((newcon->flags & CON_NBCON) && newcon->write_atomic)
+		newcon->driver_exit(newcon, flags);
+
 	console_sysfs_notify();
 
 	/*

> One solution would be to add nbcon consoles into the console_list
> under uart_port_lock().

This is what I have proposed and I think it is the most straight forward
solution.

> Another solution would be to make sure that any code serialized
> by uart_port_lock() will be already synchronized by nbcon context
> while the nbcon is added into the console_list.

I do not think this would be acceptable. It would mean that non-console
ports would need to lock the nbcon. Not only will that slow down the
non-console ports, but it will also cause serious contention between the
ports. (Remember, all the ports share the same struct console.)

> Maybe, we could do this in con->setup() callback. Something like:

This proposal would work, but IMHO it adds too much complexity by
requiring console drivers to implement the callbacks and do special
things in those callbacks.

>> The various fields have the following meaning (AFAICT):
>> 
>> port->line: An identifier to represent a particular port supported by a
>> driver.
>> 
>> port->cons: The struct console to use if this port is chosen to be a
>> console.
>> 
>> port->console: Boolean, true if this port was chosen to be a
>> console. (Used only by the tty layer.)
>> 
>> cons->index: The port chosen by printk to be a console.
>> 
> This is a great description. It would be great to have it somewhere in
> the sources. Maybe, above the locking/acquire functions.

OK.

John

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[Petr Mladek]

On Fri 2024-03-15 16:10:18, John Ogness wrote:
> On 2024-03-14, Petr Mladek <pmladek@suse.com> wrote:
> > Well, it brings another question. Does this allow to have
> > the following situation?
> >
> > CPU0				CPU1
> >
> >   some_function()
> >     uart_port_lock()
> >       // locked just with up->lock
> >       // doing something with the port
> >
> > 				register_console()
> > 				  // add struct console using the same
> > 				  // port as CPU0
> > 				  printk()
> > 				    console_try_lock()
> > 				    console_unlock()
> > 				      console_flush_all()
> > 					// acquire context for the newly
> > 					// registered nbcon
> > 					nbcon_context_try_acquire(ctxt)
> > 					  con->write()
> >
> > BANG: Both CPU0 and CPU1 are writing to the same port.
> >
> > Reason: CPU0 locked only via port->lock.
> > 	CPU1 locked only by acquiring nbcon context.
> 
> Great catch! Yes, this is possible. :-/
> 
> When the kthread series part is introduced, there will be additional
> callbacks that nbcon consoles must implement
> (driver_enter()/driver_exit()). These provide driver-level
> synchronization. In the case of serial uarts, the callbacks map to
> locking/unlocking the port lock.
> 
> If I were to introduce those callbacks in _this_ series, they can be
> used when adding a console to the list in register_console(). This
> changes your example to:
> 
> CPU0				CPU1
> 
>   some_function()
>     uart_port_lock()
>       // locked just with up->lock
>       // doing something with the port
> 
> 				register_console()
> 				  // add struct console using the same
> 				  // port as CPU0
> 				  newcon->driver_enter()
> 				    spin_lock(port_lock)
> 				    // spin on CPU0
>     uart_port_unlock()
> 				  // add new console to console list
> 				  newcon->driver_exit()
> 				    spin_unlock(port_lock)
> 				  ...
> 
> If any other CPUs come in and call uart_port_lock(), they will see the
> console as registered and will acquire the nbcon to avoid the BANG.

Looks good. See below.

> > Maybe, this is not possible because the console is registered when
> > the struct uart_port is being initialized and nobody could
> > use the same port in parallel, except for the early console.
> > Where the early console is serialized using the console_lock().
> 
> Yes, it is possible. Just check out:
> 
>     find /sys/ -name console -type f
> 
> If you echo 'Y' or 'N' into any of those files, you can dynamically
> register and unregister those consoles, respectively.
> 
> I just ran some tests to verify this and was even able to trigger a
> mainline bug because probe_baud() of the 8250 driver is not called under
> the port lock. This is essentially the same scenario you
> illustrated. But the 8250 probe_baud() issue is a driver bug and not
> related to this series.

Thanks a lot for checking it.

> Getting back to this series, my proposal would change register_console()
> like this:
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 68657d4d6649..25a0a81e8397 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3733,6 +3733,7 @@ void register_console(struct console *newcon)
>  	struct console *con;
>  	bool bootcon_registered = false;
>  	bool realcon_registered = false;
> +	unsigned long flags;
>  	int err;
>  
>  	console_list_lock();
> @@ -3831,6 +3832,19 @@ void register_console(struct console *newcon)
>  	if (newcon->flags & CON_BOOT)
>  		have_boot_console = true;
>  
> +	/*
> +	 * If another context is actively using the hardware of this new
> +	 * console, it will not be aware of the nbcon synchronization. This
> +	 * is a risk that two contexts could access the hardware
> +	 * simultaneously if this new console is used for atomic printing
> +	 * and the other context is still using the hardware.
> +	 * 
> +	 * Use the driver synchronization to ensure that the hardware is not
> +	 * in use while this new console transitions to being registered.
> +	 */
> +	if ((newcon->flags & CON_NBCON) && newcon->write_atomic)
> +		newcon->driver_enter(newcon, &flags);
> +
>  	/*
>  	 * Put this console in the list - keep the
>  	 * preferred driver at the head of the list.
> @@ -3855,6 +3869,10 @@ void register_console(struct console *newcon)
>  	 * register_console() completes.
>  	 */
>  
> +	/* This new console is now registered. */
> +	if ((newcon->flags & CON_NBCON) && newcon->write_atomic)
> +		newcon->driver_exit(newcon, flags);
> +
>  	console_sysfs_notify();
>  
>  	/*
> 
> > One solution would be to add nbcon consoles into the console_list
> > under uart_port_lock().
> 
> This is what I have proposed and I think it is the most straight forward
> solution.
> 
> > Another solution would be to make sure that any code serialized
> > by uart_port_lock() will be already synchronized by nbcon context
> > while the nbcon is added into the console_list.
> 
> I do not think this would be acceptable. It would mean that non-console
> ports would need to lock the nbcon. Not only will that slow down the
> non-console ports, but it will also cause serious contention between the
> ports. (Remember, all the ports share the same struct console.)

I actually did not want to lock the nbcon for all ports. This is why
I proposed to do it in con->setup() where con->index is already set.
It might solve the problem without adding yet another callbacks.

That said, I like your solution with newcon->driver_enter()/exit()
callbacks. It seems to have an easier and more straightforward semantic.

Go for it, especially when you need these callbacks later in
the printk kthread.

Nit: I think about renaming the callbacks to"device_lock()
     and device_unlock().

     "(un)lock" probably better describes what the callbacks do.
     register_console() does not want to do any operations
     on the serial port. It just needs to serialize adding
     the console into the list.

     I suggest "device" because the callbacks will lock/unlock
     the tty_driver pointed by "con->device".

> 
> > Maybe, we could do this in con->setup() callback. Something like:
> 
> This proposal would work, but IMHO it adds too much complexity by
> requiring console drivers to implement the callbacks and do special
> things in those callbacks.

Fair enough.

Best Regards,
Petr

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[Tony Lindgren]

* John Ogness <john.ogness@linutronix.de> [240313 09:50]:
> One nice thing that has risen from this is we are starting to see
> exactly what the console lock is needed for. At this point I would say
> its main function is synchronizing boot consoles with real
> drivers. Which means we will not be able to remove the console lock
> until we find a real solution to match boot consoles (which circumvent
> the Linux driver model) with the real drivers.

Would it help if earlycon handles all the boot consoles?
Then just have the serial driver take over when it probes?

Regards,

Tony

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[John Ogness]

On 2024-03-22, Tony Lindgren <tony@atomide.com> wrote:
> * John Ogness <john.ogness@linutronix.de> [240313 09:50]:
>> One nice thing that has risen from this is we are starting to see
>> exactly what the console lock is needed for. At this point I would say
>> its main function is synchronizing boot consoles with real
>> drivers. Which means we will not be able to remove the console lock
>> until we find a real solution to match boot consoles (which circumvent
>> the Linux driver model) with the real drivers.
>
> Would it help if earlycon handles all the boot consoles?
> Then just have the serial driver take over when it probes?

I think this would be very helpful. And it would also cleanup the boot
arguments. For example, we would no longer need the
architecture-specific arguments/options (such as "early_printk" and
"keep"). These architecture-specific arguments can be really
confusing. There have been so many times I see a developer cursing that
they can't get early debugging working, when I notice they are trying to
use "early_printk" on an arm64 system.

Browsing through

arch/x86/kernel/early_printk.c
arch/x86/boot/early_serial_console.c

you can see lots of examples of various early consoles and their special
tricks/hacks (such as pretending not to be a boot console when it really
is).

And pretty much every architecture has these. (git grep CON_BOOT)

Ideally it would be great if a console could register and say "taking
over for console X". Maybe with a new field:

struct console {
  ...
  struct console *console_to_replace;
  ...
};

John Ogness

Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[Andy Shevchenko]

On Wed, Mar 27, 2024 at 10:38:15AM +0106, John Ogness wrote:
> On 2024-03-22, Tony Lindgren <tony@atomide.com> wrote:
> > * John Ogness <john.ogness@linutronix.de> [240313 09:50]:
> >> One nice thing that has risen from this is we are starting to see
> >> exactly what the console lock is needed for. At this point I would say
> >> its main function is synchronizing boot consoles with real
> >> drivers. Which means we will not be able to remove the console lock
> >> until we find a real solution to match boot consoles (which circumvent
> >> the Linux driver model) with the real drivers.
> >
> > Would it help if earlycon handles all the boot consoles?
> > Then just have the serial driver take over when it probes?
> 
> I think this would be very helpful. And it would also cleanup the boot
> arguments. For example, we would no longer need the
> architecture-specific arguments/options (such as "early_printk" and
> "keep"). These architecture-specific arguments can be really
> confusing.

You may not get rid of earlyprintk as it affects *very* early at boot,
earlycon is simply not and may not be available at these stages.

-- 
With Best Regards,
Andy Shevchenko