Re: [PATCH tty v1] serial: 8250: Add support for console hardware flow control

[John Ogness]

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