[PATCH next v2 0/4] convert 8250 to nbcon

[PATCH next v2 0/4] 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 in the cover letter of
v1 of the NBCON series [0]. (Note that they were originally
named NOBKL consoles, but were later renamed to NBCON.)

This is v2 of a series to convert the 8250 driver to an NBCON
console, providing both threaded and atomic printing
implementations. v1 of this series is here [1].

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 [2] 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_line_ended field). However, we can abstract such
things later as more consoles are converted to NBCON.

Here are the changes since v1:

- Remove legacy write() code rather than hide it under the
  macro USE_SERIAL_8250_LEGACY_CONSOLE.

- Implement write_atomic() support for RS485 by splitting out
  the IER register modifications into separate wrapper
  functions.

- Update the RS485 call sites to use the new wrapper functions.

- Implement write_atomic() support for modem control by
  deferring to a new dedicated irq_work.

- Rename @console_newline_needed to @console_line_ended and
  invert the logic.

Note that this series is based on the "for-next" branch of the
printk git [3]. This is because the tty-next tree does not have
the NBCON series and thus causes problems for the kbuild
robots. However, this series does apply cleanly on the tty-next
tree (it just will not build).

John Ogness

[0] https://lore.kernel.org/lkml/20230302195618.156940-1-john.ogness@linutronix.de
[1] https://lore.kernel.org/lkml/20240905134719.142554-1-john.ogness@linutronix.de
[2] https://lore.kernel.org/lkml/ZsdoD6PomBRsB-ow@debarbos-thinkpadt14sgen2i.remote.csb
[3] https://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git

John Ogness (4):
  serial: 8250: Split out IER from rs485_start_tx()
  serial: 8250: Split out IER from rs485_stop_tx()
  serial: 8250: Switch to nbcon console
  serial: 8250: Revert "drop lockdep annotation from
    serial8250_clear_IER()"

 drivers/tty/serial/8250/8250.h      |   3 +
 drivers/tty/serial/8250/8250_core.c |  35 +++-
 drivers/tty/serial/8250/8250_omap.c |   2 +-
 drivers/tty/serial/8250/8250_port.c | 248 ++++++++++++++++++----------
 include/linux/serial_8250.h         |   9 +-
 5 files changed, 200 insertions(+), 97 deletions(-)


base-commit: b794563ea12fb46d9499da9e30c33d9607e33697
-- 
2.39.2

[PATCH next v2 1/4] serial: 8250: Split out IER from rs485_start_tx()

[John Ogness]

Move IER handling out of rs485_start_tx() callback and into a new
wrapper serial8250_rs485_start_tx(). Replace all callback call sites
with wrapper, except for the console write() callback, where it is
inappropriate to modify IER.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250.h      |  2 ++
 drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++------
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index e5310c65cf52..6e90223ba1d6 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -236,6 +236,8 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p);
 void serial8250_em485_destroy(struct uart_8250_port *p);
 extern struct serial_rs485 serial8250_em485_supported;
 
+void serial8250_rs485_start_tx(struct uart_8250_port *up);
+
 /* MCR <-> TIOCM conversion */
 static inline int serial8250_TIOCM_to_MCR(int tiocm)
 {
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 2786918aea98..ba8d9cefc451 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1370,7 +1370,6 @@ static void serial8250_stop_rx(struct uart_port *port)
 	serial8250_rpm_get(up);
 
 	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
-	up->port.read_status_mask &= ~UART_LSR_DR;
 	serial_port_out(port, UART_IER, up->ier);
 
 	serial8250_rpm_put(up);
@@ -1543,16 +1542,20 @@ static inline void __start_tx(struct uart_port *port)
  *
  * 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.
- * (Some chips use inverse semantics.)  Further assumes that reception is
- * 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.)
+ * (Some chips use inverse semantics.)
+ * It does not disable RX interrupts. Use the wrapper function
+ * serial8250_rs485_start_tx() if that is also needed.
  */
 void serial8250_em485_start_tx(struct uart_8250_port *up)
 {
 	unsigned char mcr = serial8250_in_MCR(up);
 
+	/*
+	 * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
+	 * disabled, so explicitly mask it.
+	 */
 	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
-		serial8250_stop_rx(&up->port);
+		up->port.read_status_mask &= ~UART_LSR_DR;
 
 	if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
 		mcr |= UART_MCR_RTS;
@@ -1562,6 +1565,18 @@ void serial8250_em485_start_tx(struct uart_8250_port *up)
 }
 EXPORT_SYMBOL_GPL(serial8250_em485_start_tx);
 
+/**
+ * serial8250_rs485_start_tx() - stop rs485 reception, enable transmission
+ * @up: uart 8250 port
+ */
+void serial8250_rs485_start_tx(struct uart_8250_port *up)
+{
+	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
+		serial8250_stop_rx(&up->port);
+
+	up->rs485_start_tx(up);
+}
+
 /* Returns false, if start_tx_timer was setup to defer TX start */
 static bool start_tx_rs485(struct uart_port *port)
 {
@@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
 	if (em485->tx_stopped) {
 		em485->tx_stopped = false;
 
-		up->rs485_start_tx(up);
+		serial8250_rs485_start_tx(up);
 
 		if (up->port.rs485.delay_rts_before_send > 0) {
 			em485->active_timer = &em485->start_tx_timer;
-- 
2.39.2

Re: [PATCH next v2 1/4] serial: 8250: Split out IER from rs485_start_tx()

[Petr Mladek]

On Fri 2024-09-13 16:11:35, John Ogness wrote:
> Move IER handling out of rs485_start_tx() callback and into a new
> wrapper serial8250_rs485_start_tx(). Replace all callback call sites
> with wrapper, except for the console write() callback, where it is
> inappropriate to modify IER.

Sigh, I am trying to review this patch but I am not familiar with the
code. Feel free to ignore me when the questions are completely off.

> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1370,7 +1370,6 @@ static void serial8250_stop_rx(struct uart_port *port)
>  	serial8250_rpm_get(up);
>  
>  	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
> -	up->port.read_status_mask &= ~UART_LSR_DR;
>  	serial_port_out(port, UART_IER, up->ier);
>  
>  	serial8250_rpm_put(up);
> @@ -1543,16 +1542,20 @@ static inline void __start_tx(struct uart_port *port)
>   *
>   * 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.
> - * (Some chips use inverse semantics.)  Further assumes that reception is
> - * 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.)
> + * (Some chips use inverse semantics.)
> + * It does not disable RX interrupts. Use the wrapper function
> + * serial8250_rs485_start_tx() if that is also needed.
>   */
>  void serial8250_em485_start_tx(struct uart_8250_port *up)
>  {
>  	unsigned char mcr = serial8250_in_MCR(up);
>  
> +	/*
> +	 * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
> +	 * disabled, so explicitly mask it.
> +	 */
>  	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> -		serial8250_stop_rx(&up->port);
> +		up->port.read_status_mask &= ~UART_LSR_DR;

This change is related to disabling UART_IER_RDI but we do not longer
disable it in this code path.

Why do we need to do it here, please?
Why is it needed only in the em485-specific path, please?


I tried to understand the code and am in doubts:

On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
so seems to be relater.

But the "Some chips set..." comment has been added by the commit
058bc104f7ca5c83d81 ("serial: 8250: Generalize rs485 software emulation").
And I do not see any explanation why it was added in this code path
even though UART_LSR_DR and UART_IER_RDI were manipulated in
serial8250_stop_rx() which can be called also in other code
paths via uport->ops->stop_rx().

Also the comment suggests that this fixes a bug in some chips but
the line has been added into 1.1.60 back in 2007.

--- a/drivers/char/ChangeLog
+++ b/drivers/char/ChangeLog
@@ -1,3 +1,28 @@
+Sat Oct 29 18:17:34 1994  Theodore Y. Ts'o  (tytso@rt-11)
+
+	* serial.c (rs_ioctl, get_lsr_info): Added patch suggested by Arne
+		Riiber so that user mode programs can tell when the
+		transmitter shift register is empty.
+
+Thu Oct 27 23:14:29 1994  Theodore Y. Ts'o  (tytso@rt-11)
+
+	* tty_ioctl.c (wait_until_sent): Added debugging printk statements
+		(under the #ifdef TTY_DEBUG_WAIT_UNTL_SENT)  
+
+	* serial.c (rs_interrupt, rs_interrupt_single, receive_chars,
+		change_speed, rs_close): rs_close now disables receiver
+		interrupts when closing the serial port.  This allows the
+		serial port to close quickly when Linux and a modem (or a
+		mouse) are engaged in an echo war; when closing the serial
+		port, we now first stop listening to incoming characters,
+		and *then* wait for the transmit buffer to drain.  
+
+		In order to make this change, the info->read_status_mask
+		is now used to control what bits of the line status
+		register are looked at in the interrupt routine in all
+		cases; previously it was only used in receive_chars to
+		select a few of the status bits.
+
--- a/drivers/char/serial.c
+++ b/drivers/char/serial.c
[...]
@@ -1780,6 +1830,15 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
 		info->normal_termios = *tty->termios;
 	if (info->flags & ASYNC_CALLOUT_ACTIVE)
 		info->callout_termios = *tty->termios;
+	/*
+	 * At this point we stop accepting input.  To do this, we
+	 * disable the receive line status interrupts, and tell the
+	 * interrut driver to stop checking the data ready bit in the
+	 * line status register.
+	 */
+	info->IER &= ~UART_IER_RLSI;
+	serial_out(info, UART_IER, info->IER);
+	info->read_status_mask &= ~UART_LSR_DR;
 	if (info->flags & ASYNC_INITIALIZED) {
 		wait_until_sent(tty, 3000); /* 30 seconds timeout */
 		/*

    => It looks like it was not a fix for a "buggy chips". It looks
       like it was part of the design.

>  
>  	if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>  		mcr |= UART_MCR_RTS;
> @@ -1562,6 +1565,18 @@ void serial8250_em485_start_tx(struct uart_8250_port *up)
>  }
>  EXPORT_SYMBOL_GPL(serial8250_em485_start_tx);
>  
> +/**
> + * serial8250_rs485_start_tx() - stop rs485 reception, enable transmission
> + * @up: uart 8250 port
> + */
> +void serial8250_rs485_start_tx(struct uart_8250_port *up)
> +{
> +	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> +		serial8250_stop_rx(&up->port);
> +
> +	up->rs485_start_tx(up);
> +}
> +
>  /* Returns false, if start_tx_timer was setup to defer TX start */
>  static bool start_tx_rs485(struct uart_port *port)
>  {
> @@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
>  	if (em485->tx_stopped) {
>  		em485->tx_stopped = false;
>  
> -		up->rs485_start_tx(up);
> +		serial8250_rs485_start_tx(up);

If I get this correctly then this keeps the existing behavior when

    up->rs485_start_tx == serial8250_em485_start_tx

Is this always the case, please?

The callback has been added by the commit 058bc104f7ca5c83d81
("serial: 8250: Generalize rs485 software emulation") because
8250_bcm2835aux.c driver needed to do something else.

Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?
Will it still work as expected?

>  
>  		if (up->port.rs485.delay_rts_before_send > 0) {
>  			em485->active_timer = &em485->start_tx_timer;

Best Regards,
Petr

Re: [PATCH next v2 1/4] serial: 8250: Split out IER from rs485_start_tx()

[John Ogness]

On 2024-09-17, Petr Mladek <pmladek@suse.com> wrote:
> Sigh, I am trying to review this patch but I am not familiar with the
> code. Feel free to ignore me when the questions are completely off.

I appreciate you researching where the code came from. I made my changes
based on what I see the code doing now.

>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>>  void serial8250_em485_start_tx(struct uart_8250_port *up)
>>  {
>>  	unsigned char mcr = serial8250_in_MCR(up);
>>  
>> +	/*
>> +	 * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
>> +	 * disabled, so explicitly mask it.
>> +	 */
>>  	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
>> -		serial8250_stop_rx(&up->port);
>> +		up->port.read_status_mask &= ~UART_LSR_DR;
>
> This change is related to disabling UART_IER_RDI but we do not longer
> disable it in this code path.

Correct. It will be disabled in the new wrapper
serial8250_em485_start_tx(). For the console write() callback, RDI is
already being disabled (IER is cleared). It will not use the wrapper.

> Why do we need to do it here, please?

Because the console write() callback also needs to clear LSR_DR. That
part of the callback needs to stay.

> Why is it needed only in the em485-specific path, please?

Only RS485 deals with controlling TX/RX directions.

> On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
> so seems to be relater.

I do not know if the LSR_DR modify is strictly necessary. I am just
preserving the existing behavior (and related comment). The disabling of
IER_RDI will still happen (via wrapper or explicitly as in the console
write() callback).

>>  static bool start_tx_rs485(struct uart_port *port)
>>  {
>> @@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
>>  	if (em485->tx_stopped) {
>>  		em485->tx_stopped = false;
>>  
>> -		up->rs485_start_tx(up);
>> +		serial8250_rs485_start_tx(up);
>
> If I get this correctly then this keeps the existing behavior when
>
>     up->rs485_start_tx == serial8250_em485_start_tx

Correct.

> Is this always the case, please?

Yes.

> Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?

Yes.

> Will it still work as expected?

Yes, but it does perform an extra read. And since someone added a
comment just to mention that, I assume it was important for some use
case.

John

Re: [PATCH next v2 1/4] serial: 8250: Split out IER from rs485_start_tx()

[Petr Mladek]

On Wed 2024-09-18 17:10:53, John Ogness wrote:
> On 2024-09-17, Petr Mladek <pmladek@suse.com> wrote:
> > Sigh, I am trying to review this patch but I am not familiar with the
> > code. Feel free to ignore me when the questions are completely off.
> 
> I appreciate you researching where the code came from. I made my changes
> based on what I see the code doing now.
> 
> >> --- a/drivers/tty/serial/8250/8250_port.c
> >> +++ b/drivers/tty/serial/8250/8250_port.c
> >>  void serial8250_em485_start_tx(struct uart_8250_port *up)
> >>  {
> >>  	unsigned char mcr = serial8250_in_MCR(up);
> >>  
> >> +	/*
> >> +	 * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
> >> +	 * disabled, so explicitly mask it.
> >> +	 */
> >>  	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> >> -		serial8250_stop_rx(&up->port);
> >> +		up->port.read_status_mask &= ~UART_LSR_DR;
> >
> > This change is related to disabling UART_IER_RDI but we do not longer
> > disable it in this code path.
> 
> Correct. It will be disabled in the new wrapper
> serial8250_em485_start_tx(). For the console write() callback, RDI is
> already being disabled (IER is cleared). It will not use the wrapper.
> 
> > Why do we need to do it here, please?
> 
> Because the console write() callback also needs to clear LSR_DR. That
> part of the callback needs to stay.
> 
> > Why is it needed only in the em485-specific path, please?
> 
> Only RS485 deals with controlling TX/RX directions.
> 
> > On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
> > so seems to be relater.
> 
> I do not know if the LSR_DR modify is strictly necessary. I am just
> preserving the existing behavior (and related comment). The disabling of
> IER_RDI will still happen (via wrapper or explicitly as in the console
> write() callback).
> 
> >>  static bool start_tx_rs485(struct uart_port *port)
> >>  {
> >> @@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
> >>  	if (em485->tx_stopped) {
> >>  		em485->tx_stopped = false;
> >>  
> >> -		up->rs485_start_tx(up);
> >> +		serial8250_rs485_start_tx(up);
> >
> > If I get this correctly then this keeps the existing behavior when
> >
> >     up->rs485_start_tx == serial8250_em485_start_tx
> 
> Correct.
> 
> > Is this always the case, please?
> 
> Yes.
> 
> > Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?
> 
> Yes.

IMHO, the answer "Yes" to both last questions can't be valid.
The 8250_bcm2835aux driver does:

static int bcm2835aux_serial_probe(struct platform_device *pdev)
{
	[...]
	up.rs485_start_tx = bcm2835aux_rs485_start_tx;
	[...]
}

As a result, the 1st "Yes" was not correct:

	up->rs485_start_tx != serial8250_em485_start_tx

and this patch would change the behavior for the 8250_bcm2835aux driver.
Before, start_tx_rs485() called directly:

	up->rs485_start_tx(up);

Newly, it would call:

	void serial8250_rs485_start_tx(struct uart_8250_port *up)
	{
		if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
			serial8250_stop_rx(&up->port);

		up->rs485_start_tx(up);
	}

It means that it could call serial8250_stop_rx() even when it was not
called by the original code.

And SER_RS485_RX_DURING_TX seems to be checked even in
drivers/tty/serial/8250/8250_bcm2835aux.c. So, it looks like it
might be (un)set even for this driver.

Or is this code path prevented in start_tx_rs485()? I mean that
em485->tx_stopped could never be true for the 8250_bcm2835aux
driver?

But I see

	static int bcm2835aux_serial_probe(struct platform_device *pdev)
	{
		[...]
		up.port.rs485_config = serial8250_em485_config;
		up.port.rs485_supported = serial8250_em485_supported;
		[...]
	}

=> It looks like even bcm2835aux driver could have the em485 thing.
   But it obviously wanted to something special in
   up->rs485_start_tx().

It looks to me that the change might either cause regression.
Or it would deserve a comment unless the validity is obvious for people
familiar with the code.

Best Regards,
Petr

[PATCH next v2 2/4] serial: 8250: Split out IER from rs485_stop_tx()

[John Ogness]

Move IER handling out of rs485_stop_tx() callback and into a new
wrapper serial8250_rs485_stop_tx(). Replace all callback call sites
with wrapper, except for the console write() callback, where it is
inappropriate to modify IER.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250.h      |  1 +
 drivers/tty/serial/8250/8250_omap.c |  2 +-
 drivers/tty/serial/8250/8250_port.c | 30 ++++++++++++++++++++---------
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 6e90223ba1d6..b1e4b5fef8cc 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -237,6 +237,7 @@ void serial8250_em485_destroy(struct uart_8250_port *p);
 extern struct serial_rs485 serial8250_em485_supported;
 
 void serial8250_rs485_start_tx(struct uart_8250_port *up);
+void serial8250_rs485_stop_tx(struct uart_8250_port *p);
 
 /* MCR <-> TIOCM conversion */
 static inline int serial8250_TIOCM_to_MCR(int tiocm)
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index afef1dd4ddf4..2b62d49a935d 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -366,7 +366,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_rs485_stop_tx(up);
 }
 
 /*
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ba8d9cefc451..7ee74ec944d2 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -558,7 +558,7 @@ static int serial8250_em485_init(struct uart_8250_port *p)
 
 deassert_rts:
 	if (p->em485->tx_stopped)
-		p->rs485_stop_tx(p);
+		serial8250_rs485_stop_tx(p);
 
 	return 0;
 }
@@ -1380,14 +1380,13 @@ static void serial8250_stop_rx(struct uart_port *port)
  * @p: uart 8250 port
  *
  * Generic callback usable by 8250 uart drivers to stop rs485 transmission.
+ * It does not restore RX interrupts. Use the wrapper function
+ * serial8250_rs485_stop_tx() if that is also needed.
  */
 void serial8250_em485_stop_tx(struct uart_8250_port *p)
 {
 	unsigned char mcr = serial8250_in_MCR(p);
 
-	/* Port locked to synchronize UART_IER access against the console. */
-	lockdep_assert_held_once(&p->port.lock);
-
 	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
 		mcr |= UART_MCR_RTS;
 	else
@@ -1397,16 +1396,29 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
 	/*
 	 * Empty the RX FIFO, we are not interested in anything
 	 * received during the half-duplex transmission.
-	 * Enable previously disabled RX interrupts.
 	 */
-	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
+	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
 		serial8250_clear_and_reinit_fifos(p);
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
+
+/**
+ * serial8250_rs485_stop_tx() - stop rs485 transmission, restore RX interrupts
+ * @p: uart 8250 port
+ */
+void serial8250_rs485_stop_tx(struct uart_8250_port *p)
+{
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&p->port.lock);
+
+	p->rs485_stop_tx(p);
 
+	/* Enable previously disabled RX interrupts. */
+	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
 		p->ier |= UART_IER_RLSI | UART_IER_RDI;
 		serial_port_out(&p->port, UART_IER, p->ier);
 	}
 }
-EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
 
 static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
 {
@@ -1418,7 +1430,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);
+		serial8250_rs485_stop_tx(p);
 		em485->active_timer = NULL;
 		em485->tx_stopped = true;
 	}
@@ -1450,7 +1462,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);
+		serial8250_rs485_stop_tx(p);
 		em485->active_timer = NULL;
 		em485->tx_stopped = true;
 	}
-- 
2.39.2

Re: [PATCH next v2 2/4] serial: 8250: Split out IER from rs485_stop_tx()

[kernel test robot]

Hi John,

kernel test robot noticed the following build errors:

[auto build test ERROR on b794563ea12fb46d9499da9e30c33d9607e33697]

url:    https://github.com/intel-lab-lkp/linux/commits/John-Ogness/serial-8250-Split-out-IER-from-rs485_start_tx/20240913-220810
base:   b794563ea12fb46d9499da9e30c33d9607e33697
patch link:    https://lore.kernel.org/r/20240913140538.221708-3-john.ogness%40linutronix.de
patch subject: [PATCH next v2 2/4] serial: 8250: Split out IER from rs485_stop_tx()
config: parisc-randconfig-r064-20240914 (https://download.01.org/0day-ci/archive/20240914/202409141849.iyZNNZgc-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409141849.iyZNNZgc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409141849.iyZNNZgc-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/locking/test-ww_mutex.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_performance.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_userspace.o
>> ERROR: modpost: "serial8250_rs485_stop_tx" [drivers/tty/serial/8250/8250_omap.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH next v2 2/4] serial: 8250: Split out IER from rs485_stop_tx()

[Petr Mladek]

On Fri 2024-09-13 16:11:36, John Ogness wrote:
> Move IER handling out of rs485_stop_tx() callback and into a new
> wrapper serial8250_rs485_stop_tx(). Replace all callback call sites
> with wrapper, except for the console write() callback, where it is
> inappropriate to modify IER.

It would be great to provide more details:

  + why it is done (IER modification requires port lock?)

  + why it is suddenly safe to call serial8250_em485_handle_stop_tx()
    without holding &p->port.lock


> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -558,7 +558,7 @@ static int serial8250_em485_init(struct uart_8250_port *p)
>  
>  deassert_rts:
>  	if (p->em485->tx_stopped)
> -		p->rs485_stop_tx(p);
> +		serial8250_rs485_stop_tx(p);

This would keep the same functionality only when

	p->rs485_stop_tx == serial8250_em485_stop_tx

Is it always the case?
Is it OK when it is not the case?

For example, serial8250_em485_init() is involved in bcm2835aux driver
probe which uses another rs485_stop_tx() callback, see below.

>  
>  	return 0;
>  }
> @@ -1397,16 +1396,29 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
>  	/*
>  	 * Empty the RX FIFO, we are not interested in anything
>  	 * received during the half-duplex transmission.
> -	 * Enable previously disabled RX interrupts.
>  	 */
> -	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> +	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>  		serial8250_clear_and_reinit_fifos(p);
> +}
> +EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
> +
> +/**
> + * serial8250_rs485_stop_tx() - stop rs485 transmission, restore RX interrupts
> + * @p: uart 8250 port
> + */
> +void serial8250_rs485_stop_tx(struct uart_8250_port *p)
> +{
> +	/* Port locked to synchronize UART_IER access against the console. */
> +	lockdep_assert_held_once(&p->port.lock);
> +
> +	p->rs485_stop_tx(p);
>  
> +	/* Enable previously disabled RX interrupts. */
> +	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
>  		p->ier |= UART_IER_RLSI | UART_IER_RDI;
>  		serial_port_out(&p->port, UART_IER, p->ier);
>  	}
>  }
> -EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
>  
>  static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
>  {
> @@ -1418,7 +1430,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);
> +		serial8250_rs485_stop_tx(p);

This causes that UART_IER is manipulated for all p->rs485_stop_tx()
callbacks. Is that correct, please?

For example, it seems serial8250_em485_handle_stop_tx() might be used
also by bcm2835aux driver. It set by:

static int serial8250_em485_init(struct uart_8250_port *p)
{
	[...]
	p->em485->stop_tx_timer.function = &serial8250_em485_handle_stop_tx;
	[...]
}

which is called via

int serial8250_em485_config(struct uart_port *port, struct ktermios *termios,
			    struct serial_rs485 *rs485)
{
	[...]
	if (rs485->flags & SER_RS485_ENABLED)
		return serial8250_em485_init(up);
	[...]
}

which is set by:

static int bcm2835aux_serial_probe(struct platform_device *pdev)
{
	[...]
	up.port.rs485_config = serial8250_em485_config;		<--------
	[...]
	up.rs485_stop_tx = bcm2835aux_rs485_stop_tx;
	[...]
}

But this same _probe() call sets

	up.rs485_stop_tx = bcm2835aux_rs485_stop_tx;

which does not manipulate UART_IER.

>  		em485->active_timer = NULL;
>  		em485->tx_stopped = true;
>  	}
> @@ -1450,7 +1462,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);
> +		serial8250_rs485_stop_tx(p);

I can't find easily whether serial8250_em485_stop_tx() is always set
as p->rs485_stop_tx callback here. I would expect that it might be
another callback. It is a callback after all.

Is it always safe?

>  		em485->active_timer = NULL;
>  		em485->tx_stopped = true;
>  	}

Best Regards,
Petr

[PATCH next v2 3/4] 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_thread() callback allows safe handover/takeover per byte.
The write_atomic() callback allows safe handover/takeover per
printk record and adds a preceding newline if it took over mid-line.

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

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

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 5f9f06911795..2d690ff32a32 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_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);
 }
 
 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 7ee74ec944d2..d58a0fa95e3b 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -691,7 +691,12 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 	serial8250_rpm_put(p);
 }
 
-static void serial8250_clear_IER(struct uart_8250_port *up)
+/*
+ * Only to be used directly by the console write callbacks, which may not
+ * require the port lock. Use serial8250_clear_IER() instead for all other
+ * cases.
+ */
+static void __serial8250_clear_IER(struct uart_8250_port *up)
 {
 	if (up->capabilities & UART_CAP_UUE)
 		serial_out(up, UART_IER, UART_IER_UUE);
@@ -699,6 +704,11 @@ static void serial8250_clear_IER(struct uart_8250_port *up)
 		serial_out(up, UART_IER, 0);
 }
 
+static inline void serial8250_clear_IER(struct uart_8250_port *up)
+{
+	__serial8250_clear_IER(up);
+}
+
 #ifdef CONFIG_SERIAL_8250_RSA
 /*
  * Attempts to turn on the RSA FIFO.  Returns zero on failure.
@@ -1864,6 +1874,8 @@ unsigned int serial8250_modem_status(struct uart_8250_port *up)
 	struct uart_port *port = &up->port;
 	unsigned int status = serial_in(up, UART_MSR);
 
+	lockdep_assert_held_once(&port->lock);
+
 	status |= up->msr_saved_flags;
 	up->msr_saved_flags = 0;
 	if (status & UART_MSR_ANY_DELTA && up->ier & UART_IER_MSI &&
@@ -1884,6 +1896,21 @@ unsigned int serial8250_modem_status(struct uart_8250_port *up)
 }
 EXPORT_SYMBOL_GPL(serial8250_modem_status);
 
+/*
+ * 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);
+}
+
 static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
 {
 	switch (iir & 0x3f) {
@@ -3296,6 +3323,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_line_ended = true;
+	else
+		up->console_line_ended = false;
 }
 
 /*
@@ -3324,65 +3356,68 @@ static void serial8250_console_restore(struct uart_8250_port *up)
 	serial8250_out_MCR(up, up->mcr | UART_MCR_DTR | UART_MCR_RTS);
 }
 
-/*
- * Print a string to the serial port using the device FIFO
- *
- * It sends fifosize bytes and then waits for the fifo
- * to get empty.
- */
-static void serial8250_console_fifo_write(struct uart_8250_port *up,
-					  const char *s, unsigned int count)
+static void __serial8250_console_write_thread(struct uart_8250_port *up,
+					      struct nbcon_write_context *wctxt)
 {
-	int i;
-	const char *end = s + count;
-	unsigned int fifosize = up->tx_loadsz;
-	bool cr_sent = false;
-
-	while (s != end) {
-		wait_for_lsr(up, UART_LSR_THRE);
-
-		for (i = 0; i < fifosize && s != end; ++i) {
-			if (*s == '\n' && !cr_sent) {
-				serial_out(up, UART_TX, '\r');
-				cr_sent = true;
-			} else {
-				serial_out(up, UART_TX, *s++);
-				cr_sent = false;
-			}
+	unsigned int len = READ_ONCE(wctxt->len);
+	struct uart_port *port = &up->port;
+	unsigned int i;
+
+	if (nbcon_exit_unsafe(wctxt)) {
+		/*
+		 * 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);
 }
 
-/*
- *	Print a string to the serial port trying not to disturb
- *	any possible real use of the port...
- *
- *	The console_lock must be held when we get here.
- *
- *	Doing runtime PM is really a bad idea for the kernel console.
- *	Thus, we assume the function is called when device is powered up.
- */
-void serial8250_console_write(struct uart_8250_port *up, const char *s,
-			      unsigned int count)
+static void __serial8250_console_write_atomic(struct uart_8250_port *up,
+					      struct nbcon_write_context *wctxt)
 {
-	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();
+	if (!up->console_line_ended)
+		uart_console_write(port, "\n", 1, serial8250_console_putchar);
+	uart_console_write(port, wctxt->outbuf, wctxt->len, serial8250_console_putchar);
+}
 
-	if (oops_in_progress)
-		locked = uart_port_trylock_irqsave(port, &flags);
-	else
-		uart_port_lock_irqsave(port, &flags);
+static 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 int ier;
+
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
 
 	/*
-	 *	First save the IER then disable the interrupts
+	 * First save IER then disable the interrupts. The special variant
+	 * to clear IER is used because console printing may occur without
+	 * holding the port lock.
 	 */
 	ier = serial_port_in(port, UART_IER);
-	serial8250_clear_IER(up);
+	__serial8250_clear_IER(up);
 
 	/* check scratch reg to see if port powered off during system sleep */
 	if (up->canary && (up->canary != serial_port_in(port, UART_SCR))) {
@@ -3396,30 +3431,10 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 		mdelay(port->rs485.delay_rts_before_send);
 	}
 
-	use_fifo = (up->capabilities & UART_CAP_FIFO) &&
-		/*
-		 * BCM283x requires to check the fifo
-		 * after each byte.
-		 */
-		!(up->capabilities & UART_CAP_MINI) &&
-		/*
-		 * tx_loadsz contains the transmit fifo size
-		 */
-		up->tx_loadsz > 1 &&
-		(up->fcr & UART_FCR_ENABLE_FIFO) &&
-		port->state &&
-		test_bit(TTY_PORT_INITIALIZED, &port->state->port.iflags) &&
-		/*
-		 * After we put a data in the fifo, the controller will send
-		 * it regardless of the CTS state. Therefore, only use fifo
-		 * if we don't use control flow.
-		 */
-		!(up->port.flags & UPF_CONS_FLOW);
-
-	if (likely(use_fifo))
-		serial8250_console_fifo_write(up, s, count);
+	if (is_atomic)
+		__serial8250_console_write_atomic(up, wctxt);
 	else
-		uart_console_write(port, s, count, serial8250_console_putchar);
+		__serial8250_console_write_thread(up, wctxt);
 
 	/*
 	 *	Finally, wait for transmitter to become empty
@@ -3442,11 +3457,32 @@ 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);
+}
+
+void serial8250_console_write_thread(struct uart_8250_port *up,
+				     struct nbcon_write_context *wctxt)
+{
+	serial8250_console_write(up, wctxt, false);
+}
+
+void serial8250_console_write_atomic(struct uart_8250_port *up,
+				     struct nbcon_write_context *wctxt)
+{
+	touch_nmi_watchdog();
+
+	serial8250_console_write(up, wctxt, true);
 }
 
 static unsigned int probe_baud(struct uart_port *port)
@@ -3466,6 +3502,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';
@@ -3475,6 +3512,9 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 	if (!port->iobase && !port->membase)
 		return -ENODEV;
 
+	up->console_line_ended = true;
+	up->modem_status_work = IRQ_WORK_INIT(modem_status_handler);
+
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
 	else if (probe)
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index fd59ed2cca53..75c1f93fec73 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -152,6 +152,9 @@ struct uart_8250_port {
 	u16			lsr_save_mask;
 #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
 	unsigned char		msr_saved_flags;
+	struct irq_work		modem_status_work;
+
+	bool			console_line_ended;	/* line fully output */
 
 	struct uart_8250_dma	*dma;
 	const struct uart_8250_ops *ops;
@@ -202,8 +205,10 @@ 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_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

Re: [PATCH next v2 3/4] serial: 8250: Switch to nbcon console

[Petr Mladek]

On Fri 2024-09-13 16:11:37, 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_thread() callback allows safe handover/takeover per byte.
> The write_atomic() callback allows safe handover/takeover per
> printk record and adds a preceding newline if it took over mid-line.
> 
> For the write_atomic() case, a new irq_work is used to defer modem
> control since it may be a context that does not allow waking up
> tasks.

It would be fair to mention that it does not longer support fifo in
the 8250 driver. It basically reverted the commit 8f3631f0f6eb42e5
("serial/8250: Use fifo in 8250 console driver").

It is not usable in write_thread() because it would not allow
a safe takeover between emitting particular characters.

It might still be used in write_atomic() but it is probably not
worth it. This callback is used "only" in emergency and panic
situations.

> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Otherwise, it looks good to me. And it even works fine.
With an updated commit message:

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

Best Regards,
Petr

Re: [PATCH next v2 3/4] serial: 8250: Switch to nbcon console

[Andy Shevchenko]

On Wed, Sep 18, 2024 at 02:26:25PM +0200, Petr Mladek wrote:
> On Fri 2024-09-13 16:11:37, 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_thread() callback allows safe handover/takeover per byte.
> > The write_atomic() callback allows safe handover/takeover per
> > printk record and adds a preceding newline if it took over mid-line.
> > 
> > For the write_atomic() case, a new irq_work is used to defer modem
> > control since it may be a context that does not allow waking up
> > tasks.
> 
> It would be fair to mention that it does not longer support fifo in
> the 8250 driver. It basically reverted the commit 8f3631f0f6eb42e5
> ("serial/8250: Use fifo in 8250 console driver").
> 
> It is not usable in write_thread() because it would not allow
> a safe takeover between emitting particular characters.
> 
> It might still be used in write_atomic() but it is probably not
> worth it. This callback is used "only" in emergency and panic
> situations.

This is unfortunate. It will drop down the efficiency of printing.
I think it should be done differently, i.e. the takeover the code
has to drop FIFO (IIRC it's easy to achieve by disabling it or so)
and switch to printing the panic/emergency message. But still at
some baud rate speeds draining the FIFO to the other end may be
not a bad idea as it takes a few dozens of microseconds.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next v2 3/4] serial: 8250: Switch to nbcon console

[Petr Mladek]

On Wed 2024-09-18 17:01:27, Andy Shevchenko wrote:
> On Wed, Sep 18, 2024 at 02:26:25PM +0200, Petr Mladek wrote:
> > On Fri 2024-09-13 16:11:37, 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_thread() callback allows safe handover/takeover per byte.
> > > The write_atomic() callback allows safe handover/takeover per
> > > printk record and adds a preceding newline if it took over mid-line.
> > > 
> > > For the write_atomic() case, a new irq_work is used to defer modem
> > > control since it may be a context that does not allow waking up
> > > tasks.
> > 
> > It would be fair to mention that it does not longer support fifo in
> > the 8250 driver. It basically reverted the commit 8f3631f0f6eb42e5
> > ("serial/8250: Use fifo in 8250 console driver").
> > 
> > It is not usable in write_thread() because it would not allow
> > a safe takeover between emitting particular characters.
> > 
> > It might still be used in write_atomic() but it is probably not
> > worth it. This callback is used "only" in emergency and panic
> > situations.
> 
> This is unfortunate. It will drop down the efficiency of printing.

The FIFO mode has been added by the commit 8f3631f0f6eb42e5
("serial/8250: Use fifo in 8250 console driver"). The interesting
parts are:

<paste>
  While investigating a bug in the RHEL kernel, I noticed that the serial
  console throughput is way below the configured speed of 115200 bps in
  a HP Proliant DL380 Gen9. I was expecting something above 10KB/s, but
  I got 2.5KB/s.

  In another machine, I measured a throughput of 11.5KB/s, with the serial
  controller taking between 80-90us to send each byte. That matches the
  expected throughput for a configuration of 115200 bps.

  This patch changes the serial8250_console_write to use the 16550 fifo
  if available. In my benchmarks I got around 25% improvement in the slow
  machine, and no performance penalty in the fast machine.
</paste>

I would translate it:

The FIFO mode helped with some buggy serial console. But it helped to gain
only small portion of the expected speed. The commit message does not
mention any gain with the normally working system.

It has been added in 2022. It was considered only because of a
"broken" system. Nobody cared enough before.

> I think it should be done differently, i.e. the takeover the code
> has to drop FIFO (IIRC it's easy to achieve by disabling it or so)
> and switch to printing the panic/emergency message. But still at
> some baud rate speeds draining the FIFO to the other end may be
> not a bad idea as it takes a few dozens of microseconds.

Sure. it is doable. But I am not convinced that it is really worth it.

Best Regards,
Petr

Re: [PATCH next v2 3/4] serial: 8250: Switch to nbcon console

[Andy Shevchenko]

On Wed, Sep 18, 2024 at 04:35:06PM +0200, Petr Mladek wrote:
> On Wed 2024-09-18 17:01:27, Andy Shevchenko wrote:
> > On Wed, Sep 18, 2024 at 02:26:25PM +0200, Petr Mladek wrote:
> > > On Fri 2024-09-13 16:11:37, 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_thread() callback allows safe handover/takeover per byte.
> > > > The write_atomic() callback allows safe handover/takeover per
> > > > printk record and adds a preceding newline if it took over mid-line.
> > > > 
> > > > For the write_atomic() case, a new irq_work is used to defer modem
> > > > control since it may be a context that does not allow waking up
> > > > tasks.
> > > 
> > > It would be fair to mention that it does not longer support fifo in
> > > the 8250 driver. It basically reverted the commit 8f3631f0f6eb42e5
> > > ("serial/8250: Use fifo in 8250 console driver").
> > > 
> > > It is not usable in write_thread() because it would not allow
> > > a safe takeover between emitting particular characters.
> > > 
> > > It might still be used in write_atomic() but it is probably not
> > > worth it. This callback is used "only" in emergency and panic
> > > situations.
> > 
> > This is unfortunate. It will drop down the efficiency of printing.
> 
> The FIFO mode has been added by the commit 8f3631f0f6eb42e5
> ("serial/8250: Use fifo in 8250 console driver"). The interesting
> parts are:
> 
> <paste>
>   While investigating a bug in the RHEL kernel, I noticed that the serial
>   console throughput is way below the configured speed of 115200 bps in
>   a HP Proliant DL380 Gen9. I was expecting something above 10KB/s, but
>   I got 2.5KB/s.
> 
>   In another machine, I measured a throughput of 11.5KB/s, with the serial
>   controller taking between 80-90us to send each byte. That matches the
>   expected throughput for a configuration of 115200 bps.
> 
>   This patch changes the serial8250_console_write to use the 16550 fifo
>   if available. In my benchmarks I got around 25% improvement in the slow
>   machine, and no performance penalty in the fast machine.
> </paste>
> 
> I would translate it:
> 
> The FIFO mode helped with some buggy serial console. But it helped to gain
> only small portion of the expected speed. The commit message does not
> mention any gain with the normally working system.
> 
> It has been added in 2022. It was considered only because of a
> "broken" system. Nobody cared enough before.
> 
> > I think it should be done differently, i.e. the takeover the code
> > has to drop FIFO (IIRC it's easy to achieve by disabling it or so)
> > and switch to printing the panic/emergency message. But still at
> > some baud rate speeds draining the FIFO to the other end may be
> > not a bad idea as it takes a few dozens of microseconds.
> 
> Sure. it is doable. But I am not convinced that it is really worth it.

Fair enough. But perhaps Cc to the author to at least notify them about
this change?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next v2 3/4] serial: 8250: Switch to nbcon console

[John Ogness]

On 2024-09-18, Petr Mladek <pmladek@suse.com> wrote:
> It would be fair to mention that it does not longer support fifo in
> the 8250 driver. It basically reverted the commit 8f3631f0f6eb42e5
> ("serial/8250: Use fifo in 8250 console driver").

Agreed.

> It is not usable in write_thread() because it would not allow
> a safe takeover between emitting particular characters.

If write_thread could exit_unsafe()/enter_unsafe() while busy-waiting,
then emergency/panic could still take over at any time. Even if it means
that atomic_write() would need to first wait for the FIFO to drain
(which it will). The important thing is that emergency/panic is able to
take over.

I dropped the optimization to keep things simple for now, but I agree
with Andy that it would be unfortunate. I will take a look at what such
an implementation could look like.

John

Re: [PATCH next v2 3/4] serial: 8250: Switch to nbcon console

[John Ogness]

On 2024-09-13, John Ogness <john.ogness@linutronix.de> wrote:
> +/*
> + * 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);
> +}

As reported [0] by the kernel test robot, I need to move
modem_status_handler() down into the "#ifdef CONFIG_SERIAL_8250_CONSOLE"
block.

John Ogness

[0] https://lore.kernel.org/oe-kbuild-all/202409140437.EP0Ryw3u-lkp@intel.com

[PATCH next v2 4/4] 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 d58a0fa95e3b..fdef0cd01b2d 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -706,6 +706,9 @@ static void __serial8250_clear_IER(struct uart_8250_port *up)
 
 static inline void serial8250_clear_IER(struct uart_8250_port *up)
 {
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&up->port.lock);
+
 	__serial8250_clear_IER(up);
 }
 
-- 
2.39.2

Re: [PATCH next v2 4/4] serial: 8250: Revert "drop lockdep annotation from serial8250_clear_IER()"

[Petr Mladek]

On Fri 2024-09-13 16:11:38, 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>

Makes sense.

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

Best Regards,
Petr