drivers/tty/serial/8250/8250_core.c | 7 +++++++ drivers/tty/serial/8250/8250_port.c | 14 ++++++++++++-- drivers/tty/serial/serial_core.c | 18 +++++++++++++++++- include/linux/serial_core.h | 8 ++++++++ 4 files changed, 44 insertions(+), 3 deletions(-)
The kernel documentation specifies that the console option 'r' can
be used to enable hardware flow control for console writes. The 8250
driver does include code for hardware flow control on the console if
the UPF_CONS_FLOW flag is set, but there is no code path that sets
this flag. However, that is not the only issue. The problems are:
1. Specifying the console option 'r' does not lead to UPF_CONS_FLOW
being set.
2. Even if UPF_CONS_FLOW would be set, serial8250_register_8250_port()
clears it.
3. When the console option 'r' is specified, uart_set_options()
attempts to initialize the port for CRTSCTS. However, afterwards
it does not set the UPSTAT_CTS_ENABLE status bit and therefore on
boot, uart_cts_enabled() is always false. This policy bit is
important for console drivers as a criteria if they may poll CTS.
4. Even though uart_set_options() attempts to initialized the port
for CRTSCTS, the 8250 set_termios() callback does not enable the
RTS signal (TIOCM_RTS) and thus the hardware is not properly
initialized for CTS polling.
5. Even if modem control was properly setup for CTS polling
(TIOCM_RTS), uart_configure_port() clears TIOCM_RTS, thus
breaking CTS polling.
6. wait_for_xmitr() and serial8250_console_write() use the
UPF_CONS_FLOW bit to decide if CTS polling should occur. However,
the condition should also include a check that it is not in rs485
mode and CRTSCTS is actually enabled in the hardware.
Address all these issues as conservatively as possible by gating them
behind checks focussed on the user specifying console hardware flow
control support and the hardware being configured for CTS polling
at the time of the write to the uart.
Since checking the UPSTAT_CTS_ENABLE status bit is a part of the new
condition gate, these changes also support runtime termios updates to
disable/enable CRTSCTS.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
drivers/tty/serial/8250/8250_core.c | 7 +++++++
drivers/tty/serial/8250/8250_port.c | 14 ++++++++++++--
drivers/tty/serial/serial_core.c | 18 +++++++++++++++++-
include/linux/serial_core.h | 8 ++++++++
4 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index a428e88938eb7..fd3441dd9a559 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -693,6 +693,7 @@ static void serial_8250_overrun_backoff_work(struct work_struct *work)
int serial8250_register_8250_port(const struct uart_8250_port *up)
{
struct uart_8250_port *uart;
+ bool console_hwflow;
int ret;
if (up->port.uartclk == 0)
@@ -716,6 +717,9 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
if (uart->port.type == PORT_8250_CIR)
return -ENODEV;
+ /* Preserve specified console hardware flow control. */
+ console_hwflow = uart->port.flags & UPF_CONS_FLOW;
+
if (uart->port.dev)
uart_remove_one_port(&serial8250_reg, &uart->port);
@@ -757,6 +761,9 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
goto err;
}
+ if (console_hwflow)
+ uart->port.flags |= UPF_CONS_FLOW;
+
if (up->port.flags & UPF_FIXED_TYPE)
uart->port.type = up->port.type;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index af78cc02f38e7..88f5309a940e9 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1988,7 +1988,7 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
wait_for_lsr(up, bits);
/* Wait up to 1s for flow control if necessary */
- if (up->port.flags & UPF_CONS_FLOW) {
+ if (uart_console_hwflow_active(&up->port)) {
for (tmout = 1000000; tmout; tmout--) {
unsigned int msr = serial_in(up, UART_MSR);
up->msr_saved_flags |= msr & MSR_SAVE_FLAGS;
@@ -2782,6 +2782,12 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
serial8250_set_efr(port, termios);
serial8250_set_divisor(port, baud, quot, frac);
serial8250_set_fcr(port, termios);
+ /* Consoles manually poll CTS for hardware flow control. */
+ if (uart_console(port) &&
+ !(port->rs485.flags & SER_RS485_ENABLED)
+ && termios->c_cflag & CRTSCTS) {
+ port->mctrl |= TIOCM_RTS;
+ }
serial8250_set_mctrl(port, port->mctrl);
}
@@ -3351,7 +3357,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
* it regardless of the CTS state. Therefore, only use fifo
* if we don't use control flow.
*/
- !(up->port.flags & UPF_CONS_FLOW);
+ !uart_console_hwflow_active(&up->port);
if (likely(use_fifo))
serial8250_console_fifo_write(up, s, count);
@@ -3421,6 +3427,10 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
if (ret)
return ret;
+ /* Allow user-specified hardware flow control. */
+ if (flow == 'r')
+ port->flags |= UPF_CONS_FLOW;
+
if (port->dev)
pm_runtime_get_sync(port->dev);
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 89cebdd278410..cce1fd728a1e4 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2235,6 +2235,15 @@ uart_set_options(struct uart_port *port, struct console *co,
port->mctrl |= TIOCM_DTR;
port->ops->set_termios(port, &termios, &dummy);
+
+ /*
+ * If console hardware flow control was specified and is supported,
+ * the related policy UPSTAT_CTS_ENABLE must be set to allow console
+ * drivers to identify if CTS should be used for polling.
+ */
+ if (flow == 'r' && (termios.c_cflag & CRTSCTS))
+ port->status |= UPSTAT_CTS_ENABLE;
+
/*
* Allow the setting of the UART parameters with a NULL console
* too:
@@ -2541,7 +2550,14 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
* We probably don't need a spinlock around this, but
*/
scoped_guard(uart_port_lock_irqsave, port) {
- port->mctrl &= TIOCM_DTR;
+ unsigned int mask = TIOCM_DTR;
+
+ /* Console hardware flow control polls CTS. */
+ if (uart_console_hwflow_active(port))
+ mask |= TIOCM_RTS;
+
+ port->mctrl &= mask;
+
if (!(port->rs485.flags & SER_RS485_ENABLED))
port->ops->set_mctrl(port, port->mctrl);
}
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 666430b478997..07bd3bd6c8355 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -1163,6 +1163,14 @@ static inline bool uart_softcts_mode(struct uart_port *uport)
return ((uport->status & mask) == UPSTAT_CTS_ENABLE);
}
+static inline bool uart_console_hwflow_active(struct uart_port *uport)
+{
+ return uart_console(uport) &&
+ !(uport->rs485.flags & SER_RS485_ENABLED) &&
+ (uport->flags & UPF_CONS_FLOW) &&
+ uart_cts_enabled(uport);
+}
+
/*
* The following are helper functions for the low level drivers.
*/
base-commit: d50dd728ced93a1900ff0be924b6f273baf59fb2
--
2.47.3
Hi,
Responding to Sashiko:
https://sashiko.dev/#/patchset/20260331141502.6233-1-john.ogness%40linutronix.de
On 2026-03-31, Sashiko wrote:
> This commit addresses issues with hardware flow control on the 8250 console.
> It modifies the driver and serial core to properly initialize CTS polling
> and handle the 'r' console option.
>
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index d2e2c5dfef99..4ca7cac6b8a7 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -716,6 +717,9 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
>> if (uart->port.type == PORT_8250_CIR)
>> return -ENODEV;
>>
>> + /* Preserve specified console hardware flow control. */
>> + console_hwflow = uart->port.flags & UPF_CONS_FLOW;
>> +
>> if (uart->port.dev)
>> uart_remove_one_port(&serial8250_reg, &uart->port);
>>
>> @@ -757,6 +761,9 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
>> goto err;
>> }
>>
>> + if (console_hwflow)
>> + uart->port.flags |= UPF_CONS_FLOW;
>> +
>
> Can this create a race window where console flow control is temporarily
> disabled? The UPF_CONS_FLOW flag is saved, but uart->port.flags is
> overwritten earlier in this function. If an asynchronous printk (such as
> from an NMI) triggers serial8250_console_write() before the flag is
> restored, it seems uart_console_hwflow_active() will evaluate to false,
> bypassing CTS polling and potentially dropping characters.
Correct. I missed that serial8250_register_8250_port() is called after
the console is already registered via univ8250_console_init(). I can
remove the window with something like the following alternative change
to serial8250_register_8250_port():
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index a428e88938eb7..ff4c9972d4576 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -693,6 +693,7 @@ static void serial_8250_overrun_backoff_work(struct work_struct *work)
int serial8250_register_8250_port(const struct uart_8250_port *up)
{
struct uart_8250_port *uart;
+ upf_t console_hwflow;
int ret;
if (up->port.uartclk == 0)
@@ -716,6 +717,9 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
if (uart->port.type == PORT_8250_CIR)
return -ENODEV;
+ /* Preserve specified console hardware flow control. */
+ console_hwflow = uart->port.flags & UPF_CONS_FLOW;
+
if (uart->port.dev)
uart_remove_one_port(&serial8250_reg, &uart->port);
@@ -729,7 +733,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
uart->port.fifosize = up->port.fifosize;
uart->port.regshift = up->port.regshift;
uart->port.iotype = up->port.iotype;
- uart->port.flags = up->port.flags | UPF_BOOT_AUTOCONF;
+ uart->port.flags = up->port.flags | UPF_BOOT_AUTOCONF | console_hwflow;
uart->bugs = up->bugs;
uart->port.mapbase = up->port.mapbase;
uart->port.mapsize = up->port.mapsize;
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 328711b5df1a..71edac12473e 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -1988,7 +1988,7 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
>> wait_for_lsr(up, bits);
>>
>> /* Wait up to 1s for flow control if necessary */
>> - if (up->port.flags & UPF_CONS_FLOW) {
>> + if (uart_console_hwflow_active(&up->port)) {
>> for (tmout = 1000000; tmout; tmout--) {
>> unsigned int msr = serial_in(up, UART_MSR);
>> up->msr_saved_flags |= msr & MSR_SAVE_FLAGS;
>> if (msr & UART_MSR_CTS)
>> break;
>> udelay(1);
>> touch_nmi_watchdog();
>> }
>> }
>> }
>
> Does this sequence of checks cause data loss when CTS is deasserted?
> If wait_for_lsr() times out because the UART halts transmission, the holding
> register remains full. When CTS is subsequently asserted, the loop breaks
> and the function returns immediately without waiting for the holding register
> to actually empty. Will the next character overwrite the current one before it
> can be shifted out?
Correct. My patch does not address errors in the CTS polling, but
perhaps it should (since I am activating CTS polling). In this case I
suppose one final wait_for_lsr() is appropriate. Something like:
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 88f5309a940e9..18bc5600df181 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1984,16 +1984,20 @@ static bool wait_for_lsr(struct uart_8250_port *up, int bits)
static void wait_for_xmitr(struct uart_8250_port *up, int bits)
{
unsigned int tmout;
+ bool lsr_timeout;
- wait_for_lsr(up, bits);
+ lsr_timeout = wait_for_lsr(up, bits);
/* Wait up to 1s for flow control if necessary */
if (uart_console_hwflow_active(&up->port)) {
for (tmout = 1000000; tmout; tmout--) {
unsigned int msr = serial_in(up, UART_MSR);
up->msr_saved_flags |= msr & MSR_SAVE_FLAGS;
- if (msr & UART_MSR_CTS)
+ if (msr & UART_MSR_CTS) {
+ if (lsr_timeout)
+ wait_for_lsr(up, bits);
break;
+ }
udelay(1);
touch_nmi_watchdog();
}
> Also, could this polling loop cause a system stall?
> If the remote peer deasserts CTS (for example, if a terminal is disconnected),
> wait_for_xmitr() will poll for 1,000,000 microseconds. Since this is called
> for every single character during a printk, a standard 50-character printk
> could potentially stall the entire system for 50 seconds while holding the
> console lock.
Yes, this is a problem (for !PREEMPT_RT). But that is how console
hardware flow control is implemented. It is out of scope of my patch to
completely re-implement the CTS polling mechanism. AFAICT the other
serial drivers that already support console hardware flow control have
the same problem. Does this need to be resolved as a pre-requisite to my
patch?
>> @@ -3421,6 +3427,10 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
>> if (ret)
>> return ret;
>>
>> + /* Allow user-specified hardware flow control. */
>> + if (flow == 'r')
>> + port->flags |= UPF_CONS_FLOW;
>> +
>> if (port->dev)
>> pm_runtime_get_sync(port->dev);
>
> Should this flag be cleared if flow != 'r'? By only setting the flag, it
> appears hardware flow control cannot be dynamically disabled once it has
> been enabled.
The flag comes from a boot argument. Boot arguments never change during
runtime. The hardware flow control can still by dynamically disabled by
disabling CRTSCTS via termios. For example:
# stty -F /dev/ttyS0 -crtscts
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 89cebdd27841..cce1fd728a1e 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -2235,6 +2235,15 @@ uart_set_options(struct uart_port *port, struct console *co,
>> port->mctrl |= TIOCM_DTR;
>>
>> port->ops->set_termios(port, &termios, &dummy);
>> +
>> + /*
>> + * If console hardware flow control was specified and is supported,
>> + * the related policy UPSTAT_CTS_ENABLE must be set to allow console
>> + * drivers to identify if CTS should be used for polling.
>> + */
>> + if (flow == 'r' && (termios.c_cflag & CRTSCTS))
>> + port->status |= UPSTAT_CTS_ENABLE;
>> +
>
> Similar to the previous question, should UPSTAT_CTS_ENABLE be cleared
> if flow != 'r' to prevent the state from leaking?
This is a valid question. My patch is trying to be maximally
conservative and only make changes to allow CTS polling to become active
when 'r' is specified. For all other cases I do not want behavior
changes. Particularly since this is a function within serial_core.
> Additionally, does port->status need to be protected by the port lock here?
> Since this is a read-modify-write operation, and uart_set_options() can
> be invoked dynamically (like from kgdboc via tty_find_polling_driver()),
> could this race with concurrent modifications to port->status?
Yes. I was only considering the boot case where the tty layer is not yet
active. :-/ It seems some drivers are only taking the port lock for
@status updates.
The comments in serial_core.h say "Must hold termios_rwsem, port mutex
and port lock to change" but AFAICT the main @status modifiers are only
taking the last 2 locks. Still, the port mutex cannot be taken in
uart_set_options().
I would really like to leverage the UPSTAT_CTS_ENABLE policy bit. I will
need to think about this some more...
John Ogness
© 2016 - 2026 Red Hat, Inc.