Re: [PATCH v3] serial: imx: Suppress false positive sysrq lockdep warning

[Johan Hovold <johan@kernel.org>]

On Fri, Oct 01, 2021 at 07:18:15AM -0300, Fabio Estevam wrote:
> The following sysrq command causes the following lockdep warning:
> 
>  # 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_prepare_sysrq_char()
> and uart_unlock_and_check_sysrq() instead.
> 
> Its commit log says:
> 
> "Instead of uart_handle_sysrq_char() use uart_prepare_sysrq_char() and
> uart_unlock_and_check_sysrq(). This will call handle_sysrq() without
> holding the port lock, which in turn let us drop the spin_trylock hack."
> 
> Do the same here to suppress the false positive lockdep warning.
> 
> As __imx_uart_rxint() drops the lock now, remove the spin_unlock()
> inside imx_uart_rxint(), which is only used on i.MX1.
> 
> Tested on a i.MX7D board via 'echo t > /proc/sysrq-trigger' in the
> command line and also by passing the "<break> + t" keys in
> the serial console.
> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v2:
> - Keep the cast when calling uart_prepare_sysrq_char() - Johan
> - Improve commit log and subject - Johan

You changed more things since v2 as that was the broken version that
just dropped the lock.

>  drivers/tty/serial/imx.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8b121cd869e9..a0135718c588 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -803,7 +803,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  				continue;
>  		}
>  
> -		if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
> +		if (uart_prepare_sysrq_char(&sport->port, (unsigned char)rx))
>  			continue;
>  
>  		if (unlikely(rx & URXD_ERR)) {
> @@ -844,6 +844,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  	}
>  
>  out:
> +	uart_unlock_and_check_sysrq(&sport->port);
>  	tty_flip_buffer_push(port);
>  
>  	return IRQ_HANDLED;
> @@ -852,15 +853,10 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  static irqreturn_t imx_uart_rxint(int irq, void *dev_id)
>  {
>  	struct imx_port *sport = dev_id;
> -	irqreturn_t ret;
>  
>  	spin_lock(&sport->port.lock);
>  
> -	ret = __imx_uart_rxint(irq, dev_id);
> -
> -	spin_unlock(&sport->port.lock);

No, no, no.

Just replace this unlock with uart_unlock_and_check_sysrq() and do the
corresponding change in imx_uart_int(). The result is an even smaller
diff than what you're currently proposing and without any performance
penalty from dropping and reacquiring the lock.

> -
> -	return ret;
> +	return __imx_uart_rxint(irq, dev_id);
>  }
>  
>  static void imx_uart_clear_rx_errors(struct imx_port *sport);
> @@ -959,6 +955,7 @@ static irqreturn_t imx_uart_int(int irq, void *dev_id)
>  		imx_uart_writel(sport, USR1_AGTIM, USR1);
>  
>  		__imx_uart_rxint(irq, dev_id);
> +		spin_lock(&sport->port.lock);
>  		ret = IRQ_HANDLED;
>  	}

Johan