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

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

[John Ogness]

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

The changes since v4:

- In wait_for_lsr() use else-condition for fallback timeout.

- In fifo_wait_for_lsr() remove unnecessary return value.

- In serial8250_console_fifo_write() enter/exit unsafe section
  for each character to avoid writing to UART_TX on
  handover/takeover.

- In serial8250_console_byte_write() enter/exit unsafe section
  for each character rather than calling nbcon_can_proceed()
  for each character because nbcon_can_proceed() only checks
  ownership but does not actually handover if within an unsafe
  section. If there is a higher priority waiter, we want to
  handover ASAP so that printing can continue in the higher
  priority context.

- In serial8250_console_write() cleanup the implementation so
  that the procedure is more obvious.

- Add detailed multi-line comment documenting
  uart_8250_port->console_line_ended.

- Add and extend comments as requested.

John Ogness

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

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

 drivers/tty/serial/8250/8250.h            |   4 +-
 drivers/tty/serial/8250/8250_bcm2835aux.c |   4 +-
 drivers/tty/serial/8250/8250_core.c       |  35 ++-
 drivers/tty/serial/8250/8250_omap.c       |   2 +-
 drivers/tty/serial/8250/8250_port.c       | 259 +++++++++++++++++-----
 include/linux/serial_8250.h               |  17 +-
 6 files changed, 254 insertions(+), 67 deletions(-)


base-commit: 2c1fd53af21b8cb13878b054894d33d3383eb1f3
-- 
2.39.5

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

[John Ogness]

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

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

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

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

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

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

[John Ogness]

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

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

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 drivers/tty/serial/8250/8250_port.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3a946ebe9139..ca8f6f3855eb 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2081,9 +2081,17 @@ static void serial8250_break_ctl(struct uart_port *port, int break_state)
 /* 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;
+	unsigned int status, tmout;
+
+	/*
+	 * Wait for a character to be sent. Fallback to a safe default
+	 * timeout value if @frame_time is not available.
+	 */
+	if (up->port.frame_time)
+		tmout = up->port.frame_time * 2 / NSEC_PER_USEC;
+	else
+		tmout = 10000;
 
-	/* Wait up to 10ms for the character(s) to be sent. */
 	for (;;) {
 		status = serial_lsr_in(up);
 
-- 
2.39.5

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

[Andy Shevchenko]

On Tue, Jan 07, 2025 at 10:32:58PM +0106, John Ogness wrote:
> Rather than using a hard-coded per-character Tx-timeout of 10ms,
> use the frame time to determine a timeout value. The value is
> doubled to ensure that a timeout is only hit during unexpected
> circumstances.
> 
> Since the frame time may not be available during early printing,
> the previous 10ms value is kept as a fallback.

...

> +	/*
> +	 * Wait for a character to be sent. Fallback to a safe default
> +	 * timeout value if @frame_time is not available.
> +	 */
> +	if (up->port.frame_time)
> +		tmout = up->port.frame_time * 2 / NSEC_PER_USEC;
> +	else
> +		tmout = 10000;

I would use it in a form of

		tmout = 10 * USEC_PER_MSEC;

This will give a hint of the real unit (10 ms in us).

-- 
With Best Regards,
Andy Shevchenko

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

[John Ogness]

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

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

This is only refactoring and has no functional change.

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

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ca8f6f3855eb..15abd95fcf06 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3298,11 +3298,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);
 }
 
 /*
@@ -3352,6 +3357,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;
@@ -3362,10 +3368,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;
 			}
 		}
@@ -3445,7 +3451,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 v5 4/6] serial: 8250: Provide flag for IER toggling for RS485

[John Ogness]

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

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

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

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

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

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

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

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

[John Ogness]

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

Add implementations for the nbcon console callbacks:

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

and add CON_NBCON to the initial @flags.

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

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

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

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

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 2b70e82dffeb..3d5a2183ff13 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -388,12 +388,34 @@ void __init serial8250_register_ports(struct uart_driver *drv, struct device *de
 
 #ifdef CONFIG_SERIAL_8250_CONSOLE
 
-static void univ8250_console_write(struct console *co, const char *s,
-				   unsigned int count)
+static void univ8250_console_write_atomic(struct console *co,
+					  struct nbcon_write_context *wctxt)
 {
 	struct uart_8250_port *up = &serial8250_ports[co->index];
 
-	serial8250_console_write(up, s, count);
+	serial8250_console_write(up, wctxt, true);
+}
+
+static void univ8250_console_write_thread(struct console *co,
+					  struct nbcon_write_context *wctxt)
+{
+	struct uart_8250_port *up = &serial8250_ports[co->index];
+
+	serial8250_console_write(up, wctxt, false);
+}
+
+static void univ8250_console_device_lock(struct console *co, unsigned long *flags)
+{
+	struct uart_port *up = &serial8250_ports[co->index].port;
+
+	__uart_port_lock_irqsave(up, flags);
+}
+
+static void univ8250_console_device_unlock(struct console *co, unsigned long flags)
+{
+	struct uart_port *up = &serial8250_ports[co->index].port;
+
+	__uart_port_unlock_irqrestore(up, flags);
 }
 
 static int univ8250_console_setup(struct console *co, char *options)
@@ -494,12 +516,15 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
 
 static struct console univ8250_console = {
 	.name		= "ttyS",
-	.write		= univ8250_console_write,
+	.write_atomic	= univ8250_console_write_atomic,
+	.write_thread	= univ8250_console_write_thread,
+	.device_lock	= univ8250_console_device_lock,
+	.device_unlock	= univ8250_console_device_unlock,
 	.device		= uart_console_device,
 	.setup		= univ8250_console_setup,
 	.exit		= univ8250_console_exit,
 	.match		= univ8250_console_match,
-	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
+	.flags		= CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON,
 	.index		= -1,
 	.data		= &serial8250_reg,
 };
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index d7976a21cca9..08466cf10d73 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -711,7 +711,12 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 	serial8250_rpm_put(p);
 }
 
-static void serial8250_clear_IER(struct uart_8250_port *up)
+/*
+ * Only to be used directly by the callback helper serial8250_console_write(),
+ * which may not require the port lock. Use serial8250_clear_IER() instead for
+ * all other cases.
+ */
+static void __serial8250_clear_IER(struct uart_8250_port *up)
 {
 	if (up->capabilities & UART_CAP_UUE)
 		serial_out(up, UART_IER, UART_IER_UUE);
@@ -719,6 +724,11 @@ static void serial8250_clear_IER(struct uart_8250_port *up)
 		serial_out(up, UART_IER, 0);
 }
 
+static inline void serial8250_clear_IER(struct uart_8250_port *up)
+{
+	__serial8250_clear_IER(up);
+}
+
 #ifdef CONFIG_SERIAL_8250_RSA
 /*
  * Attempts to turn on the RSA FIFO.  Returns zero on failure.
@@ -1406,9 +1416,6 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier)
 {
 	unsigned char mcr = serial8250_in_MCR(p);
 
-	/* Port locked to synchronize UART_IER access against the console. */
-	lockdep_assert_held_once(&p->port.lock);
-
 	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
 		mcr |= UART_MCR_RTS;
 	else
@@ -1424,6 +1431,16 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier)
 		serial8250_clear_and_reinit_fifos(p);
 
 		if (toggle_ier) {
+			/*
+			 * Port locked to synchronize UART_IER access against
+			 * the console. The lockdep_assert must be restricted
+			 * to this condition because only here is it
+			 * guaranteed that the port lock is held. The other
+			 * hardware access in this function is synchronized
+			 * by console ownership.
+			 */
+			lockdep_assert_held_once(&p->port.lock);
+
 			p->ier |= UART_IER_RLSI | UART_IER_RDI;
 			serial_port_out(&p->port, UART_IER, p->ier);
 		}
@@ -3303,7 +3320,11 @@ EXPORT_SYMBOL_GPL(serial8250_set_defaults);
 
 static void serial8250_console_putchar(struct uart_port *port, unsigned char ch)
 {
+	struct uart_8250_port *up = up_to_u8250p(port);
+
 	serial_port_out(port, UART_TX, ch);
+
+	up->console_line_ended = (ch == '\n');
 }
 
 static void serial8250_console_wait_putchar(struct uart_port *port, unsigned char ch)
@@ -3340,11 +3361,22 @@ static void serial8250_console_restore(struct uart_8250_port *up)
 	serial8250_out_MCR(up, up->mcr | UART_MCR_DTR | UART_MCR_RTS);
 }
 
-static void fifo_wait_for_lsr(struct uart_8250_port *up, unsigned int count)
+static void fifo_wait_for_lsr(struct uart_8250_port *up,
+			      struct nbcon_write_context *wctxt,
+			      unsigned int count)
 {
 	unsigned int i;
 
 	for (i = 0; i < count; i++) {
+		/*
+		 * Pass the ownership as quickly as possible to a higher
+		 * priority context. Otherwise, its attempt to take over
+		 * the ownership might timeout. The new owner will wait
+		 * for UART_LSR_THRE before reusing the fifo.
+		 */
+		if (!nbcon_can_proceed(wctxt))
+			return;
+
 		if (wait_for_lsr(up, UART_LSR_THRE))
 			return;
 	}
@@ -3357,20 +3389,29 @@ static void fifo_wait_for_lsr(struct uart_8250_port *up, unsigned int count)
  * to get empty.
  */
 static void serial8250_console_fifo_write(struct uart_8250_port *up,
-					  const char *s, unsigned int count)
+					  struct nbcon_write_context *wctxt)
 {
-	const char *end = s + count;
 	unsigned int fifosize = up->tx_loadsz;
 	struct uart_port *port = &up->port;
+	const char *s = wctxt->outbuf;
+	const char *end = s + wctxt->len;
 	unsigned int tx_count = 0;
 	bool cr_sent = false;
 	unsigned int i;
 
 	while (s != end) {
 		/* Allow timeout for each byte of a possibly full FIFO */
-		fifo_wait_for_lsr(up, fifosize);
+		fifo_wait_for_lsr(up, wctxt, fifosize);
 
+		/*
+		 * Fill the FIFO. If a handover or takeover occurs, writing
+		 * must be aborted since wctxt->outbuf and wctxt->len are no
+		 * longer valid.
+		 */
 		for (i = 0; i < fifosize && s != end; ++i) {
+			if (!nbcon_enter_unsafe(wctxt))
+				return;
+
 			if (*s == '\n' && !cr_sent) {
 				serial8250_console_putchar(port, '\r');
 				cr_sent = true;
@@ -3378,6 +3419,8 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 				serial8250_console_putchar(port, *s++);
 				cr_sent = false;
 			}
+
+			nbcon_exit_unsafe(wctxt);
 		}
 		tx_count = i;
 	}
@@ -3386,39 +3429,57 @@ static void serial8250_console_fifo_write(struct uart_8250_port *up,
 	 * Allow timeout for each byte written since the caller will only wait
 	 * for UART_LSR_BOTH_EMPTY using the timeout of a single character
 	 */
-	fifo_wait_for_lsr(up, tx_count);
+	fifo_wait_for_lsr(up, wctxt, tx_count);
+}
+
+static void serial8250_console_byte_write(struct uart_8250_port *up,
+					  struct nbcon_write_context *wctxt)
+{
+	struct uart_port *port = &up->port;
+	const char *s = wctxt->outbuf;
+	const char *end = s + wctxt->len;
+
+	/*
+	 * Write out the message. If a handover or takeover occurs, writing
+	 * must be aborted since wctxt->outbuf and wctxt->len are no longer
+	 * valid.
+	 */
+	while (s != end) {
+		if (!nbcon_enter_unsafe(wctxt))
+			return;
+
+		uart_console_write(port, s++, 1, serial8250_console_wait_putchar);
+
+		nbcon_exit_unsafe(wctxt);
+	}
 }
 
 /*
- *	Print a string to the serial port trying not to disturb
- *	any possible real use of the port...
- *
- *	The console_lock must be held when we get here.
+ * Print a string to the serial port trying not to disturb
+ * any possible real use of the port...
  *
- *	Doing runtime PM is really a bad idea for the kernel console.
- *	Thus, we assume the function is called when device is powered up.
+ * Doing runtime PM is really a bad idea for the kernel console.
+ * Thus, assume it is called when device is powered up.
  */
-void serial8250_console_write(struct uart_8250_port *up, const char *s,
-			      unsigned int count)
+void serial8250_console_write(struct uart_8250_port *up,
+			      struct nbcon_write_context *wctxt,
+			      bool is_atomic)
 {
 	struct uart_8250_em485 *em485 = up->em485;
 	struct uart_port *port = &up->port;
-	unsigned long flags;
-	unsigned int ier, use_fifo;
-	int locked = 1;
-
-	touch_nmi_watchdog();
+	unsigned int ier;
+	bool use_fifo;
 
-	if (oops_in_progress)
-		locked = uart_port_trylock_irqsave(port, &flags);
-	else
-		uart_port_lock_irqsave(port, &flags);
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
 
 	/*
-	 *	First save the IER then disable the interrupts
+	 * First, save the IER, then disable the interrupts. The special
+	 * variant to clear the IER is used because 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))) {
@@ -3432,6 +3493,18 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 		mdelay(port->rs485.delay_rts_before_send);
 	}
 
+	/* If ownership was lost, no writing is allowed */
+	if (!nbcon_can_proceed(wctxt))
+		goto skip_write;
+
+	/*
+	 * If console printer did not fully output the previous line, it must
+	 * have been handed or taken over. Insert a newline in order to
+	 * maintain clean output.
+	 */
+	if (!up->console_line_ended)
+		uart_console_write(port, "\n", 1, serial8250_console_wait_putchar);
+
 	use_fifo = (up->capabilities & UART_CAP_FIFO) &&
 		/*
 		 * BCM283x requires to check the fifo
@@ -3452,10 +3525,23 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 		 */
 		!(up->port.flags & UPF_CONS_FLOW);
 
+	nbcon_exit_unsafe(wctxt);
+
 	if (likely(use_fifo))
-		serial8250_console_fifo_write(up, s, count);
+		serial8250_console_fifo_write(up, wctxt);
 	else
-		uart_console_write(port, s, count, serial8250_console_wait_putchar);
+		serial8250_console_byte_write(up, wctxt);
+skip_write:
+	/*
+	 * If ownership was lost, this context must reacquire ownership and
+	 * re-enter the unsafe section in order to perform final actions
+	 * (such as re-enabling interrupts).
+	 */
+	if (!nbcon_can_proceed(wctxt)) {
+		do {
+			nbcon_reacquire_nobuf(wctxt);
+		} while (!nbcon_enter_unsafe(wctxt));
+	}
 
 	/*
 	 *	Finally, wait for transmitter to become empty
@@ -3478,11 +3564,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)
@@ -3500,8 +3593,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';
@@ -3511,6 +3620,9 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 	if (!port->iobase && !port->membase)
 		return -ENODEV;
 
+	up->console_line_ended = true;
+	init_irq_work(&up->modem_status_work, modem_status_handler);
+
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
 	else if (probe)
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 144de7a7948d..57875c37023a 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -150,8 +150,17 @@ struct uart_8250_port {
 #define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
 	u16			lsr_saved_flags;
 	u16			lsr_save_mask;
+
+	/*
+	 * Track when a console line has been fully written to the
+	 * hardware, i.e. true when the most recent byte written to
+	 * UART_TX by the console was '\n'.
+	 */
+	bool			console_line_ended;
+
 #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
 	unsigned char		msr_saved_flags;
+	struct irq_work		modem_status_work;
 
 	struct uart_8250_dma	*dma;
 	const struct uart_8250_ops *ops;
@@ -202,8 +211,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

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

[Petr Mladek]

On Tue 2025-01-07 22:33:01, John Ogness wrote:
> Implement the necessary callbacks to switch the 8250 console driver
> to perform as an nbcon console.
> 
> Add implementations for the nbcon console callbacks:
> 
>   ->write_atomic()
>   ->write_thread()
>   ->device_lock()
>   ->device_unlock()
> 
> and add CON_NBCON to the initial @flags.
> 
> All register access in the callbacks are within unsafe sections.
> The ->write_atomic() and ->write_thread() callbacks allow safe
> handover/takeover per byte and add a preceding newline if they
> take over from another context mid-line.
> 
> For the ->write_atomic() callback, a new irq_work is used to defer
> modem control since it may be called from a context that does not
> allow waking up tasks.
> 
> Note: A new __serial8250_clear_IER() is introduced for direct
> clearing of UART_IER. This will allow to restore the lockdep
> check to serial8250_clear_IER() in a follow-up commit.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

It looks good and seems to work fine:

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

Best Regards,
Petr

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

[Jon Hunter]

Hi John,

On 07/01/2025 21:27, John Ogness wrote:
> Implement the necessary callbacks to switch the 8250 console driver
> to perform as an nbcon console.
> 
> Add implementations for the nbcon console callbacks:
> 
>    ->write_atomic()
>    ->write_thread()
>    ->device_lock()
>    ->device_unlock()
> 
> and add CON_NBCON to the initial @flags.
> 
> All register access in the callbacks are within unsafe sections.
> The ->write_atomic() and ->write_thread() callbacks allow safe
> handover/takeover per byte and add a preceding newline if they
> take over from another context mid-line.
> 
> For the ->write_atomic() callback, a new irq_work is used to defer
> modem control since it may be called from a context that does not
> allow waking up tasks.
> 
> Note: A new __serial8250_clear_IER() is introduced for direct
> clearing of UART_IER. This will allow to restore the lockdep
> check to serial8250_clear_IER() in a follow-up commit.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>


I have noticed a suspend regression on -next for some of our 32-bit 
Tegra (ARM) devices (Tegra20, Tegra30 and Tegra124). Bisect is pointing 
to this commit and reverting this on top of -next (along with reverting 
"serial: 8250: Revert "drop lockdep annotation from 
serial8250_clear_IER()") fixes the issue. So far I have not dug in any 
further. Unfortunately, I don't have any logs to see if there is some 
crash or something happening but I will see if there is any more info I 
can get.

Thanks
Jon

-- 
nvpublic

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

[John Ogness]

On 2025-01-15, Jon Hunter <jonathanh@nvidia.com> wrote:
> I have noticed a suspend regression on -next for some of our 32-bit 
> Tegra (ARM) devices (Tegra20, Tegra30 and Tegra124). Bisect is pointing 
> to this commit and reverting this on top of -next (along with reverting 
> "serial: 8250: Revert "drop lockdep annotation from 
> serial8250_clear_IER()") fixes the issue. So far I have not dug in any 
> further. Unfortunately, I don't have any logs to see if there is some 
> crash or something happening but I will see if there is any more info I 
> can get.

Do you at least know if it is failing to suspend or failing to resume
(based on power consumption)?

John

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

[Jon Hunter]

On 15/01/2025 16:54, John Ogness wrote:
> On 2025-01-15, Jon Hunter <jonathanh@nvidia.com> wrote:
>> I have noticed a suspend regression on -next for some of our 32-bit
>> Tegra (ARM) devices (Tegra20, Tegra30 and Tegra124). Bisect is pointing
>> to this commit and reverting this on top of -next (along with reverting
>> "serial: 8250: Revert "drop lockdep annotation from
>> serial8250_clear_IER()") fixes the issue. So far I have not dug in any
>> further. Unfortunately, I don't have any logs to see if there is some
>> crash or something happening but I will see if there is any more info I
>> can get.
> 
> Do you at least know if it is failing to suspend or failing to resume
> (based on power consumption)?


Unfortunately, I don't. These are farm boards and so nothing local I can 
get my hands on. For some reason all the serial console logs are not 
available and so I am going to talk to the farm team about fixing that 
because we should at least have serial logs.

Thanks
Jon

-- 
nvpublic

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

[John Ogness]

On 2025-01-16, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Do you at least know if it is failing to suspend or failing to resume
>> (based on power consumption)?
>
>
> Unfortunately, I don't. These are farm boards and so nothing local I can 
> get my hands on. For some reason all the serial console logs are not 
> available and so I am going to talk to the farm team about fixing that 
> because we should at least have serial logs.

Can you confirm that the board is actually booting? The suspend code for
8250_tegra.c is quite simple. I am wondering if the farm tests are
failing somewhere else, such as the atomic printing during early boot.

John

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

[Jon Hunter]

On 16/01/2025 10:38, John Ogness wrote:
> On 2025-01-16, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> Do you at least know if it is failing to suspend or failing to resume
>>> (based on power consumption)?
>>
>>
>> Unfortunately, I don't. These are farm boards and so nothing local I can
>> get my hands on. For some reason all the serial console logs are not
>> available and so I am going to talk to the farm team about fixing that
>> because we should at least have serial logs.
> 
> Can you confirm that the board is actually booting? The suspend code for
> 8250_tegra.c is quite simple. I am wondering if the farm tests are
> failing somewhere else, such as the atomic printing during early boot.


Yes they are all booting fine. I have an independent boot test and that 
is passing. It is just the suspend test that is failing.

Thanks
Jon

-- 
nvpublic

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5013 bytes --]

On Thu, Jan 16, 2025 at 10:41:08AM +0000, Jon Hunter wrote:
> 
> On 16/01/2025 10:38, John Ogness wrote:
> > On 2025-01-16, Jon Hunter <jonathanh@nvidia.com> wrote:
> > > > Do you at least know if it is failing to suspend or failing to resume
> > > > (based on power consumption)?
> > > 
> > > 
> > > Unfortunately, I don't. These are farm boards and so nothing local I can
> > > get my hands on. For some reason all the serial console logs are not
> > > available and so I am going to talk to the farm team about fixing that
> > > because we should at least have serial logs.
> > 
> > Can you confirm that the board is actually booting? The suspend code for
> > 8250_tegra.c is quite simple. I am wondering if the farm tests are
> > failing somewhere else, such as the atomic printing during early boot.
> 
> 
> Yes they are all booting fine. I have an independent boot test and that is
> passing. It is just the suspend test that is failing.

I was able to capture logs, but unfortunately they don't provide much
insight either. On the first try it doesn't suspend and goes back to
userspace after a second or so:

--- >8 ---
-sh-5.1# rtcwake --device /dev/rtc1 --mode mem --seconds 5
rtcwake: assuming RTC uses UTC ...
rtcwake: wakeup from "mem" using /dev/rtc1 at Thu Jan  1 00:01:00 1970
[   36.332486] PM: suspend entry (deep)
[   36.332832] Filesystems sync: 0.000 seconds
[   36.369331] +1.8V_RUN_CAM: disabling
[   36.373884] +2.8V_RUN_CAM: disabling
[   36.375571] +1.2V_RUN_CAM_FRONT: disabling
[   36.380359] +1.05V_RUN_CAM_REAR: disabling
[   36.387399] +3.3V_RUN_TOUCH: disabling
[   36.390808] +2.8V_RUN_CAM_AF: disabling
[   36.393621] +1.8V_RUN_VPP_FUSE: disabling
[   36.408218] Freezing user space processes
[   36.413660] Freezing user space processes completed (elapsed 0.005 seconds)
[   36.413680] OOM killer disabled.
[   36.413693] Freezing remaining freezable tasks
[   36.415033] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
[   36.428474] drm drm: [drm:drm_client_dev_suspend] fbdev: ret=0
[   36.428527] drm drm: [drm:drm_atomic_state_init] Allocated atomic state 2e5cd010
[   36.428547] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:47:crtc-0] 6a6be0ef state to 2e5cd010
[   36.428561] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:63:crtc-1] 00d818c2 state to 2e5cd010
[   36.428574] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:32:plane-0] 4e145b7d state to 2e5cd010
[   36.428587] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:36:plane-1] dbf67d12 state to 2e5cd010
[   36.428597] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:40:plane-2] 763d8809 state to 2e5cd010
[   36.428608] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:44:plane-3] b6eabcf1 state to 2e5cd010
[   36.428617] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:48:plane-4] 7863878c state to 2e5cd010
[   36.428628] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:52:plane-5] 54b8029c state to 2e5cd010
[   36.428638] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:56:plane-6] 364063af state to 2e5cd010
[   36.428648] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:60:plane-7] e1c11dfb state to 2e5cd010
[   36.428662] drm drm: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:65:HDMI-A-1] 5cb32770 state to 2e5cd010
[   36.428674] drm drm: [drm:drm_atomic_state_init] Allocated atomic state 832943c7
[   36.428682] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:47:crtc-0] f09cf73d state to 832943c7
[   36.428691] drm drm: [drm:drm_atomic_add_affected_planes] Adding all current planes for [CRTC:47:crtc-0] to 832943c7
[   36.428700] drm drm: [drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:47:crtc-0] to 832943c7
[   36.428711] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:63:crtc-1] 2700922c state to 832943c7
[   36.428720] drm drm: [drm:drm_atomic_add_affected_planes] Adding all current planes for [CRTC:63:crtc-1] to 832943c7
[   36.428727] drm drm: [drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:63:crtc-1] to 832943c7
[   36.428737] drm drm: [drm:drm_atomic_check_only] checking 832943c7
[   36.428759] drm drm: [drm:drm_atomic_commit] committing 832943c7
[   36.428881] drm drm: [drm:drm_atomic_state_default_clear] Clearing atomic state 832943c7
[   36.428897] drm drm: [drm:__drm_atomic_state_free] Freeing atomic state 832943c7
[   36.429085] r8169 0000:01:00.0 eth0: Link is Down
[   36.713236] Disabling non-boot CPUs ...
-sh-5.1#
--- >8 ---

A second attempt soft-hangs:

--- >8 ---
-sh-5.1# rtcwake --device /dev/rtc1 --mode mem --seconds 5
rtcwake: assuming RTC uses UTC ...
rtcwake: wakeup from "mem" using /dev/rtc1 at Thu Jan  1 00:01:10 1970
--- >8 ---

Where "soft-hang" means it doesn't do anything after this and I can't
SIGINT out of it or anything. However, the serial seems to still be
responsive.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5553 bytes --]

On Mon, Jan 20, 2025 at 05:23:26PM +0100, Thierry Reding wrote:
> On Thu, Jan 16, 2025 at 10:41:08AM +0000, Jon Hunter wrote:
> > 
> > On 16/01/2025 10:38, John Ogness wrote:
> > > On 2025-01-16, Jon Hunter <jonathanh@nvidia.com> wrote:
> > > > > Do you at least know if it is failing to suspend or failing to resume
> > > > > (based on power consumption)?
> > > > 
> > > > 
> > > > Unfortunately, I don't. These are farm boards and so nothing local I can
> > > > get my hands on. For some reason all the serial console logs are not
> > > > available and so I am going to talk to the farm team about fixing that
> > > > because we should at least have serial logs.
> > > 
> > > Can you confirm that the board is actually booting? The suspend code for
> > > 8250_tegra.c is quite simple. I am wondering if the farm tests are
> > > failing somewhere else, such as the atomic printing during early boot.
> > 
> > 
> > Yes they are all booting fine. I have an independent boot test and that is
> > passing. It is just the suspend test that is failing.
> 
> I was able to capture logs, but unfortunately they don't provide much
> insight either. On the first try it doesn't suspend and goes back to
> userspace after a second or so:
> 
> --- >8 ---
> -sh-5.1# rtcwake --device /dev/rtc1 --mode mem --seconds 5
> rtcwake: assuming RTC uses UTC ...
> rtcwake: wakeup from "mem" using /dev/rtc1 at Thu Jan  1 00:01:00 1970
> [   36.332486] PM: suspend entry (deep)
> [   36.332832] Filesystems sync: 0.000 seconds
> [   36.369331] +1.8V_RUN_CAM: disabling
> [   36.373884] +2.8V_RUN_CAM: disabling
> [   36.375571] +1.2V_RUN_CAM_FRONT: disabling
> [   36.380359] +1.05V_RUN_CAM_REAR: disabling
> [   36.387399] +3.3V_RUN_TOUCH: disabling
> [   36.390808] +2.8V_RUN_CAM_AF: disabling
> [   36.393621] +1.8V_RUN_VPP_FUSE: disabling
> [   36.408218] Freezing user space processes
> [   36.413660] Freezing user space processes completed (elapsed 0.005 seconds)
> [   36.413680] OOM killer disabled.
> [   36.413693] Freezing remaining freezable tasks
> [   36.415033] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
> [   36.428474] drm drm: [drm:drm_client_dev_suspend] fbdev: ret=0
> [   36.428527] drm drm: [drm:drm_atomic_state_init] Allocated atomic state 2e5cd010
> [   36.428547] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:47:crtc-0] 6a6be0ef state to 2e5cd010
> [   36.428561] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:63:crtc-1] 00d818c2 state to 2e5cd010
> [   36.428574] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:32:plane-0] 4e145b7d state to 2e5cd010
> [   36.428587] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:36:plane-1] dbf67d12 state to 2e5cd010
> [   36.428597] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:40:plane-2] 763d8809 state to 2e5cd010
> [   36.428608] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:44:plane-3] b6eabcf1 state to 2e5cd010
> [   36.428617] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:48:plane-4] 7863878c state to 2e5cd010
> [   36.428628] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:52:plane-5] 54b8029c state to 2e5cd010
> [   36.428638] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:56:plane-6] 364063af state to 2e5cd010
> [   36.428648] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:60:plane-7] e1c11dfb state to 2e5cd010
> [   36.428662] drm drm: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:65:HDMI-A-1] 5cb32770 state to 2e5cd010
> [   36.428674] drm drm: [drm:drm_atomic_state_init] Allocated atomic state 832943c7
> [   36.428682] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:47:crtc-0] f09cf73d state to 832943c7
> [   36.428691] drm drm: [drm:drm_atomic_add_affected_planes] Adding all current planes for [CRTC:47:crtc-0] to 832943c7
> [   36.428700] drm drm: [drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:47:crtc-0] to 832943c7
> [   36.428711] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:63:crtc-1] 2700922c state to 832943c7
> [   36.428720] drm drm: [drm:drm_atomic_add_affected_planes] Adding all current planes for [CRTC:63:crtc-1] to 832943c7
> [   36.428727] drm drm: [drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:63:crtc-1] to 832943c7
> [   36.428737] drm drm: [drm:drm_atomic_check_only] checking 832943c7
> [   36.428759] drm drm: [drm:drm_atomic_commit] committing 832943c7
> [   36.428881] drm drm: [drm:drm_atomic_state_default_clear] Clearing atomic state 832943c7
> [   36.428897] drm drm: [drm:__drm_atomic_state_free] Freeing atomic state 832943c7
> [   36.429085] r8169 0000:01:00.0 eth0: Link is Down
> [   36.713236] Disabling non-boot CPUs ...
> -sh-5.1#
> --- >8 ---
> 
> A second attempt soft-hangs:
> 
> --- >8 ---
> -sh-5.1# rtcwake --device /dev/rtc1 --mode mem --seconds 5
> rtcwake: assuming RTC uses UTC ...
> rtcwake: wakeup from "mem" using /dev/rtc1 at Thu Jan  1 00:01:10 1970
> --- >8 ---
> 
> Where "soft-hang" means it doesn't do anything after this and I can't
> SIGINT out of it or anything. However, the serial seems to still be
> responsive.

To clarify, this was on top of next-20250120 and reverting the patches
that Jon mentioned suspend/resume is fixed for me as well.

I do have a local device that I can test on, so if there's any patches
you want me to try, or any options to enable to get more information,
please let me know.

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jon Hunter]

Hi John,

On 20/01/2025 16:34, Thierry Reding wrote:
> On Mon, Jan 20, 2025 at 05:23:26PM +0100, Thierry Reding wrote:
>> On Thu, Jan 16, 2025 at 10:41:08AM +0000, Jon Hunter wrote:
>>>
>>> On 16/01/2025 10:38, John Ogness wrote:
>>>> On 2025-01-16, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>>> Do you at least know if it is failing to suspend or failing to resume
>>>>>> (based on power consumption)?
>>>>>
>>>>>
>>>>> Unfortunately, I don't. These are farm boards and so nothing local I can
>>>>> get my hands on. For some reason all the serial console logs are not
>>>>> available and so I am going to talk to the farm team about fixing that
>>>>> because we should at least have serial logs.
>>>>
>>>> Can you confirm that the board is actually booting? The suspend code for
>>>> 8250_tegra.c is quite simple. I am wondering if the farm tests are
>>>> failing somewhere else, such as the atomic printing during early boot.
>>>
>>>
>>> Yes they are all booting fine. I have an independent boot test and that is
>>> passing. It is just the suspend test that is failing.
>>
>> I was able to capture logs, but unfortunately they don't provide much
>> insight either. On the first try it doesn't suspend and goes back to
>> userspace after a second or so:
>>
>> --- >8 ---
>> -sh-5.1# rtcwake --device /dev/rtc1 --mode mem --seconds 5
>> rtcwake: assuming RTC uses UTC ...
>> rtcwake: wakeup from "mem" using /dev/rtc1 at Thu Jan  1 00:01:00 1970
>> [   36.332486] PM: suspend entry (deep)
>> [   36.332832] Filesystems sync: 0.000 seconds
>> [   36.369331] +1.8V_RUN_CAM: disabling
>> [   36.373884] +2.8V_RUN_CAM: disabling
>> [   36.375571] +1.2V_RUN_CAM_FRONT: disabling
>> [   36.380359] +1.05V_RUN_CAM_REAR: disabling
>> [   36.387399] +3.3V_RUN_TOUCH: disabling
>> [   36.390808] +2.8V_RUN_CAM_AF: disabling
>> [   36.393621] +1.8V_RUN_VPP_FUSE: disabling
>> [   36.408218] Freezing user space processes
>> [   36.413660] Freezing user space processes completed (elapsed 0.005 seconds)
>> [   36.413680] OOM killer disabled.
>> [   36.413693] Freezing remaining freezable tasks
>> [   36.415033] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
>> [   36.428474] drm drm: [drm:drm_client_dev_suspend] fbdev: ret=0
>> [   36.428527] drm drm: [drm:drm_atomic_state_init] Allocated atomic state 2e5cd010
>> [   36.428547] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:47:crtc-0] 6a6be0ef state to 2e5cd010
>> [   36.428561] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:63:crtc-1] 00d818c2 state to 2e5cd010
>> [   36.428574] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:32:plane-0] 4e145b7d state to 2e5cd010
>> [   36.428587] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:36:plane-1] dbf67d12 state to 2e5cd010
>> [   36.428597] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:40:plane-2] 763d8809 state to 2e5cd010
>> [   36.428608] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:44:plane-3] b6eabcf1 state to 2e5cd010
>> [   36.428617] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:48:plane-4] 7863878c state to 2e5cd010
>> [   36.428628] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:52:plane-5] 54b8029c state to 2e5cd010
>> [   36.428638] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:56:plane-6] 364063af state to 2e5cd010
>> [   36.428648] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:60:plane-7] e1c11dfb state to 2e5cd010
>> [   36.428662] drm drm: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:65:HDMI-A-1] 5cb32770 state to 2e5cd010
>> [   36.428674] drm drm: [drm:drm_atomic_state_init] Allocated atomic state 832943c7
>> [   36.428682] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:47:crtc-0] f09cf73d state to 832943c7
>> [   36.428691] drm drm: [drm:drm_atomic_add_affected_planes] Adding all current planes for [CRTC:47:crtc-0] to 832943c7
>> [   36.428700] drm drm: [drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:47:crtc-0] to 832943c7
>> [   36.428711] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:63:crtc-1] 2700922c state to 832943c7
>> [   36.428720] drm drm: [drm:drm_atomic_add_affected_planes] Adding all current planes for [CRTC:63:crtc-1] to 832943c7
>> [   36.428727] drm drm: [drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:63:crtc-1] to 832943c7
>> [   36.428737] drm drm: [drm:drm_atomic_check_only] checking 832943c7
>> [   36.428759] drm drm: [drm:drm_atomic_commit] committing 832943c7
>> [   36.428881] drm drm: [drm:drm_atomic_state_default_clear] Clearing atomic state 832943c7
>> [   36.428897] drm drm: [drm:__drm_atomic_state_free] Freeing atomic state 832943c7
>> [   36.429085] r8169 0000:01:00.0 eth0: Link is Down
>> [   36.713236] Disabling non-boot CPUs ...
>> -sh-5.1#
>> --- >8 ---
>>
>> A second attempt soft-hangs:
>>
>> --- >8 ---
>> -sh-5.1# rtcwake --device /dev/rtc1 --mode mem --seconds 5
>> rtcwake: assuming RTC uses UTC ...
>> rtcwake: wakeup from "mem" using /dev/rtc1 at Thu Jan  1 00:01:10 1970
>> --- >8 ---
>>
>> Where "soft-hang" means it doesn't do anything after this and I can't
>> SIGINT out of it or anything. However, the serial seems to still be
>> responsive.
> 
> To clarify, this was on top of next-20250120 and reverting the patches
> that Jon mentioned suspend/resume is fixed for me as well.
> 
> I do have a local device that I can test on, so if there's any patches
> you want me to try, or any options to enable to get more information,
> please let me know.


Any feedback on this? Our boards are still broken with this change.

Thanks!
Jon

-- 
nvpublic

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

[Petr Mladek]

On Mon 2025-01-27 14:54:25, Jon Hunter wrote:
> Hi John,
> 
> On 20/01/2025 16:34, Thierry Reding wrote:
> > On Mon, Jan 20, 2025 at 05:23:26PM +0100, Thierry Reding wrote:
> > > On Thu, Jan 16, 2025 at 10:41:08AM +0000, Jon Hunter wrote:
> > > > 
> > > > On 16/01/2025 10:38, John Ogness wrote:
> > > > > On 2025-01-16, Jon Hunter <jonathanh@nvidia.com> wrote:
> > > > > > > Do you at least know if it is failing to suspend or failing to resume
> > > > > > > (based on power consumption)?
> > > > > > 
> > > > > > 
> > > > > > Unfortunately, I don't. These are farm boards and so nothing local I can
> > > > > > get my hands on. For some reason all the serial console logs are not
> > > > > > available and so I am going to talk to the farm team about fixing that
> > > > > > because we should at least have serial logs.
> > > > > 
> > > > > Can you confirm that the board is actually booting? The suspend code for
> > > > > 8250_tegra.c is quite simple. I am wondering if the farm tests are
> > > > > failing somewhere else, such as the atomic printing during early boot.
> > > > 
> > > > 
> > > > Yes they are all booting fine. I have an independent boot test and that is
> > > > passing. It is just the suspend test that is failing.
> > > 
> > > I was able to capture logs, but unfortunately they don't provide much
> > > insight either. On the first try it doesn't suspend and goes back to
> > > userspace after a second or so:
> > > 
> > > --- >8 ---
> > > -sh-5.1# rtcwake --device /dev/rtc1 --mode mem --seconds 5
> > > rtcwake: assuming RTC uses UTC ...
> > > rtcwake: wakeup from "mem" using /dev/rtc1 at Thu Jan  1 00:01:00 1970
> > > [   36.332486] PM: suspend entry (deep)
> > > [   36.332832] Filesystems sync: 0.000 seconds
> > > [   36.369331] +1.8V_RUN_CAM: disabling
> > > [   36.373884] +2.8V_RUN_CAM: disabling
> > > [   36.375571] +1.2V_RUN_CAM_FRONT: disabling
> > > [   36.380359] +1.05V_RUN_CAM_REAR: disabling
> > > [   36.387399] +3.3V_RUN_TOUCH: disabling
> > > [   36.390808] +2.8V_RUN_CAM_AF: disabling
> > > [   36.393621] +1.8V_RUN_VPP_FUSE: disabling
> > > [   36.408218] Freezing user space processes
> > > [   36.413660] Freezing user space processes completed (elapsed 0.005 seconds)
> > > [   36.413680] OOM killer disabled.
> > > [   36.413693] Freezing remaining freezable tasks
> > > [   36.415033] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
> > > [   36.428474] drm drm: [drm:drm_client_dev_suspend] fbdev: ret=0
> > > [   36.428527] drm drm: [drm:drm_atomic_state_init] Allocated atomic state 2e5cd010
> > > [   36.428547] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:47:crtc-0] 6a6be0ef state to 2e5cd010
> > > [   36.428561] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:63:crtc-1] 00d818c2 state to 2e5cd010
> > > [   36.428574] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:32:plane-0] 4e145b7d state to 2e5cd010
> > > [   36.428587] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:36:plane-1] dbf67d12 state to 2e5cd010
> > > [   36.428597] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:40:plane-2] 763d8809 state to 2e5cd010
> > > [   36.428608] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:44:plane-3] b6eabcf1 state to 2e5cd010
> > > [   36.428617] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:48:plane-4] 7863878c state to 2e5cd010
> > > [   36.428628] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:52:plane-5] 54b8029c state to 2e5cd010
> > > [   36.428638] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:56:plane-6] 364063af state to 2e5cd010
> > > [   36.428648] drm drm: [drm:drm_atomic_get_plane_state] Added [PLANE:60:plane-7] e1c11dfb state to 2e5cd010
> > > [   36.428662] drm drm: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:65:HDMI-A-1] 5cb32770 state to 2e5cd010
> > > [   36.428674] drm drm: [drm:drm_atomic_state_init] Allocated atomic state 832943c7
> > > [   36.428682] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:47:crtc-0] f09cf73d state to 832943c7
> > > [   36.428691] drm drm: [drm:drm_atomic_add_affected_planes] Adding all current planes for [CRTC:47:crtc-0] to 832943c7
> > > [   36.428700] drm drm: [drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:47:crtc-0] to 832943c7
> > > [   36.428711] drm drm: [drm:drm_atomic_get_crtc_state] Added [CRTC:63:crtc-1] 2700922c state to 832943c7
> > > [   36.428720] drm drm: [drm:drm_atomic_add_affected_planes] Adding all current planes for [CRTC:63:crtc-1] to 832943c7
> > > [   36.428727] drm drm: [drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:63:crtc-1] to 832943c7
> > > [   36.428737] drm drm: [drm:drm_atomic_check_only] checking 832943c7
> > > [   36.428759] drm drm: [drm:drm_atomic_commit] committing 832943c7
> > > [   36.428881] drm drm: [drm:drm_atomic_state_default_clear] Clearing atomic state 832943c7
> > > [   36.428897] drm drm: [drm:__drm_atomic_state_free] Freeing atomic state 832943c7
> > > [   36.429085] r8169 0000:01:00.0 eth0: Link is Down
> > > [   36.713236] Disabling non-boot CPUs ...
> > > -sh-5.1#
> > > --- >8 ---
> > > 
> > > A second attempt soft-hangs:
> > > 
> > > --- >8 ---
> > > -sh-5.1# rtcwake --device /dev/rtc1 --mode mem --seconds 5
> > > rtcwake: assuming RTC uses UTC ...
> > > rtcwake: wakeup from "mem" using /dev/rtc1 at Thu Jan  1 00:01:10 1970
> > > --- >8 ---
> > > 
> > > Where "soft-hang" means it doesn't do anything after this and I can't
> > > SIGINT out of it or anything. However, the serial seems to still be
> > > responsive.
> > 
> > To clarify, this was on top of next-20250120 and reverting the patches
> > that Jon mentioned suspend/resume is fixed for me as well.
> > 
> > I do have a local device that I can test on, so if there's any patches
> > you want me to try, or any options to enable to get more information,
> > please let me know.
> 
> 
> Any feedback on this? Our boards are still broken with this change.

AFAIK, this patch has been reverted in linux-next last week, see
https://lore.kernel.org/r/84wmenqm03.fsf@jogness.linutronix.de

John had a training. I believe that he would look at these
suspend-related problems before sending another revision
of the patchset.

Best Regards,
Petr

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

[John Ogness]

Hi Jon,

On 2025-01-27, Jon Hunter <jonathanh@nvidia.com> wrote:
> Any feedback on this? Our boards are still broken with this change.

I have not yet been able to reproduce it (mostly battling brokenness in
am335x pm dependencies). For now the change has been reverted in
linux-next. I will send you a patch once I have something to send.

John

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

[Jon Hunter]

On 27/01/2025 15:21, John Ogness wrote:
> Hi Jon,
> 
> On 2025-01-27, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Any feedback on this? Our boards are still broken with this change.
> 
> I have not yet been able to reproduce it (mostly battling brokenness in
> am335x pm dependencies). For now the change has been reverted in
> linux-next. I will send you a patch once I have something to send.


OK great and yes appears to be passing since next-20250123. I should 
have checked!

Thanks
Jon

-- 
nvpublic

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

[John Ogness]

Hi Jon,

On 2025-01-15, Jon Hunter <jonathanh@nvidia.com> wrote:
> I have noticed a suspend regression on -next for some of our 32-bit 
> Tegra (ARM) devices (Tegra20, Tegra30 and Tegra124). Bisect is pointing 
> to this commit and reverting this on top of -next (along with reverting 
> "serial: 8250: Revert "drop lockdep annotation from 
> serial8250_clear_IER()") fixes the issue. So far I have not dug in any 
> further. Unfortunately, I don't have any logs to see if there is some 
> crash or something happening but I will see if there is any more info I 
> can get.

I have been looking into reproducing this using other 8250/ARM boards
(BeagleBone Black and Phytec WEGA). Unfortunately it is just showing me
all kinds of other brokenness (in mainline) and essentially making it
impossible to confirm that I am seeing what you are seeing, since
suspend/resume is randomly dying without my 8250-nbcon patch.

Before I start spending weeks investigating/fixing most likely totally
unrelated PM or BSP issues, is it possible that I could receive one of
the boards you mentioned so that I can reproduce and debug the actual
problem you are reporting? If this is possible, feel free to take this
conversation offline so that we can discuss delivery details. Thanks!

John Ogness

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

[Jon Hunter]

Hi John,

On 08/10/2025 16:56, John Ogness wrote:
> Hi Jon,
> 
> On 2025-01-15, Jon Hunter <jonathanh@nvidia.com> wrote:
>> I have noticed a suspend regression on -next for some of our 32-bit
>> Tegra (ARM) devices (Tegra20, Tegra30 and Tegra124). Bisect is pointing
>> to this commit and reverting this on top of -next (along with reverting
>> "serial: 8250: Revert "drop lockdep annotation from
>> serial8250_clear_IER()") fixes the issue. So far I have not dug in any
>> further. Unfortunately, I don't have any logs to see if there is some
>> crash or something happening but I will see if there is any more info I
>> can get.
> 
> I have been looking into reproducing this using other 8250/ARM boards
> (BeagleBone Black and Phytec WEGA). Unfortunately it is just showing me
> all kinds of other brokenness (in mainline) and essentially making it
> impossible to confirm that I am seeing what you are seeing, since
> suspend/resume is randomly dying without my 8250-nbcon patch.
> 
> Before I start spending weeks investigating/fixing most likely totally
> unrelated PM or BSP issues, is it possible that I could receive one of
> the boards you mentioned so that I can reproduce and debug the actual
> problem you are reporting? If this is possible, feel free to take this
> conversation offline so that we can discuss delivery details. Thanks!

These boards are really old now and so I don't really have any that we 
can ship. It would be great to get this change merged as I see that it 
is needed for RT support. I could see if I can resurrect a Tegra124 
Jetson TK1 and test again on that to see if we can get some logs.

Thierry, do you have a Tegra124 Jetson TK1 handy to test this change on?

Jon

-- 
nvpublic

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2631 bytes --]

On Wed, Oct 08, 2025 at 08:21:49PM +0100, Jon Hunter wrote:
> Hi John,
> 
> On 08/10/2025 16:56, John Ogness wrote:
> > Hi Jon,
> > 
> > On 2025-01-15, Jon Hunter <jonathanh@nvidia.com> wrote:
> > > I have noticed a suspend regression on -next for some of our 32-bit
> > > Tegra (ARM) devices (Tegra20, Tegra30 and Tegra124). Bisect is pointing
> > > to this commit and reverting this on top of -next (along with reverting
> > > "serial: 8250: Revert "drop lockdep annotation from
> > > serial8250_clear_IER()") fixes the issue. So far I have not dug in any
> > > further. Unfortunately, I don't have any logs to see if there is some
> > > crash or something happening but I will see if there is any more info I
> > > can get.
> > 
> > I have been looking into reproducing this using other 8250/ARM boards
> > (BeagleBone Black and Phytec WEGA). Unfortunately it is just showing me
> > all kinds of other brokenness (in mainline) and essentially making it
> > impossible to confirm that I am seeing what you are seeing, since
> > suspend/resume is randomly dying without my 8250-nbcon patch.
> > 
> > Before I start spending weeks investigating/fixing most likely totally
> > unrelated PM or BSP issues, is it possible that I could receive one of
> > the boards you mentioned so that I can reproduce and debug the actual
> > problem you are reporting? If this is possible, feel free to take this
> > conversation offline so that we can discuss delivery details. Thanks!
> 
> These boards are really old now and so I don't really have any that we can
> ship. It would be great to get this change merged as I see that it is needed
> for RT support. I could see if I can resurrect a Tegra124 Jetson TK1 and
> test again on that to see if we can get some logs.
> 
> Thierry, do you have a Tegra124 Jetson TK1 handy to test this change on?

Yes, I do. I reapplied patches 5 and 6 from the series (resolved a tiny
conflict for patch 5) and reran the tests. Same results as back in
January, though. Basically the first suspend doesn't work (it exits back
to userspace after a few seconds) and the second attempt then hangs. No
idea why that would be happpening.

I looked a bit at the code, but nothing jumped out that would explain
this. Not that I'm very familiar with any of this code, or the specifics
needed by nbcon. no_console_suspend doesn't have any noticeable effect,
other than providing a few more messages during suspend, but nothing
that would indicate what's going wrong.

John, I'm happy to test any other patches if you've got any ideas on
what could be wrong.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[John Ogness]

Hi Thierry,

On 2025-10-09, Thierry Reding <thierry.reding@gmail.com> wrote:
>> Thierry, do you have a Tegra124 Jetson TK1 handy to test this change on?
>
> Yes, I do. I reapplied patches 5 and 6 from the series (resolved a tiny
> conflict for patch 5) and reran the tests. Same results as back in
> January, though. Basically the first suspend doesn't work (it exits back
> to userspace after a few seconds) and the second attempt then hangs. No
> idea why that would be happpening.

[...]

> John, I'm happy to test any other patches if you've got any ideas on
> what could be wrong.

Thanks for your support. I created a branch on a public git repository
[0] so that we have a common source to work with. I will push additional
debug-commits on top.

I am taking this email-debugging-session offlist. I will post again to
this thread once we come to some conclusion.

John

[0] https://github.com/Linutronix/linux (branch nvidia/debug-8250-nbcon)

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

[Petr Mladek]

On Thu 2025-10-09 12:04:13, Thierry Reding wrote:
> On Wed, Oct 08, 2025 at 08:21:49PM +0100, Jon Hunter wrote:
> > Hi John,
> > 
> > On 08/10/2025 16:56, John Ogness wrote:
> > > Hi Jon,
> > > 
> > > On 2025-01-15, Jon Hunter <jonathanh@nvidia.com> wrote:
> > > > I have noticed a suspend regression on -next for some of our 32-bit
> > > > Tegra (ARM) devices (Tegra20, Tegra30 and Tegra124). Bisect is pointing
> > > > to this commit and reverting this on top of -next (along with reverting
> > > > "serial: 8250: Revert "drop lockdep annotation from
> > > > serial8250_clear_IER()") fixes the issue. So far I have not dug in any
> > > > further. Unfortunately, I don't have any logs to see if there is some
> > > > crash or something happening but I will see if there is any more info I
> > > > can get.
> > > 
> > > I have been looking into reproducing this using other 8250/ARM boards
> > > (BeagleBone Black and Phytec WEGA). Unfortunately it is just showing me
> > > all kinds of other brokenness (in mainline) and essentially making it
> > > impossible to confirm that I am seeing what you are seeing, since
> > > suspend/resume is randomly dying without my 8250-nbcon patch.
> > > 
> > > Before I start spending weeks investigating/fixing most likely totally
> > > unrelated PM or BSP issues, is it possible that I could receive one of
> > > the boards you mentioned so that I can reproduce and debug the actual
> > > problem you are reporting? If this is possible, feel free to take this
> > > conversation offline so that we can discuss delivery details. Thanks!
> > 
> > These boards are really old now and so I don't really have any that we can
> > ship. It would be great to get this change merged as I see that it is needed
> > for RT support. I could see if I can resurrect a Tegra124 Jetson TK1 and
> > test again on that to see if we can get some logs.
> > 
> > Thierry, do you have a Tegra124 Jetson TK1 handy to test this change on?
> 
> Yes, I do. I reapplied patches 5 and 6 from the series (resolved a tiny
> conflict for patch 5) and reran the tests. Same results as back in
> January, though. Basically the first suspend doesn't work (it exits back
> to userspace after a few seconds)

I remember a mail from Jacky Bai (added into Cc, the mail was off-list).
It pointed out that that ARM-specific suspend code checks whether
there are pending interrupts and eventually cancels the suspend,
see
https://github.com/ARM-software/arm-trusted-firmware/blob/f831058437f281e70c2409a9b79828116d4c2915/lib/psci/psci_suspend.c#L154

It might explain this first failure after a timeout.

A possible solution would be to avoid waking consoles in vprintk_emit()
when they are suspended. The functions nbcon_kthreads_wake(),
defer_console_output(), and wake_up_klogd() queue irq_work()
which could not get proceed when interrupts are disabled, ...


> and the second attempt then hangs. No
> idea why that would be happpening.

No possible explanation comes to my mind :-(

> I looked a bit at the code, but nothing jumped out that would explain
> this. Not that I'm very familiar with any of this code, or the specifics
> needed by nbcon. no_console_suspend doesn't have any noticeable effect,
> other than providing a few more messages during suspend, but nothing
> that would indicate what's going wrong.
> 
> John, I'm happy to test any other patches if you've got any ideas on
> what could be wrong.

Best Regards,
Petr

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

[John Ogness]

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

This reverts commit 3d9e6f556e235ddcdc9f73600fdd46fe1736b090.

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

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

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

[Petr Mladek]

On Tue 2025-01-07 22:33:02, 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.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

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

Best Regards,
Petr