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

[Fabio Estevam <festevam@denx.de>]

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