[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 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

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 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 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 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