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

[Fabio Estevam <festevam@xxxxxxx>]

Hi Johan,

On 01/10/2021 10:56, Johan Hovold wrote:

> > @@ -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

This does not work as uart_unlock_and_check_sysrq() needs to be
called inside __imx_uart_rxint() prior to tty_flip_buffer_push().

> 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.

Yes, I can avoid the unnecessary reacquiring of the lock as you
suggested:

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8b121cd869e9..5e38bf8fb7b8 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);
-
-	return ret;
+	return __imx_uart_rxint(irq, dev_id);
 }

 static void imx_uart_clear_rx_errors(struct imx_port *sport);
@@ -955,13 +951,6 @@ static irqreturn_t imx_uart_int(int irq, void 
*dev_id)
 	if ((ucr4 & UCR4_OREN) == 0)
 		usr2 &= ~USR2_ORE;

-	if (usr1 & (USR1_RRDY | USR1_AGTIM)) {
-		imx_uart_writel(sport, USR1_AGTIM, USR1);
-
-		__imx_uart_rxint(irq, dev_id);
-		ret = IRQ_HANDLED;
-	}
-
 	if ((usr1 & USR1_TRDY) || (usr2 & USR2_TXDC)) {
 		imx_uart_transmit_buffer(sport);
 		ret = IRQ_HANDLED;
@@ -991,8 +980,17 @@ static irqreturn_t imx_uart_int(int irq, void 
*dev_id)
 		ret = IRQ_HANDLED;
 	}

+	if (usr1 & (USR1_RRDY | USR1_AGTIM)) {
+		imx_uart_writel(sport, USR1_AGTIM, USR1);
+
+		__imx_uart_rxint(irq, dev_id);
+		ret = IRQ_HANDLED;
+		goto out;
+	}
+
 	spin_unlock(&sport->port.lock);

+out:
 	return ret;
 }

@@ -1977,9 +1975,7 @@ imx_uart_console_write(struct console *co, const 
char *s, unsigned int count)
 	unsigned int ucr1;
 	int locked = 1;

-	if (sport->port.sysrq)
-		locked = 0;
-	else if (oops_in_progress)
+	if (oops_in_progress)
 		locked = spin_trylock_irqsave(&sport->port.lock, flags);
 	else
 		spin_lock_irqsave(&sport->port.lock, flags);