Re: [PATCH tty v4 2/6] serial: Replace driver usage of UPF_CONS_FLOW

[John Ogness]

Hi,

(This email is particularly directed at the Samsung folks.)

Responding to Sashiko:

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

On 2026-05-06, Sashiko wrote:
>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>> index e27806bf2cf3e..2f94fc798cffb 100644
>> --- a/drivers/tty/serial/samsung_tty.c
>> +++ b/drivers/tty/serial/samsung_tty.c
>> @@ -319,7 +319,7 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
>>  	ourport->tx_enabled = 0;
>>  	ourport->tx_in_progress = 0;
>>  
>> -	if (port->flags & UPF_CONS_FLOW)
>> +	if (uart_cons_flow_enabled(port))
>>  		s3c24xx_serial_rx_enable(port);
>
> This isn't a new bug introduced by this patch, but does this code
> result in a recursive spinlock acquisition and deadlock when console
> flow control is enabled?
>
> The serial core framework calls port->ops->stop_tx() with the
> port->lock spinlock already held. When uart_cons_flow_enabled(port)
> evaluates to true, s3c24xx_serial_stop_tx() invokes
> s3c24xx_serial_rx_enable().
>
> Looking at s3c24xx_serial_rx_enable(), it unconditionally attempts to
> acquire the exact same lock again:
>
> static void s3c24xx_serial_rx_enable(struct uart_port *port)
> {
> 	struct s3c24xx_uart_port *ourport = to_ourport(port);
> 	unsigned long flags;
> 	...
> 	uart_port_lock_irqsave(port, &flags);
> 	...
> }
>
> Because spinlocks are not recursive in the kernel, it appears this
> sequence would cause an immediate deadlock on the same CPU.

Although out of scope for my series, the reported issue is real. Perhaps
something for Krzysztof or Alim to look into.

>> @@ -493,7 +493,7 @@ static void s3c24xx_serial_start_tx(struct uart_port *port)
>>  	struct tty_port *tport = &port->state->port;
>>  
>>  	if (!ourport->tx_enabled) {
>> -		if (port->flags & UPF_CONS_FLOW)
>> +		if (uart_cons_flow_enabled(port))
>>  			s3c24xx_serial_rx_disable(port);
>
> Similarly here, s3c24xx_serial_start_tx() is also called with
> port->lock held, and s3c24xx_serial_rx_disable() will attempt to
> acquire port->lock again:
>
> static void s3c24xx_serial_rx_disable(struct uart_port *port)
> {
> 	struct s3c24xx_uart_port *ourport = to_ourport(port);
> 	unsigned long flags;
> 	...
> 	uart_port_lock_irqsave(port, &flags);
> 	...
> }
>
> Could this pre-existing locking issue in the samsung_tty driver be
> addressed so that the rx enable/disable helpers do not try to take the
> port lock when it is already held by the caller?

Also legitimate. But out of scope for my series.

John Ogness