[PATCH tty v8 0/2] Convert 8250 to NBCON, take 2

[PATCH tty v8 0/2] Convert 8250 to NBCON, take 2

[John Ogness]

Hi,

This is v8 of a series to convert the 8250 driver to an NBCON
console, providing both threaded and atomic printing
implementations. v7 of this series is here [0].

Note that v5 was pulled into the 6.14 merge window, but was
then reverted before the 6.14-rc1 release due to kernel test
robot problems. It turned out that the problems were related
to general 8250 driver issues. Those issues have now been
addressed mainline, paving the way for a second take at
converting the 8250 driver to NBCON.

The changes since v7:

- Perform the following renames to make it clear that these
  fields and functions are only related to the console:

  In struct uart_8250_port:

    avoid_modem_status_work -> console_msr_work_allow
    modem_status_work       -> console_msr_work

  The irq_work handler:

    modem_status_handler()  -> console_msr_handler()

  Note that this also inverts the flag for allowing msr_work
  in order to avoid using a double-negative for the reader:

    if (!avoid_modem_status_work)

  changes to:

    if (console_msr_work_allow)

- Modify uart_8250_port->console_msr_work_allow under the
  port->lock in order to synchronize with its reader
  serial8250_console_write().

- In serial8250_suspend_port() and serial8250_resume_port(),
  move the @console_msr_work_allow update inside
  "if (uart_console(port))". This involves refactoring the
  neighboring code as well and has the nice side-effect of
  grouping all the console specialties into a single block.

- Change the IER-disabling comments in
  serial8250_console_write() and serial8250_put_poll_char()
  to mention what type of synchronization is used when the
  port->lock is not taken.

The changes v6 -> v7:

- Add irq_work_sync() calls to make sure there is no pending
  modem status work when suspending or unregistering the
  console.

- In the put_poll_char() callback, use __serial8250_clear_IER()
  because KDB/KGDB do not acquire the port lock to print.

- In serial8250_console_write(), re-enter unsafe section after
  printing text, even if ownership was _not_ lost. The final
  actions need to be performed within an unsafe section.

John Ogness

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

John Ogness (2):
  serial: 8250: Switch to nbcon console, take 2
  Revert "serial: 8250: drop lockdep annotation from
    serial8250_clear_IER()"

 drivers/tty/serial/8250/8250.h      |   4 +-
 drivers/tty/serial/8250/8250_core.c |  65 +++++++--
 drivers/tty/serial/8250/8250_dw.c   |   2 +-
 drivers/tty/serial/8250/8250_port.c | 200 +++++++++++++++++++++++-----
 include/linux/serial_8250.h         |  16 ++-
 5 files changed, 236 insertions(+), 51 deletions(-)


base-commit: 782f4dbd1794b4f30dc116a7ca42c5962c409be8
-- 
2.47.3

[PATCH tty v8 1/2] serial: 8250: Switch to nbcon console, take 2

[John Ogness]

Implement the necessary callbacks to switch the 8250 console driver
to perform as an nbcon console.

Add implementations for the nbcon console callbacks:

  ->write_atomic()
  ->write_thread()
  ->device_lock()
  ->device_unlock()

and add CON_NBCON to the initial @flags.

All hardware access in the callbacks is within unsafe sections.
The ->write_atomic() and ->write_thread() callbacks allow safe
handover/takeover per byte and add a preceding newline if they
take over from another context mid-line.

For the ->write_atomic() callback, a new irq_work is used to defer
modem control since it may be called from a context that does not
allow waking up tasks. During suspend/resume the irq_work is not
used as this has been shown to cause suspend problems for some
hardware. Upon resume, any pending modem control is performed.

Note: A new __serial8250_clear_IER() is introduced for direct
clearing of UART_IER during console writing (which will not be
holding the port lock for atomic printing or KDB/KGDB). This
allows restoring a lockdep check to serial8250_clear_IER() in
a follow-up commit.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250.h      |   4 +-
 drivers/tty/serial/8250/8250_core.c |  65 +++++++--
 drivers/tty/serial/8250/8250_dw.c   |   2 +-
 drivers/tty/serial/8250/8250_port.c | 197 +++++++++++++++++++++++-----
 include/linux/serial_8250.h         |  16 ++-
 5 files changed, 233 insertions(+), 51 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index b62f88eec881f..d672b014ade02 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -178,7 +178,9 @@ static unsigned int __maybe_unused serial_icr_read(struct uart_8250_port *up,
 
 void serial8250_clear_fifos(struct uart_8250_port *p);
 void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p);
-void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up, unsigned int count);
+void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up,
+				       struct nbcon_write_context *wctxt,
+				       unsigned int count);
 
 void serial8250_rpm_get(struct uart_8250_port *p);
 void serial8250_rpm_put(struct uart_8250_port *p);
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index c0e8a4efbdcc8..be6ff90732bd9 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -390,12 +390,34 @@ void __init serial8250_register_ports(struct uart_driver *drv, struct device *de
 
 #ifdef CONFIG_SERIAL_8250_CONSOLE
 
-static void univ8250_console_write(struct console *co, const char *s,
-				   unsigned int count)
+static void univ8250_console_write_atomic(struct console *co,
+					  struct nbcon_write_context *wctxt)
 {
 	struct uart_8250_port *up = &serial8250_ports[co->index];
 
-	serial8250_console_write(up, s, count);
+	serial8250_console_write(up, wctxt, true);
+}
+
+static void univ8250_console_write_thread(struct console *co,
+					  struct nbcon_write_context *wctxt)
+{
+	struct uart_8250_port *up = &serial8250_ports[co->index];
+
+	serial8250_console_write(up, wctxt, false);
+}
+
+static void univ8250_console_device_lock(struct console *co, unsigned long *flags)
+{
+	struct uart_port *up = &serial8250_ports[co->index].port;
+
+	__uart_port_lock_irqsave(up, flags);
+}
+
+static void univ8250_console_device_unlock(struct console *co, unsigned long flags)
+{
+	struct uart_port *up = &serial8250_ports[co->index].port;
+
+	__uart_port_unlock_irqrestore(up, flags);
 }
 
 static int univ8250_console_setup(struct console *co, char *options)
@@ -496,12 +518,15 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
 
 static struct console univ8250_console = {
 	.name		= "ttyS",
-	.write		= univ8250_console_write,
+	.write_atomic	= univ8250_console_write_atomic,
+	.write_thread	= univ8250_console_write_thread,
+	.device_lock	= univ8250_console_device_lock,
+	.device_unlock	= univ8250_console_device_unlock,
 	.device		= uart_console_device,
 	.setup		= univ8250_console_setup,
 	.exit		= univ8250_console_exit,
 	.match		= univ8250_console_match,
-	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
+	.flags		= CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON,
 	.index		= -1,
 	.data		= &serial8250_reg,
 };
@@ -584,13 +609,19 @@ void serial8250_suspend_port(int line)
 	struct uart_8250_port *up = &serial8250_ports[line];
 	struct uart_port *port = &up->port;
 
-	if (!console_suspend_enabled && uart_console(port) &&
-	    port->type != PORT_8250) {
-		unsigned char canary = 0xa5;
+	if (uart_console(port)) {
+		/* No irq_work may be queued when suspending. */
+		scoped_guard(uart_port_lock_irqsave, port)
+			up->console_msr_work_allow = false;
+		irq_work_sync(&up->console_msr_work);
 
-		serial_out(up, UART_SCR, canary);
-		if (serial_in(up, UART_SCR) == canary)
-			up->canary = canary;
+		if (!console_suspend_enabled && port->type != PORT_8250) {
+			unsigned char canary = 0xa5;
+
+			serial_out(up, UART_SCR, canary);
+			if (serial_in(up, UART_SCR) == canary)
+				up->canary = canary;
+		}
 	}
 
 	uart_suspend_port(&serial8250_reg, port);
@@ -620,6 +651,18 @@ void serial8250_resume_port(int line)
 		port->uartclk = 921600*16;
 	}
 	uart_resume_port(&serial8250_reg, port);
+
+	if (uart_console(port)) {
+
+		guard(uart_port_lock_irqsave)(port);
+
+		/* irq_work allowed again. */
+		up->console_msr_work_allow = true;
+
+		/* Handle any pending MSR changes. */
+		if (up->msr_saved_flags)
+			serial8250_modem_status(up);
+	}
 }
 EXPORT_SYMBOL(serial8250_resume_port);
 
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 5fba913f33010..51d026f20825a 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -156,7 +156,7 @@ static int dw8250_idle_enter(struct uart_port *p)
 	 *
 	 * FIXME: frame_time delay is too long with very low baudrates.
 	 */
-	serial8250_fifo_wait_for_lsr_thre(up, p->fifosize);
+	serial8250_fifo_wait_for_lsr_thre(up, NULL, p->fifosize);
 	ndelay(p->frame_time);
 
 	serial_port_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 8c241ec7f4f29..7726cca00d364 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -704,7 +704,12 @@ static void serial8250_clear_interrupts(struct uart_port *port)
 	serial_port_in(port, UART_MSR);
 }
 
-static void serial8250_clear_IER(struct uart_8250_port *up)
+/*
+ * Only to be directly used by serial8250_console_write() and
+ * serial8250_put_poll_char(), which do not require the port lock.
+ * Use serial8250_clear_IER() instead for all other cases.
+ */
+static void __serial8250_clear_IER(struct uart_8250_port *up)
 {
 	if (up->capabilities & UART_CAP_UUE)
 		serial_out(up, UART_IER, UART_IER_UUE);
@@ -712,6 +717,11 @@ static void serial8250_clear_IER(struct uart_8250_port *up)
 		serial_out(up, UART_IER, 0);
 }
 
+static inline void serial8250_clear_IER(struct uart_8250_port *up)
+{
+	__serial8250_clear_IER(up);
+}
+
 /*
  * This is a quickie test to see how big the FIFO is.
  * It doesn't work at all the time, more's the pity.
@@ -1284,9 +1294,6 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier)
 {
 	unsigned char mcr = serial8250_in_MCR(p);
 
-	/* Port locked to synchronize UART_IER access against the console. */
-	lockdep_assert_held_once(&p->port.lock);
-
 	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
 		mcr |= UART_MCR_RTS;
 	else
@@ -1302,6 +1309,16 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier)
 		serial8250_clear_and_reinit_fifos(p);
 
 		if (toggle_ier) {
+			/*
+			 * Port locked to synchronize UART_IER access against
+			 * the console. The lockdep_assert must be restricted
+			 * to this condition because only here is it
+			 * guaranteed that the port lock is held. The other
+			 * hardware access in this function is synchronized
+			 * by console ownership.
+			 */
+			lockdep_assert_held_once(&p->port.lock);
+
 			p->ier |= UART_IER_RLSI | UART_IER_RDI;
 			serial_port_out(&p->port, UART_IER, p->ier);
 		}
@@ -2053,10 +2070,13 @@ static void serial8250_put_poll_char(struct uart_port *port,
 
 	guard(serial8250_rpm)(up);
 	/*
-	 *	First save the IER then disable the interrupts
+	 * First, save the IER, then disable the interrupts. The special
+	 * variant to clear the IER is used because KDB/KGDB printing
+	 * is synchronized via CPU quiescence without holding the port
+	 * lock.
 	 */
 	ier = serial_port_in(port, UART_IER);
-	serial8250_clear_IER(up);
+	__serial8250_clear_IER(up);
 
 	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
 	/*
@@ -3198,11 +3218,22 @@ void serial8250_set_defaults(struct uart_8250_port *up)
 }
 EXPORT_SYMBOL_GPL(serial8250_set_defaults);
 
-void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up, unsigned int count)
+void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up,
+				       struct nbcon_write_context *wctxt,
+				       unsigned int count)
 {
 	unsigned int i;
 
 	for (i = 0; i < count; i++) {
+		/*
+		 * Pass the ownership as quickly as possible to a higher
+		 * priority context. Otherwise, its attempt to take over
+		 * the ownership might timeout. The new owner will wait
+		 * for UART_LSR_THRE before reusing the fifo.
+		 */
+		if (wctxt && !nbcon_can_proceed(wctxt))
+			return;
+
 		if (wait_for_lsr(up, UART_LSR_THRE))
 			return;
 	}
@@ -3213,7 +3244,11 @@ EXPORT_SYMBOL_NS_GPL(serial8250_fifo_wait_for_lsr_thre, "SERIAL_8250");
 
 static void serial8250_console_putchar(struct uart_port *port, unsigned char ch)
 {
+	struct uart_8250_port *up = up_to_u8250p(port);
+
 	serial_port_out(port, UART_TX, ch);
+
+	up->console_line_ended = (ch == '\n');
 }
 
 static void serial8250_console_wait_putchar(struct uart_port *port, unsigned char ch)
@@ -3257,20 +3292,29 @@ static void serial8250_console_restore(struct uart_8250_port *up)
  * to get empty.
  */
 static void serial8250_console_fifo_write(struct uart_8250_port *up,
-					  const char *s, unsigned int count)
+					  struct nbcon_write_context *wctxt)
 {
-	const char *end = s + count;
 	unsigned int fifosize = up->tx_loadsz;
 	struct uart_port *port = &up->port;
+	const char *s = wctxt->outbuf;
+	const char *end = s + wctxt->len;
 	unsigned int tx_count = 0;
 	bool cr_sent = false;
 	unsigned int i;
 
 	while (s != end) {
 		/* Allow timeout for each byte of a possibly full FIFO */
-		serial8250_fifo_wait_for_lsr_thre(up, fifosize);
+		serial8250_fifo_wait_for_lsr_thre(up, wctxt, fifosize);
 
+		/*
+		 * Fill the FIFO. If a handover or takeover occurs, writing
+		 * must be aborted since wctxt->outbuf and wctxt->len are no
+		 * longer valid.
+		 */
 		for (i = 0; i < fifosize && s != end; ++i) {
+			if (!nbcon_enter_unsafe(wctxt))
+				return;
+
 			if (*s == '\n' && !cr_sent) {
 				serial8250_console_putchar(port, '\r');
 				cr_sent = true;
@@ -3278,6 +3322,8 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 				serial8250_console_putchar(port, *s++);
 				cr_sent = false;
 			}
+
+			nbcon_exit_unsafe(wctxt);
 		}
 		tx_count = i;
 	}
@@ -3286,39 +3332,58 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 	 * Allow timeout for each byte written since the caller will only wait
 	 * for UART_LSR_BOTH_EMPTY using the timeout of a single character
 	 */
-	serial8250_fifo_wait_for_lsr_thre(up, tx_count);
+	serial8250_fifo_wait_for_lsr_thre(up, wctxt, tx_count);
+}
+
+static void serial8250_console_byte_write(struct uart_8250_port *up,
+					  struct nbcon_write_context *wctxt)
+{
+	struct uart_port *port = &up->port;
+	const char *s = wctxt->outbuf;
+	const char *end = s + wctxt->len;
+
+	/*
+	 * Write out the message. If a handover or takeover occurs, writing
+	 * must be aborted since wctxt->outbuf and wctxt->len are no longer
+	 * valid.
+	 */
+	while (s != end) {
+		if (!nbcon_enter_unsafe(wctxt))
+			return;
+
+		uart_console_write(port, s++, 1, serial8250_console_wait_putchar);
+
+		nbcon_exit_unsafe(wctxt);
+	}
 }
 
 /*
- *	Print a string to the serial port trying not to disturb
- *	any possible real use of the port...
- *
- *	The console_lock must be held when we get here.
+ * Print a string to the serial port trying not to disturb
+ * any possible real use of the port...
  *
- *	Doing runtime PM is really a bad idea for the kernel console.
- *	Thus, we assume the function is called when device is powered up.
+ * Doing runtime PM is really a bad idea for the kernel console.
+ * Thus, assume it is called when device is powered up.
  */
-void serial8250_console_write(struct uart_8250_port *up, const char *s,
-			      unsigned int count)
+void serial8250_console_write(struct uart_8250_port *up,
+			      struct nbcon_write_context *wctxt,
+			      bool is_atomic)
 {
 	struct uart_8250_em485 *em485 = up->em485;
 	struct uart_port *port = &up->port;
-	unsigned long flags;
-	unsigned int ier, use_fifo;
-	int locked = 1;
-
-	touch_nmi_watchdog();
+	unsigned int ier;
+	bool use_fifo;
 
-	if (oops_in_progress)
-		locked = uart_port_trylock_irqsave(port, &flags);
-	else
-		uart_port_lock_irqsave(port, &flags);
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
 
 	/*
-	 *	First save the IER then disable the interrupts
+	 * First, save the IER, then disable the interrupts. The special
+	 * variant to clear the IER is used because emergency and panic
+	 * printing is synchronized only by nbcon ownership without
+	 * holding the port lock.
 	 */
 	ier = serial_port_in(port, UART_IER);
-	serial8250_clear_IER(up);
+	__serial8250_clear_IER(up);
 
 	/* check scratch reg to see if port powered off during system sleep */
 	if (up->canary && (up->canary != serial_port_in(port, UART_SCR))) {
@@ -3332,6 +3397,18 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 		mdelay(port->rs485.delay_rts_before_send);
 	}
 
+	/* If ownership was lost, no writing is allowed */
+	if (!nbcon_can_proceed(wctxt))
+		goto skip_write;
+
+	/*
+	 * If console printer did not fully output the previous line, it must
+	 * have been handed or taken over. Insert a newline in order to
+	 * maintain clean output.
+	 */
+	if (!up->console_line_ended)
+		uart_console_write(port, "\n", 1, serial8250_console_wait_putchar);
+
 	use_fifo = (up->capabilities & UART_CAP_FIFO) &&
 		/*
 		 * BCM283x requires to check the fifo
@@ -3352,10 +3429,20 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 		 */
 		!uart_console_hwflow_active(&up->port);
 
+	nbcon_exit_unsafe(wctxt);
+
 	if (likely(use_fifo))
-		serial8250_console_fifo_write(up, s, count);
+		serial8250_console_fifo_write(up, wctxt);
 	else
-		uart_console_write(port, s, count, serial8250_console_wait_putchar);
+		serial8250_console_byte_write(up, wctxt);
+skip_write:
+	/*
+	 * Re-enter the unsafe section in order to perform final actions
+	 * (such as re-enabling interrupts). If ownership was lost, this
+	 * context must reacquire ownership.
+	 */
+	while (!nbcon_enter_unsafe(wctxt))
+		nbcon_reacquire_nobuf(wctxt);
 
 	/*
 	 *	Finally, wait for transmitter to become empty
@@ -3378,11 +3465,25 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 	 *	call it if we have saved something in the saved flags
 	 *	while processing with interrupts off.
 	 */
-	if (up->msr_saved_flags)
-		serial8250_modem_status(up);
+	if (up->msr_saved_flags) {
+		if (is_atomic) {
+			/*
+			 * For atomic, MSR handling must be deferred to
+			 * irq_work because this may be a context that does
+			 * not permit waking up tasks.
+			 *
+			 * But no irq_work may be queued when suspending.
+			 * In that case, the MSR handling will occur during
+			 * resume in serial8250_resume_port().
+			 */
+			if (up->console_msr_work_allow)
+				irq_work_queue(&up->console_msr_work);
+		} else {
+			serial8250_modem_status(up);
+		}
+	}
 
-	if (locked)
-		uart_port_unlock_irqrestore(port, flags);
+	nbcon_exit_unsafe(wctxt);
 }
 
 static unsigned int probe_baud(struct uart_port *port)
@@ -3400,8 +3501,24 @@ static unsigned int probe_baud(struct uart_port *port)
 	return (port->uartclk / 16) / quot;
 }
 
+/*
+ * irq_work handler to perform modem control during console output.
+ * Only triggered via ->write_atomic() callback because it may be
+ * in a scheduler or NMI context, unable to wake tasks.
+ */
+static void console_msr_handler(struct irq_work *iwp)
+{
+	struct uart_8250_port *up = container_of(iwp, struct uart_8250_port, console_msr_work);
+	struct uart_port *port = &up->port;
+
+	guard(uart_port_lock)(port);
+
+	serial8250_modem_status(up);
+}
+
 int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 {
+	struct uart_8250_port *up = up_to_u8250p(port);
 	int baud = 9600;
 	int bits = 8;
 	int parity = 'n';
@@ -3411,6 +3528,10 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 	if (!port->iobase && !port->membase)
 		return -ENODEV;
 
+	up->console_line_ended = true;
+	up->console_msr_work_allow = true;
+	init_irq_work(&up->console_msr_work, console_msr_handler);
+
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
 	else if (probe)
@@ -3431,6 +3552,10 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 
 int serial8250_console_exit(struct uart_port *port)
 {
+	struct uart_8250_port *up = up_to_u8250p(port);
+
+	irq_work_sync(&up->console_msr_work);
+
 	if (port->dev)
 		pm_runtime_put_sync(port->dev);
 
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index a95b2d143d248..0db2dd5569f4a 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -150,8 +150,20 @@ struct uart_8250_port {
 #define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
 	u16			lsr_saved_flags;
 	u16			lsr_save_mask;
+
+	/*
+	 * Track when a console line has been fully written to the
+	 * hardware, i.e. true when the most recent byte written to
+	 * UART_TX by the console was '\n'.
+	 */
+	bool			console_line_ended;
+
+	/* Allow queuing irq_work for MSR handling. */
+	bool			console_msr_work_allow;
+
 #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
 	unsigned char		msr_saved_flags;
+	struct irq_work		console_msr_work;
 
 	struct uart_8250_dma	*dma;
 	const struct uart_8250_ops *ops;
@@ -203,8 +215,8 @@ void serial8250_tx_chars(struct uart_8250_port *up);
 unsigned int serial8250_modem_status(struct uart_8250_port *up);
 void serial8250_init_port(struct uart_8250_port *up);
 void serial8250_set_defaults(struct uart_8250_port *up);
-void serial8250_console_write(struct uart_8250_port *up, const char *s,
-			      unsigned int count);
+void serial8250_console_write(struct uart_8250_port *up,
+			      struct nbcon_write_context *wctxt, bool in_atomic);
 int serial8250_console_setup(struct uart_port *port, char *options, bool probe);
 int serial8250_console_exit(struct uart_port *port);
 
-- 
2.47.3

Re: [PATCH tty v8 1/2] serial: 8250: Switch to nbcon console, take 2

[John Ogness]

On 2026-07-22, Sashiko wrote:
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 8c241ec7f4f29..7726cca00d364 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -3198,11 +3218,22 @@ void serial8250_set_defaults(struct uart_8250_port *up)
>>  }
>>  EXPORT_SYMBOL_GPL(serial8250_set_defaults);
>>  
>> -void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up, unsigned int count)
>> +void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up,
>> +				       struct nbcon_write_context *wctxt,
>> +				       unsigned int count)
>>  {
>>  	unsigned int i;
>>  
>>  	for (i = 0; i < count; i++) {
>> +		/*
>> +		 * Pass the ownership as quickly as possible to a higher
>> +		 * priority context. Otherwise, its attempt to take over
>> +		 * the ownership might timeout. The new owner will wait
>> +		 * for UART_LSR_THRE before reusing the fifo.
>> +		 */
>> +		if (wctxt && !nbcon_can_proceed(wctxt))
>> +			return;
>> +
>>  		if (wait_for_lsr(up, UART_LSR_THRE))
>>  			return;
>>  	}
>
> Does this leave the hardware state vulnerable to corruption during an
> emergency console takeover?
>
> The serial8250_fifo_wait_for_lsr_thre() function polls the UART_LSR
> register by calling wait_for_lsr() and modifies up->lsr_saved_flags
> non-atomically.  Since reading UART_LSR clears hardware error flags
> (parity, framing, overrun, break), concurrent reads can cause one CPU
> to consume and lose the flags before the other can observe them.
>
> Because serial8250_fifo_wait_for_lsr_thre() is called outside the
> nbcon_enter_unsafe() block, the nbcon system might grant a safe
> takeover to an emergency message (e.g. sysrq, panic, or NMI printk)
> preempting this loop, as it assumes no hardware access is taking
> place. The emergency writer will then access the same hardware
> registers concurrently.

Sashiko is correct. I did not know that the serial_lsr_in() macro (used
by wait_for_lsr()) updates @lsr_saved_flags! I looked at the other
serial_lsr_in() call sites and they are all in nbcon unsafe sections.

The for-loop in serial8250_fifo_wait_for_lsr_thre() should look like
this:

	for (i = 0; i < count; i++) {
		/* ... */
		if (wctxt && !nbcon_enter_unsafe(wctxt))
			return;

		if (wait_for_lsr(up, UART_LSR_THRE))
			return;

		if (wctxt)
			nbcon_exit_unsafe(wctxt);
	}

Note that Sashiko already reported [0] this in v7, but I did not look
deep enough at that comment, thinking Sashiko was misunderstanding nbcon
ownership. Instead it was me who was misunderstanding. :-/

John

[0] https://sashiko.dev/#/patchset/20260720135407.3925-1-john.ogness%40linutronix.de

Re: [PATCH tty v8 1/2] serial: 8250: Switch to nbcon console, take 2

[Petr Mladek]

On Wed 2026-07-22 16:59:09, John Ogness wrote:
> On 2026-07-22, Sashiko wrote:
> >> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> >> index 8c241ec7f4f29..7726cca00d364 100644
> >> --- a/drivers/tty/serial/8250/8250_port.c
> >> +++ b/drivers/tty/serial/8250/8250_port.c
> >> @@ -3198,11 +3218,22 @@ void serial8250_set_defaults(struct uart_8250_port *up)
> >>  }
> >>  EXPORT_SYMBOL_GPL(serial8250_set_defaults);
> >>  
> >> -void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up, unsigned int count)
> >> +void serial8250_fifo_wait_for_lsr_thre(struct uart_8250_port *up,
> >> +				       struct nbcon_write_context *wctxt,
> >> +				       unsigned int count)
> >>  {
> >>  	unsigned int i;
> >>  
> >>  	for (i = 0; i < count; i++) {
> >> +		/*
> >> +		 * Pass the ownership as quickly as possible to a higher
> >> +		 * priority context. Otherwise, its attempt to take over
> >> +		 * the ownership might timeout. The new owner will wait
> >> +		 * for UART_LSR_THRE before reusing the fifo.
> >> +		 */
> >> +		if (wctxt && !nbcon_can_proceed(wctxt))
> >> +			return;
> >> +
> >>  		if (wait_for_lsr(up, UART_LSR_THRE))
> >>  			return;
> >>  	}
> >
> > Does this leave the hardware state vulnerable to corruption during an
> > emergency console takeover?
> >
> > The serial8250_fifo_wait_for_lsr_thre() function polls the UART_LSR
> > register by calling wait_for_lsr() and modifies up->lsr_saved_flags
> > non-atomically.  Since reading UART_LSR clears hardware error flags
> > (parity, framing, overrun, break), concurrent reads can cause one CPU
> > to consume and lose the flags before the other can observe them.
> >
> > Because serial8250_fifo_wait_for_lsr_thre() is called outside the
> > nbcon_enter_unsafe() block, the nbcon system might grant a safe
> > takeover to an emergency message (e.g. sysrq, panic, or NMI printk)
> > preempting this loop, as it assumes no hardware access is taking
> > place. The emergency writer will then access the same hardware
> > registers concurrently.
> 
> Sashiko is correct. I did not know that the serial_lsr_in() macro (used
> by wait_for_lsr()) updates @lsr_saved_flags! I looked at the other
> serial_lsr_in() call sites and they are all in nbcon unsafe sections.

Yeah, I missed this as well. I would not expect that a function
called serial_lsr_in() would do a write. But it updates the internal
structure....

> The for-loop in serial8250_fifo_wait_for_lsr_thre() should look like
> this:
> 
> 	for (i = 0; i < count; i++) {
> 		/* ... */
> 		if (wctxt && !nbcon_enter_unsafe(wctxt))
> 			return;
> 
> 		if (wait_for_lsr(up, UART_LSR_THRE))
> 			return;
> 
> 		if (wctxt)
> 			nbcon_exit_unsafe(wctxt);
> 	}

LGTM.

> Note that Sashiko already reported [0] this in v7, but I did not look
> deep enough at that comment, thinking Sashiko was misunderstanding nbcon
> ownership. Instead it was me who was misunderstanding. :-/

I was confused as well.

Best Regards,
Petr

Re: [PATCH tty v8 1/2] serial: 8250: Switch to nbcon console, take 2

[Petr Mladek]

On Wed 2026-07-22 13:15:18, John Ogness wrote:
> Implement the necessary callbacks to switch the 8250 console driver
> to perform as an nbcon console.
> 
> Add implementations for the nbcon console callbacks:
> 
>   ->write_atomic()
>   ->write_thread()
>   ->device_lock()
>   ->device_unlock()
> 
> and add CON_NBCON to the initial @flags.
> 
> All hardware access in the callbacks is within unsafe sections.
> The ->write_atomic() and ->write_thread() callbacks allow safe
> handover/takeover per byte and add a preceding newline if they
> take over from another context mid-line.
> 
> For the ->write_atomic() callback, a new irq_work is used to defer
> modem control since it may be called from a context that does not
> allow waking up tasks. During suspend/resume the irq_work is not
> used as this has been shown to cause suspend problems for some
> hardware. Upon resume, any pending modem control is performed.
> 
> Note: A new __serial8250_clear_IER() is introduced for direct
> clearing of UART_IER during console writing (which will not be
> holding the port lock for atomic printing or KDB/KGDB). This
> allows restoring a lockdep check to serial8250_clear_IER() in
> a follow-up commit.
> 
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -584,13 +609,19 @@ void serial8250_suspend_port(int line)
>  	struct uart_8250_port *up = &serial8250_ports[line];
>  	struct uart_port *port = &up->port;
>  
> -	if (!console_suspend_enabled && uart_console(port) &&
> -	    port->type != PORT_8250) {
> -		unsigned char canary = 0xa5;
> +	if (uart_console(port)) {
> +		/* No irq_work may be queued when suspending. */
> +		scoped_guard(uart_port_lock_irqsave, port)

Strictly speaking, it should be enough to call "uart_port_lock_irq"
instead of the "_irqsafe" variant.

This code must be called in a schedulable context. Otherwise,
we can't call irq_work_sync() below.

Honestly, I have been a bit lost in the maze of macros and callbacks.
And I wanted to be sure where this is called. So, I added WARN_ON(1)
and dumped all locks.

And it seems that we are on the safe side. This code is
called here:

[   78.676626] CPU: 7 UID: 0 PID: 1542 Comm: bash Kdump: loaded Not tainted 7.2.0-rc4-default+ #24 PREEMPT(full)  252cd4fa4cf2d3bc860af0a8bfee0a3e125ae7f8
[   78.676634] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.17.0-2-g4f253b9b-prebuilt.qemu.org 04/01/2014
[   78.676640] RIP: 0010:serial8250_suspend_port.cold+0x1f/0x1e4
[   78.676648] Code: dc e8 70 f6 02 00 e9 b3 28 df 00 53 48 c7 c7 d0 6d 0d 9f 48 83 ec 20 48 89 54 24 10 4c 89 4c 24 08 4c 89 04 24 e8 2c 1b fb ff <0f> 0b e8 d5 ee fa ff 48 8b 3c 24 e8 6c fa
 49 01 83 3d 79 0c 6b 02
[   78.676653] RSP: 0018:ffffd16d82743c10 EFLAGS: 00010246
[   78.676662] RAX: 0000000000000041 RBX: ffff8d8a00ab3800 RCX: 0000000000000001
[   78.676667] RDX: 0000000000000000 RSI: ffffffff9f1ae0d7 RDI: 00000000ffffffff
[   78.676672] RBP: 0000000000000001 R08: 0000000000000000 R09: 0000000000000001
[   78.676677] R10: 0000000000000007 R11: 0000000000000000 R12: ffffffff9f7f5a80
[   78.676681] R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000000
[   78.676689] FS:  00007fadbce4e200(0000) GS:ffff8d8ada0a0000(0000) knlGS:0000000000000000
[   78.676695] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   78.676699] CR2: 000055b5982aea10 CR3: 000000010cb6b000 CR4: 0000000000750ef0
[   78.676707] PKRU: 55555554
[   78.676712] Call Trace:
[   78.676718]  <TASK>
[   78.676732]  serial_pnp_suspend+0x11/0x20
[   78.676745]  __pnp_bus_suspend+0x3d/0x100
[   78.676761]  ? __pfx_pnp_bus_freeze+0x10/0x10
[   78.676770]  dpm_run_callback+0x66/0x1f0
[   78.676790]  device_suspend+0x235/0x700
[   78.676798]  ? srso_alias_return_thunk+0x5/0xfbef5
[   78.676807]  ? __lock_release.isra.0+0x1c9/0x300
[   78.676825]  ? __pfx_dpm_watchdog_handler+0x10/0x10
[   78.676851]  ? __mutex_unlock_slowpath+0x1a9/0x410
[   78.676874]  dpm_suspend+0x18f/0x400
[   78.676888]  hibernation_snapshot+0xcb/0x1e0
[   78.676908]  hibernate.cold+0xfd/0x48b
[   78.676925]  state_store+0xc3/0xd0
[   78.676940]  kernfs_fop_write_iter+0x163/0x240
[   78.676960]  vfs_write+0x222/0x560
[   78.676997]  ksys_write+0x70/0xf0
[   78.677012]  do_syscall_64+0xb1/0x6d0
[   78.677021]  ? srso_alias_return_thunk+0x5/0xfbef5
[   78.677036]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   78.677043] RIP: 0033:0x7fadbc106af3

And the locks are:

[   78.677780] 7 locks held by bash/1542:
[   78.677784]  #0: ffff8d8a038a4470 (sb_writers#5){.+.+}-{0:0}, at: ksys_write+0x70/0xf0
[   78.677820]  #1: ffff8d8a08f44288 (&of->mutex#2){+.+.}-{4:4}, at: kernfs_fop_write_iter+0x105/0x240
[   78.677849]  #2: ffff8d8a00d9a230 (kn->active#105){.+.+}-{0:0}, at: kernfs_fop_write_iter+0x11a/0x240
[   78.677878]  #3: ffffffff9f48c0a8 (system_transition_mutex){+.+.}-{4:4}, at: lock_system_sleep+0x2b/0x40
[   78.677904]  #4: ffffffff9f804968 (device_hotplug_lock){+.+.}-{4:4}, at: hibernate.cold+0xac/0x48b
[   78.677930]  #5: ffff8d8a00ab3938 (&dev->mutex){....}-{4:4}, at: device_suspend+0x1ff/0x700
[   78.677955]  #6: ffffffff9f5ab1a0 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire.constprop.0+0x7/0x30

, where the rcu_read_lock() is from debug_show_all_locks() itself.

The function serial8250_suspend_port() is called under dev->mutex
so we are on the safe side.


> +			up->console_msr_work_allow = false;
> +		irq_work_sync(&up->console_msr_work);
>  
> -		serial_out(up, UART_SCR, canary);
> -		if (serial_in(up, UART_SCR) == canary)
> -			up->canary = canary;
> +		if (!console_suspend_enabled && port->type != PORT_8250) {
> +			unsigned char canary = 0xa5;
> +
> +			serial_out(up, UART_SCR, canary);
> +			if (serial_in(up, UART_SCR) == canary)
> +				up->canary = canary;
> +		}
>  	}
>  
>  	uart_suspend_port(&serial8250_reg, port);
> @@ -620,6 +651,18 @@ void serial8250_resume_port(int line)
>  		port->uartclk = 921600*16;
>  	}
>  	uart_resume_port(&serial8250_reg, port);
> +
> +	if (uart_console(port)) {
> +
> +		guard(uart_port_lock_irqsave)(port);

This should be symetrical -> s/irqsave/irq

> +		/* irq_work allowed again. */
> +		up->console_msr_work_allow = true;
> +
> +		/* Handle any pending MSR changes. */
> +		if (up->msr_saved_flags)
> +			serial8250_modem_status(up);
> +	}
>  }
>  EXPORT_SYMBOL(serial8250_resume_port);


Plus the problem reported by Sashiko.

Otherwise, it looks good to me.

Best Regards,
Petr

[PATCH tty v8 2/2] Revert "serial: 8250: drop lockdep annotation from serial8250_clear_IER()"

[John Ogness]

This reverts commit 3d9e6f556e235ddcdc9f73600fdd46fe1736b090.

The 8250 driver no longer depends on @oops_in_progress and
will no longer violate the port->lock locking constraints.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_port.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 7726cca00d364..dfbdaf1fcc79c 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -719,6 +719,9 @@ static void __serial8250_clear_IER(struct uart_8250_port *up)
 
 static inline void serial8250_clear_IER(struct uart_8250_port *up)
 {
+	/* Port locked to synchronize UART_IER access against the console */
+	lockdep_assert_held_once(&up->port.lock);
+
 	__serial8250_clear_IER(up);
 }
 
-- 
2.47.3

Re: [PATCH tty v8 2/2] Revert "serial: 8250: drop lockdep annotation from serial8250_clear_IER()"

[Petr Mladek]

On Wed 2026-07-22 13:15:19, John Ogness wrote:
> This reverts commit 3d9e6f556e235ddcdc9f73600fdd46fe1736b090.
> 
> The 8250 driver no longer depends on @oops_in_progress and
> will no longer violate the port->lock locking constraints.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Makes sense.

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

Best Regards,
Petr