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

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

[John Ogness]

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

To test this version I acquired real hardware (TI AM3358
BeagleBone Black) and tested the following modes:

RS232
- no flow control
- software flow control
  (UPF_SOFT_FLOW, UPSTAT_AUTOXOFF)
- hardware flow control
  (UPF_HARD_FLOW, UPSTAT_AUTOCTS, UPSTAT_AUTORTS)
- software emulated hardware flow control
  (UPF_CONS_FLOW, UPSTAT_CTS_ENABLE)

RS485
- with SER_RS485_RX_DURING_TX
- without SER_RS485_RX_DURING_TX

The tests focussed on kernel logging in various combinations of
normal, warning, and panic situations. Although not related to
the console printing code changes, the tests also included
using a getty/login session on the console.

Note that this UART (TI16750) supports a 64-byte TX-FIFO, which
is used in all console printing modes except for the software
emulated hardware flow control.

Here are the changes since v2:

- For RS485 start/stop TX, specify if called in console
  context.

- For RS485 start/stop TX, when in console context, do not
  disable/enable interrupts.

- Relocate modem_status_handler() to avoid unused static
  function for some configs.

- Move LSR_THRE waiting into a new
  serial8250_console_wait_putchar() function.

- For serial8250_console_fifo_write(), use
  serial8250_console_putchar() for writing. This allows newline
  tracking for FIFO mode as well.

- For serial8250_console_fifo_write(), allow 10ms timeout for
  each byte written.

- Use FIFO mode for thread and atomic modes when available.

- Introduce serial8250_console_byte_write() to handle writing
  when not using the FIFO mode.

- Consolidate thread and atomic callbacks. Now the only
  difference is modem control: For atomic, called as irq_work.
  For thread, called direct.

John Ogness

[0] 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 high-level write function for FIFO
  serial: 8250: Split out rx stop/start code into helpers
  serial: 8250: Specify console context for rs485_start/stop_tx
  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       | 267 +++++++++++++++++-----
 include/linux/serial_8250.h               |  11 +-
 6 files changed, 251 insertions(+), 72 deletions(-)


base-commit: 44059790a5cb9258ae6137387e4c39b717fd2ced
-- 
2.39.5

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

[John Ogness]

After a console has fed a line into TX, it uses wait_for_xmitr()
to wait until the data has been sent out before returning to the
printk code. 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.
However, 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 max 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>
---
 drivers/tty/serial/8250/8250_port.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3509af7dc52b..adc48eeeac2b 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2059,11 +2059,12 @@ 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;
 
-	/* Wait up to 10ms for the character(s) to be sent. */
+	/* Wait up to 10ms for the character to be sent. */
 	for (;;) {
 		status = serial_lsr_in(up);
 
@@ -2074,10 +2075,13 @@ 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
+ *	with timeout
  */
 static void wait_for_xmitr(struct uart_8250_port *up, int bits)
 {
@@ -3306,13 +3310,18 @@ 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. */
+		for (i = 0; i < fifosize; i++) {
+			if (wait_for_lsr(up, UART_LSR_THRE))
+				break;
+		}
 
 		for (i = 0; i < fifosize && s != end; ++i) {
 			if (*s == '\n' && !cr_sent) {
@@ -3323,6 +3332,13 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 				cr_sent = false;
 			}
 		}
+		tx_count = i;
+	}
+
+	/* Allow timeout for each byte written. */
+	for (i = 0; i < tx_count; i++) {
+		if (wait_for_lsr(up, UART_LSR_THRE))
+			break;
 	}
 }
 
-- 
2.39.5

[PATCH tty-next v3 2/6] serial: 8250: Use high-level write 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. That 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>
---
 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 adc48eeeac2b..8f7c9968ad41 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3268,11 +3268,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);
 }
 
 /*
@@ -3312,6 +3317,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;
@@ -3325,10 +3331,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;
 			}
 		}
@@ -3408,7 +3414,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 v3 3/6] serial: 8250: Split out rx stop/start code into helpers

[John Ogness]

The rx stop/start callbacks also disable/enable interrupts. This
is not acceptable for the console write callback since it must
manage all interrupt disabling/enabling.

Move the interrupt disabling/enabling/masking into helper
functions so that the console write callback can make use of
the appropriate parts in a follow-up commit.

This is essentially refactoring and should cause no functional
change.

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

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 8f7c9968ad41..09ac521d232a 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1360,18 +1360,37 @@ static void autoconfig_irq(struct uart_8250_port *up)
 	port->irq = (irq > 0) ? irq : 0;
 }
 
-static void serial8250_stop_rx(struct uart_port *port)
+static void __serial8250_stop_rx_mask_dr(struct uart_port *port)
 {
-	struct uart_8250_port *up = up_to_u8250p(port);
+	port->read_status_mask &= ~UART_LSR_DR;
+}
 
+static void __serial8250_stop_rx_int(struct uart_8250_port *p)
+{
 	/* Port locked to synchronize UART_IER access against the console. */
-	lockdep_assert_held_once(&port->lock);
+	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 __serial8250_start_rx_int(struct uart_8250_port *p)
+{
+	/* 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);
+}
+
+static void serial8250_stop_rx(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
 
 	serial8250_rpm_get(up);
 
-	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
-	up->port.read_status_mask &= ~UART_LSR_DR;
-	serial_port_out(port, UART_IER, up->ier);
+	__serial8250_stop_rx_mask_dr(port);
+	__serial8250_stop_rx_int(up);
 
 	serial8250_rpm_put(up);
 }
@@ -1386,9 +1405,6 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
 {
 	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
@@ -1403,8 +1419,7 @@ 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);
+		__serial8250_start_rx_int(p);
 	}
 }
 EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
-- 
2.39.5

[PATCH tty-next v3 4/6] serial: 8250: Specify console context for rs485_start/stop_tx

[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/stop_tx() callbacks. However, these
callbacks will disable/enable interrupts, which is a problem
for console write, as it must be responsible for
disabling/enabling interrupts.

Add an argument @in_con to the rs485_start/stop_tx() callbacks
to specify if they are being called from console write. If so,
the callbacks will not handle interrupt disabling/enabling.

For all call sites other than console write, 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       | 34 +++++++++++++++--------
 include/linux/serial_8250.h               |  4 +--
 5 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index e5310c65cf52..0d8717be0df7 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 in_con);
+void serial8250_em485_stop_tx(struct uart_8250_port *p, bool in_con);
 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..c9a106a86b56 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 in_con)
 {
 	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 in_con)
 {
 	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 b3be0fb184a3..fcbed7e98231 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, false);
 }
 
 /*
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 09ac521d232a..7c50387194ad 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -558,7 +558,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, false);
 
 	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
+ * @in_con: true if called from console write, otherwise false
  *
  * 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 in_con)
 {
 	unsigned char mcr = serial8250_in_MCR(p);
 
@@ -1419,7 +1420,9 @@ 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);
 
-		__serial8250_start_rx_int(p);
+		/* In console context, caller handles interrupt enabling. */
+		if (!in_con)
+			__serial8250_start_rx_int(p);
 	}
 }
 EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
@@ -1434,7 +1437,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, false);
 		em485->active_timer = NULL;
 		em485->tx_stopped = true;
 	}
@@ -1466,7 +1469,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, false);
 		em485->active_timer = NULL;
 		em485->tx_stopped = true;
 	}
@@ -1555,6 +1558,7 @@ static inline void __start_tx(struct uart_port *port)
 /**
  * serial8250_em485_start_tx() - generic ->rs485_start_tx() callback
  * @up: uart 8250 port
+ * @in_con: true if called from console write, otherwise false
  *
  * 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.
@@ -1562,12 +1566,20 @@ 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 in_con)
 {
 	unsigned char mcr = serial8250_in_MCR(up);
 
-	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
-		serial8250_stop_rx(&up->port);
+	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
+		/*
+		 * In console context, caller handles interrupt disabling. So
+		 * only LSR_DR masking is needed.
+		 */
+		if (in_con)
+			__serial8250_stop_rx_mask_dr(&up->port);
+		else
+			serial8250_stop_rx(&up->port);
+	}
 
 	if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
 		mcr |= UART_MCR_RTS;
@@ -1600,7 +1612,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, false);
 
 		if (up->port.rs485.delay_rts_before_send > 0) {
 			em485->active_timer = &em485->start_tx_timer;
@@ -3402,7 +3414,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, true);
 		mdelay(port->rs485.delay_rts_before_send);
 	}
 
@@ -3440,7 +3452,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, true);
 	}
 
 	serial_port_out(port, UART_IER, ier);
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index e0717c8393d7..c25c026d173d 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 in_con);
+	void			(*rs485_stop_tx)(struct uart_8250_port *up, bool in_con);
 
 	/* Serial port overrun backoff */
 	struct delayed_work overrun_backoff;
-- 
2.39.5

[PATCH tty-next v3 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 callbacks allow safe handover/takeover per byte and add
a preceding newline if they take over mid-line.

For the write_atomic() case, a new irq_work is used to defer modem
control since it may be a context that does not allow waking up
tasks.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_core.c |  35 +++++-
 drivers/tty/serial/8250/8250_port.c | 159 ++++++++++++++++++++++------
 include/linux/serial_8250.h         |   7 +-
 3 files changed, 164 insertions(+), 37 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 5f9f06911795..7184100129bd 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 *con, unsigned long *flags)
+{
+	struct uart_port *up = &serial8250_ports[con->index].port;
+
+	__uart_port_lock_irqsave(up, flags);
+}
+
+static void univ8250_console_device_unlock(struct console *con, unsigned long flags)
+{
+	struct uart_port *up = &serial8250_ports[con->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 7c50387194ad..0b3596fab061 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -691,7 +691,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 console write callbacks, 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);
@@ -699,6 +704,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.
@@ -3296,7 +3306,14 @@ 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);
+
+	if (ch == '\n')
+		up->console_line_ended = true;
+	else
+		up->console_line_ended = false;
 }
 
 static void serial8250_console_wait_putchar(struct uart_port *port, unsigned char ch)
@@ -3340,9 +3357,10 @@ 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;
+	const char *s = READ_ONCE(wctxt->outbuf);
+	const char *end = s + READ_ONCE(wctxt->len);
 	unsigned int fifosize = up->tx_loadsz;
 	struct uart_port *port = &up->port;
 	unsigned int tx_count = 0;
@@ -3352,10 +3370,19 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 	while (s != end) {
 		/* Allow timeout for each byte of a possibly full FIFO. */
 		for (i = 0; i < fifosize; i++) {
+			if (!nbcon_enter_unsafe(wctxt))
+				return;
+
 			if (wait_for_lsr(up, UART_LSR_THRE))
 				break;
+
+			if (!nbcon_exit_unsafe(wctxt))
+				return;
 		}
 
+		if (!nbcon_enter_unsafe(wctxt))
+			return;
+
 		for (i = 0; i < fifosize && s != end; ++i) {
 			if (*s == '\n' && !cr_sent) {
 				serial8250_console_putchar(port, '\r');
@@ -3366,45 +3393,74 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 			}
 		}
 		tx_count = i;
+
+		if (!nbcon_exit_unsafe(wctxt))
+			return;
 	}
 
 	/* Allow timeout for each byte written. */
 	for (i = 0; i < tx_count; i++) {
+		if (!nbcon_enter_unsafe(wctxt))
+			return;
+
 		if (wait_for_lsr(up, UART_LSR_THRE))
 			break;
+
+		if (!nbcon_exit_unsafe(wctxt))
+			return;
+	}
+}
+
+static void serial8250_console_byte_write(struct uart_8250_port *up,
+					  struct nbcon_write_context *wctxt)
+{
+	const char *s = READ_ONCE(wctxt->outbuf);
+	const char *end = s + READ_ONCE(wctxt->len);
+	struct uart_port *port = &up->port;
+
+	/*
+	 * Write out the message. Toggle unsafe for each byte in order to give
+	 * another (higher priority) context the opportunity for a friendly
+	 * takeover. If such a takeover occurs, this must abort writing 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);
+
+		if (!nbcon_exit_unsafe(wctxt))
+			return;
 	}
 }
 
 /*
- *	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 IER then disable the interrupts. The special variant
+	 * to clear 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))) {
@@ -3418,6 +3474,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
@@ -3438,10 +3502,19 @@ 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);
-	else
-		uart_console_write(port, s, count, serial8250_console_wait_putchar);
+	if (nbcon_exit_unsafe(wctxt)) {
+		if (likely(use_fifo))
+			serial8250_console_fifo_write(up, wctxt);
+		else
+			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
@@ -3464,11 +3537,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)
@@ -3486,8 +3566,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';
@@ -3497,6 +3593,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;
+	up->modem_status_work = IRQ_WORK_INIT(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 c25c026d173d..c6c391b15efc 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -152,6 +152,9 @@ struct uart_8250_port {
 	u16			lsr_save_mask;
 #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
 	unsigned char		msr_saved_flags;
+	struct irq_work		modem_status_work;
+
+	bool			console_line_ended;	/* line fully output */
 
 	struct uart_8250_dma	*dma;
 	const struct uart_8250_ops *ops;
@@ -202,8 +205,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 v3 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>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 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 0b3596fab061..5c8778ec30a3 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -706,6 +706,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 v3 1/6] serial: 8250: Adjust the timeout for FIFO mode

[Andy Shevchenko]

On Fri, Oct 25, 2024 at 01:03:23PM +0206, John Ogness wrote:
> After a console has fed a line into TX, it uses wait_for_xmitr()
> to wait until the data has been sent out before returning to the
> printk code. However, wait_for_xmitr() will timeout after 10ms,

printk here is a function reference or module?
For the latter I would use the filename to be sure it's clear,
like printk.c. For the former (and it seems you know that)
we may use printk().

> 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.
> However, 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 max timeout. Increasing the timeout

maximum
(in order not to mix with max() function)

> does not affect performance since ideally the timeout never
> occurs.

...

>  /*
>   *	Wait for transmitter & holding register to empty
> + *	with timeout

Can you fix the style while at it?

>   */

 /* Wait for transmitter & holding register to empty with timeout */

...

>  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. */

Does the one-line comment style in this file use periods? If not, drop,
otherwise apply it to the above proposal.

> +		for (i = 0; i < fifosize; i++) {
> +			if (wait_for_lsr(up, UART_LSR_THRE))
> +				break;
> +		}

> +	}
> +
> +	/* Allow timeout for each byte written. */
> +	for (i = 0; i < tx_count; i++) {
> +		if (wait_for_lsr(up, UART_LSR_THRE))
> +			break;

This effectively repeats the above. Even for the fix case I would still add
a new helper to deduplicate.

>  	}
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v3 2/6] serial: 8250: Use high-level write function for FIFO

[Andy Shevchenko]

On Fri, Oct 25, 2024 at 01:03:24PM +0206, John Ogness wrote:
> 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. That would defeat the purpose of the FIFO code.

You can slightly reformat the above to make it less shaky in terms
of the efficiency of a line capacity usage.

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. That 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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Oct 25, 2024 at 04:45:02PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 25, 2024 at 01:03:23PM +0206, John Ogness wrote:
> > After a console has fed a line into TX, it uses wait_for_xmitr()
> > to wait until the data has been sent out before returning to the
> > printk code. However, wait_for_xmitr() will timeout after 10ms,
> 
> printk here is a function reference or module?
> For the latter I would use the filename to be sure it's clear,
> like printk.c. For the former (and it seems you know that)
> we may use printk().
> 
> > 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.
> > However, 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 max timeout. Increasing the timeout
> 
> maximum
> (in order not to mix with max() function)
> 
> > does not affect performance since ideally the timeout never
> > occurs.
> 
> ...
> 
> >  /*
> >   *	Wait for transmitter & holding register to empty
> > + *	with timeout
> 
> Can you fix the style while at it?
> 
> >   */
> 
>  /* Wait for transmitter & holding register to empty with timeout */
> 
> ...
> 
> >  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. */
> 
> Does the one-line comment style in this file use periods? If not, drop,
> otherwise apply it to the above proposal.
> 
> > +		for (i = 0; i < fifosize; i++) {
> > +			if (wait_for_lsr(up, UART_LSR_THRE))
> > +				break;
> > +		}
> 
> > +	}
> > +
> > +	/* Allow timeout for each byte written. */
> > +	for (i = 0; i < tx_count; i++) {
> > +		if (wait_for_lsr(up, UART_LSR_THRE))
> > +			break;
> 
> This effectively repeats the above. Even for the fix case I would still add
> a new helper to deduplicate.
> 
> >  	}
> >  }

Forgot to add, with the above being addressed, feel free to add
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v3 3/6] serial: 8250: Split out rx stop/start code into helpers

[Andy Shevchenko]

On Fri, Oct 25, 2024 at 01:03:25PM +0206, John Ogness wrote:
> The rx stop/start callbacks also disable/enable interrupts. This

disable/enable --> toggle ?

> is not acceptable for the console write callback since it must
> manage all interrupt disabling/enabling.

toggling ?

> Move the interrupt disabling/enabling/masking into helper

toggling and masking ?

> functions so that the console write callback can make use of
> the appropriate parts in a follow-up commit.
> 
> This is essentially refactoring and should cause no functional
> change.

Please, be consistent in the commit messages on how you apply terms Rx and Tx
(or TX and RX, but I think the former is more usual WRT UART). This applies
to the whole series.

Code wise looks fine to me
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Oct 25, 2024 at 01:03:22PM +0206, John Ogness wrote:
> This is v3 of a series to convert the 8250 driver to an NBCON
> console, providing both threaded and atomic printing
> implementations. v2 of this series is here [0], which also
> contains additional background information about NBCON consoles
> in general in the cover letter.
> 
> To test this version I acquired real hardware (TI AM3358
> BeagleBone Black) and tested the following modes:
> 
> RS232
> - no flow control
> - software flow control
>   (UPF_SOFT_FLOW, UPSTAT_AUTOXOFF)
> - hardware flow control
>   (UPF_HARD_FLOW, UPSTAT_AUTOCTS, UPSTAT_AUTORTS)
> - software emulated hardware flow control
>   (UPF_CONS_FLOW, UPSTAT_CTS_ENABLE)
> 
> RS485
> - with SER_RS485_RX_DURING_TX
> - without SER_RS485_RX_DURING_TX
> 
> The tests focussed on kernel logging in various combinations of
> normal, warning, and panic situations. Although not related to
> the console printing code changes, the tests also included
> using a getty/login session on the console.
> 
> Note that this UART (TI16750) supports a 64-byte TX-FIFO, which
> is used in all console printing modes except for the software
> emulated hardware flow control.

Thank you for the update.

I am going to review some patches at some point, but what I want to say here
is that if you have a new functions to utilise something, please also check
if the rest of 8250*.c may have an advantage of. It would reduce churn in case
if your series already exports APIs or provides inliners for such cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v3 4/6] serial: 8250: Specify console context for rs485_start/stop_tx

[Andy Shevchenko]

On Fri, Oct 25, 2024 at 01:03:26PM +0206, 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/stop_tx() callbacks. However, these

It would be nice if you be consistent across the commit messages and also
provide the names of the callbacks in full, because I dunno if we have a local
stop_tx() or rs485_start() or whatever the above means.

If it is "the rs485_start_tx() / rs485_stop_tx() callbacks.", it's
much clearer for the reader.

> callbacks will disable/enable interrupts, which is a problem

toggle?

> for console write, as it must be responsible for
> disabling/enabling interrupts.

toggling ?

> Add an argument @in_con to the rs485_start/stop_tx() callbacks

As per above.

> to specify if they are being called from console write. If so,
> the callbacks will not handle interrupt disabling/enabling.

toggling ?

> For all call sites other than console write, there is no
> functional change.

So, why not call the parameter better to emphasize that it's about IRQ
toggling? bool toggle_irq ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v3 6/6] serial: 8250: Revert "drop lockdep annotation from serial8250_clear_IER()"

[Andy Shevchenko]

On Fri, Oct 25, 2024 at 01:03:28PM +0206, John Ogness wrote:
> The 8250 driver no longer depends on @oops_in_progress and
> will no longer violate the port->lock locking constraints.
> 
> This reverts commit 3d9e6f556e235ddcdc9f73600fdd46fe1736b090.

...

> +	/* Port locked to synchronize UART_IER access against the console. */

Nevertheless, the same comment about the one-liner comment style.
Do we need a period? Perhaps drop it here while at it?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Oct 25, 2024 at 01:03:27PM +0206, 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.

Again, use consistent style for the references to the callbacks.
it may be .func, or ->func(), or something else, but make it consistent.

> All register access in the callbacks are within unsafe sections.
> The write callbacks allow safe handover/takeover per byte and add
> a preceding newline if they take over mid-line.
> 
> For the write_atomic() case, a new irq_work is used to defer modem
> control since it may be a context that does not allow waking up
> tasks.

...

> +/*
> + * Only to be used directly by the console write callbacks, 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);

>  		serial_out(up, UART_IER, 0);
>  }
>  
> +static inline void serial8250_clear_IER(struct uart_8250_port *up)
> +{
> +	__serial8250_clear_IER(up);

Shouldn't this have a lockdep annotation to differentiate with the above?

> +}

...

> +static void serial8250_console_byte_write(struct uart_8250_port *up,
> +					  struct nbcon_write_context *wctxt)
> +{
> +	const char *s = READ_ONCE(wctxt->outbuf);
> +	const char *end = s + READ_ONCE(wctxt->len);

Is there any possibility that outbuf value be changed before we get the len and
at the end we get the wrong pointer?

> +	struct uart_port *port = &up->port;
> +
> +	/*
> +	 * Write out the message. Toggle unsafe for each byte in order to give
> +	 * another (higher priority) context the opportunity for a friendly
> +	 * takeover. If such a takeover occurs, this must abort writing 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);
> +
> +		if (!nbcon_exit_unsafe(wctxt))
> +			return;
>  	}
>  }

...

> +/*
> + * irq_work handler to perform modem control. Only triggered via
> + * write_atomic() callback because it may be in a scheduler or NMI

Also make same style for the callback reference in the comments.

> + * context, unable to wake tasks.
> + */

...

>  struct uart_8250_port {

>  	u16			lsr_save_mask;
>  #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
>  	unsigned char		msr_saved_flags;
> +	struct irq_work		modem_status_work;
> +
> +	bool			console_line_ended;	/* line fully output */
>  
>  	struct uart_8250_dma	*dma;
>  	const struct uart_8250_ops *ops;

Btw, have you run `pahole` on this? Perhaps there are better places for new
members?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v3 4/6] serial: 8250: Specify console context for rs485_start/stop_tx

[John Ogness]

On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> Add an argument @in_con to the rs485_start/stop_tx() callbacks
>> to specify if they are being called from console write. If so,
>> the callbacks will not handle interrupt disabling/enabling.
>
> toggling ?
>
>> For all call sites other than console write, there is no
>> functional change.
>
> So, why not call the parameter better to emphasize that it's about IRQ
> toggling? bool toggle_irq ?

Currently there are only 2 users:

serial8250_em485_stop_tx()
bcm2835aux_rs485_stop_tx()

The first one toggles the IER bits, the second one does not. I figured
it would make more sense to specify the context rather than what needs
to be done and let the 8250-variant decide what it should do.

But I have no problems renaming it to toggle_irq. It is an 8250-specific
callback with few users. And really the IER bits is the only reason that
the argument even needs to exist.

John

Re: [PATCH tty-next v3 4/6] serial: 8250: Specify console context for rs485_start/stop_tx

[Andy Shevchenko]

On Fri, Oct 25, 2024 at 04:31:05PM +0206, John Ogness wrote:
> On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >> Add an argument @in_con to the rs485_start/stop_tx() callbacks
> >> to specify if they are being called from console write. If so,
> >> the callbacks will not handle interrupt disabling/enabling.
> >
> > toggling ?
> >
> >> For all call sites other than console write, there is no
> >> functional change.
> >
> > So, why not call the parameter better to emphasize that it's about IRQ
> > toggling? bool toggle_irq ?
> 
> Currently there are only 2 users:
> 
> serial8250_em485_stop_tx()
> bcm2835aux_rs485_stop_tx()
> 
> The first one toggles the IER bits, the second one does not. I figured
> it would make more sense to specify the context rather than what needs
> to be done and let the 8250-variant decide what it should do.
> 
> But I have no problems renaming it to toggle_irq. It is an 8250-specific
> callback with few users. And really the IER bits is the only reason that
> the argument even needs to exist.

Maybe toggle_ier will be better than? I haven't looked deeply into the
implementations, so choose whichever describes better what's behind it.

-- 
With Best Regards,
Andy Shevchenko

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

[John Ogness]

On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> +/*
>> + * Only to be used directly by the console write callbacks, 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);
>
>>  		serial_out(up, UART_IER, 0);
>>  }
>>  
>> +static inline void serial8250_clear_IER(struct uart_8250_port *up)
>> +{
>> +	__serial8250_clear_IER(up);
>
> Shouldn't this have a lockdep annotation to differentiate with the
> above?

Yes, but the follow-up patch adds the annotation as a clean "revert
patch". I can add a line about that in the commit message.

>> +static void serial8250_console_byte_write(struct uart_8250_port *up,
>> +					  struct nbcon_write_context *wctxt)
>> +{
>> +	const char *s = READ_ONCE(wctxt->outbuf);
>> +	const char *end = s + READ_ONCE(wctxt->len);
>
> Is there any possibility that outbuf value be changed before we get
> the len and at the end we get the wrong pointer?

No. I was concerned about compiler optimization, since @outbuf can
become NULL. However, it can only become NULL if ownership was
transferred, and that is properly checked anyway. I will remove the
READ_ONCE() usage for v4.

>>  struct uart_8250_port {
>
>>  	u16			lsr_save_mask;
>>  #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
>>  	unsigned char		msr_saved_flags;
>> +	struct irq_work		modem_status_work;
>> +
>> +	bool			console_line_ended;	/* line fully output */
>>  
>>  	struct uart_8250_dma	*dma;
>>  	const struct uart_8250_ops *ops;
>
> Btw, have you run `pahole` on this? Perhaps there are better places
> for new members?

Indeed there are. Placing it above the MSR_SAVE_FLAGS macro will reduce
an existing 3-byte hole to 2-bytes and avoid creating a new 7-byte
hole.

Thanks.

John

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

[Wander Lairson Costa]

On Fri, Oct 25, 2024 at 04:45:02PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 25, 2024 at 01:03:23PM +0206, John Ogness wrote:
> > After a console has fed a line into TX, it uses wait_for_xmitr()
> > to wait until the data has been sent out before returning to the
> > printk code. However, wait_for_xmitr() will timeout after 10ms,
> 
> printk here is a function reference or module?
> For the latter I would use the filename to be sure it's clear,
> like printk.c. For the former (and it seems you know that)
> we may use printk().
> 
> > 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.
> > However, 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 max timeout. Increasing the timeout
> 
> maximum
> (in order not to mix with max() function)
> 
> > does not affect performance since ideally the timeout never
> > occurs.
> 
> ...
> 
> >  /*
> >   *	Wait for transmitter & holding register to empty
> > + *	with timeout
> 
> Can you fix the style while at it?
> 
> >   */
> 
>  /* Wait for transmitter & holding register to empty with timeout */
> 
> ...
> 
> >  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. */
> 
> Does the one-line comment style in this file use periods? If not, drop,
> otherwise apply it to the above proposal.
> 
> > +		for (i = 0; i < fifosize; i++) {
> > +			if (wait_for_lsr(up, UART_LSR_THRE))
> > +				break;
> > +		}
> 
> > +	}
> > +
> > +	/* Allow timeout for each byte written. */
> > +	for (i = 0; i < tx_count; i++) {
> > +		if (wait_for_lsr(up, UART_LSR_THRE))
> > +			break;
> 
> This effectively repeats the above. Even for the fix case I would still add
> a new helper to deduplicate.

+1

With this fixed, Reviewed-by: Wander Lairson Costa <wander@redhat.com>
> 
> >  	}
> >  }
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[Jiri Slaby]

On 25. 10. 24, 12:57, John Ogness wrote:
> After a console has fed a line into TX, it uses wait_for_xmitr()
> to wait until the data has been sent out before returning to the
> printk code. 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.
> However, 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 max 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>
> ---
>   drivers/tty/serial/8250/8250_port.c | 24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 3509af7dc52b..adc48eeeac2b 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2059,11 +2059,12 @@ 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;
>   
> -	/* Wait up to 10ms for the character(s) to be sent. */
> +	/* Wait up to 10ms for the character to be sent. */
>   	for (;;) {
>   		status = serial_lsr_in(up);
>   
> @@ -2074,10 +2075,13 @@ 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
> + *	with timeout
>    */
>   static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>   {
> @@ -3306,13 +3310,18 @@ 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. */
> +		for (i = 0; i < fifosize; i++) {
> +			if (wait_for_lsr(up, UART_LSR_THRE))
> +				break;
> +		}

THRE only signals there is a space for one character. Multiplying it 
with fifosize does not make much sense to me. You perhaps want only to 
increase the timeout. Or somehow incorporate port->frame_time into the 
accounting (I am not sure it is available at this point already).

>   		for (i = 0; i < fifosize && s != end; ++i) {
>   			if (*s == '\n' && !cr_sent) {
> @@ -3323,6 +3332,13 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
>   				cr_sent = false;
>   			}
>   		}
> +		tx_count = i;
> +	}
> +
> +	/* Allow timeout for each byte written. */
> +	for (i = 0; i < tx_count; i++) {
> +		if (wait_for_lsr(up, UART_LSR_THRE))

This ensures you sent one character from the FIFO. The FIFO still can 
contain plenty of them. Did you want UART_LSR_TEMT?

But what's the purpose of spinning _here_? The kernel can run and FIFO 
too. Without the kernel waiting for the FIFO.

thanks,
-- 
js
suse labs

Re: [PATCH tty-next v3 4/6] serial: 8250: Specify console context for rs485_start/stop_tx

[Jiri Slaby]

On 25. 10. 24, 12:57, 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/stop_tx() callbacks. However, these
> callbacks will disable/enable interrupts, which is a problem
> for console write, as it must be responsible for
> disabling/enabling interrupts.
> 
> Add an argument @in_con to the rs485_start/stop_tx() callbacks
> to specify if they are being called from console write. If so,
> the callbacks will not handle interrupt disabling/enabling.
> 
> For all call sites other than console write, there is no
> functional change.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
...
> @@ -1562,12 +1566,20 @@ 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 in_con)
>   {
>   	unsigned char mcr = serial8250_in_MCR(up);
>   
> -	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> -		serial8250_stop_rx(&up->port);
> +	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> +		/*
> +		 * In console context, caller handles interrupt disabling. So
> +		 * only LSR_DR masking is needed.
> +		 */
> +		if (in_con)
> +			__serial8250_stop_rx_mask_dr(&up->port);
> +		else
> +			serial8250_stop_rx(&up->port);

Would it make sense to propagate in_con into serial8250_stop_rx() and do 
the logic there? That would effectively eliminate patch 2/6.

thanks,
-- 
js
suse labs

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

[Jiri Slaby]

On 25. 10. 24, 12:57, 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 callbacks allow safe handover/takeover per byte and add
> a preceding newline if they take over mid-line.
> 
> For the write_atomic() case, a new irq_work is used to defer modem
> control since it may be a context that does not allow waking up
> tasks.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>   drivers/tty/serial/8250/8250_core.c |  35 +++++-
>   drivers/tty/serial/8250/8250_port.c | 159 ++++++++++++++++++++++------
>   include/linux/serial_8250.h         |   7 +-
>   3 files changed, 164 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 5f9f06911795..7184100129bd 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,

Once '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,

Second time 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 *con, unsigned long *flags)

And suddenly, it is 'con'.

> +{
> +	struct uart_port *up = &serial8250_ports[con->index].port;
> +
> +	__uart_port_lock_irqsave(up, flags);
> +}
> +
> +static void univ8250_console_device_unlock(struct console *con, unsigned long flags)
> +{
> +	struct uart_port *up = &serial8250_ports[con->index].port;
> +
> +	__uart_port_unlock_irqrestore(up, flags);
>   }
>   

...
>   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);
> +
> +	if (ch == '\n')
> +		up->console_line_ended = true;
> +	else
> +		up->console_line_ended = false;

So simply:
    up->console_line_ended = ch == '\n';
?

...
> -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 IER then disable the interrupts. The special variant

When you are at it:
"First, save the IER, then"

(BTW why did you remove the "the"?)

> +	 * to clear 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))) {

> @@ -3497,6 +3593,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;
> +	up->modem_status_work = IRQ_WORK_INIT(modem_status_handler);

Looks weird ^^^.

Do:
   init_irq_work(&up->modem_status_work, modem_status_handler)

thanks,
-- 
js
suse labs

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

[Maciej W. Rozycki]

On Wed, 30 Oct 2024, Jiri Slaby wrote:

> > @@ -3306,13 +3310,18 @@ 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. */
> > +		for (i = 0; i < fifosize; i++) {
> > +			if (wait_for_lsr(up, UART_LSR_THRE))
> > +				break;
> > +		}
> 
> THRE only signals there is a space for one character.

 Nope[1]:

"In the FIFO mode, THRE is set when the transmit FIFO is empty; it is 
cleared when at least one byte is written to the transmit FIFO."

It seems common enough a misconception that once I actually had to fix the 
bad interpretation of THRE in an unpublished platform driver to get decent 
performance out of it at higher rates such as 230400bps, as it only pushed 
one byte at a time to the FIFO while it had it all available once THRE has 
been set.

> > +	/* Allow timeout for each byte written. */
> > +	for (i = 0; i < tx_count; i++) {
> > +		if (wait_for_lsr(up, UART_LSR_THRE))
> 
> This ensures you sent one character from the FIFO. The FIFO still can contain
> plenty of them. Did you want UART_LSR_TEMT?

 The difference between THRE and TEMT is the state of the shift register 
only[2]:

"In the FIFO mode, TEMT is set when the transmitter FIFO and shift 
register are both empty."

References:

[1] "TL16C550C, TL16C550CI Asynchronous Communications Element with 
    Autoflow Control", Texas Instruments, SLLS177F -- March 1994 -- 
    Revised March 2001, p. 30

[2] same

  Maciej

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

[John Ogness]

Hi Maciej,

Thanks for jumping in with some ref-manual quotes. Some more comments
from me below...

On 2024-10-31, "Maciej W. Rozycki" <macro@orcam.me.uk> wrote:
> On Wed, 30 Oct 2024, Jiri Slaby wrote:
>> > @@ -3306,13 +3310,18 @@ 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. */
>> > +		for (i = 0; i < fifosize; i++) {
>> > +			if (wait_for_lsr(up, UART_LSR_THRE))
>> > +				break;
>> > +		}
>> 
>> THRE only signals there is a space for one character.
>
>  Nope[1]:
>
> "In the FIFO mode, THRE is set when the transmit FIFO is empty; it is 
> cleared when at least one byte is written to the transmit FIFO."
>
> It seems common enough a misconception that once I actually had to fix the 
> bad interpretation of THRE in an unpublished platform driver to get decent 
> performance out of it at higher rates such as 230400bps, as it only pushed 
> one byte at a time to the FIFO while it had it all available once THRE has 
> been set.

I do not know if this is true for all 8250-variants. If there is some
variant where it functions as Jiri expected, then it would mean
significant text loss during longer messages. But that would already be
a problem in the current mainline driver.

>> > +	/* Allow timeout for each byte written. */
>> > +	for (i = 0; i < tx_count; i++) {
>> > +		if (wait_for_lsr(up, UART_LSR_THRE))
>> 
>> This ensures you sent one character from the FIFO. The FIFO still can contain
>> plenty of them. Did you want UART_LSR_TEMT?
>
>  The difference between THRE and TEMT is the state of the shift register 
> only[2]:
>
> "In the FIFO mode, TEMT is set when the transmitter FIFO and shift 
> register are both empty."

If we wait for TEMT, we lose significant advantages of having the FIFO.

>> But what's the purpose of spinning _here_? The kernel can run and FIFO
>> too. Without the kernel waiting for the FIFO.

When serial8250_console_fifo_write() exits, the caller just does a
single wait_for_xmitr() ... with a 10ms timeout. In the FIFO case, for
<=56k baudrates, it can easily hit the timeout and thus continue before
the FIFO has been emptied.

By waiting on UART_LSR_THRE after filling the FIFO,
serial8250_console_fifo_write() waits until the hardware has had a
chance to shift out all the data. Then the final wait_for_xmitr() in the
caller only waits for the final byte to go out on the line.

Please keep in mind that none of these timeouts should trigger during
normal operation.

For v4 I am doing some refactoring (as suggested by Andy) so that the
wait-code looks a bit cleaner.

John

> References:
>
> [1] "TL16C550C, TL16C550CI Asynchronous Communications Element with 
>     Autoflow Control", Texas Instruments, SLLS177F -- March 1994 -- 
>     Revised March 2001, p. 30
>
> [2] same

Re: [PATCH tty-next v3 4/6] serial: 8250: Specify console context for rs485_start/stop_tx

[John Ogness]

On 2024-10-30, Jiri Slaby <jirislaby@kernel.org> wrote:
>> -void serial8250_em485_start_tx(struct uart_8250_port *up)
>> +void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con)
>>   {
>>   	unsigned char mcr = serial8250_in_MCR(up);
>>   
>> -	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
>> -		serial8250_stop_rx(&up->port);
>> +	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
>> +		/*
>> +		 * In console context, caller handles interrupt disabling. So
>> +		 * only LSR_DR masking is needed.
>> +		 */
>> +		if (in_con)
>> +			__serial8250_stop_rx_mask_dr(&up->port);
>> +		else
>> +			serial8250_stop_rx(&up->port);
>
> Would it make sense to propagate in_con into serial8250_stop_rx() and do 
> the logic there? That would effectively eliminate patch 2/6.

I considered this, however:

1. The whole idea of stopping RX in order to do TX is an RS485
issue. Modifying the general ->stop_rx() callback for this purpose is
kind of out of place.

2. The ->stop_rx() callback is a general uart_ops callback. Changing its
prototype would literally affect all serial drivers. OTOH the
->rs485_start_tx() callback is specific to the 8250 driver. (It seems
each driver has implemented their own method for handling the RS485
hacks.)

So I would prefer to keep the necessary RS485 changes 8250-specific for
now.

John

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

[John Ogness]

On 2024-10-30, Jiri Slaby <jirislaby@kernel.org> wrote:
>> -static void univ8250_console_write(struct console *co, const char *s,
>> -				   unsigned int count)
>> +static void univ8250_console_write_atomic(struct console *co,
>
> Once 'co'.

>> +static void univ8250_console_write_thread(struct console *co,
>
> Second time co.

>> +static void univ8250_console_device_lock(struct console *con, unsigned long *flags)
>
> And suddenly, it is 'con'.

Sorry. The printk folks like "con". The 8250 folks seem to like "co". I
will switch to "co" for the 8250 changes.

>>   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);
>> +
>> +	if (ch == '\n')
>> +		up->console_line_ended = true;
>> +	else
>> +		up->console_line_ended = false;
>
> So simply:
>     up->console_line_ended = ch == '\n';

OK, although I would also add parenthesis to make the inline boolean
evaluation visually more obvious:

	up->console_line_ended = (ch == '\n');

>>   	/*
>> -	 *	First save the IER then disable the interrupts
>> +	 * First save IER then disable the interrupts. The special variant
>
> When you are at it:
> "First, save the IER, then"

OK.

> (BTW why did you remove the "the"?)

If IER is the name of a register, the "the" is inappropriate. If IER is
just an abbreviation for "interrupt enable register" then the "the" is
correct. In this case, both are correct, so it depends on how you read
it. ;-)

Anyway, I have no problems leaving the "the" in place.

>> +	up->console_line_ended = true;
>> +	up->modem_status_work = IRQ_WORK_INIT(modem_status_handler);
>
> Looks weird ^^^.
>
> Do:
>    init_irq_work(&up->modem_status_work, modem_status_handler)

Right. Thanks.

John

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

[Maciej W. Rozycki]

On Thu, 31 Oct 2024, John Ogness wrote:

> >> THRE only signals there is a space for one character.
> >
> >  Nope[1]:
> >
> > "In the FIFO mode, THRE is set when the transmit FIFO is empty; it is 
> > cleared when at least one byte is written to the transmit FIFO."
> >
> > It seems common enough a misconception that once I actually had to fix the 
> > bad interpretation of THRE in an unpublished platform driver to get decent 
> > performance out of it at higher rates such as 230400bps, as it only pushed 
> > one byte at a time to the FIFO while it had it all available once THRE has 
> > been set.
> 
> I do not know if this is true for all 8250-variants. If there is some
> variant where it functions as Jiri expected, then it would mean
> significant text loss during longer messages. But that would already be
> a problem in the current mainline driver.

 Or rather in my case it would prevent communication from working at all; 
I actually had to fix the issue for networking over a serial line rather 
than just exchanging text messages, and hence a particular need to make it 
run fast.

 I don't expect any 550 clone to work in a different manner, but I find 
the TI manual particularly unambiguous and well-written, and also old 
enough for the 550 to be the state of the art rather than just legacy.

  Maciej

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

[Andy Shevchenko]

On Fri, Nov 01, 2024 at 01:24:53AM +0000, Maciej W. Rozycki wrote:
> On Thu, 31 Oct 2024, John Ogness wrote:

>  I don't expect any 550 clone to work in a different manner, but I find
> the TI manual particularly unambiguous and well-written, and also old
> enough for the 550 to be the state of the art rather than just legacy.

Oh, one is living in the ideal world... :-)

-- 
With Best Regards,
Andy Shevchenko

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

[Jiri Slaby]

On 31. 10. 24, 5:44, Maciej W. Rozycki wrote:
> On Wed, 30 Oct 2024, Jiri Slaby wrote:
> 
>>> @@ -3306,13 +3310,18 @@ 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. */
>>> +		for (i = 0; i < fifosize; i++) {
>>> +			if (wait_for_lsr(up, UART_LSR_THRE))
>>> +				break;
>>> +		}
>>
>> THRE only signals there is a space for one character.
> 
>   Nope[1]:
> 
> "In the FIFO mode, THRE is set when the transmit FIFO is empty; it is
> cleared when at least one byte is written to the transmit FIFO."

Hmm, I was confused by NXP's 16c650b [1] datasheet then (or I cannot parse):
===
The THR empty flag in the LSR register will be set to a logic 1 when the 
transmitter is empty or when data is transferred to the TSR. Note that a 
write operation can be performed when the THR empty flag is set
(logic 0 = FIFO full; logic 1 = at least one FIFO location available).
===

But indeed in the LSR[5] bit description, they state:
===
In the FIFO mode, this bit is set when the transmit FIFO is
empty; it is cleared when at least 1 byte is written to the transmit FIFO.
===

Anyway, it still does not answer the original question: Instead of 
looping fifosize multiplied by random timeout, can we re-use 
port->frame_time?

[1] SC16C650B -- 5 V, 3.3 V and 2.5 V UART with 32-byte FIFOs and 
infrared (IrDA) encoder/decoder; Rev. 04 — 14 September 2009; Product 
data sheet

>>> +	/* Allow timeout for each byte written. */
>>> +	for (i = 0; i < tx_count; i++) {
>>> +		if (wait_for_lsr(up, UART_LSR_THRE))
>>
>> This ensures you sent one character from the FIFO. The FIFO still can contain
>> plenty of them. Did you want UART_LSR_TEMT?
> 
>   The difference between THRE and TEMT is the state of the shift register
> only[2]:
> 
> "In the FIFO mode, TEMT is set when the transmitter FIFO and shift
> register are both empty."

Sure. The question still holds:

 > But what's the purpose of spinning _here_? The kernel can run and 
FIFO too. Without the kernel waiting for the FIFO.

If we want to wait for fifo to empty, why not *also* the TSR. Meaning:

 > Did you want UART_LSR_TEMT?

thanks,
-- 
js
suse labs

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

[Jiri Slaby]

On 31. 10. 24, 9:49, John Ogness wrote:
>>>> +	/* Allow timeout for each byte written. */
>>>> +	for (i = 0; i < tx_count; i++) {
>>>> +		if (wait_for_lsr(up, UART_LSR_THRE))
>>>
>>> This ensures you sent one character from the FIFO. The FIFO still can contain
>>> plenty of them. Did you want UART_LSR_TEMT?
>>
>>   The difference between THRE and TEMT is the state of the shift register
>> only[2]:
>>
>> "In the FIFO mode, TEMT is set when the transmitter FIFO and shift
>> register are both empty."
> 
> If we wait for TEMT, we lose significant advantages of having the FIFO.

But you wait for THRE, so effectively waiting for FIFO to flush. The 
difference is only one byte (TSR), or what am I missing?

>>> But what's the purpose of spinning _here_? The kernel can run and FIFO
>>> too. Without the kernel waiting for the FIFO.
> 
> When serial8250_console_fifo_write() exits, the caller just does a
> single wait_for_xmitr() ... with a 10ms timeout. In the FIFO case, for
> <=56k baudrates, it can easily hit the timeout and thus continue before
> the FIFO has been emptied.
>> By waiting on UART_LSR_THRE after filling the FIFO,
> serial8250_console_fifo_write() waits until the hardware has had a
> chance to shift out all the data. Then the final wait_for_xmitr() in the
> caller only waits for the final byte to go out on the line.

For the first loop, that's all right. But why would you want to wait for 
the FIFO to flush at the end of the function? It's not only the last 
byte, it's the last batch (aka 'tx_count'), right?

> Please keep in mind that none of these timeouts should trigger during
> normal operation.
> 
> For v4 I am doing some refactoring (as suggested by Andy) so that the
> wait-code looks a bit cleaner.

OK, let's see then :).

thanks,
-- 
js
suse labs

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

[John Ogness]

On 2024-11-04, Jiri Slaby <jirislaby@kernel.org> wrote:
> Instead of looping fifosize multiplied by random timeout, can we
> re-use port->frame_time?

Rather than 10k loops, we could loop

	(port->frame_time * some_scaled_padding) / 1000

times. The padding is important because we should not timeout in the
normal scenario. Perhaps using ~2 as @some_padding. Something like:

	port->frame_time >> 9

?

>>   The difference between THRE and TEMT is the state of the shift register
>> only[2]:
>> 
>> "In the FIFO mode, TEMT is set when the transmitter FIFO and shift
>> register are both empty."
>
> But what's the purpose of spinning _here_? The kernel can run and
> FIFO too. Without the kernel waiting for the FIFO.
>
> If we want to wait for fifo to empty, why not *also* the TSR. Meaning:
>
> Did you want UART_LSR_TEMT?

Let us assume we have a line with 640 characters and a FIFO of 64
bytes. For this line, we must wait for the FIFO to empty 10 times. It is
enough to wait for THRE for each of the 64-byte blocks because we are
only interested in refilling the FIFO at this point. Only at the very
end (in the caller...  serial8250_console_write()) do we need to wait
for everything to flush to the wire (TEMT).

By waiting on TEMT for each of the 64-byte blocks, we are waiting longer
than necessary. This creates a small window where the FIFO is empty and
there is nothing being transmitted.

I did a simple test on my beaglebone-black hardware, sending 100 lines
of 924 bytes at 9600 bps. Since my hardware uses a 64-byte FIFO, each
line would have 14 such windows.

And indeed, waiting for TEMT rather than only THRE for the 64-byte
blocks resulted in an extra 30ms total transfer for all 92400
bytes. That is about 20us lost in each window by unnecessarily waiting
for TEMT.

Of course, we are only talking about console output, which is horribly
inefficient on system resources. But I would argue, if we do not care
about unnecessary waiting, then why even have the FIFO optimization in
the first place?

John

Re: [PATCH tty-next v3 2/6] serial: 8250: Use high-level write function for FIFO

[Petr Mladek]

On Fri 2024-10-25 13:03:24, John Ogness wrote:
> 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. That 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>

Looks good:

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

Best Regards,
Petr

Re: [PATCH tty-next v3 3/6] serial: 8250: Split out rx stop/start code into helpers

[Petr Mladek]

On Fri 2024-10-25 13:03:25, John Ogness wrote:
> The rx stop/start callbacks also disable/enable interrupts. This
> is not acceptable for the console write callback since it must
> manage all interrupt disabling/enabling.
> 
> Move the interrupt disabling/enabling/masking into helper
> functions so that the console write callback can make use of
> the appropriate parts in a follow-up commit.
> 
> This is essentially refactoring and should cause no functional
> change.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

The changes look reasonable and do what described:

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

Best Regards,
Petr

Re: [PATCH tty-next v3 4/6] serial: 8250: Specify console context for rs485_start/stop_tx

[Petr Mladek]

On Fri 2024-10-25 13:03:26, 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/stop_tx() callbacks. However, these
> callbacks will disable/enable interrupts, which is a problem
> for console write, as it must be responsible for
> disabling/enabling interrupts.

It is not clear to me what exactly is the problem. Is the main
problem calling pm_runtime*() API because it uses extra locks
and can cause deadlocks? Or is it more complicated?

IMHO, it would deserve some explanation.

> Add an argument @in_con to the rs485_start/stop_tx() callbacks
> to specify if they are being called from console write. If so,
> the callbacks will not handle interrupt disabling/enabling.
> 
> For all call sites other than console write, there is no
> functional change.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

It looks like the code does what the description says. And honestly,
I do not have any idea how to improve the naming. I would keep
it as is after reading John's answers in the thread.

IMHO, one thing which makes things comlicated is that
serial8250_em485_start_tx() and serial8250_em485_stop_tx()
are not completely reversible operations. Especially,
the change done by __serial8250_stop_rx_mask_dr() is
not reverted in serial8250_em485_stop_tx(). It makes
things look tricky. But I think that it is beyond the scope
of this patchset to do anything about it.

Just 2 my cents.

Best Regaards,
Petr

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

[Petr Mladek]

On Mon 2024-10-28 14:28:35, John Ogness wrote:
> On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >> +/*
> >> + * Only to be used directly by the console write callbacks, 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);
> >
> >>  		serial_out(up, UART_IER, 0);
> >>  }
> >>  
> >> +static inline void serial8250_clear_IER(struct uart_8250_port *up)
> >> +{
> >> +	__serial8250_clear_IER(up);
> >
> > Shouldn't this have a lockdep annotation to differentiate with the
> > above?
> 
> Yes, but the follow-up patch adds the annotation as a clean "revert
> patch". I can add a line about that in the commit message.
> 
> >> +static void serial8250_console_byte_write(struct uart_8250_port *up,
> >> +					  struct nbcon_write_context *wctxt)
> >> +{
> >> +	const char *s = READ_ONCE(wctxt->outbuf);
> >> +	const char *end = s + READ_ONCE(wctxt->len);
> >
> > Is there any possibility that outbuf value be changed before we get
> > the len and at the end we get the wrong pointer?
> 
> No. I was concerned about compiler optimization, since @outbuf can
> become NULL. However, it can only become NULL if ownership was
> transferred, and that is properly checked anyway. I will remove the
> READ_ONCE() usage for v4.

I agree that we do not need READ_ONCE() here.

Just to be sure that I understand it correctly.

The struct nbcon_write_context passed by *wctxt should be created on
stack of the caller. Only this process/interrupt context could change
it.

Namely, it might happen when nbcon_enter_unsafe() fails. It is done
later in this function by the code:

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

and this function does not access *s or *end after this code.

Other CPUs could not change the structure in parallel

   => READ_ONCE() is not needed.


Just for completeness. The buffer could not disappear.
wctxt->outbuf always points to a static buffer.

Also the content of the buffer could not change if we read it only
after successful nbcon_enter_unsafe(). Only the panic CPU is allowed
to takeover the ownership in this case and it would use another static
buffer.

Best Regards,
Petr

PS: I do not have anything more to add for this patch. It seems to
    work work as expected.

Re: [PATCH tty-next v3 4/6] serial: 8250: Specify console context for rs485_start/stop_tx

[John Ogness]

On 2024-11-06, Petr Mladek <pmladek@suse.com> 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/stop_tx() callbacks. However, these
>> callbacks will disable/enable interrupts, which is a problem
>> for console write, as it must be responsible for
>> disabling/enabling interrupts.
>
> It is not clear to me what exactly is the problem.

serial8250_em485_stop_tx() blindly sets the RX interrupt bits in IER,
because it assumes they were cleared in serial8250_stop_rx(). This is
fine for the driver in general, but it is wrong for the console
->write(), which restores those bits on its own later.

> Is the main problem calling pm_runtime*() API because it uses extra
> locks and can cause deadlocks? Or is it more complicated?

pm_runtime*() is a second issue. In the v1 feeback we talked about
it. tglx summarized it well here:

https://lore.kernel.org/lkml/8734mbdwrf.ffs@tglx/

as well as explaining the need to split off the console-write code from
the generic driver code.

> IMHO, it would deserve some explanation.

This commit message only talks about the first issue, which is enough to
justify the patch. I will add that the callbacks are also not
appropriate because they call into the PM code, which is not needed by
console ->write() and is even unsafe in some contexts.

> IMHO, one thing which makes things comlicated is that
> serial8250_em485_start_tx() and serial8250_em485_stop_tx()
> are not completely reversible operations. Especially,
> the change done by __serial8250_stop_rx_mask_dr() is
> not reverted in serial8250_em485_stop_tx(). It makes
> things look tricky. But I think that it is beyond the scope
> of this patchset to do anything about it.

I agree that it is strange that the driver does not unmask DR later. I
have now run tests and it seems the use of @read_status_mask is
partially broken. I did some historical digging on it...

For Linux 1.1.60 [0] the @read_status_mask usage was extended to support
"stop listening to incoming characters" (text from the changelog
[1]). Looking at that version, it is clear why and how it was used.

For Linux 2.1.8 [2], the async handling was reworked, basically
reverting the change from 1.1.60. However, that revert forgot the piece
that clears the UART_LSR_DR bit in serial8250_stop_rx() (back then
called rs_close()).

And indeed, if you track the @read_status_mask value today, that bit
remains cleared until serial8250_do_set_termios() happens to be
called. But it didn't matter that the bit was not set again because that
bit was not being evaluated at any call sites.

For 4.6, RS485 support was added, but with a bug about re-enabling
interrupts. When that bug was fixed [3], the fix did not set the
UART_LSR_DR bit in @read_status_mask. Still that was not a problem
because at that time, that bit still had no users.

For 5.7, support was added to avoid reading characters when
throttling. This re-introduced a user of the UART_LSR_DR bit in
@read_status_mask. And thus now there _is_ a bug that the bit is not set
when starting RX in __do_stop_tx_rs485(). Interestingly enough, the OMAP
variant of the 8250 _did_ implement setting the bit when unthrottling
[5] (also from the same series).

So in summary, I will add a patch to my series that fixes [3] (or is it
fixing [4]?) by setting the bit in __do_stop_tx_rs485() when re-enabling
the RX interrupts.

John

[0] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/serial.c?id=ba97e35a1a8b45ff87ed37a58fca3ecf39c1c893
[1] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/ChangeLog?id=ba97e35a1a8b45ff87ed37a58fca3ecf39c1c893
[2] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/serial.c?id=0f9cac5b27076f801b29a0867868e1bce7310e00
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=0c66940d584d1aac92f6a78460dc0ba2efd3b7ba
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=f19c3f6c8109b8bab000afd35580929958e087a9
[5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=f4b042a050062b2dec456adfced13d61341939e2

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

[Maciej W. Rozycki]

On Mon, 4 Nov 2024, Jiri Slaby wrote:

> > > THRE only signals there is a space for one character.
> > 
> >   Nope[1]:
> > 
> > "In the FIFO mode, THRE is set when the transmit FIFO is empty; it is
> > cleared when at least one byte is written to the transmit FIFO."
> 
> Hmm, I was confused by NXP's 16c650b [1] datasheet then (or I cannot parse):
> ===
> The THR empty flag in the LSR register will be set to a logic 1 when the
> transmitter is empty or when data is transferred to the TSR. Note that a write
> operation can be performed when the THR empty flag is set
> (logic 0 = FIFO full; logic 1 = at least one FIFO location available).
> ===

 This description seems broken indeed and I find your interpretation of it
correct.  I do hope this is just an editorial mistake with the NXP device.

> But indeed in the LSR[5] bit description, they state:
> ===
> In the FIFO mode, this bit is set when the transmit FIFO is
> empty; it is cleared when at least 1 byte is written to the transmit FIFO.
> ===

 FWIW I chased documentation for the original 16550A device and it uses 
analogous wording[1]:

"Bit 5: This bit is the Transmitter Holding Register Empty (THRE)
indicator. Bit 5 indicates that the UART is ready to
accept a new character for transmission. In addition, this bit
causes the UART to issue an interrupt to the CPU when the
Transmit Holding Register Empty Interrupt enable is set
high. The THRE bit is set to a logic 1 when a character is
transferred from the Transmitter Holding Register into the
Transmitter Shift Register. The bit is reset to logic 0 concur-
rently with the loading of the Transmitter Holding Register
by the CPU. In the FIFO mode this bit is set when the XMIT
FIFO is empty; it is cleared when at least 1 byte is written to
the XMIT FIFO."

It further documents polled operation for THRE[2]:

"LSR5 will indicate when the XMIT FIFO is empty."

and provides further details as to the transmit FIFO[3]:

"The NS16550A transmitter optimizes the CPU/UART data
transaction via the following features:

"1. The depth of the Transmitter (Tx) FIFO ensures that as
    many as 16 characters can be transferred when the
    CPU services the Tx interrupt. Once again, this effec-
    tively buffers the CPU transfer rate from the serial data
    rate.

"2. The Transmitter (Tx) FIFO is similar in structure to
    FIFOs the user may have previously set up in RAM. The
    Tx depth allows the CPU to load 16 characters each
    time it switches context to the service routine. This re-
    duces the impact of the CPU time lost in context switch-
    ing.

"3. Since a time lag in servicing an asynchronous transmit-
    ter usually has no penalty, CPU latency time is of no
    concern to transmitter operation."

This design choice may result in "choppy" transmission due to the transmit 
handler invocation latency, but as noted above it is intentional.  DMA can 
be used with the NS16550A if it is required to avoid this issue, and later 
compatible UART designs such as the 650 give more flexibility as to the Tx 
FIFO fill level trigger threshold.

References:

[1] National Semiconductor Corporation, "Microcommunication Elements 
    Databook, 1987 Edition", "NS16550A Universal Asynchronous 
    Receiver/Transmitter with FIFOs", Section 8.4 "LINE STATUS REGISTER", 
    p. 2-73

[2] same, Section 8.12 "FIFO POLLED MODE OPERATION", p. 2-75

[3] same, Martin S. Michael, Daniel G. Durich, Application Note 491, "The 
    NS 16550A: UART Design and Application Considerations", Section 1.0 
    "Design Considerations and Operation of the New UART Features", p. 5-4

Available from: 
<http://bitsavers.org/components/national/_dataBooks/1987_National_Microcommunications_Elements_Data_Book.pdf>.

 HTH,

  Maciej

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

[Jiri Slaby]

On 04. 11. 24, 15:13, John Ogness wrote:
> On 2024-11-04, Jiri Slaby <jirislaby@kernel.org> wrote:
>> Instead of looping fifosize multiplied by random timeout, can we
>> re-use port->frame_time?
> 
> Rather than 10k loops, we could loop
> 
> 	(port->frame_time * some_scaled_padding) / 1000
> 
> times. The padding is important because we should not timeout in the
> normal scenario. Perhaps using ~2 as @some_padding. Something like:
> 
> 	port->frame_time >> 9
> 
> ?

No, spell it out as you did above:
   port->frame_time * 2 / NSEC_PER_USEC

>>>    The difference between THRE and TEMT is the state of the shift register
>>> only[2]:
>>>
>>> "In the FIFO mode, TEMT is set when the transmitter FIFO and shift
>>> register are both empty."
>>
>> But what's the purpose of spinning _here_? The kernel can run and
>> FIFO too. Without the kernel waiting for the FIFO.
>>
>> If we want to wait for fifo to empty, why not *also* the TSR. Meaning:
>>
>> Did you want UART_LSR_TEMT?
> 
> Let us assume we have a line with 640 characters and a FIFO of 64
> bytes. For this line, we must wait for the FIFO to empty 10 times. It is
> enough to wait for THRE for each of the 64-byte blocks because we are
> only interested in refilling the FIFO at this point. Only at the very
> end (in the caller...  serial8250_console_write()) do we need to wait
> for everything to flush to the wire (TEMT).
> 
> By waiting on TEMT for each of the 64-byte blocks, we are waiting longer
> than necessary. This creates a small window where the FIFO is empty and
> there is nothing being transmitted.
> 
> I did a simple test on my beaglebone-black hardware, sending 100 lines
> of 924 bytes at 9600 bps. Since my hardware uses a 64-byte FIFO, each
> line would have 14 such windows.
> 
> And indeed, waiting for TEMT rather than only THRE for the 64-byte
> blocks resulted in an extra 30ms total transfer for all 92400
> bytes. That is about 20us lost in each window by unnecessarily waiting
> for TEMT.

Sure -- you still misunderstand me. I am still asking why do you want to 
wait for the TX machinery at the *end* (for the last 64 B of the 640 B 
line) of transmission at all? It occurs to me as wasted cycles.

thanks,
-- 
js
suse labs

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

[John Ogness]

On 2024-12-02, Jiri Slaby <jirislaby@kernel.org> wrote:
> I am still asking why do you want to wait for the TX machinery at the
> *end* (for the last 64 B of the 640 B line) of transmission at all? It
> occurs to me as wasted cycles.

The printk-framework has always expected that when console->write()
returns, the data has been flushed out of the hardware. I am guessing
because it is easiest to avoid possible data loss, for example, due to
suspending hardware.

If you want me to change the current behavior, I can do that in a
separate patch. I would like this patch to only be about fixing the FIFO
timeout issue.

John