Re: [PATCH 2/3] serial: stm32: fix threaded interrupt handling

[Valentin CARON - foss]

Hi, Johan

On 4/16/21 4:05 PM, Johan Hovold wrote:
> When DMA is enabled the receive handler runs in a threaded handler, but
> the primary handler up until very recently neither disabled interrupts
> in the device or used IRQF_ONESHOT. This would lead to a deadlock if an
> interrupt comes in while the threaded receive handler is running under
> the port lock.
>
> Commit ad7676812437 ("serial: stm32: fix a deadlock condition with
> wakeup event") claimed to fix an unrelated deadlock, but unfortunately
> also disabled interrupts in the threaded handler. While this prevents
> the deadlock mentioned in the previous paragraph it also defeats the
> purpose of using a threaded handler in the first place.
>
> Fix this by making the interrupt one-shot and not disabling interrupts
> in the threaded handler.
>
> Note that (receive) DMA must not be used for a console port as the
> threaded handler could be interrupted while holding the port lock,
> something which could lead to a deadlock in case an interrupt handler
> ends up calling printk.
>
> Fixes: ad7676812437 ("serial: stm32: fix a deadlock condition with wakeup event")
> Fixes: 3489187204eb ("serial: stm32: adding dma support")
> Cc: stable@xxxxxxxxxxxxxxx      # 4.9
> Cc: Alexandre TORGUE <alexandre.torgue@xxxxxx>
> Cc: Gerald Baeza <gerald.baeza@xxxxxx>
> Signed-off-by: Johan Hovold <johan@xxxxxxxxxx>

Reviewed-by: Valentin Caron<valentin.caron@xxxxxxxxxxx>

> ---
>   drivers/tty/serial/stm32-usart.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 4d277804c63e..3524ed2c0c73 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -214,14 +214,11 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)
>   	struct tty_port *tport = &port->state->port;
>   	struct stm32_port *stm32_port = to_stm32_port(port);
>   	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> -	unsigned long c, flags;
> +	unsigned long c;
>   	u32 sr;
>   	char flag;
>   
> -	if (threaded)
> -		spin_lock_irqsave(&port->lock, flags);
> -	else
> -		spin_lock(&port->lock);
> +	spin_lock(&port->lock);
>   
>   	while (stm32_usart_pending_rx(port, &sr, &stm32_port->last_res,
>   				      threaded)) {
> @@ -278,10 +275,7 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)
>   		uart_insert_char(port, sr, USART_SR_ORE, c, flag);
>   	}
>   
> -	if (threaded)
> -		spin_unlock_irqrestore(&port->lock, flags);
> -	else
> -		spin_unlock(&port->lock);
> +	spin_unlock(&port->lock);
>   
>   	tty_flip_buffer_push(tport);
>   }
> @@ -667,7 +661,8 @@ static int stm32_usart_startup(struct uart_port *port)
>   
>   	ret = request_threaded_irq(port->irq, stm32_usart_interrupt,
>   				   stm32_usart_threaded_interrupt,
> -				   IRQF_NO_SUSPEND, name, port);
> +				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
> +				   name, port);
>   	if (ret)
>   		return ret;
>   
> @@ -1156,6 +1151,13 @@ static int stm32_usart_of_dma_rx_probe(struct stm32_port *stm32port,
>   	struct dma_async_tx_descriptor *desc = NULL;
>   	int ret;
>   
> +	/*
> +	 * Using DMA and threaded handler for the console could lead to
> +	 * deadlocks.
> +	 */
> +	if (uart_console(port))
> +		return -ENODEV;
> +
>   	/* Request DMA RX channel */
>   	stm32port->rx_ch = dma_request_slave_channel(dev, "rx");
>   	if (!stm32port->rx_ch) {