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

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

[John Ogness]

Hi,

This is v6 of a series to convert the 8250 driver to an NBCON
console, providing both threaded and atomic printing
implementations. v5 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.

This time we have Sashiko, which is sure to provide some
insights that we oversaw with v5.

The changes since v5:

- A new @avoid_modem_status_work field is added to struct
  uart_8250_port in order to avoid handling modem control
  during the suspend/resume window.

- Handle any pending modem control on resume.

- Allow serial8250_fifo_wait_for_lsr_thre() to be called
  with wctxt=NULL since the helper is now used in
  dw8250_idle_enter().

John Ogness

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

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

 drivers/tty/serial/8250/8250.h      |   4 +-
 drivers/tty/serial/8250/8250_core.c |  44 ++++++-
 drivers/tty/serial/8250/8250_dw.c   |   2 +-
 drivers/tty/serial/8250/8250_port.c | 193 +++++++++++++++++++++++-----
 include/linux/serial_8250.h         |  16 ++-
 5 files changed, 216 insertions(+), 43 deletions(-)


base-commit: 782f4dbd1794b4f30dc116a7ca42c5962c409be8
-- 
2.47.3

[PATCH tty v6 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 may not be
holding the port lock for atomic printing). 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 |  44 ++++++-
 drivers/tty/serial/8250/8250_dw.c   |   2 +-
 drivers/tty/serial/8250/8250_port.c | 190 +++++++++++++++++++++++-----
 include/linux/serial_8250.h         |  16 ++-
 5 files changed, 213 insertions(+), 43 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..f504621c5e24a 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,6 +609,9 @@ void serial8250_suspend_port(int line)
 	struct uart_8250_port *up = &serial8250_ports[line];
 	struct uart_port *port = &up->port;
 
+	/* No irq_work may be queued when suspending. */
+	up->avoid_modem_status_work = true;
+
 	if (!console_suspend_enabled && uart_console(port) &&
 	    port->type != PORT_8250) {
 		unsigned char canary = 0xa5;
@@ -620,6 +648,12 @@ void serial8250_resume_port(int line)
 		port->uartclk = 921600*16;
 	}
 	uart_resume_port(&serial8250_reg, port);
+
+	/* irq_work allowed again. Handle MSR now if pending. */
+	up->avoid_modem_status_work = false;
+	guard(uart_port_lock_irqsave)(port);
+	if (uart_console(port) && 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..3e623b71dadbc 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 used directly by the callback helper serial8250_console_write(),
+ * which may 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);
 		}
@@ -3198,11 +3215,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 +3241,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 +3289,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 +3319,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 +3329,57 @@ 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...
+ * 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.
- *
- *	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 console printing may
+	 * occur 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 +3393,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 +3425,23 @@ 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:
+	/*
+	 * If ownership was lost, this context must reacquire ownership and
+	 * re-enter the unsafe section in order to perform final actions
+	 * (such as re-enabling interrupts).
+	 */
+	if (!nbcon_can_proceed(wctxt)) {
+		do {
+			nbcon_reacquire_nobuf(wctxt);
+		} while (!nbcon_enter_unsafe(wctxt));
+	}
 
 	/*
 	 *	Finally, wait for transmitter to become empty
@@ -3378,11 +3464,29 @@ 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) {
+		/*
+		 * For atomic, it must be deferred to irq_work because this
+		 * may be a context that does not permit waking up tasks.
+		 */
+		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->avoid_modem_status_work)
+				irq_work_queue(&up->modem_status_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 +3504,23 @@ static unsigned int probe_baud(struct uart_port *port)
 	return (port->uartclk / 16) / quot;
 }
 
+/*
+ * irq_work handler to perform modem control. Only triggered via
+ * ->write_atomic() callback because it may be in a scheduler or
+ * NMI context, unable to wake tasks.
+ */
+static void modem_status_handler(struct irq_work *iwp)
+{
+	struct uart_8250_port *up = container_of(iwp, struct uart_8250_port, modem_status_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 +3530,9 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 	if (!port->iobase && !port->membase)
 		return -ENODEV;
 
+	up->console_line_ended = true;
+	init_irq_work(&up->modem_status_work, modem_status_handler);
+
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
 	else if (probe)
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index a95b2d143d248..12c2f6aadd420 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;
+
+	/* Do not queue irq_work for MSR handling when suspending. */
+	bool			avoid_modem_status_work;
+
 #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
 	unsigned char		msr_saved_flags;
+	struct irq_work		modem_status_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 v6 1/2] serial: 8250: Switch to nbcon console, take 2

[John Ogness]

On 2026-07-20, Sashiko wrote:
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index c0e8a4efbdcc8..f504621c5e24a 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -584,6 +609,9 @@ void serial8250_suspend_port(int line)
>>  	struct uart_8250_port *up = &serial8250_ports[line];
>>  	struct uart_port *port = &up->port;
>>  
>> +	/* No irq_work may be queued when suspending. */
>> +	up->avoid_modem_status_work = true;
>> +
>>  	if (!console_suspend_enabled && uart_console(port) &&
>>  	    port->type != PORT_8250) {
>>  		unsigned char canary = 0xa5;
>
> Is a call to irq_work_sync(&up->modem_status_work) needed here to
> flush any already queued work before the port is suspended?
>
> If an atomic console write triggers irq_work_queue() just as the
> system initiates suspend, this flag blocks new work but doesn't
> synchronize existing work.
>
> The asynchronous handler could then execute after the device clock is
> gated or memory unmapped, which might crash on serial_in(up,
> UART_MSR). (A similar teardown might also be needed in
> serial8250_console_exit()).

Sashiko is correct. For v7 I will add irq_work_sync() here and in
serial8250_console_exit().

>> -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 console printing may
>> +	 * occur without holding the port lock.
>>  	 */
>>  	ier = serial_port_in(port, UART_IER);
>> -	serial8250_clear_IER(up);
>> +	__serial8250_clear_IER(up);
>
> Since UART_IER is now read and saved into a local variable without
> holding port->lock during an atomic write, can concurrent driver
> updates be clobbered?
>
> If a normal driver operation like serial8250_start_tx() runs
> concurrently on another CPU, takes port->lock, and modifies UART_IER
> (e.g., setting UART_IER_THRI), the atomic console writer will later
> restore UART_IER from the stale local ier variable at the end of this
> function.
>
> Does this cause the concurrent driver update to be lost?

No, this is safe because the the port->lock is taken via wrappers
uart_port_lock*(), which also acquire nbcon ownership. For atomic
writes, nbcon ownership is acquired.

>> @@ -3352,10 +3425,23 @@ 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:
>> +	/*
>> +	 * If ownership was lost, this context must reacquire ownership and
>> +	 * re-enter the unsafe section in order to perform final actions
>> +	 * (such as re-enabling interrupts).
>> +	 */
>> +	if (!nbcon_can_proceed(wctxt)) {
>> +		do {
>> +			nbcon_reacquire_nobuf(wctxt);
>> +		} while (!nbcon_enter_unsafe(wctxt));
>> +	}
>
> Does this path leave the nbcon unsafe section unbalanced?
>
> If console ownership is not lost, nbcon_can_proceed(wctxt) evaluates
> to true, so the if block is skipped and nbcon_enter_unsafe() is never
> called.
>
> This leaves the subsequent hardware access wait_for_xmitr()
> unprotected, and the final nbcon_exit_unsafe(wctxt) call will
> underflow the unsafe section tracker because it was never re-entered.

Sashiko is correct. Because nbcon ownership can be lost at any time,
such underflows cannot be detected. For v7 I will change this block to:

	/*
	 * 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
>> 	 *	and restore the IER
>> 	 */
>> 	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
>> 	serial_port_out(port, UART_IER, ier);
>
> Since serial8250_console_write() no longer holds port->lock during an
> atomic write, do calls to wait_for_xmitr() and
> serial8250_fifo_wait_for_lsr_thre() introduce data races on
> up->lsr_saved_flags and up->msr_saved_flags?
>
> For example, wait_for_xmitr() calls serial_in(up, UART_MSR) which
> updates flags like so:
>
> wait_for_xmitr()
>     msr = serial_in(up, UART_MSR);
>     up->msr_saved_flags |= msr & MSR_SAVE_FLAGS;
>
> This read-modify-write races with the normal
> serial8250_handle_irq_locked() IRQ handler running concurrently under
> port->lock, potentially overwriting and permanently dropping hardware
> events like parity errors or modem control changes.

No, this is safe because the the port->lock is taken via wrappers
uart_port_lock*(), which also acquire nbcon ownership. wait_for_xmitr()
and serial8250_fifo_wait_for_lsr_thre() are called with nbcon ownership.

John

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

[Petr Mladek]

On Mon 2026-07-20 12:38:35, 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 may not be
> holding the port lock for atomic printing). 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,6 +609,9 @@ void serial8250_suspend_port(int line)
>  	struct uart_8250_port *up = &serial8250_ports[line];
>  	struct uart_port *port = &up->port;
>  
> +	/* No irq_work may be queued when suspending. */
> +	up->avoid_modem_status_work = true;

Do we need to synchronize this against serial8250_console_write()
where this flag is checked, please?

My understanding is that we should be on the safe side. Otherwise
there might be bigger problems.

I believe that this is called after both console_suspend_all() and
console_suspend(uport->cons). The later makes sure that the console
is not longer used even when @console_suspend_enabled is false.
And these functions even call synchronize_srcu(&console_srcu).

This might even answer the question from Sashiko AI whether
we should flush the related irq_work() here, see
https://sashiko.dev/#/patchset/20260720103242.7265-1-john.ogness%40linutronix.de

That said, I am not sure about RT_PREEMPT. AFAIK, it handles IRQs
in a kthread. In this case, synchronize_srcu() would not make
sure that the irq_work was procceed.

Note that Sashiko AI suggests that we might need to flush the irq_work
even in serial8250_console_exit(). I guess that the situation is
the same there. It is called after synchronize_srcu()...

> +
>  	if (!console_suspend_enabled && uart_console(port) &&
>  	    port->type != PORT_8250) {
>  		unsigned char canary = 0xa5;
> @@ -620,6 +648,12 @@ void serial8250_resume_port(int line)
>  		port->uartclk = 921600*16;
>  	}
>  	uart_resume_port(&serial8250_reg, port);
> +
> +	/* irq_work allowed again. Handle MSR now if pending. */
> +	up->avoid_modem_status_work = false;
> +	guard(uart_port_lock_irqsave)(port);
> +	if (uart_console(port) && up->msr_saved_flags)
> +		serial8250_modem_status(up);

I would use scoped_guard() to make the scope clear. Something like:

	scoped_guard(uart_port_lock_irqsave, port) {
		if (uart_console(port) && up->msr_saved_flags)
			serial8250_modem_status(up);
	}

Motivation: The guard() is pretty hidden. It can easily get overlooked
	when people add more code at the end of this function.

Wait, this should not be needed if we make sure that the work
was flushed in serial8250_suspend_port().

>  }
>  EXPORT_SYMBOL(serial8250_resume_port);
>  
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3286,39 +3329,57 @@ 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...
> + * 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.
> - *
> - *	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 console printing may
> +	 * occur without holding the port lock.

I would make the comment more clear when it might happen and if it is
safe. Something like:

	 * First, save the IER, then disable the interrupts. The special
	 * variant to clear the IER is used because an emergency and panic
	 * console printing is synchronized only by nbcon context 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 +3393,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 +3425,23 @@ 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:
> +	/*
> +	 * If ownership was lost, this context must reacquire ownership and
> +	 * re-enter the unsafe section in order to perform final actions
> +	 * (such as re-enabling interrupts).
> +	 */
> +	if (!nbcon_can_proceed(wctxt)) {

This should be:

	if (!nbcon_enter_unsafe(wctxt))

or even better:

	while (!nbcon_enter_unsafe(wctxt))
		nbcon_reacquire_nobuf(wctxt);

Otherwise, we would not be in the unsafe_context when
nbcon_can_proceed() succeeded.

Note: I have missed this. It was actually found by Sashiko...


> +		do {
> +			nbcon_reacquire_nobuf(wctxt);
> +		} while (!nbcon_enter_unsafe(wctxt));
> +	}
>  
>  	/*
>  	 *	Finally, wait for transmitter to become empty

Otherwise, it looks good to me.

Best Regards,
Petr

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

[John Ogness]

On 2026-07-20, Petr Mladek <pmladek@suse.com> wrote:
> On Mon 2026-07-20 12:38:35, John Ogness wrote:
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -584,6 +609,9 @@ void serial8250_suspend_port(int line)
>>  	struct uart_8250_port *up = &serial8250_ports[line];
>>  	struct uart_port *port = &up->port;
>>  
>> +	/* No irq_work may be queued when suspending. */
>> +	up->avoid_modem_status_work = true;
>
> Do we need to synchronize this against serial8250_console_write()
> where this flag is checked, please?

Yes. @avoid_modem_status_work needs to be modified under the port->lock
to ensure that serial8250_console_write() does not queue any new
irq_work. It is evaluated in atomic context, but taking the port->lock
also acquires nbcon ownership.

I will add this for v8 (in both places where @avoid_modem_status_work is
modified).

> This might even answer the question from Sashiko AI whether
> we should flush the related irq_work() here, see
> https://sashiko.dev/#/patchset/20260720103242.7265-1-john.ogness%40linutronix.de
>
> That said, I am not sure about RT_PREEMPT. AFAIK, it handles IRQs
> in a kthread. In this case, synchronize_srcu() would not make
> sure that the irq_work was procceed.
>
> Note that Sashiko AI suggests that we might need to flush the irq_work
> even in serial8250_console_exit(). I guess that the situation is
> the same there. It is called after synchronize_srcu()...

Yes, I will add irq_work_sync() in both locations.

>> @@ -620,6 +648,12 @@ void serial8250_resume_port(int line)
>>  		port->uartclk = 921600*16;
>>  	}
>>  	uart_resume_port(&serial8250_reg, port);
>> +
>> +	/* irq_work allowed again. Handle MSR now if pending. */
>> +	up->avoid_modem_status_work = false;
>> +	guard(uart_port_lock_irqsave)(port);
>> +	if (uart_console(port) && up->msr_saved_flags)
>> +		serial8250_modem_status(up);
>
> I would use scoped_guard() to make the scope clear. Something like:
>
> 	scoped_guard(uart_port_lock_irqsave, port) {
> 		if (uart_console(port) && up->msr_saved_flags)
> 			serial8250_modem_status(up);
> 	}
>
> Motivation: The guard() is pretty hidden. It can easily get overlooked
> 	when people add more code at the end of this function.

OK.

> Wait, this should not be needed if we make sure that the work
> was flushed in serial8250_suspend_port().

When !console_suspend_enabled, the driver will continue to print in
atomic mode, but will avoid the irq_work. It will however notice if
there are any MSR changes. Those changes are then handled here.

I suppose I should add an "if (!console_suspend_enabled)" to the
conditions.

>> -	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 console printing may
>> +	 * occur without holding the port lock.
>
> I would make the comment more clear when it might happen and if it is
> safe. Something like:
>
> 	 * First, save the IER, then disable the interrupts. The special
> 	 * variant to clear the IER is used because an emergency and panic
> 	 * console printing is synchronized only by nbcon context without
> 	 * holding the port lock.

OK.

>> +skip_write:
>> +	/*
>> +	 * If ownership was lost, this context must reacquire ownership and
>> +	 * re-enter the unsafe section in order to perform final actions
>> +	 * (such as re-enabling interrupts).
>> +	 */
>> +	if (!nbcon_can_proceed(wctxt)) {
>
> This should be:
>
> 	if (!nbcon_enter_unsafe(wctxt))
>
> or even better:
>
> 	while (!nbcon_enter_unsafe(wctxt))
> 		nbcon_reacquire_nobuf(wctxt);

Right. I will take the while loop variant.

Thanks for your review! (You can ignore my v7 as you caught everything
that I missed in my v7.)

John

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

[John Ogness]

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

This reverts commit 3d9e6f556e235ddcdc9f73600fdd46fe1736b090.

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 3e623b71dadbc..ca736b1bd0726 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 v6 2/2] serial: 8250: Revert "drop lockdep annotation from serial8250_clear_IER()"

[John Ogness]

On 2026-07-20, Sashiko wrote:
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 3e623b71dadbc..ca736b1bd0726 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);
>>  }
>
> Will restoring this lockdep assertion cause a false-positive warning
> during KGDB polling?
>
> Looking at serial8250_put_poll_char(), it explicitly avoids taking the
> port lock because CPUs are quiesced during KDB/KGDB use:
>
>     /*
>      * Normally the port is locked to synchronize UART_IER access
>      * against the console. However, this function is only used by
>      * KDB/KGDB, where it may not be possible to acquire the port
>      * lock because all other CPUs are quiesced. The quiescence
>      * should allow safe lockless usage here.
>      */
>
> However, it then calls serial8250_clear_IER():
>
>     ier = serial_port_in(port, UART_IER);
>     serial8250_clear_IER(up);
>
> Should serial8250_put_poll_char() be updated to use
> __serial8250_clear_IER() instead to avoid triggering this lockdep
> splat?

Sashiko is correct. For v7 this will be changed to call
__serial8250_clear_IER().

John