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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 07/05/2026 11:50, John Ogness wrote:
> 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.


Thanks for letting us know. Deadlock did not happen so far, so something
is missing in Sashiko's report. :)

I am just back from vacation, so I won't have time to dig into this, but
I will keep your email in the inbox. Cc-ing also a few Linaro folks
which are using this platform and might be able to help us here.

Best regards,
Krzysztof