Re: [PATCH v2] serial: imx: Fix sysrq deadlock

[Johan Hovold <johan@kernel.org>]

On Wed, Sep 29, 2021 at 06:43:24PM -0300, Fabio Estevam wrote:
> The following sysrq command causes the following deadlock:
> 
>  # echo t > /proc/sysrq-trigger
>  ....
> [   20.325246] ======================================================
> [   20.325252] WARNING: possible circular locking dependency detected
> [   20.325260] 5.15.0-rc2-next-20210924-00004-gd2d6e664f29f-dirty #163
> Not tainted
> [   20.325273] ------------------------------------------------------
> [   20.325279] sh/236 is trying to acquire lock:
> [   20.325293] c1618614 (console_owner){-...}-{0:0}, at:
> console_unlock+0x180/0x5bc
> [   20.325361]
> [   20.325361] but task is already holding lock:
> [   20.325368] eefccc90 (&pool->lock){-.-.}-{2:2}, at:
> show_workqueue_state+0x104/0x3c8
> [   20.325432]
> [   20.325432] which lock already depends on the new lock.
> 
> ...
> 
> [   20.325657] -> #2 (&pool->lock/1){-.-.}-{2:2}:
> [   20.325690]        __queue_work+0x114/0x810
> [   20.325710]        queue_work_on+0x54/0x94
> [   20.325727]        __imx_uart_rxint.constprop.0+0x1b4/0x2e0
> [   20.325760]        imx_uart_int+0x270/0x310
> 
> This problem happens because uart_handle_sysrq_char() is called
> with the lock held.
> 
> Fix this by using the same approach done in commit 5697df7322fe ("serial:
> fsl_lpuart: split sysrq handling"), which calls 
> uart_unlock_and_check_sysrq() to drop the lock prior to 
> uart_handle_sysrq_char().
> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - I noticed that when sending break + t via the terminal, the characters
> were sometimes lost. Do the minimal changes to fix the deadlock without
> missing the sysrq input.
> 
>  drivers/tty/serial/imx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8b121cd869e9..1c768dd3896d 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -788,6 +788,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  	unsigned int rx, flg, ignored = 0;
>  	struct tty_port *port = &sport->port.state->port;
>  
> +	uart_unlock_and_check_sysrq(&sport->port);

This is just so broken; you can't just drop the lock. And you clearly
haven't even tried to understand how uart_unlock_and_check_sysrq()
works.

Please take a closer look at the commit you're trying to mimic.

>  	while (imx_uart_readl(sport, USR2) & USR2_RDR) {
>  		u32 usr2;
>  
> @@ -846,6 +847,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  out:
>  	tty_flip_buffer_push(port);
>  
> +	spin_lock(&sport->port.lock);
>  	return IRQ_HANDLED;
>  }

Johan