[PATCH printk v4 00/27] wire up write_atomic() printing

[PATCH printk v4 00/27] wire up write_atomic() printing

[John Ogness]

Hi,

This is v4 of a series to wire up the nbcon consoles so that
they actually perform printing using their write_atomic()
callback. v3 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, nbcon consoles are flushed directly and legacy
  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.

Note: With this series, a system with _only_ nbcon consoles
      registered will not perform console printing unless the
      console lock is used (for synchronization), or when
      exiting emergency sections, or on panic. This is on
      purpose. When nbcon kthreads are introduced, they will
      fill the gaps.

The changes since v3:

- Modify the documentation of console_srcu_read_flags() to
  clarify that it is needed anytime a console _might_ be
  registered and the caller is not holding the
  console_list_lock. Hopefully this makes it clear when this
  helper function is needed.

- Create a function uart_port_set_cons() for setting @cons of
  struct uart_port. It modifies @cons under the port lock to
  avoid possible races within the port lock wrapper. All (5)
  code sites are modified to use the new function.

- Introduce 2 new required nbcon console callbacks
  device_lock()/device_unlock() to implement any internal
  locking required by the driver. (For example, for uart serial
  consoles it is locking/unlocking the port lock.) This is used
  during console registration to ensure that the hardware is
  not in use while the console transitions to registered. This
  avoids the risk that the port lock wrappers do not lock the
  nbcon console lock while the console was being registered on
  another CPU. These callbacks also will be used later by the
  printing kthreads.

- Introduce struct nbcon_drvdata to track ownership state when
  using the port lock wrappers. This provides a race-free
  alternative to the @nbcon_locked_port flag used in v3.

- Split the functionality of uart_nbcon_acquire() and
  uart_nbcon_release() into driver-specific and generic parts.
  The generic functions are named nbcon_driver_acquire() and
  nbcon_driver_release(). The driver-specific part is moved
  into serial_core.h into the new helper functions
  __uart_port_nbcon_acquire() and __uart_port_nbcon_release().

- Rename nbcon_atomic_flush_all() to
  nbcon_atomic_flush_pending() to emphasize that it only prints
  up to the latest record at the time of the call. Also, flush
  all the pending records of a console (without releasing
  ownership in between) before flushing the next nbcon console.
  This allows the full emergency block to be printed on at
  least one atomic console before trying the next.

- Flush nbcon consoles directly in the caller context when
  exiting an emergency section.

- If a CPU is in EMERGENCY context, do not trigger printing
  of legacy consoles via irq_work.

- In panic, allow synchronous legacy printing before calling
  the panic handlers. Attempt to flush there in the panic
  context as well.

- Remove the return value for the nbcon console atomic_write()
  callback. If ownership has not been lost, it is assumed the
  printing was successful.

- Add a WARN_ON_ONCE if nbcon_emit_next_record() is called for
  a console that has not provided a write_atomic() callback.

- Change the meaning of the return value of
  nbcon_atomic_emit_one() to allow
  nbcon_legacy_emit_next_record() to have the same return value
  meaning as console_emit_next_record().

- Remove all legacy @seq handling from nbcon.c. For nbcon
  consoles, printk.c handles the transfer and resetting of the
  legacy @seq value to @nbcon_seq.

- Add a compiler barrier in __pr_flush() to ensure the compiler
  does not optimize out a local variable by replacing it with
  a racy read of multiple global variables.

- Let __wake_up_klogd() remove unnecessary flags before
  possibly queuing irq_work.

- Eliminate header proxying in nbcon.c.

- Mark _all_ lockdep output blocks as emergency sections.

- Mark _all_ rcu stall blocks as emergency sections.

- Remove "(Optional)" in the documentation of the
  write_atomic() callback. Once threads are available, it will
  be optional. But at this point in the rework it is not.

John Ogness

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

John Ogness (23):
  printk: Add notation to console_srcu locking
  printk: Properly deal with nbcon consoles on seq init
  printk: nbcon: Remove return value for write_atomic()
  printk: nbcon: Add detailed doc for write_atomic()
  printk: nbcon: Add callbacks to synchronize with driver
  printk: nbcon: Use driver synchronization while registering
  serial: core: Provide low-level functions to lock port
  printk: nbcon: Implement processing in port->lock wrapper
  printk: nbcon: Do not rely on proxy headers
  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 sections in lockdep splats

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

Thomas Gleixner (3):
  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_core.c |   6 +-
 drivers/tty/serial/amba-pl011.c     |   2 +-
 drivers/tty/serial/serial_core.c    |   2 +-
 include/linux/console.h             | 138 ++++++++--
 include/linux/printk.h              |  32 ++-
 include/linux/serial_core.h         | 116 ++++++++-
 kernel/locking/lockdep.c            |  91 ++++++-
 kernel/panic.c                      |   9 +
 kernel/printk/internal.h            |  56 +++-
 kernel/printk/nbcon.c               | 382 ++++++++++++++++++++++++++--
 kernel/printk/printk.c              | 287 ++++++++++++++++-----
 kernel/printk/printk_ringbuffer.h   |   2 +
 kernel/printk/printk_safe.c         |  12 +
 kernel/rcu/tree_exp.h               |   7 +
 kernel/rcu/tree_stall.h             |   9 +
 15 files changed, 1038 insertions(+), 113 deletions(-)


base-commit: a2b4cab9da7746c42f87c13721d305baf0085a20
-- 
2.39.2

[PATCH printk v4 08/27] serial: core: Provide low-level functions to lock port

[John Ogness]

It will be necessary at times for the uart nbcon console
drivers to acquire the port lock directly (without the
additional nbcon functionality of the port lock wrappers).
These are special cases such as the implementation of the
device_lock()/device_unlock() callbacks or for internal
port lock wrapper synchronization.

Provide low-level variants __uart_port_lock_irqsave() and
__uart_port_unlock_irqrestore() for this purpose.

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

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 55b1f3ba48ac..bb3324d49453 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -588,6 +588,24 @@ struct uart_port {
 	void			*private_data;		/* generic platform data pointer */
 };
 
+/*
+ * Only for console->device_lock()/_unlock() callbacks and internal
+ * port lock wrapper synchronization.
+ */
+static inline void __uart_port_lock_irqsave(struct uart_port *up, unsigned long *flags)
+{
+	spin_lock_irqsave(&up->lock, *flags);
+}
+
+/*
+ * Only for console->device_lock()/_unlock() callbacks and internal
+ * port lock wrapper synchronization.
+ */
+static inline void __uart_port_unlock_irqrestore(struct uart_port *up, unsigned long flags)
+{
+	spin_unlock_irqrestore(&up->lock, flags);
+}
+
 /**
  * uart_port_lock - Lock the UART port
  * @up:		Pointer to UART port structure
-- 
2.39.2

[PATCH printk v4 09/27] 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.

Introduce a new struct nbcon_drvdata within struct console that
provides the necessary components for the port lock wrappers to
acquire the nbcon console and track its ownership.

Also introduce uart_port_set_cons() as a wrapper to set @cons
of a uart_port. The wrapper sets @cons under the port lock in
order to prevent @cons from disappearing while another context
owns the port lock via the port lock wrappers.

Also cleanup the description of the console_srcu_read_flags()
function. It is used by the port lock wrappers to ensure a
console cannot be fully unregistered while another context
owns the port lock via the port lock wrappers.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_core.c |  6 +-
 drivers/tty/serial/amba-pl011.c     |  2 +-
 drivers/tty/serial/serial_core.c    |  2 +-
 include/linux/console.h             | 57 +++++++++++++----
 include/linux/printk.h              | 13 ++++
 include/linux/serial_core.h         | 98 ++++++++++++++++++++++++++++-
 kernel/printk/nbcon.c               | 52 +++++++++++++++
 7 files changed, 212 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index b62ad9006780..41d74ee3d95a 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -627,11 +627,11 @@ static int univ8250_console_setup(struct console *co, char *options)
 
 	port = &serial8250_ports[co->index].port;
 	/* link port to console */
-	port->cons = co;
+	uart_port_set_cons(port, co);
 
 	retval = serial8250_console_setup(port, options, false);
 	if (retval != 0)
-		port->cons = NULL;
+		uart_port_set_cons(port, NULL);
 	return retval;
 }
 
@@ -689,7 +689,7 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
 			continue;
 
 		co->index = i;
-		port->cons = co;
+		uart_port_set_cons(port, co);
 		return serial8250_console_setup(port, options, true);
 	}
 
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index cf2c890a560f..347aacf8400f 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2496,7 +2496,7 @@ static int pl011_console_match(struct console *co, char *name, int idx,
 			continue;
 
 		co->index = i;
-		port->cons = co;
+		uart_port_set_cons(port, co);
 		return pl011_console_setup(co, options);
 	}
 
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index d6a58a9e072a..2652b4d5c944 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3146,7 +3146,7 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
 	uport->state = state;
 
 	state->pm_state = UART_PM_STATE_UNDEFINED;
-	uport->cons = drv->cons;
+	uart_port_set_cons(uport, drv->cons);
 	uport->minor = drv->tty_driver->minor_start + uport->line;
 	uport->name = kasprintf(GFP_KERNEL, "%s%d", drv->dev_name,
 				drv->tty_driver->name_base + uport->line);
diff --git a/include/linux/console.h b/include/linux/console.h
index ad85594e070e..e7c35c686720 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -282,6 +282,25 @@ struct nbcon_write_context {
 	bool			unsafe_takeover;
 };
 
+/**
+ * struct nbcon_drvdata - Data to allow nbcon acquire in non-print context
+ * @ctxt:		The core console context
+ * @srcu_cookie:	Storage for a console_srcu_lock cookie, if needed
+ * @owner_index:	Storage for the owning console index, if needed
+ * @locked:		Storage for the locked state, if needed
+ *
+ * All fields (except for @ctxt) are available exclusively to the driver to
+ * use as needed. They are not used by the printk subsystem.
+ */
+struct nbcon_drvdata {
+	struct nbcon_context	__private ctxt;
+
+	/* reserved for driver use */
+	int			srcu_cookie;
+	short			owner_index;
+	bool			locked;
+};
+
 /**
  * struct console - The console descriptor structure
  * @name:		The name of the console driver
@@ -396,6 +415,21 @@ struct console {
 
 	atomic_t		__private nbcon_state;
 	atomic_long_t		__private nbcon_seq;
+
+	/**
+	 * @nbcon_drvdata:
+	 *
+	 * Data for nbcon ownership tracking to allow acquiring nbcon consoles
+	 * in non-printing contexts.
+	 *
+	 * Drivers may need to acquire nbcon consoles in non-printing
+	 * contexts. This is achieved by providing a struct nbcon_drvdata.
+	 * Then the driver can call nbcon_driver_acquire() and
+	 * nbcon_driver_release(). The struct does not require any special
+	 * initialization.
+	 */
+	struct nbcon_drvdata	*nbcon_drvdata;
+
 	struct printk_buffers	*pbufs;
 };
 
@@ -425,28 +459,29 @@ extern void console_list_unlock(void) __releases(console_mutex);
 extern struct hlist_head console_list;
 
 /**
- * console_srcu_read_flags - Locklessly read the console flags
+ * console_srcu_read_flags - Locklessly read flags of a possibly registered
+ *				console
  * @con:	struct console pointer of console to read flags from
  *
- * This function provides the necessary READ_ONCE() and data_race()
- * notation for locklessly reading the console flags. The READ_ONCE()
- * in this function matches the WRITE_ONCE() when @flags are modified
- * for registered consoles with console_srcu_write_flags().
+ * Locklessly reading @con->flags provides a consistent read value because
+ * there is at most one CPU modifying @con->flags and that CPU is using only
+ * read-modify-write operations to do so.
  *
- * Only use this function to read console flags when locklessly
- * iterating the console list via srcu.
+ * Requires console_srcu_read_lock to be held, which implies that @con might
+ * be a registered console. If the caller is holding the console_list_lock or
+ * it is certain that the console is not registered, the caller may read
+ * @con->flags directly instead.
  *
  * Context: Any context.
+ * Return: The current value of the @con->flags field.
  */
 static inline short console_srcu_read_flags(const struct console *con)
 {
 	WARN_ON_ONCE(!console_srcu_read_lock_is_held());
 
 	/*
-	 * 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 READ_ONCE() matches the WRITE_ONCE() when @flags are modified
+	 * for registered consoles with console_srcu_write_flags().
 	 */
 	return data_race(READ_ONCE(con->flags));
 }
diff --git a/include/linux/printk.h b/include/linux/printk.h
index d8b3f51d9e98..0ad3ee752635 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 console;
+
 extern const char linux_banner[];
 extern const char linux_proc_banner[];
 
@@ -193,6 +195,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 nbcon_driver_acquire(struct console *con);
+extern void nbcon_driver_release(struct console *con);
 #else
 static inline __printf(1, 0)
 int vprintk(const char *s, va_list args)
@@ -272,6 +276,15 @@ static inline void dump_stack(void)
 static inline void printk_trigger_flush(void)
 {
 }
+
+static inline void nbcon_driver_acquire(struct console *con)
+{
+}
+
+static inline void nbcon_driver_release(struct console *con)
+{
+}
+
 #endif
 
 bool this_cpu_in_panic(void);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index bb3324d49453..9a73dee32ad9 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -8,10 +8,13 @@
 #define LINUX_SERIAL_CORE_H
 
 #include <linux/bitops.h>
+#include <linux/bug.h>
 #include <linux/compiler.h>
 #include <linux/console.h>
 #include <linux/interrupt.h>
 #include <linux/circ_buf.h>
+#include <linux/lockdep.h>
+#include <linux/printk.h>
 #include <linux/spinlock.h>
 #include <linux/sched.h>
 #include <linux/tty.h>
@@ -606,6 +609,83 @@ static inline void __uart_port_unlock_irqrestore(struct uart_port *up, unsigned
 	spin_unlock_irqrestore(&up->lock, flags);
 }
 
+/**
+ * uart_port_set_cons - Safely set the @cons field for a uart
+ * @up:		The uart port to set
+ * @con:	The new console to set to
+ *
+ * This function must be used to set @up->cons. It uses the port lock to
+ * synchronize with the port lock wrappers in order to ensure that the console
+ * cannot change or disappear while another context is holding the port lock.
+ */
+static inline void uart_port_set_cons(struct uart_port *up, struct console *con)
+{
+	unsigned long flags;
+
+	__uart_port_lock_irqsave(up, &flags);
+	up->cons = con;
+	__uart_port_unlock_irqrestore(up, flags);
+}
+
+/* Only for internal port lock wrapper usage. */
+static inline void __uart_port_nbcon_acquire(struct uart_port *up)
+{
+	lockdep_assert_held_once(&up->lock);
+
+	if (likely(!uart_console(up)))
+		return;
+
+	if (up->cons->nbcon_drvdata) {
+		/*
+		 * If @up->cons is registered, prevent it from fully
+		 * unregistering until this context releases the nbcon.
+		 */
+		int cookie = console_srcu_read_lock();
+
+		/* Ensure console is registered and is an nbcon console. */
+		if (!hlist_unhashed_lockless(&up->cons->node) &&
+		    (console_srcu_read_flags(up->cons) & CON_NBCON)) {
+			WARN_ON_ONCE(up->cons->nbcon_drvdata->locked);
+
+			nbcon_driver_acquire(up->cons);
+
+			/*
+			 * Record @up->line to be used during release because
+			 * @up->cons->index can change while the port and
+			 * nbcon are locked.
+			 */
+			up->cons->nbcon_drvdata->owner_index = up->line;
+			up->cons->nbcon_drvdata->srcu_cookie = cookie;
+			up->cons->nbcon_drvdata->locked = true;
+		} else {
+			console_srcu_read_unlock(cookie);
+		}
+	}
+}
+
+/* Only for internal port lock wrapper usage. */
+static inline void __uart_port_nbcon_release(struct uart_port *up)
+{
+	lockdep_assert_held_once(&up->lock);
+
+	/*
+	 * uart_console() cannot be used here because @up->cons->index might
+	 * have changed. Check against @up->cons->nbcon_drvdata->owner_index
+	 * instead.
+	 */
+
+	if (unlikely(up->cons &&
+		     up->cons->nbcon_drvdata &&
+		     up->cons->nbcon_drvdata->locked &&
+		     up->cons->nbcon_drvdata->owner_index == up->line)) {
+		WARN_ON_ONCE(!up->cons->nbcon_drvdata->locked);
+
+		up->cons->nbcon_drvdata->locked = false;
+		nbcon_driver_release(up->cons);
+		console_srcu_read_unlock(up->cons->nbcon_drvdata->srcu_cookie);
+	}
+}
+
 /**
  * uart_port_lock - Lock the UART port
  * @up:		Pointer to UART port structure
@@ -613,6 +693,7 @@ static inline void __uart_port_unlock_irqrestore(struct uart_port *up, unsigned
 static inline void uart_port_lock(struct uart_port *up)
 {
 	spin_lock(&up->lock);
+	__uart_port_nbcon_acquire(up);
 }
 
 /**
@@ -622,6 +703,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_port_nbcon_acquire(up);
 }
 
 /**
@@ -632,6 +714,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_port_nbcon_acquire(up);
 }
 
 /**
@@ -642,7 +725,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_port_nbcon_acquire(up);
+	return true;
 }
 
 /**
@@ -654,7 +741,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_port_nbcon_acquire(up);
+	return true;
 }
 
 /**
@@ -663,6 +754,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_port_nbcon_release(up);
 	spin_unlock(&up->lock);
 }
 
@@ -672,6 +764,7 @@ static inline void uart_port_unlock(struct uart_port *up)
  */
 static inline void uart_port_unlock_irq(struct uart_port *up)
 {
+	__uart_port_nbcon_release(up);
 	spin_unlock_irq(&up->lock);
 }
 
@@ -682,6 +775,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_port_nbcon_release(up);
 	spin_unlock_irqrestore(&up->lock, flags);
 }
 
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 2516449f921d..38328cf0fd5c 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -3,9 +3,12 @@
 // Copyright (C) 2022 Intel, Thomas Gleixner
 
 #include <linux/kernel.h>
+#include <linux/bug.h>
 #include <linux/console.h>
 #include <linux/delay.h>
+#include <linux/export.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include "internal.h"
 /*
  * Printk console printing implementation for consoles which does not depend
@@ -988,3 +991,52 @@ void nbcon_free(struct console *con)
 
 	con->pbufs = NULL;
 }
+
+/**
+ * nbcon_driver_acquire - Acquire nbcon console and enter unsafe section
+ * @con:	The nbcon console to acquire
+ *
+ * Context:	Any context which could not be migrated to another CPU.
+ *
+ * Console drivers will usually use their own internal synchronization
+ * mechasism to synchronize between console printing and non-printing
+ * activities (such as setting baud rates). However, nbcon console drivers
+ * supporting atomic consoles may also want to mark unsafe sections when
+ * performing non-printing activities.
+ *
+ * This function acquires the nbcon console using priority NBCON_PRIO_NORMAL
+ * and marks it unsafe for handover/takeover.
+ *
+ * Console drivers using this function must have provided @nbcon_drvdata in
+ * their struct console, which is used to track ownership and state
+ * information.
+ */
+void nbcon_driver_acquire(struct console *con)
+{
+	struct nbcon_context *ctxt = &ACCESS_PRIVATE(con->nbcon_drvdata, ctxt);
+
+	cant_migrate();
+
+	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));
+}
+EXPORT_SYMBOL_GPL(nbcon_driver_acquire);
+
+/**
+ * nbcon_driver_release - Exit unsafe section and release the nbcon console
+ * @con:	The nbcon console acquired in nbcon_driver_acquire()
+ */
+void nbcon_driver_release(struct console *con)
+{
+	struct nbcon_context *ctxt = &ACCESS_PRIVATE(con->nbcon_drvdata, ctxt);
+
+	if (nbcon_context_exit_unsafe(ctxt))
+		nbcon_context_release(ctxt);
+}
+EXPORT_SYMBOL_GPL(nbcon_driver_release);
-- 
2.39.2

Re: [PATCH printk v4 09/27] printk: nbcon: Implement processing in port->lock wrapper

[John Ogness]

On 2024-04-03, John Ogness <john.ogness@linutronix.de> wrote:
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index d6a58a9e072a..2652b4d5c944 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3146,7 +3146,7 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
>  	uport->state = state;
>  
>  	state->pm_state = UART_PM_STATE_UNDEFINED;
> -	uport->cons = drv->cons;
> +	uart_port_set_cons(uport, drv->cons);
>  	uport->minor = drv->tty_driver->minor_start + uport->line;
>  	uport->name = kasprintf(GFP_KERNEL, "%s%d", drv->dev_name,
>  				drv->tty_driver->name_base + uport->line);

Sebastian Siewior pointed out that the port lock is initialized shortly
after this code. Since uart_port_set_cons() uses the port lock, the
spinlock initialization must come first. The changes for serial_core.c
should be:

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index d6a58a9e072a..0c13ea6a3afa 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3145,8 +3145,15 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
 	state->uart_port = uport;
 	uport->state = state;
 
+	/*
+	 * If this port is in use as a console then the spinlock is already
+	 * initialised.
+	 */
+	if (!uart_console_registered(uport))
+		uart_port_spin_lock_init(uport);
+
 	state->pm_state = UART_PM_STATE_UNDEFINED;
-	uport->cons = drv->cons;
+	uart_port_set_cons(uport, drv->cons);
 	uport->minor = drv->tty_driver->minor_start + uport->line;
 	uport->name = kasprintf(GFP_KERNEL, "%s%d", drv->dev_name,
 				drv->tty_driver->name_base + uport->line);
@@ -3155,13 +3162,6 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
 		goto out;
 	}
 
-	/*
-	 * If this port is in use as a console then the spinlock is already
-	 * initialised.
-	 */
-	if (!uart_console_registered(uport))
-		uart_port_spin_lock_init(uport);
-
 	if (uport->cons && uport->dev)
 		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
 

Re: [PATCH printk v4 08/27] serial: core: Provide low-level functions to lock port

[Petr Mladek]

On Wed 2024-04-03 00:17:10, John Ogness wrote:
> It will be necessary at times for the uart nbcon console
> drivers to acquire the port lock directly (without the
> additional nbcon functionality of the port lock wrappers).
> These are special cases such as the implementation of the
> device_lock()/device_unlock() callbacks or for internal
> port lock wrapper synchronization.
> 
> Provide low-level variants __uart_port_lock_irqsave() and
> __uart_port_unlock_irqrestore() for this purpose.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

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

Best Regards,
Petr

Re: [PATCH printk v4 08/27] serial: core: Provide low-level functions to lock port

[Greg Kroah-Hartman]

On Wed, Apr 03, 2024 at 12:17:10AM +0206, John Ogness wrote:
> It will be necessary at times for the uart nbcon console
> drivers to acquire the port lock directly (without the
> additional nbcon functionality of the port lock wrappers).
> These are special cases such as the implementation of the
> device_lock()/device_unlock() callbacks or for internal
> port lock wrapper synchronization.
> 
> Provide low-level variants __uart_port_lock_irqsave() and
> __uart_port_unlock_irqrestore() for this purpose.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  include/linux/serial_core.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH printk v4 09/27] printk: nbcon: Implement processing in port->lock wrapper

[Petr Mladek]

On Wed 2024-04-03 00:17:11, 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.
> 
> Introduce a new struct nbcon_drvdata within struct console that
> provides the necessary components for the port lock wrappers to
> acquire the nbcon console and track its ownership.
> 
> Also introduce uart_port_set_cons() as a wrapper to set @cons
> of a uart_port. The wrapper sets @cons under the port lock in
> order to prevent @cons from disappearing while another context
> owns the port lock via the port lock wrappers.
> 
> Also cleanup the description of the console_srcu_read_flags()
> function. It is used by the port lock wrappers to ensure a
> console cannot be fully unregistered while another context
> owns the port lock via the port lock wrappers.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -689,7 +689,7 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
>  			continue;
>  
>  		co->index = i;
> -		port->cons = co;
> +		uart_port_set_cons(port, co);
>  		return serial8250_console_setup(port, options, true);

I just noticed that this is a newcon->match() callback. It does:

  + univ8250_console_match()
    + serial8250_console_setup(port, options, true)   // @probe == true
      + probe_baud(port)

which manipulates the serial port.

We should call also the con->match() callback under console_lock()
in try_enable_preferred_console() like we do with con->setup,
see the commit 801410b26a0e8 ("serial: Lock console when calling into
driver before registration").

Maybe, we should just take console_lock() in register_console()
around these try_enable_*() calls.

Well, this is for a separated patch or separate patchset.

>  	}
>  
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -282,6 +282,25 @@ struct nbcon_write_context {
>  	bool			unsafe_takeover;
>  };
>  
> +/**
> + * struct nbcon_drvdata - Data to allow nbcon acquire in non-print context
> + * @ctxt:		The core console context
> + * @srcu_cookie:	Storage for a console_srcu_lock cookie, if needed
> + * @owner_index:	Storage for the owning console index, if needed
> + * @locked:		Storage for the locked state, if needed
> + *
> + * All fields (except for @ctxt) are available exclusively to the driver to
> + * use as needed. They are not used by the printk subsystem.
> + */
> +struct nbcon_drvdata {
> +	struct nbcon_context	__private ctxt;
> +
> +	/* reserved for driver use */
> +	int			srcu_cookie;
> +	short			owner_index;
> +	bool			locked;
> +};
> +
>  /**
>   * struct console - The console descriptor structure
>   * @name:		The name of the console driver
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -606,6 +609,83 @@ static inline void __uart_port_unlock_irqrestore(struct uart_port *up, unsigned
>  	spin_unlock_irqrestore(&up->lock, flags);
>  }
>  
> +/**
> + * uart_port_set_cons - Safely set the @cons field for a uart
> + * @up:		The uart port to set
> + * @con:	The new console to set to
> + *
> + * This function must be used to set @up->cons. It uses the port lock to
> + * synchronize with the port lock wrappers in order to ensure that the console
> + * cannot change or disappear while another context is holding the port lock.
> + */
> +static inline void uart_port_set_cons(struct uart_port *up, struct console *con)
> +{
> +	unsigned long flags;
> +
> +	__uart_port_lock_irqsave(up, &flags);
> +	up->cons = con;
> +	__uart_port_unlock_irqrestore(up, flags);
> +}
> +
> +/* Only for internal port lock wrapper usage. */
> +static inline void __uart_port_nbcon_acquire(struct uart_port *up)
> +{
> +	lockdep_assert_held_once(&up->lock);
> +
> +	if (likely(!uart_console(up)))
> +		return;
> +
> +	if (up->cons->nbcon_drvdata) {
> +		/*
> +		 * If @up->cons is registered, prevent it from fully
> +		 * unregistering until this context releases the nbcon.
> +		 */
> +		int cookie = console_srcu_read_lock();

[ later update: maybe skip 30 lines and read the "Hum, ho" part first]
[ even later update: or skip 60 lines and read "Win win" part first.]

OK, this makes sense. But I feel like we are in a lock cycle.
This code is called under "up->lock()". It will be taken also by
the newcon->device_lock() in register_console() so we would have:

  + register_console()
    + console_list_lock()   // serializes SRCU access to console list.
      + con->device_lock()
	+spin_lock(&up->lock)

  => console_list_lock -> up->lock

and here

  + uart_port_lock()
    + spin_lock(&up->lock)
     + __uart_port_nbcon_acquire()
       + console_srcu_read_lock()   // SRCU read lock serialized by console_list_lock

  => up->lock -> SRCU read lock serialized by console_list_lock

and for completeness:

  + unregister_console()
    + console_list_lock()
      + unregister_console_locked()
	+ synchronize_srcu(&console_srcu);


OK, it works because because scru_read_lock() is not blocking.
The synchronize_srcu() is called under console_list_lock(). So that
the only important thing is not taking console_list_lock() under
console_srcu_read_lock().


Hum, ho, it took me some time to create a mental model around this.
It is not that complicated after all:

    + console_list_lock(): serializes the entire (un)register console
	console operation. Well, it primary serializes
	the console_list manipulation, including up->cons->node
	which is tested below.

    + console_lock():  serializes emitting messages on legacy and
	boot consoles

    + con->device_lock aka port->lock: serializes more actions:

	1. any non-printk related access to the device (HW) like
	   a generic read/write.

	2. Access to the device by con->write() for legacy consoles.

	3. console registration, in particular console_list
	   manipulation, including up->cons->node. It is needed
	   to avoid races when the non-printk code has to decide
	   whether it needs to get serialized against nbcon
	   consoles or not.

	  For example, it should prevent races in
	   __uart_port_nbcon_acquire(up) and
	   __uart_port_nbcon_release(up) which are added in this patch.

But wait, we take con->device_lock() only in register_console().

Is this correct?

IMHO, it is a bug. We should (must) take con->device_lock()
also in unregister_console() when manipulating the
console_list and up->cons->node. Otherwise, uart_console(up)
would provide racy results.


Win win situation:

    If we take con->device_lock() in unregister_console() around
    console_list manipulation then the console could never
    disappear or be assigned to another port when both:

       uart_console(up) && hlist_unhashed_lockless(&up->cons->node)

    are "true"

    => we would not need to take console_scru_read_lock() here
    => we would not need to store/check up->line

    Heh, we even would not need "bool locked" because

       uart_console(up) && hlist_unhashed_lockless(&up->cons->node)

    would always return the same even in  __uart_port_nbcon_release()

    => easier code, straight serialization rules, no races.


> +
> +		/* Ensure console is registered and is an nbcon console. */
> +		if (!hlist_unhashed_lockless(&up->cons->node) &&
> +		    (console_srcu_read_flags(up->cons) & CON_NBCON)) {
> +			WARN_ON_ONCE(up->cons->nbcon_drvdata->locked);
> +
> +			nbcon_driver_acquire(up->cons);
> +
> +			/*
> +			 * Record @up->line to be used during release because
> +			 * @up->cons->index can change while the port and
> +			 * nbcon are locked.
> +			 */
> +			up->cons->nbcon_drvdata->owner_index = up->line;
> +			up->cons->nbcon_drvdata->srcu_cookie = cookie;
> +			up->cons->nbcon_drvdata->locked = true;
> +		} else {
> +			console_srcu_read_unlock(cookie);
> +		}
> +	}
> +}
> +
> +/* Only for internal port lock wrapper usage. */
> +static inline void __uart_port_nbcon_release(struct uart_port *up)
> +{
> +	lockdep_assert_held_once(&up->lock);
> +
> +	/*
> +	 * uart_console() cannot be used here because @up->cons->index might
> +	 * have changed. Check against @up->cons->nbcon_drvdata->owner_index
> +	 * instead.
> +	 */
> +
> +	if (unlikely(up->cons &&
> +		     up->cons->nbcon_drvdata &&
> +		     up->cons->nbcon_drvdata->locked &&
> +		     up->cons->nbcon_drvdata->owner_index == up->line)) {
> +		WARN_ON_ONCE(!up->cons->nbcon_drvdata->locked);

The WARN_ON_ONCE() would never trigger because
"up->cons->nbcon_drvdata->locked" is checked by the above
if-condition.

I hope that we would replace this by the same checks as in acquire()
part as proposed above.


> +
> +		up->cons->nbcon_drvdata->locked = false;
> +		nbcon_driver_release(up->cons);
> +		console_srcu_read_unlock(up->cons->nbcon_drvdata->srcu_cookie);
> +	}
> +}
> +
>  /**
>   * uart_port_lock - Lock the UART port
>   * @up:		Pointer to UART port structure
> @@ -654,7 +741,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_port_nbcon_acquire(up);

I would feel more comfortable if we created
__uart_port_nbcon_try_acquire(up). It would give up when it could
not acquire the context in the given timeout.

It would by similar to acquire(). The only difference would be that
it would return false on failure. And it would call:

/**
 * nbcon_driver_try_acquire - Try acquire nbcon console and enter unsafe section
 * @con:	The nbcon console to acquire
 *
 * Context:	Any context which could not be migrated to another CPU.
 *
 * Console drivers will usually use their own internal synchronization
 * mechanism to synchronize between console printing and non-printing
 * activities (such as setting baud rates). However, nbcon console drivers
 * supporting atomic consoles may also want to mark unsafe sections when
 * performing non-printing activities.
 *
 * This function tries to acquires the nbcon console using priority
 * NBCON_PRIO_NORMAL and marks it unsafe for handover/takeover.
 *
 * Return: true on success, false when it was not able to acquire the
 *	console and set it "usafe" for a takeover.
 */
bool nbcon_driver_try_acquire(struct console *con)
{
	struct nbcon_context *ctxt = &ACCESS_PRIVATE(con->nbcon_drvdata, ctxt);

	cant_migrate();

	memset(ctxt, 0, sizeof(*ctxt));
	ctxt->console	= con;
	ctxt->prio	= NBCON_PRIO_NORMAL;

	if (!nbcon_context_try_acquire(ctxt))
		return false;

	if (!nbcon_context_enter_unsafe(ctxt))
		return false;
}

It is probably not that important because it should not block emitting
the emergency or panic messages. They would use NBCON_PRIO_EMERGENCY
or NBCON_PRIO_PANIC in the important code paths.

But it looks semantically wrong to use a potentially blocking function
in a try_lock() API. IMHO, it would be a call for troubles.

> +	return true;
>  }
>  
>  /**
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 2516449f921d..38328cf0fd5c 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -988,3 +991,52 @@ void nbcon_free(struct console *con)
>  
>  	con->pbufs = NULL;
>  }
> +
> +/**
> + * nbcon_driver_acquire - Acquire nbcon console and enter unsafe section
> + * @con:	The nbcon console to acquire
> + *
> + * Context:	Any context which could not be migrated to another CPU.
> + *
> + * Console drivers will usually use their own internal synchronization
> + * mechasism to synchronize between console printing and non-printing
> + * activities (such as setting baud rates). However, nbcon console drivers
> + * supporting atomic consoles may also want to mark unsafe sections when
> + * performing non-printing activities.
> + *
> + * This function acquires the nbcon console using priority NBCON_PRIO_NORMAL
> + * and marks it unsafe for handover/takeover.
> + *
> + * Console drivers using this function must have provided @nbcon_drvdata in
> + * their struct console, which is used to track ownership and state
> + * information.
> + */
> +void nbcon_driver_acquire(struct console *con)
> +{
> +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(con->nbcon_drvdata, ctxt);

Hmm, we need to store somewhere the "struct nbcon_context" for this
generic purpose. If we agreed to remove struct nbcon_drvdata then
I would store it in struct console as

struct console {
[...]
	/**
	 * @device_nbcon_context:
	 *
	 * nbcon_context used to serialize non-printing operations on
	 * the same device.
	 *
	 * The device drivers synchronize these operations with a driver-specific
	 * lock, such as port->lock in the serial consoles. When the
	 * device is registered as a console, they additionally have to acquire
	 * this nbcon context to get serialized against the atomic_write()
	 * callback using the same device.
	 *
	 * The struct does not require any special initialization.
	 */
	struct nbcon_context	driver_nbcon_context;
[...]
	};

It will be unused for legacy consoles. But the plan is convert all
console drivers anyway.

IMHO, passing it via an optional pointer is not worth the complexity.

> +
> +	cant_migrate();
> +
> +	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));
> +}
> +EXPORT_SYMBOL_GPL(nbcon_driver_acquire);

Best Regards,
Petr