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) {
