[PATCH tty-next v1 0/2] convert 8250 to nbcon

[PATCH tty-next v1 0/2] convert 8250 to nbcon

[John Ogness]

The recent printk rework introduced a new type of console NBCON
that will perform printing via a dedicated kthread during
normal operation. For times when the kthread is not available
(early boot, panic, reboot/shutdown) the NBCON console will
print directly from the printk() calling context (even if from
NMI).

Futher details about NBCON consoles are available here [0].

This series converts the 8250 driver to an NBCON console,
providing both threaded and atomic printing implementations.
Users can verify the UART console is an NBCON console via the
proc filesystem. For example:

$  cat /proc/consoles
ttyS0                -W- (EC N  a)    4:64

The 'N' shows that it is an NBCON console.

There will also be a dedicated printing kthread. For example:

$ ps ax | grep pr/
   16 root       0:00 [pr/ttyS0]

Derek Barbosa performed extensive tests [1] using this driver
and encountered no issues. On the contrary, his tests showed
the improved reliability and non-interference features of the
NBCON-based driver.

Since this is the first console driver to be converted to an
NBCON console, it may include variables and functions that
could be abstracted to all UART consoles (such as the
@console_newline_needed field). However, we can abstract such
things later as more consoles are converted to NBCON.

John Ogness

[0] https://lore.kernel.org/lkml/20230302195618.156940-1-john.ogness@linutronix.de
[1] https://lore.kernel.org/lkml/ZsdoD6PomBRsB-ow@debarbos-thinkpadt14sgen2i.remote.csb

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

 drivers/tty/serial/8250/8250_core.c |  42 +++++++-
 drivers/tty/serial/8250/8250_port.c | 151 +++++++++++++++++++++++++++-
 include/linux/serial_8250.h         |   6 ++
 3 files changed, 196 insertions(+), 3 deletions(-)


base-commit: f1ec92a066b2608e7c971dfce28ebe2d2cdb056e
-- 
2.39.2

[PATCH next v1 1/2] 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.

The legacy code is kept in order to easily switch back to legacy mode
by defining USE_SERIAL_8250_LEGACY_CONSOLE.

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

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 29e4b83e0376..d7079931dd7e 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -388,6 +388,7 @@ void __init serial8250_register_ports(struct uart_driver *drv, struct device *de
 
 #ifdef CONFIG_SERIAL_8250_CONSOLE
 
+#ifdef USE_SERIAL_8250_LEGACY_CONSOLE
 static void univ8250_console_write(struct console *co, const char *s,
 				   unsigned int count)
 {
@@ -395,6 +396,37 @@ static void univ8250_console_write(struct console *co, const char *s,
 
 	serial8250_console_write(up, s, count);
 }
+#else
+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_atomic(up, wctxt);
+}
+
+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_thread(up, wctxt);
+}
+
+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);
+}
+#endif /* USE_SERIAL_8250_LEGACY_CONSOLE */
 
 static int univ8250_console_setup(struct console *co, char *options)
 {
@@ -494,12 +526,20 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
 
 static struct console univ8250_console = {
 	.name		= "ttyS",
+#ifdef USE_SERIAL_8250_LEGACY_CONSOLE
 	.write		= univ8250_console_write,
+	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
+#else
+	.write_atomic	= univ8250_console_write_atomic,
+	.write_thread	= univ8250_console_write_thread,
+	.device_lock	= univ8250_console_device_lock,
+	.device_unlock	= univ8250_console_device_unlock,
+	.flags		= CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON,
+#endif
 	.device		= uart_console_device,
 	.setup		= univ8250_console_setup,
 	.exit		= univ8250_console_exit,
 	.match		= univ8250_console_match,
-	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
 	.index		= -1,
 	.data		= &serial8250_reg,
 };
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3509af7dc52b..ce841c62900d 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -546,6 +546,13 @@ static int serial8250_em485_init(struct uart_8250_port *p)
 	if (!p->em485)
 		return -ENOMEM;
 
+#ifndef USE_SERIAL_8250_LEGACY_CONSOLE
+	if (uart_console(&p->port)) {
+		dev_warn(p->port.dev, "no atomic printing for rs485 consoles\n");
+		p->port.cons->write_atomic = NULL;
+	}
+#endif
+
 	hrtimer_init(&p->em485->stop_tx_timer, CLOCK_MONOTONIC,
 		     HRTIMER_MODE_REL);
 	hrtimer_init(&p->em485->start_tx_timer, CLOCK_MONOTONIC,
@@ -691,7 +698,11 @@ 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 by write_atomic() and the legacy write(), which do not
+ * require port lock.
+ */
+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 +710,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.
@@ -3269,6 +3285,11 @@ static void serial8250_console_putchar(struct uart_port *port, unsigned char ch)
 
 	wait_for_xmitr(up, UART_LSR_THRE);
 	serial_port_out(port, UART_TX, ch);
+
+	if (ch == '\n')
+		up->console_newline_needed = false;
+	else
+		up->console_newline_needed = true;
 }
 
 /*
@@ -3297,6 +3318,7 @@ static void serial8250_console_restore(struct uart_8250_port *up)
 	serial8250_out_MCR(up, up->mcr | UART_MCR_DTR | UART_MCR_RTS);
 }
 
+#ifdef USE_SERIAL_8250_LEGACY_CONSOLE
 /*
  * Print a string to the serial port using the device FIFO
  *
@@ -3355,7 +3377,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 	 *	First save the IER then disable the interrupts
 	 */
 	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))) {
@@ -3421,6 +3443,125 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 	if (locked)
 		uart_port_unlock_irqrestore(port, flags);
 }
+#else
+void serial8250_console_write_thread(struct uart_8250_port *up,
+				     struct nbcon_write_context *wctxt)
+{
+	struct uart_8250_em485 *em485 = up->em485;
+	struct uart_port *port = &up->port;
+	unsigned int ier;
+
+	touch_nmi_watchdog();
+
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
+
+	/* First save IER then disable the interrupts. */
+	ier = serial_port_in(port, UART_IER);
+	serial8250_clear_IER(up);
+
+	/* Check scratch reg if port powered off during system sleep. */
+	if (up->canary && (up->canary != serial_port_in(port, UART_SCR))) {
+		serial8250_console_restore(up);
+		up->canary = 0;
+	}
+
+	if (em485) {
+		if (em485->tx_stopped)
+			up->rs485_start_tx(up);
+		mdelay(port->rs485.delay_rts_before_send);
+	}
+
+	if (nbcon_exit_unsafe(wctxt)) {
+		int len = READ_ONCE(wctxt->len);
+		int i;
+
+		/*
+		 * 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.
+		 */
+		for (i = 0; i < len; i++) {
+			if (!nbcon_enter_unsafe(wctxt))
+				break;
+
+			uart_console_write(port, wctxt->outbuf + i, 1, serial8250_console_putchar);
+
+			if (!nbcon_exit_unsafe(wctxt))
+				break;
+		}
+	}
+
+	/*
+	 * 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 and restore IER. */
+	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
+	if (em485) {
+		mdelay(port->rs485.delay_rts_after_send);
+		if (em485->tx_stopped)
+			up->rs485_stop_tx(up);
+	}
+	serial_port_out(port, UART_IER, ier);
+
+	/*
+	 * The receive handling will happen properly because the receive ready
+	 * bit will still be set; it is not cleared on read.  However, modem
+	 * control will not, we must 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);
+
+	nbcon_exit_unsafe(wctxt);
+}
+
+void serial8250_console_write_atomic(struct uart_8250_port *up,
+				     struct nbcon_write_context *wctxt)
+{
+	struct uart_port *port = &up->port;
+	unsigned int ier;
+
+	/* Atomic console not supported for rs485 mode. */
+	if (WARN_ON_ONCE(up->em485))
+		return;
+
+	touch_nmi_watchdog();
+
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
+
+	/*
+	 * First save IER then disable the interrupts. The special variant to
+	 * clear IER is used because atomic printing may occur without holding
+	 * the port lock.
+	 */
+	ier = serial_port_in(port, UART_IER);
+	__serial8250_clear_IER(up);
+
+	/* Check scratch reg if port powered off during system sleep. */
+	if (up->canary && (up->canary != serial_port_in(port, UART_SCR))) {
+		serial8250_console_restore(up);
+		up->canary = 0;
+	}
+
+	if (up->console_newline_needed)
+		uart_console_write(port, "\n", 1, serial8250_console_putchar);
+	uart_console_write(port, wctxt->outbuf, wctxt->len, serial8250_console_putchar);
+
+	/* Finally, wait for transmitter to become empty and restore IER. */
+	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
+	serial_port_out(port, UART_IER, ier);
+
+	nbcon_exit_unsafe(wctxt);
+}
+#endif /* USE_SERIAL_8250_LEGACY_CONSOLE */
 
 static unsigned int probe_baud(struct uart_port *port)
 {
@@ -3439,6 +3580,7 @@ static unsigned int probe_baud(struct uart_port *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';
@@ -3448,6 +3590,8 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 	if (!port->iobase && !port->membase)
 		return -ENODEV;
 
+	up->console_newline_needed = false;
+
 	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 e0717c8393d7..a968e6941237 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -153,6 +153,8 @@ struct uart_8250_port {
 #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
 	unsigned char		msr_saved_flags;
 
+	bool			console_newline_needed;
+
 	struct uart_8250_dma	*dma;
 	const struct uart_8250_ops *ops;
 
@@ -204,6 +206,10 @@ 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_atomic(struct uart_8250_port *up,
+				     struct nbcon_write_context *wctxt);
+void serial8250_console_write_thread(struct uart_8250_port *up,
+				     struct nbcon_write_context *wctxt);
 int serial8250_console_setup(struct uart_port *port, char *options, bool probe);
 int serial8250_console_exit(struct uart_port *port);
 
-- 
2.39.2

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

[John Ogness]

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

This reverts commit 3d9e6f556e235ddcdc9f73600fdd46fe1736b090.

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

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ce841c62900d..1b4524f6d9b9 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -712,6 +712,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.2

Re: [PATCH tty-next v1 0/2] convert 8250 to nbcon

[Andy Shevchenko]

On Thu, Sep 05, 2024 at 03:53:17PM +0206, John Ogness wrote:
> The recent printk rework introduced a new type of console NBCON
> that will perform printing via a dedicated kthread during
> normal operation. For times when the kthread is not available
> (early boot, panic, reboot/shutdown) the NBCON console will
> print directly from the printk() calling context (even if from
> NMI).
> 
> Futher details about NBCON consoles are available here [0].
> 
> This series converts the 8250 driver to an NBCON console,
> providing both threaded and atomic printing implementations.
> Users can verify the UART console is an NBCON console via the
> proc filesystem. For example:
> 
> $  cat /proc/consoles
> ttyS0                -W- (EC N  a)    4:64
> 
> The 'N' shows that it is an NBCON console.
> 
> There will also be a dedicated printing kthread. For example:
> 
> $ ps ax | grep pr/
>    16 root       0:00 [pr/ttyS0]

Wondering if this can use the DEVNAME instead of opaque (to some extent) ttySx?
Or is it using the same what has been passed to the console= kernel command line
parameter?

> Derek Barbosa performed extensive tests [1] using this driver
> and encountered no issues. On the contrary, his tests showed
> the improved reliability and non-interference features of the
> NBCON-based driver.
> 
> Since this is the first console driver to be converted to an
> NBCON console, it may include variables and functions that
> could be abstracted to all UART consoles (such as the
> @console_newline_needed field). However, we can abstract such
> things later as more consoles are converted to NBCON.
> 
> John Ogness
> 
> [0] https://lore.kernel.org/lkml/20230302195618.156940-1-john.ogness@linutronix.de
> [1] https://lore.kernel.org/lkml/ZsdoD6PomBRsB-ow@debarbos-thinkpadt14sgen2i.remote.csb

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v1 0/2] convert 8250 to nbcon

[John Ogness]

On 2024-09-05, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> $  cat /proc/consoles
>> ttyS0                -W- (EC N  a)    4:64
>> 
>> The 'N' shows that it is an NBCON console.
>> 
>> There will also be a dedicated printing kthread. For example:
>> 
>> $ ps ax | grep pr/
>>    16 root       0:00 [pr/ttyS0]
>
> Wondering if this can use the DEVNAME instead of opaque (to some extent) ttySx?
> Or is it using the same what has been passed to the console= kernel command line
> parameter?

fs/proc/consoles.c:show_console_dev() is just using:

"%s%d", con->name, con->index

The same goes for drivers/tty/tty_io.c:show_cons_active(), which is
responsible for the contents of:

/sys/devices/virtual/tty/console/active

John Ogness

Re: [PATCH tty-next v1 0/2] convert 8250 to nbcon

[Greg Kroah-Hartman]

On Thu, Sep 05, 2024 at 03:53:17PM +0206, John Ogness wrote:
> The recent printk rework introduced a new type of console NBCON
> that will perform printing via a dedicated kthread during
> normal operation. For times when the kthread is not available
> (early boot, panic, reboot/shutdown) the NBCON console will
> print directly from the printk() calling context (even if from
> NMI).
> 
> Futher details about NBCON consoles are available here [0].

Really?  That link calls them "NOBKL", is that the same thing?

confused,

greg k-h

Re: [PATCH tty-next v1 0/2] convert 8250 to nbcon

[John Ogness]

On 2024-09-05, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Thu, Sep 05, 2024 at 03:53:17PM +0206, John Ogness wrote:
>> The recent printk rework introduced a new type of console NBCON
>> that will perform printing via a dedicated kthread during
>> normal operation. For times when the kthread is not available
>> (early boot, panic, reboot/shutdown) the NBCON console will
>> print directly from the printk() calling context (even if from
>> NMI).
>> 
>> Futher details about NBCON consoles are available here [0].
>
> Really?  That link calls them "NOBKL", is that the same thing?

Sorry. Yes, they are the same thing. It was renamed because NOBKL did
not look nice.

NBCON stands for "No BKL Console".

John

Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[Andy Shevchenko]

On Thu, Sep 05, 2024 at 03:53:18PM +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.
> 
> The legacy code is kept in order to easily switch back to legacy mode
> by defining USE_SERIAL_8250_LEGACY_CONSOLE.

...

>  static struct console univ8250_console = {
>  	.name		= "ttyS",
> +#ifdef USE_SERIAL_8250_LEGACY_CONSOLE

Can it be done at run-time (theoretically or even practically)?
(Note that we have already knob to disable / enable consoles.)

>  	.write		= univ8250_console_write,
> +	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
> +#else
> +	.write_atomic	= univ8250_console_write_atomic,
> +	.write_thread	= univ8250_console_write_thread,
> +	.device_lock	= univ8250_console_device_lock,
> +	.device_unlock	= univ8250_console_device_unlock,
> +	.flags		= CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON,
> +#endif
>  	.device		= uart_console_device,
>  	.setup		= univ8250_console_setup,
>  	.exit		= univ8250_console_exit,
>  	.match		= univ8250_console_match,
> -	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
>  	.index		= -1,
>  	.data		= &serial8250_reg,
>  };

I would arrange this slightly differently, but not a big deal.

static struct console univ8250_console = {
	.name		= "ttyS",
	.device		= uart_console_device,
#ifndef USE_SERIAL_8250_LEGACY_CONSOLE
	.flags		= CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON,
	.write_atomic	= univ8250_console_write_atomic,
	.write_thread	= univ8250_console_write_thread,
	.device_lock	= univ8250_console_device_lock,
	.device_unlock	= univ8250_console_device_unlock,
#else
	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
	.write		= univ8250_console_write,
#endif
	.setup		= univ8250_console_setup,
	.exit		= univ8250_console_exit,
	.match		= univ8250_console_match,
	.index		= -1,
	.data		= &serial8250_reg,
};

...

> +	if (nbcon_exit_unsafe(wctxt)) {
> +		int len = READ_ONCE(wctxt->len);

> +		int i;

unsigned ?

> +		/*
> +		 * 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.
> +		 */
> +		for (i = 0; i < len; i++) {
> +			if (!nbcon_enter_unsafe(wctxt))
> +				break;
> +
> +			uart_console_write(port, wctxt->outbuf + i, 1, serial8250_console_putchar);
> +
> +			if (!nbcon_exit_unsafe(wctxt))
> +				break;
> +		}
> +	}

...

> +	/* Finally, wait for transmitter to become empty and restore IER. */
> +	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
> +	if (em485) {
> +		mdelay(port->rs485.delay_rts_after_send);
> +		if (em485->tx_stopped)
> +			up->rs485_stop_tx(up);
> +	}
> +	serial_port_out(port, UART_IER, ier);
> +
> +	/*
> +	 * The receive handling will happen properly because the receive ready
> +	 * bit will still be set; it is not cleared on read.  However, modem
> +	 * control will not, we must 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);

(1)

...

> +	/* Atomic console not supported for rs485 mode. */

RS485

...

> +	/*
> +	 * First save IER then disable the interrupts. The special variant to
> +	 * clear IER is used because atomic printing may occur without holding
> +	 * the port lock.
> +	 */
> +	ier = serial_port_in(port, UART_IER);
> +	__serial8250_clear_IER(up);
> +
> +	/* Check scratch reg if port powered off during system sleep. */
> +	if (up->canary && (up->canary != serial_port_in(port, UART_SCR))) {
> +		serial8250_console_restore(up);
> +		up->canary = 0;
> +	}
> +
> +	if (up->console_newline_needed)
> +		uart_console_write(port, "\n", 1, serial8250_console_putchar);
> +	uart_console_write(port, wctxt->outbuf, wctxt->len, serial8250_console_putchar);
> +
> +	/* Finally, wait for transmitter to become empty and restore IER. */
> +	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
> +	serial_port_out(port, UART_IER, ier);

(2)

Feels like parts (1) and (2) duplicates existing pieces of code. May it be
refactored to minimize the duplication?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v1 0/2] convert 8250 to nbcon

[Andy Shevchenko]

On Thu, Sep 05, 2024 at 04:18:31PM +0206, John Ogness wrote:
> On 2024-09-05, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > On Thu, Sep 05, 2024 at 03:53:17PM +0206, John Ogness wrote:
> >> The recent printk rework introduced a new type of console NBCON
> >> that will perform printing via a dedicated kthread during
> >> normal operation. For times when the kthread is not available
> >> (early boot, panic, reboot/shutdown) the NBCON console will
> >> print directly from the printk() calling context (even if from
> >> NMI).
> >> 
> >> Futher details about NBCON consoles are available here [0].
> >
> > Really?  That link calls them "NOBKL", is that the same thing?
> 
> Sorry. Yes, they are the same thing. It was renamed because NOBKL did
> not look nice.
> 
> NBCON stands for "No BKL Console".

New Brave Console :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[John Ogness]

On 2024-09-05, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> On Thu, Sep 05, 2024 at 03:53:18PM +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.
>> 
>> The legacy code is kept in order to easily switch back to legacy mode
>> by defining USE_SERIAL_8250_LEGACY_CONSOLE.
>
> ...
>
>>  static struct console univ8250_console = {
>>  	.name		= "ttyS",
>> +#ifdef USE_SERIAL_8250_LEGACY_CONSOLE
>
> Can it be done at run-time (theoretically or even practically)?
> (Note that we have already knob to disable / enable consoles.)

We don't want to maintain the legacy variant and really people should
not be using it either. NBCON is the way forward for all console
drivers.

I will just remove it for v2. If someone wants to use the old code, they
will need to revert the patch.


>> +	if (nbcon_exit_unsafe(wctxt)) {
>> +		int len = READ_ONCE(wctxt->len);
>
>> +		int i;
>
> unsigned ?

ACK.

>> +	/* Atomic console not supported for rs485 mode. */
>
> RS485

ACK.

> Feels like parts (1) and (2) duplicates existing pieces of code. May it be
> refactored to minimize the duplication?

When I remove the unused legacy code, the duplication
disappears. write_thread() and write_atomic() have very little in
common.

John Ogness

Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[Andy Shevchenko]

On Thu, Sep 05, 2024 at 09:29:06PM +0206, John Ogness wrote:
> On 2024-09-05, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Sep 05, 2024 at 03:53:18PM +0206, John Ogness wrote:

...

> >> +#ifdef USE_SERIAL_8250_LEGACY_CONSOLE
> >
> > Can it be done at run-time (theoretically or even practically)?
> > (Note that we have already knob to disable / enable consoles.)
> 
> We don't want to maintain the legacy variant and really people should
> not be using it either. NBCON is the way forward for all console
> drivers.
> 
> I will just remove it for v2. If someone wants to use the old code, they
> will need to revert the patch.

This is even better!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[Greg Kroah-Hartman]

On Thu, Sep 05, 2024 at 03:53:18PM +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.
> 
> The legacy code is kept in order to easily switch back to legacy mode
> by defining USE_SERIAL_8250_LEGACY_CONSOLE.

define it where?

And ick, having #ifdef like this is rough to maintain, why is it needed?
If this is working well, let's just switch over to the new stuff and not
look back!

thanks,

greg k-h

Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[Petr Mladek]

On Thu 2024-09-05 15:53:18, John Ogness wrote:
> Implement the necessary callbacks to switch the 8250 console driver
> to perform as an nbcon console.
> 
> Add implementations for the nbcon console callbacks (write_atomic,
> write_thread, device_lock, device_unlock) and add CON_NBCON to the
> initial flags.
> 
> The legacy code is kept in order to easily switch back to legacy mode
> by defining USE_SERIAL_8250_LEGACY_CONSOLE.
> 
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -388,6 +388,7 @@ void __init serial8250_register_ports(struct uart_driver *drv, struct device *de
>  
>  #ifdef CONFIG_SERIAL_8250_CONSOLE
>  
> +#ifdef USE_SERIAL_8250_LEGACY_CONSOLE

Just for record. I agree that it is better to simply remove the
obsolete legacy code.

Or maybe, we would need to keep it for the rs485 consoles, see below.

>  static void univ8250_console_write(struct console *co, const char *s,
>  				   unsigned int count)
>  {
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -546,6 +546,13 @@ static int serial8250_em485_init(struct uart_8250_port *p)
>  	if (!p->em485)
>  		return -ENOMEM;
>  
> +#ifndef USE_SERIAL_8250_LEGACY_CONSOLE
> +	if (uart_console(&p->port)) {
> +		dev_warn(p->port.dev, "no atomic printing for rs485 consoles\n");
> +		p->port.cons->write_atomic = NULL;
> +	}

Wait! This makes the rs485 consoles much less usable for debugging.
They might have troubles to see the emergency and panic messages.
Or do I miss anything, please?

Is this acceptable? Why?
Why is this limitation exactly needed?

Is is because the following code is not safe enough for the write_atomic
variant when it is guarded only by the nbcon context ownership?

void serial8250_console_write_thread(struct uart_8250_port *up,
				     struct nbcon_write_context *wctxt)
{
[...]
	if (em485) {
		if (em485->tx_stopped)
			up->rs485_start_tx(up);
		mdelay(port->rs485.delay_rts_before_send);
	}
[...]
	if (em485) {
		mdelay(port->rs485.delay_rts_after_send);
		if (em485->tx_stopped)
			up->rs485_stop_tx(up);
	}
[...]

Would it break even the nbcon console context it taken over the safe
way? Or only by "unsafe" takeover?

IMHO, we should risk the "unsafe" takeover. We still would be
in a better situation than the legacy code which ignores
the port->lock during panic() all the time (after bust_

> +#endif
> +
>  	hrtimer_init(&p->em485->stop_tx_timer, CLOCK_MONOTONIC,
>  		     HRTIMER_MODE_REL);
>  	hrtimer_init(&p->em485->start_tx_timer, CLOCK_MONOTONIC,
> @@ -3269,6 +3285,11 @@ static void serial8250_console_putchar(struct uart_port *port, unsigned char ch)
>  
>  	wait_for_xmitr(up, UART_LSR_THRE);
>  	serial_port_out(port, UART_TX, ch);
> +
> +	if (ch == '\n')
> +		up->console_newline_needed = false;
> +	else
> +		up->console_newline_needed = true;

I might be just dumb but this code confused me. I missed that the
variable was actually set after printing the character. I inverted
the logic in my head and it did not make sense.

I vote for adding a comment. Or better make the code more
straightforward by renaming the variable and inverting the logic:

	if (ch == '\n')
		up->console_line_ended = true;
	else
		up->console_line_ended = false;

>  }
>  
>  /*
> @@ -3421,6 +3443,125 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>  	if (locked)
>  		uart_port_unlock_irqrestore(port, flags);
>  }
> +#else
> +void serial8250_console_write_thread(struct uart_8250_port *up,
> +				     struct nbcon_write_context *wctxt)
> +{
> +	struct uart_8250_em485 *em485 = up->em485;
> +	struct uart_port *port = &up->port;
> +	unsigned int ier;
> +
> +	touch_nmi_watchdog();

This should not be needed in the write_thread() variant because
it allows to schedule after emitting one record.

> +	if (!nbcon_enter_unsafe(wctxt))
> +		return;
> +

The rest looks good.

Best Regards,
Petr

Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[John Ogness]

On 2024-09-06, Petr Mladek <pmladek@suse.com> wrote:
>> +#ifdef USE_SERIAL_8250_LEGACY_CONSOLE
>
> Just for record. I agree that it is better to simply remove the
> obsolete legacy code.

Agreed. I will be removing it for v2.

>> +#ifndef USE_SERIAL_8250_LEGACY_CONSOLE
>> +	if (uart_console(&p->port)) {
>> +		dev_warn(p->port.dev, "no atomic printing for rs485 consoles\n");
>> +		p->port.cons->write_atomic = NULL;
>> +	}
>
> Wait! This makes the rs485 consoles much less usable for debugging.
> They might have troubles to see the emergency and panic messages.
> Or do I miss anything, please?
>
> Is this acceptable? Why?

It is not acceptable. I am looking into making the atomic part work for
RS485 as well. My main problem is testing since I will need to get my
hands or real RS485 hardware.

>>  	wait_for_xmitr(up, UART_LSR_THRE);
>>  	serial_port_out(port, UART_TX, ch);
>> +
>> +	if (ch == '\n')
>> +		up->console_newline_needed = false;
>> +	else
>> +		up->console_newline_needed = true;
>
> I might be just dumb but this code confused me. I missed that the
> variable was actually set after printing the character. I inverted
> the logic in my head and it did not make sense.
>
> I vote for adding a comment. Or better make the code more
> straightforward by renaming the variable and inverting the logic:
>
> 	if (ch == '\n')
> 		up->console_line_ended = true;
> 	else
> 		up->console_line_ended = false;

OK. I will add a comment, rename the variable, and invert the logic.

>> +void serial8250_console_write_thread(struct uart_8250_port *up,
>> +				     struct nbcon_write_context *wctxt)
>> +{
>> +	struct uart_8250_em485 *em485 = up->em485;
>> +	struct uart_port *port = &up->port;
>> +	unsigned int ier;
>> +
>> +	touch_nmi_watchdog();
>
> This should not be needed in the write_thread() variant because
> it allows to schedule after emitting one record.

Agreed.

Thanks.

John

Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[John Ogness]

On 2024-09-06, John Ogness <john.ogness@linutronix.de> wrote:
>> Wait! This makes the rs485 consoles much less usable for debugging.
>> They might have troubles to see the emergency and panic messages.
>>
>> Is this acceptable? Why?
>
> It is not acceptable. I am looking into making the atomic part work for
> RS485 as well.

So there are 2 things _not_ supported by the write_atomic() callback:

1. RS485 mode. This is due to the need to start up TX for the
write, which can lead to:

up->rs485_start_tx()
  serial8250_em485_start_tx()
    serial8250_stop_rx()
      serial8250_rpm_get()
        pm_runtime_get_sync()
          __pm_runtime_resume()
            spin_lock_irqsave()

Taking a spin lock is not safe from NMI and thus disqualifies this call
chain for write_atomic().

If UART_CAP_RPM is not set, the pm_runtime_get_sync() is avoided. So I
could only disable atomic RS485 if UART_CAP_RPM is set. But the OMAP
variant of the 8250 does set this capability.

2. Modem control. This is due to waiting for inputs, which can lead to:

serial8250_modem_status()
  wake_up_interruptible()

Performing wakes is not safe from scheduler or NMI and thus disqualifies
this call chain for write_atomic().

It would probably be acceptable to move serial8250_modem_status() into
an irq_work.

I would be grateful for any insights on how best to handle these 2
issues if we want full write_atomic() support for all 8250 variants.

John

Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[Thomas Gleixner]

On Fri, Sep 06 2024 at 18:44, John Ogness wrote:
> So there are 2 things _not_ supported by the write_atomic() callback:
>
> 1. RS485 mode. This is due to the need to start up TX for the
> write, which can lead to:
>
> up->rs485_start_tx()
>   serial8250_em485_start_tx()
>     serial8250_stop_rx()
>       serial8250_rpm_get()
>         pm_runtime_get_sync()
>           __pm_runtime_resume()
>             spin_lock_irqsave()
>
> Taking a spin lock is not safe from NMI and thus disqualifies this call
> chain for write_atomic().

Correct. __pm_runtime_resume() can sleep as well :)

> If UART_CAP_RPM is not set, the pm_runtime_get_sync() is avoided. So I
> could only disable atomic RS485 if UART_CAP_RPM is set. But the OMAP
> variant of the 8250 does set this capability.

Sure, but none of this makes sense. What's so special about that em485
muck that serial8250_stop_rx() needs to do that PM dance?

It writes the IER register, which serial8250_console_write() just wrote
to in serial8250_clear_IER() without doing this PM dance. So for the
console write path this stop part is not required at all.  That said,
serial8250_em485_stop_tx() doesn't have this PM dance either.

I'm 100% that this is just a problem of blindly sharing this with the
regular uart code and not because there is a requirement. See what
serial8250_console_setup() does at the end:

        if (port->dev)
                pm_runtime_get_sync(port->dev);

The corresponding put() is in serial8250_console_exit(). So there is
absolutely zero reason for power management in the console write
functions. It's the usual voodoo programming which nobody noticed
because it did not immediately blow up in their face.

There is another minor issue in that em485 muck. One code path arms a
hrtimer, which does not work from NMI like contexts, but that is only
taken when the transmitter is not empty, so probably a non-issue
because the console write code waits for it to be drained.

There are also a few lockdep_assert_held_once(port->lock) in that code
which will trigger when called from the nbcon write functions. They are
already broken today when oops_in_progress is set and the trylock of
port::lock fails...

So splitting this up into a clean and lean set for the console write
functions will make all these horrors just go away. The current sharing
is just fragile as hell and makes no sense at all.

> 2. Modem control. This is due to waiting for inputs, which can lead to:
>
> serial8250_modem_status()
>   wake_up_interruptible()
>
> Performing wakes is not safe from scheduler or NMI and thus disqualifies
> this call chain for write_atomic().
>
> It would probably be acceptable to move serial8250_modem_status() into
> an irq_work.

Yes, but serial8250_modem_status() has more problems than that:

See uart_handle_dcd_change() and uart_handle_cts_change(). They call
into the tty layer and do their own wakeups.

So no, serial8250_modem_status() cannot be invoked there at all.

You have to defer this whole status dance to irq work and this really
needs to be done inside the write_atomic() callback. Otherwise a status
change could get lost, which is bad in non-panic situations.

That needs a bit of thought vs. port->msr_saved_flags, because in a
hostile takeover situation that needs to take into account that the
interrupted context might be fiddling with msr_saved_flags too, which
might on resume overwrite the write_atomic() modifications due to RMW.
Shouldn't be hard.

That brings me to that USE_SERIAL_8250_LEGACY_CONSOLE #ifdeffery, which
started this conversation.

The nbcon conversion does not make things worse than they are today. Any
problem which happens in the atomic_write() callback has existed before
already. So just get rid of the legacy code and be done with it.

At some point you have to bite the bullet and deal with the fallout when
it's reported. Remember, perfect is the enemy of good and you will never
reach perfect.

Thanks,

        tglx

Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[Andy Shevchenko]

On Fri, Sep 06, 2024 at 06:44:41PM +0206, John Ogness wrote:
> On 2024-09-06, John Ogness <john.ogness@linutronix.de> wrote:
> >> Wait! This makes the rs485 consoles much less usable for debugging.
> >> They might have troubles to see the emergency and panic messages.
> >>
> >> Is this acceptable? Why?
> >
> > It is not acceptable. I am looking into making the atomic part work for
> > RS485 as well.
> 
> So there are 2 things _not_ supported by the write_atomic() callback:
> 
> 1. RS485 mode. This is due to the need to start up TX for the
> write, which can lead to:
> 
> up->rs485_start_tx()
>   serial8250_em485_start_tx()
>     serial8250_stop_rx()
>       serial8250_rpm_get()
>         pm_runtime_get_sync()
>           __pm_runtime_resume()
>             spin_lock_irqsave()
> 
> Taking a spin lock is not safe from NMI and thus disqualifies this call
> chain for write_atomic().
> 
> If UART_CAP_RPM is not set, the pm_runtime_get_sync() is avoided. So I
> could only disable atomic RS485 if UART_CAP_RPM is set. But the OMAP
> variant of the 8250 does set this capability.

Please, don't add a new code which relies on UART_CAP_RPM.
The idea is to enable runtime PM for all users who provides respective
callbacks. Rather, you should ask runtime PM for this information.

> 2. Modem control. This is due to waiting for inputs, which can lead to:
> 
> serial8250_modem_status()
>   wake_up_interruptible()
> 
> Performing wakes is not safe from scheduler or NMI and thus disqualifies
> this call chain for write_atomic().
> 
> It would probably be acceptable to move serial8250_modem_status() into
> an irq_work.
> 
> I would be grateful for any insights on how best to handle these 2
> issues if we want full write_atomic() support for all 8250 variants.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[Andy Shevchenko]

On Sat, Sep 07, 2024 at 10:39:00PM +0200, Thomas Gleixner wrote:
> On Fri, Sep 06 2024 at 18:44, John Ogness wrote:

...

> I'm 100% that this is just a problem of blindly sharing this with the
> regular uart code and not because there is a requirement. See what
> serial8250_console_setup() does at the end:
> 
>         if (port->dev)
>                 pm_runtime_get_sync(port->dev);
> 
> The corresponding put() is in serial8250_console_exit(). So there is
> absolutely zero reason for power management in the console write
> functions. It's the usual voodoo programming which nobody noticed
> because it did not immediately blow up in their face.

It might be historical, but yes, the above is for a reason.
And if somebody needs a kernel console to be shutdown, they should remove
it from the active consoles.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

[Thomas Gleixner]

On Mon, Sep 09 2024 at 12:53, Andy Shevchenko wrote:
> On Sat, Sep 07, 2024 at 10:39:00PM +0200, Thomas Gleixner wrote:
>> On Fri, Sep 06 2024 at 18:44, John Ogness wrote:
>
> ...
>
>> I'm 100% that this is just a problem of blindly sharing this with the
>> regular uart code and not because there is a requirement. See what
>> serial8250_console_setup() does at the end:
>> 
>>         if (port->dev)
>>                 pm_runtime_get_sync(port->dev);
>> 
>> The corresponding put() is in serial8250_console_exit(). So there is
>> absolutely zero reason for power management in the console write
>> functions. It's the usual voodoo programming which nobody noticed
>> because it did not immediately blow up in their face.
>
> It might be historical, but yes, the above is for a reason.
> And if somebody needs a kernel console to be shutdown, they should remove
> it from the active consoles.

Correct, because you cant do PM from arbitrary contexts.

Ergo the code which does PM in the context of the console write function
is bogus.

Thanks,

        tglx