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

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

[John Ogness]

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

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

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

RS485
- with SER_RS485_RX_DURING_TX
- without SER_RS485_RX_DURING_TX

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

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

Here are the changes since v2:

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

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

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

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

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

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

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

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

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

John Ogness

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

John Ogness (6):
  serial: 8250: Adjust the timeout for FIFO mode
  serial: 8250: Use high-level write function for FIFO
  serial: 8250: Split out rx stop/start code into helpers
  serial: 8250: Specify console context for rs485_start/stop_tx
  serial: 8250: Switch to nbcon console
  serial: 8250: Revert "drop lockdep annotation from
    serial8250_clear_IER()"

 drivers/tty/serial/8250/8250.h            |   4 +-
 drivers/tty/serial/8250/8250_bcm2835aux.c |   4 +-
 drivers/tty/serial/8250/8250_core.c       |  35 ++-
 drivers/tty/serial/8250/8250_omap.c       |   2 +-
 drivers/tty/serial/8250/8250_port.c       | 267 +++++++++++++++++-----
 include/linux/serial_8250.h               |  11 +-
 6 files changed, 251 insertions(+), 72 deletions(-)


base-commit: 44059790a5cb9258ae6137387e4c39b717fd2ced
-- 
2.39.5

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

[John Ogness]

For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
console write callback needs to enable/disable TX. It does this
by calling the rs485_start/stop_tx() callbacks. However, these
callbacks will disable/enable interrupts, which is a problem
for console write, as it must be responsible for
disabling/enabling interrupts.

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

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

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

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index e5310c65cf52..0d8717be0df7 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -231,8 +231,8 @@ void serial8250_rpm_put_tx(struct uart_8250_port *p);
 
 int serial8250_em485_config(struct uart_port *port, struct ktermios *termios,
 			    struct serial_rs485 *rs485);
-void serial8250_em485_start_tx(struct uart_8250_port *p);
-void serial8250_em485_stop_tx(struct uart_8250_port *p);
+void serial8250_em485_start_tx(struct uart_8250_port *p, bool in_con);
+void serial8250_em485_stop_tx(struct uart_8250_port *p, bool in_con);
 void serial8250_em485_destroy(struct uart_8250_port *p);
 extern struct serial_rs485 serial8250_em485_supported;
 
diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index fdb53b54e99e..c9a106a86b56 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -46,7 +46,7 @@ struct bcm2835aux_data {
 	u32 cntl;
 };
 
-static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up)
+static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up, bool in_con)
 {
 	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
 		struct bcm2835aux_data *data = dev_get_drvdata(up->port.dev);
@@ -65,7 +65,7 @@ static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up)
 		serial8250_out_MCR(up, UART_MCR_RTS);
 }
 
-static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up)
+static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up, bool in_con)
 {
 	if (up->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
 		serial8250_out_MCR(up, 0);
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index b3be0fb184a3..fcbed7e98231 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -365,7 +365,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
 
 	if (up->port.rs485.flags & SER_RS485_ENABLED &&
 	    up->port.rs485_config == serial8250_em485_config)
-		serial8250_em485_stop_tx(up);
+		serial8250_em485_stop_tx(up, false);
 }
 
 /*
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 09ac521d232a..7c50387194ad 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -558,7 +558,7 @@ static int serial8250_em485_init(struct uart_8250_port *p)
 
 deassert_rts:
 	if (p->em485->tx_stopped)
-		p->rs485_stop_tx(p);
+		p->rs485_stop_tx(p, false);
 
 	return 0;
 }
@@ -1398,10 +1398,11 @@ static void serial8250_stop_rx(struct uart_port *port)
 /**
  * serial8250_em485_stop_tx() - generic ->rs485_stop_tx() callback
  * @p: uart 8250 port
+ * @in_con: true if called from console write, otherwise false
  *
  * Generic callback usable by 8250 uart drivers to stop rs485 transmission.
  */
-void serial8250_em485_stop_tx(struct uart_8250_port *p)
+void serial8250_em485_stop_tx(struct uart_8250_port *p, bool in_con)
 {
 	unsigned char mcr = serial8250_in_MCR(p);
 
@@ -1419,7 +1420,9 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
 	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
 		serial8250_clear_and_reinit_fifos(p);
 
-		__serial8250_start_rx_int(p);
+		/* In console context, caller handles interrupt enabling. */
+		if (!in_con)
+			__serial8250_start_rx_int(p);
 	}
 }
 EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
@@ -1434,7 +1437,7 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
 	serial8250_rpm_get(p);
 	uart_port_lock_irqsave(&p->port, &flags);
 	if (em485->active_timer == &em485->stop_tx_timer) {
-		p->rs485_stop_tx(p);
+		p->rs485_stop_tx(p, false);
 		em485->active_timer = NULL;
 		em485->tx_stopped = true;
 	}
@@ -1466,7 +1469,7 @@ static void __stop_tx_rs485(struct uart_8250_port *p, u64 stop_delay)
 		em485->active_timer = &em485->stop_tx_timer;
 		hrtimer_start(&em485->stop_tx_timer, ns_to_ktime(stop_delay), HRTIMER_MODE_REL);
 	} else {
-		p->rs485_stop_tx(p);
+		p->rs485_stop_tx(p, false);
 		em485->active_timer = NULL;
 		em485->tx_stopped = true;
 	}
@@ -1555,6 +1558,7 @@ static inline void __start_tx(struct uart_port *port)
 /**
  * serial8250_em485_start_tx() - generic ->rs485_start_tx() callback
  * @up: uart 8250 port
+ * @in_con: true if called from console write, otherwise false
  *
  * Generic callback usable by 8250 uart drivers to start rs485 transmission.
  * Assumes that setting the RTS bit in the MCR register means RTS is high.
@@ -1562,12 +1566,20 @@ static inline void __start_tx(struct uart_port *port)
  * stoppable by disabling the UART_IER_RDI interrupt.  (Some chips set the
  * UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.)
  */
-void serial8250_em485_start_tx(struct uart_8250_port *up)
+void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con)
 {
 	unsigned char mcr = serial8250_in_MCR(up);
 
-	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
-		serial8250_stop_rx(&up->port);
+	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
+		/*
+		 * In console context, caller handles interrupt disabling. So
+		 * only LSR_DR masking is needed.
+		 */
+		if (in_con)
+			__serial8250_stop_rx_mask_dr(&up->port);
+		else
+			serial8250_stop_rx(&up->port);
+	}
 
 	if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
 		mcr |= UART_MCR_RTS;
@@ -1600,7 +1612,7 @@ static bool start_tx_rs485(struct uart_port *port)
 	if (em485->tx_stopped) {
 		em485->tx_stopped = false;
 
-		up->rs485_start_tx(up);
+		up->rs485_start_tx(up, false);
 
 		if (up->port.rs485.delay_rts_before_send > 0) {
 			em485->active_timer = &em485->start_tx_timer;
@@ -3402,7 +3414,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 
 	if (em485) {
 		if (em485->tx_stopped)
-			up->rs485_start_tx(up);
+			up->rs485_start_tx(up, true);
 		mdelay(port->rs485.delay_rts_before_send);
 	}
 
@@ -3440,7 +3452,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 	if (em485) {
 		mdelay(port->rs485.delay_rts_after_send);
 		if (em485->tx_stopped)
-			up->rs485_stop_tx(up);
+			up->rs485_stop_tx(up, true);
 	}
 
 	serial_port_out(port, UART_IER, ier);
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index e0717c8393d7..c25c026d173d 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -161,8 +161,8 @@ struct uart_8250_port {
 	void			(*dl_write)(struct uart_8250_port *up, u32 value);
 
 	struct uart_8250_em485 *em485;
-	void			(*rs485_start_tx)(struct uart_8250_port *);
-	void			(*rs485_stop_tx)(struct uart_8250_port *);
+	void			(*rs485_start_tx)(struct uart_8250_port *up, bool in_con);
+	void			(*rs485_stop_tx)(struct uart_8250_port *up, bool in_con);
 
 	/* Serial port overrun backoff */
 	struct delayed_work overrun_backoff;
-- 
2.39.5

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

[Andy Shevchenko]

On Fri, Oct 25, 2024 at 01:03:26PM +0206, John Ogness wrote:
> For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
> console write callback needs to enable/disable TX. It does this
> by calling the rs485_start/stop_tx() callbacks. However, these

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

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

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

toggle?

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

toggling ?

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

As per above.

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

toggling ?

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[John Ogness]

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

Currently there are only 2 users:

serial8250_em485_stop_tx()
bcm2835aux_rs485_stop_tx()

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

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

John

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

[Andy Shevchenko]

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Jiri Slaby]

On 25. 10. 24, 12:57, John Ogness wrote:
> For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
> console write callback needs to enable/disable TX. It does this
> by calling the rs485_start/stop_tx() callbacks. However, these
> callbacks will disable/enable interrupts, which is a problem
> for console write, as it must be responsible for
> disabling/enabling interrupts.
> 
> Add an argument @in_con to the rs485_start/stop_tx() callbacks
> to specify if they are being called from console write. If so,
> the callbacks will not handle interrupt disabling/enabling.
> 
> For all call sites other than console write, there is no
> functional change.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
...
> @@ -1562,12 +1566,20 @@ static inline void __start_tx(struct uart_port *port)
>    * stoppable by disabling the UART_IER_RDI interrupt.  (Some chips set the
>    * UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.)
>    */
> -void serial8250_em485_start_tx(struct uart_8250_port *up)
> +void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con)
>   {
>   	unsigned char mcr = serial8250_in_MCR(up);
>   
> -	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> -		serial8250_stop_rx(&up->port);
> +	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> +		/*
> +		 * In console context, caller handles interrupt disabling. So
> +		 * only LSR_DR masking is needed.
> +		 */
> +		if (in_con)
> +			__serial8250_stop_rx_mask_dr(&up->port);
> +		else
> +			serial8250_stop_rx(&up->port);

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

thanks,
-- 
js
suse labs

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

[John Ogness]

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

I considered this, however:

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

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

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

John

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

[Petr Mladek]

On Fri 2024-10-25 13:03:26, John Ogness wrote:
> For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
> console write callback needs to enable/disable TX. It does this
> by calling the rs485_start/stop_tx() callbacks. However, these
> callbacks will disable/enable interrupts, which is a problem
> for console write, as it must be responsible for
> disabling/enabling interrupts.

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

IMHO, it would deserve some explanation.

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

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

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

Just 2 my cents.

Best Regaards,
Petr

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

[John Ogness]

On 2024-11-06, Petr Mladek <pmladek@suse.com> wrote:
>> For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
>> console write callback needs to enable/disable TX. It does this
>> by calling the rs485_start/stop_tx() callbacks. However, these
>> callbacks will disable/enable interrupts, which is a problem
>> for console write, as it must be responsible for
>> disabling/enabling interrupts.
>
> It is not clear to me what exactly is the problem.

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

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

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

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

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

> IMHO, it would deserve some explanation.

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

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

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

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

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

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

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

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

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

John

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

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

[Andy Shevchenko]

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

Thank you for the update.

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

-- 
With Best Regards,
Andy Shevchenko