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

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

[John Ogness]

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

The changes since v3:

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

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

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

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

- Use shorter code syntax when setting @console_line_ended.

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

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

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

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

- Commit message and comment style cleanups as requested.

John Ogness

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

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

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


base-commit: 2c1fd53af21b8cb13878b054894d33d3383eb1f3
-- 
2.39.5

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

[John Ogness]

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

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

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

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

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

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

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

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

[Andy Shevchenko]

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

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

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[John Ogness]

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

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

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

   and

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

John

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

[John Ogness]

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

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

John

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

[Petr Mladek]

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

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

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

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

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

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

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

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

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

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

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

All in all, the patch looks good to me.

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

Best Regards,
Petr

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

[John Ogness]

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

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

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

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

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

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

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

John

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

[Petr Mladek]

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

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

To be more clear. By the nested context I meant

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

call by serial8250_console_write(). The original code did:

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

, where serial8250_stop_rx() cleared the flags:

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

But the flags were already cleared by:

	__serial8250_clear_IER(up);

called by serial8250_console_write() _earlier_. Which did:

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

}


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

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

	up->rs485_stop_tx(up)

which did:

	+ serial8250_em485_stop_tx

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

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

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

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


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

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

Best Regards,
Petr