Re: [PATCH tty v2 2/2] serial: 8250: Add support for console hardware flow control

[John Ogness]

Hi,

Responding to Sashiko:

https://sashiko.dev/#/patchset/20260410144949.16581-1-john.ogness%40linutronix.de


On 2026-04-10, Sashiko wrote:
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index a739350e634f9..6c8830943b0c0 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -3425,6 +3431,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;
>> +
>
> Could this lead to a data race if port->flags is updated without
> holding the port mutex? include/linux/serial_core.h specifies that
> flags must be updated while holding the port mutex.

Yes. However, taking that lock makes no sense in the general context
(the lock probably does not even exist yet). The problem stems from the
fact that this console-only policy flag is being stored in a variable
used for tty properties. It does not belong there. For v3 I will move
this console-only policy flag into its own boolean.

> Also, if the console is dynamically reconfigured at runtime to disable flow
> control (e.g., flow == 'n'), should this code clear UPF_CONS_FLOW so the
> port does not erroneously retain its previous state?

Ack.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 89cebdd278410..a9ea10df4fb8b 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2235,6 +2235,18 @@ 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)) {
> +		/* Synchronize @status RMW update against the console. */
> +		guard(uart_port_lock_irq)(port);
> +		port->status |= UPSTAT_CTS_ENABLE;
> +	}
> +

> Will this unconditionally enable interrupts during early boot and
> cause a crash?
>
> uart_set_options() is called by serial8250_console_setup() during
> early boot console registration, which occurs when local interrupts
> are disabled.  When the guard goes out of scope, spin_unlock_irq()
> will unconditionally enable interrupts before the system is fully
> initialized.  Is it possible to use the IRQ-saving variant here, such
> as scoped_guard(uart_port_lock_irqsave, port), to safely restore the
> previous interrupt state?

Ack. v3 will switch to irqsave.

> Additionally, if the console is dynamically reconfigured at runtime to
> disable flow control, should UPSTAT_CTS_ENABLE be cleared in an else
> branch to prevent the console from getting stuck polling CTS?

I do not want to restrict drivers from setting the UPSTAT_CTS_ENABLE
policy bit when console flow control is disabled. For the case that
console flow control is requested, UPSTAT_CTS_ENABLE _must_ be set in
order for the CTS polling to function.

John Ogness