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

From: John Ogness
Date: Tue Apr 02 2024 - 18:13:48 EST

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