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

[Tudor Ambarus <tudor.ambarus@linaro.org>]

On 5/15/26 10:53 AM, John Ogness wrote:
> On 2026-05-13, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> (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. :)
> 
> Nothing is missing. I am guessing you never use console flow
> control. The deadlock is clearly visible:
> 
> ->stop_tx() (always called with the port locked)
>   s3c24xx_serial_stop_tx()
>     s3c24xx_serial_rx_enable()
>       uart_port_lock_irqsave() (DEADLOCK!)
> 

Right.

The lock acquisitions in the rx helper functions are redundant and shall be
removed.

The serial core framework invokes the .stop_tx() and .start_tx() callbacks
with the port->lock spinlock already held. Furthermore, all internal driver
paths that invoke stop_tx/start_tx also acquire port->lock prior to calling
them.
    
However, s3c24xx_serial_rx_enable() and s3c24xx_serial_rx_disable()
unconditionally attempt to acquire port->lock again using
uart_port_lock_irqsave(). Since kernel spinlocks are not recursive, this
causes a deadlock on the same CPU when console flow control is engaged.

Just removing the redundant lock acquisitions shall fix it. I'll prepare
a patch.

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index e27806bf2cf3..17cd5bb100b1 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -245,12 +245,9 @@ static bool s3c24xx_serial_txempty_nofifo(const struct uart_port *port)
 static void s3c24xx_serial_rx_enable(struct uart_port *port)
 {
        struct s3c24xx_uart_port *ourport = to_ourport(port);
-       unsigned long flags;
        int count = 10000;
        u32 ucon, ufcon;
 
-       uart_port_lock_irqsave(port, &flags);
-
        while (--count && !s3c24xx_serial_txempty_nofifo(port))
                udelay(100);
 
@@ -263,23 +260,18 @@ static void s3c24xx_serial_rx_enable(struct uart_port *port)
        wr_regl(port, S3C2410_UCON, ucon);
 
        ourport->rx_enabled = 1;
-       uart_port_unlock_irqrestore(port, flags);
 }
 
 static void s3c24xx_serial_rx_disable(struct uart_port *port)
 {
        struct s3c24xx_uart_port *ourport = to_ourport(port);
-       unsigned long flags;
        u32 ucon;
 
-       uart_port_lock_irqsave(port, &flags);
-
        ucon = rd_regl(port, S3C2410_UCON);
        ucon &= ~S3C2410_UCON_RXIRQMODE;
        wr_regl(port, S3C2410_UCON, ucon);
 
        ourport->rx_enabled = 0;
-       uart_port_unlock_irqrestore(port, flags);
 }
 
 static void s3c24xx_serial_stop_tx(struct uart_port *port)