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

On 2026-05-13, Krzysztof Kozlowski <[email protected]> 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!)

John Ogness