Hi Johan,

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

quoted

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