[PATCH tty-next v4 0/6] convert 8250 to nbcon

[PATCH tty-next v4 0/6] convert 8250 to nbcon

[John Ogness]

This is v4 of a series to convert the 8250 driver to an NBCON
console, providing both threaded and atomic printing
implementations. v3 of this series is here [0]. Additional
background information about NBCON consoles in general is
available in the cover letter of v2 [1].

The changes since v3:

- For callbacks ->rs485_stop_tx() and ->rs485_start_tx(),
  rename argument @in_con to @toggle_ier (inverts meaning).

- For univ8250_console_device_lock() and
  univ8250_console_device_unlock(), rename argument @con to @co.

- Do not introduce helpers __serial8250_stop_rx_mask_dr(),
  __serial8250_stop_rx_int(), __serial8250_start_rx_int().

- Use @frame_time to determine per-character timeout, fallback
  to 10ms if @frame_time not available.

- Use shorter code syntax when setting @console_line_ended.

- Introduce helper function fifo_wait_for_lsr() to wait for
  multiple characters.

- For serial8250_console_fifo_write() and
  serial8250_console_byte_write(), remove unnecessary
  READ_ONCE() usage.

- For serial8250_console_fifo_write() and
  serial8250_console_byte_write(), use nbcon_can_proceed()
  rather than repeatedly enter/exit unsafe regions.

- Initialize @modem_status_work using init_irq_work() rather
  than IRQ_WORK_INIT().

- Commit message and comment style cleanups as requested.

John Ogness

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

John Ogness (6):
  serial: 8250: Adjust the timeout for FIFO mode
  serial: 8250: Use frame rate to determine timeout
  serial: 8250: Use high-level writing function for FIFO
  serial: 8250: Provide flag for IER toggling for RS485
  serial: 8250: Switch to nbcon console
  serial: 8250: Revert "drop lockdep annotation from
    serial8250_clear_IER()"

 drivers/tty/serial/8250/8250.h            |   4 +-
 drivers/tty/serial/8250/8250_bcm2835aux.c |   4 +-
 drivers/tty/serial/8250/8250_core.c       |  35 +++-
 drivers/tty/serial/8250/8250_omap.c       |   2 +-
 drivers/tty/serial/8250/8250_port.c       | 223 +++++++++++++++++-----
 include/linux/serial_8250.h               |  12 +-
 6 files changed, 214 insertions(+), 66 deletions(-)


base-commit: 2c1fd53af21b8cb13878b054894d33d3383eb1f3
-- 
2.39.5

[PATCH tty-next v4 1/6] serial: 8250: Adjust the timeout for FIFO mode

[John Ogness]

After a console has written a record into UART_TX, it uses
wait_for_xmitr() to wait until the data has been sent out before
returning. However, wait_for_xmitr() will timeout after 10ms,
regardless if the data has been transmitted or not.

For single bytes, this timeout is sufficient even at very slow
baud rates, such as 1200bps. However, when FIFO mode is used,
there may be 64 bytes pushed into the FIFO at once. At a baud
rate of 115200bps, the 10ms timeout is still sufficient. But
when using lower baud rates (such as 57600bps), the timeout
is _not_ sufficient. This causes longer lines to be cut off,
resulting in lost and horribly misformatted output on the
console.

When using FIFO mode, take the number of bytes into account to
determine an appropriate maximum timeout. Increasing the timeout
does not affect performance since ideally the timeout never
occurs.

Fixes: 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Wander Lairson Costa <wander@redhat.com>
---
 drivers/tty/serial/8250/8250_port.c | 32 +++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 1a65d3e5f3c0..3a946ebe9139 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2078,7 +2078,8 @@ static void serial8250_break_ctl(struct uart_port *port, int break_state)
 	serial8250_rpm_put(up);
 }
 
-static void wait_for_lsr(struct uart_8250_port *up, int bits)
+/* Returns true if @bits were set, false on timeout */
+static bool wait_for_lsr(struct uart_8250_port *up, int bits)
 {
 	unsigned int status, tmout = 10000;
 
@@ -2093,11 +2094,11 @@ static void wait_for_lsr(struct uart_8250_port *up, int bits)
 		udelay(1);
 		touch_nmi_watchdog();
 	}
+
+	return (tmout != 0);
 }
 
-/*
- *	Wait for transmitter & holding register to empty
- */
+/* Wait for transmitter and holding register to empty with timeout */
 static void wait_for_xmitr(struct uart_8250_port *up, int bits)
 {
 	unsigned int tmout;
@@ -3322,6 +3323,16 @@ static void serial8250_console_restore(struct uart_8250_port *up)
 	serial8250_out_MCR(up, up->mcr | UART_MCR_DTR | UART_MCR_RTS);
 }
 
+static void fifo_wait_for_lsr(struct uart_8250_port *up, unsigned int count)
+{
+	unsigned int i;
+
+	for (i = 0; i < count; i++) {
+		if (wait_for_lsr(up, UART_LSR_THRE))
+			return;
+	}
+}
+
 /*
  * Print a string to the serial port using the device FIFO
  *
@@ -3331,13 +3342,15 @@ static void serial8250_console_restore(struct uart_8250_port *up)
 static void serial8250_console_fifo_write(struct uart_8250_port *up,
 					  const char *s, unsigned int count)
 {
-	int i;
 	const char *end = s + count;
 	unsigned int fifosize = up->tx_loadsz;
+	unsigned int tx_count = 0;
 	bool cr_sent = false;
+	unsigned int i;
 
 	while (s != end) {
-		wait_for_lsr(up, UART_LSR_THRE);
+		/* Allow timeout for each byte of a possibly full FIFO */
+		fifo_wait_for_lsr(up, fifosize);
 
 		for (i = 0; i < fifosize && s != end; ++i) {
 			if (*s == '\n' && !cr_sent) {
@@ -3348,7 +3361,14 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 				cr_sent = false;
 			}
 		}
+		tx_count = i;
 	}
+
+	/*
+	 * Allow timeout for each byte written since the caller will only wait
+	 * for UART_LSR_BOTH_EMPTY using the timeout of a single character
+	 */
+	fifo_wait_for_lsr(up, tx_count);
 }
 
 /*
-- 
2.39.5

[PATCH tty-next v4 2/6] serial: 8250: Use frame rate to determine timeout

[John Ogness]

Rather than using a hard-coded per-character Tx-timeout of 10ms,
use the frame rate to determine a timeout value. The value is
doubled to ensure that a timeout is only hit during unexpected
circumstances.

Since the frame rate may not be available during early printing,
the previous 10ms value is kept as a fallback.

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

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3a946ebe9139..aeacf9d452e4 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2083,7 +2083,14 @@ static bool wait_for_lsr(struct uart_8250_port *up, int bits)
 {
 	unsigned int status, tmout = 10000;
 
-	/* Wait up to 10ms for the character(s) to be sent. */
+	/*
+	 * Wait for a character to be sent. Fallback to a safe default
+	 * timeout value if @frame_time is not available.
+	 */
+
+	if (up->port.frame_time)
+		tmout = up->port.frame_time * 2 / NSEC_PER_USEC;
+
 	for (;;) {
 		status = serial_lsr_in(up);
 
-- 
2.39.5

[PATCH tty-next v4 3/6] serial: 8250: Use high-level writing function for FIFO

[John Ogness]

Currently serial8250_console_fifo_write() directly writes into
the UART_TX register rather than using the high-level function
serial8250_console_putchar(). This is because
serial8250_console_putchar() waits for the holding register to
become empty, which would defeat the purpose of the FIFO code.

Move the LSR_THRE waiting to a new function
serial8250_console_wait_putchar() so that the FIFO code can use
serial8250_console_putchar(). This will be particularly important
for a follow-up commit, where output bytes are inspected to track
newlines.

This is only refactoring and has no functional change.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 drivers/tty/serial/8250/8250_port.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index aeacf9d452e4..812f003c252d 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3297,11 +3297,16 @@ EXPORT_SYMBOL_GPL(serial8250_set_defaults);
 #ifdef CONFIG_SERIAL_8250_CONSOLE
 
 static void serial8250_console_putchar(struct uart_port *port, unsigned char ch)
+{
+	serial_port_out(port, UART_TX, ch);
+}
+
+static void serial8250_console_wait_putchar(struct uart_port *port, unsigned char ch)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 
 	wait_for_xmitr(up, UART_LSR_THRE);
-	serial_port_out(port, UART_TX, ch);
+	serial8250_console_putchar(port, ch);
 }
 
 /*
@@ -3351,6 +3356,7 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 {
 	const char *end = s + count;
 	unsigned int fifosize = up->tx_loadsz;
+	struct uart_port *port = &up->port;
 	unsigned int tx_count = 0;
 	bool cr_sent = false;
 	unsigned int i;
@@ -3361,10 +3367,10 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 
 		for (i = 0; i < fifosize && s != end; ++i) {
 			if (*s == '\n' && !cr_sent) {
-				serial_out(up, UART_TX, '\r');
+				serial8250_console_putchar(port, '\r');
 				cr_sent = true;
 			} else {
-				serial_out(up, UART_TX, *s++);
+				serial8250_console_putchar(port, *s++);
 				cr_sent = false;
 			}
 		}
@@ -3444,7 +3450,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 	if (likely(use_fifo))
 		serial8250_console_fifo_write(up, s, count);
 	else
-		uart_console_write(port, s, count, serial8250_console_putchar);
+		uart_console_write(port, s, count, serial8250_console_wait_putchar);
 
 	/*
 	 *	Finally, wait for transmitter to become empty
-- 
2.39.5

[PATCH tty-next v4 4/6] serial: 8250: Provide flag for IER toggling for RS485

[John Ogness]

For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
console ->write() callback needs to enable/disable Tx. It does
this by calling the ->rs485_start_tx() and ->rs485_stop_tx()
callbacks. However, some of these callbacks also disable/enable
interrupts and makes power management calls. This causes 2
problems for console writing:

1. A console write can occur in contexts that are illegal for
   pm_runtime_*(). It is not even necessary for console writing
   to use pm_runtime_*() because a console already does this in
   serial8250_console_setup() and serial8250_console_exit().

2. The console ->write() callback already handles
   disabling/enabling the interrupts by properly restoring the
   previous IER value.

Add an argument @toggle_ier to the ->rs485_start_tx() and
->rs485_stop_tx() callbacks to specify if they may disable/enable
receive interrupts while using pm_runtime_*(). Console writing
will not allow the toggling.

For all call sites other than console writing there is no
functional change.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250.h            |  4 ++--
 drivers/tty/serial/8250/8250_bcm2835aux.c |  4 ++--
 drivers/tty/serial/8250/8250_omap.c       |  2 +-
 drivers/tty/serial/8250/8250_port.c       | 26 +++++++++++++----------
 include/linux/serial_8250.h               |  4 ++--
 5 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index e5310c65cf52..11e05aa014e5 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -231,8 +231,8 @@ void serial8250_rpm_put_tx(struct uart_8250_port *p);
 
 int serial8250_em485_config(struct uart_port *port, struct ktermios *termios,
 			    struct serial_rs485 *rs485);
-void serial8250_em485_start_tx(struct uart_8250_port *p);
-void serial8250_em485_stop_tx(struct uart_8250_port *p);
+void serial8250_em485_start_tx(struct uart_8250_port *p, bool toggle_ier);
+void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier);
 void serial8250_em485_destroy(struct uart_8250_port *p);
 extern struct serial_rs485 serial8250_em485_supported;
 
diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index fdb53b54e99e..0609582a62f7 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -46,7 +46,7 @@ struct bcm2835aux_data {
 	u32 cntl;
 };
 
-static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up)
+static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up, bool toggle_ier)
 {
 	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
 		struct bcm2835aux_data *data = dev_get_drvdata(up->port.dev);
@@ -65,7 +65,7 @@ static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up)
 		serial8250_out_MCR(up, UART_MCR_RTS);
 }
 
-static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up)
+static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up, bool toggle_ier)
 {
 	if (up->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
 		serial8250_out_MCR(up, 0);
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 42b4aa56b902..c2b75e3f106d 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -365,7 +365,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
 
 	if (up->port.rs485.flags & SER_RS485_ENABLED &&
 	    up->port.rs485_config == serial8250_em485_config)
-		serial8250_em485_stop_tx(up);
+		serial8250_em485_stop_tx(up, true);
 }
 
 /*
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 812f003c252d..e38271f477d1 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -578,7 +578,7 @@ static int serial8250_em485_init(struct uart_8250_port *p)
 
 deassert_rts:
 	if (p->em485->tx_stopped)
-		p->rs485_stop_tx(p);
+		p->rs485_stop_tx(p, true);
 
 	return 0;
 }
@@ -1398,10 +1398,11 @@ static void serial8250_stop_rx(struct uart_port *port)
 /**
  * serial8250_em485_stop_tx() - generic ->rs485_stop_tx() callback
  * @p: uart 8250 port
+ * @toggle_ier: true to allow enabling receive interrupts
  *
  * Generic callback usable by 8250 uart drivers to stop rs485 transmission.
  */
-void serial8250_em485_stop_tx(struct uart_8250_port *p)
+void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier)
 {
 	unsigned char mcr = serial8250_in_MCR(p);
 
@@ -1422,8 +1423,10 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
 	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
 		serial8250_clear_and_reinit_fifos(p);
 
-		p->ier |= UART_IER_RLSI | UART_IER_RDI;
-		serial_port_out(&p->port, UART_IER, p->ier);
+		if (toggle_ier) {
+			p->ier |= UART_IER_RLSI | UART_IER_RDI;
+			serial_port_out(&p->port, UART_IER, p->ier);
+		}
 	}
 }
 EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
@@ -1438,7 +1441,7 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
 	serial8250_rpm_get(p);
 	uart_port_lock_irqsave(&p->port, &flags);
 	if (em485->active_timer == &em485->stop_tx_timer) {
-		p->rs485_stop_tx(p);
+		p->rs485_stop_tx(p, true);
 		em485->active_timer = NULL;
 		em485->tx_stopped = true;
 	}
@@ -1470,7 +1473,7 @@ static void __stop_tx_rs485(struct uart_8250_port *p, u64 stop_delay)
 		em485->active_timer = &em485->stop_tx_timer;
 		hrtimer_start(&em485->stop_tx_timer, ns_to_ktime(stop_delay), HRTIMER_MODE_REL);
 	} else {
-		p->rs485_stop_tx(p);
+		p->rs485_stop_tx(p, true);
 		em485->active_timer = NULL;
 		em485->tx_stopped = true;
 	}
@@ -1559,6 +1562,7 @@ static inline void __start_tx(struct uart_port *port)
 /**
  * serial8250_em485_start_tx() - generic ->rs485_start_tx() callback
  * @up: uart 8250 port
+ * @toggle_ier: true to allow disabling receive interrupts
  *
  * Generic callback usable by 8250 uart drivers to start rs485 transmission.
  * Assumes that setting the RTS bit in the MCR register means RTS is high.
@@ -1566,11 +1570,11 @@ static inline void __start_tx(struct uart_port *port)
  * stoppable by disabling the UART_IER_RDI interrupt.  (Some chips set the
  * UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.)
  */
-void serial8250_em485_start_tx(struct uart_8250_port *up)
+void serial8250_em485_start_tx(struct uart_8250_port *up, bool toggle_ier)
 {
 	unsigned char mcr = serial8250_in_MCR(up);
 
-	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
+	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX) && toggle_ier)
 		serial8250_stop_rx(&up->port);
 
 	if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
@@ -1604,7 +1608,7 @@ static bool start_tx_rs485(struct uart_port *port)
 	if (em485->tx_stopped) {
 		em485->tx_stopped = false;
 
-		up->rs485_start_tx(up);
+		up->rs485_start_tx(up, true);
 
 		if (up->port.rs485.delay_rts_before_send > 0) {
 			em485->active_timer = &em485->start_tx_timer;
@@ -3423,7 +3427,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 
 	if (em485) {
 		if (em485->tx_stopped)
-			up->rs485_start_tx(up);
+			up->rs485_start_tx(up, false);
 		mdelay(port->rs485.delay_rts_before_send);
 	}
 
@@ -3461,7 +3465,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 	if (em485) {
 		mdelay(port->rs485.delay_rts_after_send);
 		if (em485->tx_stopped)
-			up->rs485_stop_tx(up);
+			up->rs485_stop_tx(up, false);
 	}
 
 	serial_port_out(port, UART_IER, ier);
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index e0717c8393d7..144de7a7948d 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -161,8 +161,8 @@ struct uart_8250_port {
 	void			(*dl_write)(struct uart_8250_port *up, u32 value);
 
 	struct uart_8250_em485 *em485;
-	void			(*rs485_start_tx)(struct uart_8250_port *);
-	void			(*rs485_stop_tx)(struct uart_8250_port *);
+	void			(*rs485_start_tx)(struct uart_8250_port *up, bool toggle_ier);
+	void			(*rs485_stop_tx)(struct uart_8250_port *up, bool toggle_ier);
 
 	/* Serial port overrun backoff */
 	struct delayed_work overrun_backoff;
-- 
2.39.5

[PATCH tty-next v4 5/6] serial: 8250: Switch to nbcon console

[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 register access in the callbacks are within unsafe sections.
The ->write_atomic and ->write_therad() 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.

Note: A new __serial8250_clear_IER() is introduced for direct
clearing of UART_IER. This will allow to restore the 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_core.c |  35 ++++++-
 drivers/tty/serial/8250/8250_port.c | 149 +++++++++++++++++++++-------
 include/linux/serial_8250.h         |   8 +-
 3 files changed, 150 insertions(+), 42 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 2b70e82dffeb..3d5a2183ff13 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -388,12 +388,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)
@@ -494,12 +516,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,
 };
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index e38271f477d1..fbba4746eccc 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -711,7 +711,12 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 	serial8250_rpm_put(p);
 }
 
-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);
@@ -719,6 +724,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);
+}
+
 #ifdef CONFIG_SERIAL_8250_RSA
 /*
  * Attempts to turn on the RSA FIFO.  Returns zero on failure.
@@ -1406,9 +1416,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
@@ -1424,6 +1431,12 @@ 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
+			 */
+			lockdep_assert_held_once(&p->port.lock);
+
 			p->ier |= UART_IER_RLSI | UART_IER_RDI;
 			serial_port_out(&p->port, UART_IER, p->ier);
 		}
@@ -3302,7 +3315,11 @@ EXPORT_SYMBOL_GPL(serial8250_set_defaults);
 
 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)
@@ -3339,14 +3356,21 @@ static void serial8250_console_restore(struct uart_8250_port *up)
 	serial8250_out_MCR(up, up->mcr | UART_MCR_DTR | UART_MCR_RTS);
 }
 
-static void fifo_wait_for_lsr(struct uart_8250_port *up, unsigned int count)
+static int fifo_wait_for_lsr(struct uart_8250_port *up,
+			      struct nbcon_write_context *wctxt,
+			      unsigned int count)
 {
 	unsigned int i;
 
 	for (i = 0; i < count; i++) {
+		if (!nbcon_can_proceed(wctxt))
+			return -EPERM;
+
 		if (wait_for_lsr(up, UART_LSR_THRE))
-			return;
+			return 0;
 	}
+
+	return -ETIMEDOUT;
 }
 
 /*
@@ -3356,18 +3380,20 @@ static void fifo_wait_for_lsr(struct uart_8250_port *up, unsigned int count)
  * 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 */
-		fifo_wait_for_lsr(up, fifosize);
+		if (fifo_wait_for_lsr(up, wctxt, fifosize) == -EPERM)
+			return;
 
 		for (i = 0; i < fifosize && s != end; ++i) {
 			if (*s == '\n' && !cr_sent) {
@@ -3385,39 +3411,51 @@ 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
 	 */
-	fifo_wait_for_lsr(up, tx_count);
+	fifo_wait_for_lsr(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 && nbcon_can_proceed(wctxt))
+		uart_console_write(port, s++, 1, serial8250_console_wait_putchar);
 }
 
 /*
- *	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))) {
@@ -3431,6 +3469,14 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 		mdelay(port->rs485.delay_rts_before_send);
 	}
 
+	/*
+	 * 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 && nbcon_can_proceed(wctxt))
+		uart_console_write(port, "\n", 1, serial8250_console_wait_putchar);
+
 	use_fifo = (up->capabilities & UART_CAP_FIFO) &&
 		/*
 		 * BCM283x requires to check the fifo
@@ -3452,9 +3498,16 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 		!(up->port.flags & UPF_CONS_FLOW);
 
 	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);
+
+	/*
+	 * If ownership was lost, this context must reacquire ownership in
+	 * order to perform final actions (such as re-enabling interrupts).
+	 */
+	while (!nbcon_enter_unsafe(wctxt))
+		nbcon_reacquire_nobuf(wctxt);
 
 	/*
 	 *	Finally, wait for transmitter to become empty
@@ -3477,11 +3530,18 @@ 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)
+			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)
@@ -3499,8 +3559,24 @@ 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;
+
+	uart_port_lock(port);
+	serial8250_modem_status(up);
+	uart_port_unlock(port);
+}
+
 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';
@@ -3510,6 +3586,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 144de7a7948d..5a804dc5e05e 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -150,8 +150,12 @@ struct uart_8250_port {
 #define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
 	u16			lsr_saved_flags;
 	u16			lsr_save_mask;
+
+	bool			console_line_ended;	/* line fully output */
+
 #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;
@@ -202,8 +206,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.39.5

[PATCH tty-next v4 6/6] 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 fbba4746eccc..bc70e660a403 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -726,6 +726,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.39.5

Re: [PATCH tty-next v4 2/6] serial: 8250: Use frame rate to determine timeout

[Andy Shevchenko]

On Fri, Dec 27, 2024 at 11:51:18PM +0106, John Ogness wrote:
> Rather than using a hard-coded per-character Tx-timeout of 10ms,
> use the frame rate to determine a timeout value. The value is
> doubled to ensure that a timeout is only hit during unexpected
> circumstances.
> 
> Since the frame rate may not be available during early printing,
> the previous 10ms value is kept as a fallback.

...

>  	unsigned int status, tmout = 10000;
>  
> -	/* Wait up to 10ms for the character(s) to be sent. */
> +	/*
> +	 * Wait for a character to be sent. Fallback to a safe default
> +	 * timeout value if @frame_time is not available.
> +	 */

> +

Redundant blank line (esp. after addressing below).

> +	if (up->port.frame_time)
> +		tmout = up->port.frame_time * 2 / NSEC_PER_USEC;

This will be harder to maintain in case some new code will be squeezed in
between, so I propose to make it if-else.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v4 5/6] serial: 8250: Switch to nbcon console

[Andy Shevchenko]

On Fri, Dec 27, 2024 at 11:51:21PM +0106, 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 register access in the callbacks are within unsafe sections.
> The ->write_atomic and ->write_therad() callbacks allow safe

->write_atomic()

> 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.
> 
> Note: A new __serial8250_clear_IER() is introduced for direct
> clearing of UART_IER. This will allow to restore the lockdep
> check to serial8250_clear_IER() in a follow-up commit.

...

>  		if (toggle_ier) {
> +			/*
> +			 * Port locked to synchronize UART_IER access
> +			 * against the console

Missing period in multi-line comment.

> +			 */
> +			lockdep_assert_held_once(&p->port.lock);
> +
>  			p->ier |= UART_IER_RLSI | UART_IER_RDI;
>  			serial_port_out(&p->port, UART_IER, p->ier);
>  		}

...

> -static void fifo_wait_for_lsr(struct uart_8250_port *up, unsigned int count)
> +static int fifo_wait_for_lsr(struct uart_8250_port *up,
> +			      struct nbcon_write_context *wctxt,
> +			      unsigned int count)
>  {
>  	unsigned int i;
>  
>  	for (i = 0; i < count; i++) {
> +		if (!nbcon_can_proceed(wctxt))
> +			return -EPERM;
> +
>  		if (wait_for_lsr(up, UART_LSR_THRE))
> -			return;
> +			return 0;
>  	}
> +
> +	return -ETIMEDOUT;
>  }

...

>  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 */
> -		fifo_wait_for_lsr(up, fifosize);
> +		if (fifo_wait_for_lsr(up, wctxt, fifosize) == -EPERM)
> +			return;

Perhaps it was already discussed and I forgot about it or hadn't read,
but I don't get how per-byte check for NBCON permission can help if there
is something already in the HW FIFO?

I mean in _this_ case wouldn't be enough to check FIFO to drain and only after
that check the permission?

>  		for (i = 0; i < fifosize && s != end; ++i) {
>  			if (*s == '\n' && !cr_sent) {

>  	 * Allow timeout for each byte written since the caller will only wait
>  	 * for UART_LSR_BOTH_EMPTY using the timeout of a single character
>  	 */
> -	fifo_wait_for_lsr(up, tx_count);
> +	fifo_wait_for_lsr(up, wctxt, tx_count);
> +}


...

> +	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)
> +			irq_work_queue(&up->modem_status_work);
> +		else
> +			serial8250_modem_status(up);

Hmm... Shouldn't this be part of _modem_status() for all drivers using this call?

> +	}

...

> +	bool			console_line_ended;	/* line fully output */

Sorry, I didn't get the comment. Do you meant "line is fully printed out
[to HW]"?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v4 0/6] convert 8250 to nbcon

[Andy Shevchenko]

On Fri, Dec 27, 2024 at 11:51:16PM +0106, John Ogness wrote:
> This is v4 of a series to convert the 8250 driver to an NBCON
> console, providing both threaded and atomic printing
> implementations. v3 of this series is here [0]. Additional
> background information about NBCON consoles in general is
> available in the cover letter of v2 [1].

Just to be sure I understand the side effect of this series, i.e.
the

https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/tty/serial/8250/8250_port.c#L44
https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/tty/serial/8250/8250_pci.c#L9

are not needed anymore (the first one can be replaced to something like
dev_dbg() or analogue)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v4 5/6] serial: 8250: Switch to nbcon console

[John Ogness]

On 2024-12-29, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> -static void fifo_wait_for_lsr(struct uart_8250_port *up, unsigned int count)
>> +static int fifo_wait_for_lsr(struct uart_8250_port *up,
>> +			      struct nbcon_write_context *wctxt,
>> +			      unsigned int count)
>>  {
>>  	unsigned int i;
>>  
>>  	for (i = 0; i < count; i++) {
>> +		if (!nbcon_can_proceed(wctxt))
>> +			return -EPERM;
>> +
>>  		if (wait_for_lsr(up, UART_LSR_THRE))
>> -			return;
>> +			return 0;
>>  	}
>> +
>> +	return -ETIMEDOUT;
>>  }
>
> ...
>
>>  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 */
>> -		fifo_wait_for_lsr(up, fifosize);
>> +		if (fifo_wait_for_lsr(up, wctxt, fifosize) == -EPERM)
>> +			return;
>
> Perhaps it was already discussed and I forgot about it or hadn't read,
> but I don't get how per-byte check for NBCON permission can help if there
> is something already in the HW FIFO?
>
> I mean in _this_ case wouldn't be enough to check FIFO to drain and only after
> that check the permission?

If we did that and other CPUs had taken over printing, then this CPU
would possibly busy-wait the maximum timeout because the FIFO keeps
getting refilled by the other CPUs. If this CPU knows that it has lost
ownership then it can abort waiting and trying to write ASAP.

The CPU taking over will perform the necessary waiting for the FIFO to
drain what this CPU had wrote into the FIFO.

>
>>  		for (i = 0; i < fifosize && s != end; ++i) {

That being said, this loop is not checking for lost ownership between
bytes. I will add an nbcon_can_proceed() check here as well. If
ownership was lost, this CPU must not continue writing into the FIFO.

	for (i = 0; i < fifosize && s != end && nbcon_can_proceed(wctxt); ++i) {

>>  			if (*s == '\n' && !cr_sent) {
>
>>  	 * Allow timeout for each byte written since the caller will only wait
>>  	 * for UART_LSR_BOTH_EMPTY using the timeout of a single character
>>  	 */
>> -	fifo_wait_for_lsr(up, tx_count);
>> +	fifo_wait_for_lsr(up, wctxt, tx_count);
>> +}
>
>
> ...
>
>> +	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)
>> +			irq_work_queue(&up->modem_status_work);
>> +		else
>> +			serial8250_modem_status(up);
>
> Hmm... Shouldn't this be part of _modem_status() for all drivers using this call?

I am not sure I follow what you are saying.

Only NBCON drivers (that have implemented the ->write_atomic() callback)
will print from any context. OTOH legacy console drivers do not print
directly from scheduling or NMI contexts and instead defer to a printk
irq_work.

As we convert more console drivers to NBCON, it might make sense to move
the irq_work into the generic struct uart_port and possibly consolidate
the _modem_status() implementations. They seem to be mostly copy/paste
code.

>> +	bool			console_line_ended;	/* line fully output */
>
> Sorry, I didn't get the comment. Do you meant "line is fully printed out
> [to HW]"?

'\n' was the most recent byte written to the TX register by the console
driver.

Tracking this is necessary during takeovers so that the CPU taking over
can politely add an extra newline for cleaner output.

The wording for this comment was suggested by Petr Mladek. Please
suggest an acceptable alternative if you require something more concise.

John

Re: [PATCH tty-next v4 0/6] convert 8250 to nbcon

[John Ogness]

On 2024-12-29, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> On Fri, Dec 27, 2024 at 11:51:16PM +0106, John Ogness wrote:
>> This is v4 of a series to convert the 8250 driver to an NBCON
>> console, providing both threaded and atomic printing
>> implementations. v3 of this series is here [0]. Additional
>> background information about NBCON consoles in general is
>> available in the cover letter of v2 [1].
>
> Just to be sure I understand the side effect of this series, i.e.
> the
>
> https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/tty/serial/8250/8250_port.c#L44
> https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/tty/serial/8250/8250_pci.c#L9
>
> are not needed anymore (the first one can be replaced to something like
> dev_dbg() or analogue)?

Correct. With NBCON console drivers it is safe to call printk() while
holding the port lock for non-console-printing purposes because:

1. printing via ->write_atomic() does not use the port lock

   and

2. printing via ->write_thread() is performed in a separate dedicated
   printing kthread that can safely spin on the port lock

John

Re: [PATCH tty-next v4 0/6] convert 8250 to nbcon

[John Ogness]

On 2024-12-27, John Ogness <john.ogness@linutronix.de> wrote:
> The changes since v3:
>
> - For serial8250_console_fifo_write() and
>   serial8250_console_byte_write(), use nbcon_can_proceed()
>   rather than repeatedly enter/exit unsafe regions.

I will revert this particular change for v5. It is necessary to
exit/enter unsafe regions per character so that handovers can occur
mid-line. Using nbcon_can_proceed() only allows the hostile takeover
case to perform mid-line interruptions.

John

Re: [PATCH tty-next v4 1/6] serial: 8250: Adjust the timeout for FIFO mode

[Petr Mladek]

On Fri 2024-12-27 23:51:17, John Ogness wrote:
> After a console has written a record into UART_TX, it uses
> wait_for_xmitr() to wait until the data has been sent out before
> returning. However, wait_for_xmitr() will timeout after 10ms,
> regardless if the data has been transmitted or not.
> 
> For single bytes, this timeout is sufficient even at very slow
> baud rates, such as 1200bps. However, when FIFO mode is used,
> there may be 64 bytes pushed into the FIFO at once. At a baud
> rate of 115200bps, the 10ms timeout is still sufficient. But
> when using lower baud rates (such as 57600bps), the timeout
> is _not_ sufficient. This causes longer lines to be cut off,
> resulting in lost and horribly misformatted output on the
> console.
> 
> When using FIFO mode, take the number of bytes into account to
> determine an appropriate maximum timeout. Increasing the timeout
> does not affect performance since ideally the timeout never
> occurs.
> 
> Fixes: 8f3631f0f6eb ("serial/8250: Use fifo in 8250 console driver")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Wander Lairson Costa <wander@redhat.com>

Looks good to me:

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

Best Regards,
Petr

Re: [PATCH tty-next v4 2/6] serial: 8250: Use frame rate to determine timeout

[Petr Mladek]

On Fri 2024-12-27 23:51:18, John Ogness wrote:
> Rather than using a hard-coded per-character Tx-timeout of 10ms,
> use the frame rate to determine a timeout value. The value is
> doubled to ensure that a timeout is only hit during unexpected
> circumstances.
> 
> Since the frame rate may not be available during early printing,
> the previous 10ms value is kept as a fallback.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

It makes sense and looks good to me with and even without
the changes proposed by Andy:

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

Best Regards,
Petr

Re: [PATCH tty-next v4 4/6] serial: 8250: Provide flag for IER toggling for RS485

[Petr Mladek]

On Fri 2024-12-27 23:51:20, John Ogness wrote:
> For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
> console ->write() callback needs to enable/disable Tx. It does
> this by calling the ->rs485_start_tx() and ->rs485_stop_tx()
> callbacks. However, some of these callbacks also disable/enable
> interrupts and makes power management calls. This causes 2
> problems for console writing:
> 
> 1. A console write can occur in contexts that are illegal for
>    pm_runtime_*(). It is not even necessary for console writing
>    to use pm_runtime_*() because a console already does this in
>    serial8250_console_setup() and serial8250_console_exit().
>
> 2. The console ->write() callback already handles
>    disabling/enabling the interrupts by properly restoring the
>    previous IER value.

I was a bit confused by the description of the 2nd problem.
It is not clear whether it actually is a problem.

My understanding is that the nested IER manipulation does not
harm. And it is not needed at the same time. As a result,
the related pr_runtime_*() calls are not needed either.
So this is 2nd reason why the problematic pr_runtime_*() calls
can be removed in the serial8250_console_write() code path.

> Add an argument @toggle_ier to the ->rs485_start_tx() and
> ->rs485_stop_tx() callbacks to specify if they may disable/enable
> receive interrupts while using pm_runtime_*(). Console writing
> will not allow the toggling.
> 
> For all call sites other than console writing there is no
> functional change.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

It seems that the patch does what is says. I'll try to describe
it using my words to explain how I understand it.

IMHO, the main motivation is to prevent calling pm_runtime_*() in
serial8250_console_write(). It is not safe in some contexts,
especially in NMI. And it is not needed from two reasons:

  1. The console->write() callback is used only by registered consoles.
     And the pm_runtime usage counter is already bumped during
     the console registration by serial8250_console_setup().

  2. The pm_runtime_*() calls are used in the following code path

	+ serial8250_console_write()
	  + up->rs485_start_tx()
	    + serial8250_em485_start_tx()
	      + serial8250_stop_rx()

     This code is not needed because serial8250_console_write()
     always clears IER by __serial8250_clear_IER(up) anyway.

     In fact, the extra IER manipulation in serial8250_em485_start_tx()
     might be seen as a bug. Well, it is a NOP.

All in all, the patch looks good to me.

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

Best Regards,
Petr

Re: [PATCH tty-next v4 5/6] serial: 8250: Switch to nbcon console

[Petr Mladek]

On Fri 2024-12-27 23:51:21, 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 register access in the callbacks are within unsafe sections.
> The ->write_atomic and ->write_therad() 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.
> 
> Note: A new __serial8250_clear_IER() is introduced for direct
> clearing of UART_IER. This will allow to restore the lockdep
> check to serial8250_clear_IER() in a follow-up commit.
> 
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1406,9 +1416,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);

We should explain why it is OK to move the assert.

IMHO, most poeple would not understand the port lock is needed only
for UART_IER manipulation and not for UART_MCR manipulation.

We should probably explain that even the UART_MCR manipulation
is synchronized either by the port lock or by nbcon context
ownership. Where the nbcon context owner ship actually provides
synchronization against the port lock in all situations
except for the final unsafe flush in panic().

In the ideal world it would be nice to check the nbcon context
ownership. But it would require passing struct nbcon_write_context
which does not fit well in this low level API.

A comment might be enough.

Or do I miss something?

> -
>  	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
>  		mcr |= UART_MCR_RTS;
>  	else
> @@ -3339,14 +3356,21 @@ static void serial8250_console_restore(struct uart_8250_port *up)
>  	serial8250_out_MCR(up, up->mcr | UART_MCR_DTR | UART_MCR_RTS);
>  }
>  
> -static void fifo_wait_for_lsr(struct uart_8250_port *up, unsigned int count)
> +static int fifo_wait_for_lsr(struct uart_8250_port *up,
> +			      struct nbcon_write_context *wctxt,
> +			      unsigned int count)
>  {
>  	unsigned int i;
>  
>  	for (i = 0; i < count; i++) {

This would deserve a comment. The following comes to my mind:

		/*
		 * 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 lsr before reusing the fifo.
		 */

> +		if (!nbcon_can_proceed(wctxt))
> +			return -EPERM;
> +
>  		if (wait_for_lsr(up, UART_LSR_THRE))
> -			return;
> +			return 0;
>  	}
> +
> +	return -ETIMEDOUT;
>  }
>  
>  /*
> @@ -3356,18 +3380,20 @@ static void fifo_wait_for_lsr(struct uart_8250_port *up, unsigned int count)
>   * 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 */
> -		fifo_wait_for_lsr(up, fifosize);
> +		if (fifo_wait_for_lsr(up, wctxt, fifosize) == -EPERM)
> +			return;
>  
>  		for (i = 0; i < fifosize && s != end; ++i) {
>  			if (*s == '\n' && !cr_sent) {

We might want to check nbcon_can_proceed() after each character.
I think that you have already suggested this in the reply to Andy.

But maybe, it is not that important because filling the FIFO buffer is
probably fast.

> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -150,8 +150,12 @@ struct uart_8250_port {
>  #define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
>  	u16			lsr_saved_flags;
>  	u16			lsr_save_mask;
> +
> +	bool			console_line_ended;	/* line fully output */

I agree with Andy that the comment is confusing. I am sorry if it was
my proposal ;-)

I wonder if the following is better:

	bool			console_line_ended;	/* finished writing full line */

> +
>  #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;

Otherwise, it looks good. Well, I might want to double check it
on Monday morning.

Best Regards,
Petr

Re: [PATCH tty-next v4 4/6] serial: 8250: Provide flag for IER toggling for RS485

[John Ogness]

On 2025-01-03, Petr Mladek <pmladek@suse.com> wrote:
> My understanding is that the nested IER manipulation does not
> harm.

This statement implies that it is OK for UART_IER_RLSI|UART_IER_RDI bits
to be set in UART_IER even though the console will write to UART_TX,
because the _nesting_ contexts would set those bits rather than
restoring the original value of 0x0.

I ran some tests and leaving these bits set during Tx does not appear to
cause an issue, but it is difficult to say because the context
interrupted by a nesting context will only print at most 1
character. Also, it is writing under spin_lock_irqsave() so that might
be masking any effects. Perhaps UART_IER is temporarly cleared because
of other bits that would cause problems during Tx?

I would need to create a specific test to investigate this
further. Regardless, it still is a cosmetic ugliness that bits are not
being properly restored, even if it turns out these particular bits are
not problematic during Tx.

As was with your LSR_DR comment, you are good at triggering fundamental
investigations into the history of the 8250 driver. ;-)

> All in all, the patch looks good to me.
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Thanks for the review. I interpret it to mean that I do not need to make
any changes to this patch for v5.

John

Re: [PATCH tty-next v4 5/6] serial: 8250: Switch to nbcon console

[John Ogness]

On 2025-01-03, Petr Mladek <pmladek@suse.com> wrote:
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -1406,9 +1416,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);
>
> We should explain why it is OK to move the assert.
>
> IMHO, most poeple would not understand the port lock is needed only
> for UART_IER manipulation and not for UART_MCR manipulation.
>
> We should probably explain that even the UART_MCR manipulation
> is synchronized either by the port lock or by nbcon context
> ownership. Where the nbcon context owner ship actually provides
> synchronization against the port lock in all situations
> except for the final unsafe flush in panic().

Correct, although the "except for the final unsafe flush in panic()" is
the reason that even an nbcon context ownership assert could not be used
here.

> A comment might be enough.

OK. I will extend the comment at the new lockdep_assert site explaining
why the port lock is only guaranteed to be held for the toggle_ier==true
situation.

>> +static int fifo_wait_for_lsr(struct uart_8250_port *up,
>> +			      struct nbcon_write_context *wctxt,
>> +			      unsigned int count)
>>  {
>>  	unsigned int i;
>>  
>>  	for (i = 0; i < count; i++) {
>
> This would deserve a comment. The following comes to my mind:
>
> 		/*
> 		 * 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 lsr before reusing the fifo.
> 		 */

OK.

>>  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 */
>> -		fifo_wait_for_lsr(up, fifosize);
>> +		if (fifo_wait_for_lsr(up, wctxt, fifosize) == -EPERM)
>> +			return;
>>  
>>  		for (i = 0; i < fifosize && s != end; ++i) {
>>  			if (*s == '\n' && !cr_sent) {
>
> We might want to check nbcon_can_proceed() after each character.
> I think that you have already suggested this in the reply to Andy.

Yes v5 will check per character.

> But maybe, it is not that important because filling the FIFO buffer is
> probably fast.

Surely it could fill the FIFO in less time than the timeout (currently
hard-coded at 2ms). But I do not think we should rely on such timings
since they could change later. Also, since the CPU is busy-waiting
during the full Tx anyway, the check is not wasting any CPU cycles or
slowing the Tx.

>> +	bool			console_line_ended;	/* line fully output */
>
> I wonder if the following is better:
>
> 	bool			console_line_ended;	/* finished writing full line */

I would prefer to make it more technically exact. It would require a
multi-line comment and this header uses 2 different styles for
multi-line field comments.

How about:

	/*
	 * Track when a console line has been fully written to the
	 * hardware, i.e. true when the most recent byte written by
	 * the console to UART_TX was '\n'.
	 */
	bool			console_line_ended;

John

Re: [PATCH tty-next v4 4/6] serial: 8250: Provide flag for IER toggling for RS485

[Petr Mladek]

On Sun 2025-01-05 01:32:00, John Ogness wrote:
> On 2025-01-03, Petr Mladek <pmladek@suse.com> wrote:
> > My understanding is that the nested IER manipulation does not
> > harm.
> 
> This statement implies that it is OK for UART_IER_RLSI|UART_IER_RDI bits
> to be set in UART_IER even though the console will write to UART_TX,
> because the _nesting_ contexts would set those bits rather than
> restoring the original value of 0x0.

This is a misunderstanding. I am sorry I was not clear enough.

To be more clear. By the nested context I meant

	if (em485) {
		if (em485->tx_stopped)
			up->rs485_start_tx(up);

call by serial8250_console_write(). The original code did:

	+ up->rs485_start_tx()
	  + serial8250_em485_start_tx()
	    + serial8250_stop_rx()

, where serial8250_stop_rx() cleared the flags:

static void serial8250_stop_rx(struct uart_port *port)
{
[...]
	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
	serial_port_out(port, UART_IER, up->ier);
[...]
}

But the flags were already cleared by:

	__serial8250_clear_IER(up);

called by serial8250_console_write() _earlier_. Which did:

static void __serial8250_clear_IER(struct uart_8250_port *up)
{
	if (up->capabilities & UART_CAP_UUE)
		serial_out(up, UART_IER, UART_IER_UUE);
	else
		serial_out(up, UART_IER, 0);

}


It means that the nested serial8250_stop_rx() did not change anything.
It was a NOP. The bits were already cleared.

Similar, the counter part. The bits were later set by

	up->rs485_stop_tx(up)

which did:

	+ serial8250_em485_stop_tx

void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier)
{
[...]
	p->ier |= UART_IER_RLSI | UART_IER_RDI;
	serial_port_out(&p->port, UART_IER, p->ier);
[...]
}

But it was after the writing the message so that it did not affect
the operation.

> I ran some tests and leaving these bits set during Tx does not appear to
> cause an issue, but it is difficult to say because the context
> interrupted by a nesting context will only print at most 1
> character. Also, it is writing under spin_lock_irqsave() so that might
> be masking any effects. Perhaps UART_IER is temporarly cleared because
> of other bits that would cause problems during Tx?
> 
> I would need to create a specific test to investigate this
> further. Regardless, it still is a cosmetic ugliness that bits are not
> being properly restored, even if it turns out these particular bits are
> not problematic during Tx.

I think that you do not need to investigate it further. We should keep
IER cleared when writing the message.


> > All in all, the patch looks good to me.
> >
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> Thanks for the review. I interpret it to mean that I do not need to make
> any changes to this patch for v5.

Yup, I am fine with the patch as it is.

Best Regards,
Petr

Re: [PATCH tty-next v4 5/6] serial: 8250: Switch to nbcon console

[Petr Mladek]

On Sun 2025-01-05 02:03:41, John Ogness wrote:
> On 2025-01-03, Petr Mladek <pmladek@suse.com> wrote:
> >> --- a/drivers/tty/serial/8250/8250_port.c
> >> +++ b/drivers/tty/serial/8250/8250_port.c
> >> @@ -1406,9 +1416,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);
> >
> > We should explain why it is OK to move the assert.
> >
> > IMHO, most poeple would not understand the port lock is needed only
> > for UART_IER manipulation and not for UART_MCR manipulation.
> >
> > We should probably explain that even the UART_MCR manipulation
> > is synchronized either by the port lock or by nbcon context
> > ownership. Where the nbcon context owner ship actually provides
> > synchronization against the port lock in all situations
> > except for the final unsafe flush in panic().
> 
> Correct, although the "except for the final unsafe flush in panic()" is
> the reason that even an nbcon context ownership assert could not be used
> here.

Good point. Well, lockdep should be disabled by debug_locks_off()
before nbcon_atomic_flush_unsafe() is called. I hope that we could
keep the assert.

> > A comment might be enough.
> 
> OK. I will extend the comment at the new lockdep_assert site explaining
> why the port lock is only guaranteed to be held for the toggle_ier==true
> situation.

Thanks.

> >> +	bool			console_line_ended;	/* line fully output */
> >
> > I wonder if the following is better:
> >
> > 	bool			console_line_ended;	/* finished writing full line */
> 
> I would prefer to make it more technically exact. It would require a
> multi-line comment and this header uses 2 different styles for
> multi-line field comments.
> 
> How about:
> 
> 	/*
> 	 * Track when a console line has been fully written to the
> 	 * hardware, i.e. true when the most recent byte written by
> 	 * the console to UART_TX was '\n'.
> 	 */
> 	bool			console_line_ended;

Looks good to me.

Best Regards,
Petr