[PATCH v1] serial: 8250: Check UPF_IRQ_SHARED in advance

[PATCH v1] serial: 8250: Check UPF_IRQ_SHARED in advance

[Andy Shevchenko]

The commit 54e53b2e8081
  ("tty: serial: 8250: pass IRQ shared flag to UART ports")
nicely explained the problem:

---8<---8<---

On some systems IRQ lines between multiple UARTs might be shared. If so, the
irqflags have to be configured accordingly. The reason is: The 8250 port startup
code performs IRQ tests *before* the IRQ handler for that particular port is
registered. This is performed in serial8250_do_startup(). This function checks
whether IRQF_SHARED is configured and only then disables the IRQ line while
testing.

This test is performed upon each open() of the UART device. Imagine two UARTs
share the same IRQ line: On is already opened and the IRQ is active. When the
second UART is opened, the IRQ line has to be disabled while performing IRQ
tests. Otherwise an IRQ might handler might be invoked, but the IRQ itself
cannot be handled, because the corresponding handler isn't registered,
yet. That's because the 8250 code uses a chain-handler and invokes the
corresponding port's IRQ handling routines himself.

Unfortunately this IRQF_SHARED flag isn't configured for UARTs probed via device
tree even if the IRQs are shared. This way, the actual and shared IRQ line isn't
disabled while performing tests and the kernel correctly detects a spurious
IRQ. So, adding this flag to the DT probe solves the issue.

Note: The UPF_SHARE_IRQ flag is configured unconditionally. Therefore, the
IRQF_SHARED flag can be set unconditionally as well.

Example stack trace by performing `echo 1 > /dev/ttyS2` on a non-patched system:

|irq 85: nobody cared (try booting with the "irqpoll" option)
| [...]
|handlers:
|[<ffff0000080fc628>] irq_default_primary_handler threaded [<ffff00000855fbb8>] serial8250_interrupt
|Disabling IRQ #85

---8<---8<---

But unfortunately didn't fix the root cause. Let's try again here by moving
IRQ flag assignment from serial_link_irq_chain() to serial8250_do_startup().

This should fix the similar issue reported for 8250_pnp case.

Since this change we don't need to have custom solutions in 8250_aspeed_vuart
and 8250_of drivers, thus, drop them.

Fixes: 1c2f04937b3e ("serial: 8250: add IRQ trigger support")
Reported-by: Li RongQing <lirongqing@baidu.com>
Cc: Kurt Kanzenbach <kurt@linutronix.de>
Cc: Vikram Pandita <vikram.pandita@ti.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 1 -
 drivers/tty/serial/8250/8250_core.c         | 5 ++---
 drivers/tty/serial/8250/8250_of.c           | 1 -
 drivers/tty/serial/8250/8250_port.c         | 4 ++++
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index d657aa14c3e4..c33e02cbde93 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -446,7 +446,6 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 		port.port.line = rc;
 
 	port.port.irq = irq_of_parse_and_map(np, 0);
-	port.port.irqflags = IRQF_SHARED;
 	port.port.handle_irq = aspeed_vuart_handle_irq;
 	port.port.iotype = UPIO_MEM;
 	port.port.type = PORT_16550A;
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index ee03eaa80b34..6fb3e9c399f8 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -174,7 +174,7 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
 	struct hlist_head *h;
 	struct hlist_node *n;
 	struct irq_info *i;
-	int ret, irq_flags = up->port.flags & UPF_SHARE_IRQ ? IRQF_SHARED : 0;
+	int ret;
 
 	mutex_lock(&hash_mutex);
 
@@ -209,9 +209,8 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
 		INIT_LIST_HEAD(&up->list);
 		i->head = &up->list;
 		spin_unlock_irq(&i->lock);
-		irq_flags |= up->port.irqflags;
 		ret = request_irq(up->port.irq, serial8250_interrupt,
-				  irq_flags, up->port.name, i);
+				  up->port.irqflags, up->port.name, i);
 		if (ret < 0)
 			serial_do_unlink(i, up);
 	}
diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index 531ad67395e0..f6687756ec5e 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -202,7 +202,6 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
 
 	port->type = type;
 	port->uartclk = clk;
-	port->irqflags |= IRQF_SHARED;
 
 	if (of_property_read_bool(np, "no-loopback-test"))
 		port->flags |= UPF_SKIP_TEST;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ef96042ab3e8..d60c4ccaf312 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2137,6 +2137,10 @@ int serial8250_do_startup(struct uart_port *port)
 		}
 	}
 
+	/* Check if we need to have shared IRQs */
+	if (port->irq && (up->port.flags & UPF_SHARE_IRQ))
+		up->port.irqflags |= IRQF_SHARED;
+
 	if (port->irq && !(up->port.flags & UPF_NO_THRE_TEST)) {
 		unsigned char iir1;
 		/*
-- 
2.25.0

Re: [PATCH v1] serial: 8250: Check UPF_IRQ_SHARED in advance

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 2588 bytes --]

Hi,

On Tue Feb 11 2020, Andy Shevchenko wrote:
> The commit 54e53b2e8081
>   ("tty: serial: 8250: pass IRQ shared flag to UART ports")
> nicely explained the problem:
>
> ---8<---8<---
>
> On some systems IRQ lines between multiple UARTs might be shared. If so, the
> irqflags have to be configured accordingly. The reason is: The 8250 port startup
> code performs IRQ tests *before* the IRQ handler for that particular port is
> registered. This is performed in serial8250_do_startup(). This function checks
> whether IRQF_SHARED is configured and only then disables the IRQ line while
> testing.
>
> This test is performed upon each open() of the UART device. Imagine two UARTs
> share the same IRQ line: On is already opened and the IRQ is active. When the
> second UART is opened, the IRQ line has to be disabled while performing IRQ
> tests. Otherwise an IRQ might handler might be invoked, but the IRQ itself
> cannot be handled, because the corresponding handler isn't registered,
> yet. That's because the 8250 code uses a chain-handler and invokes the
> corresponding port's IRQ handling routines himself.
>
> Unfortunately this IRQF_SHARED flag isn't configured for UARTs probed via device
> tree even if the IRQs are shared. This way, the actual and shared IRQ line isn't
> disabled while performing tests and the kernel correctly detects a spurious
> IRQ. So, adding this flag to the DT probe solves the issue.
>
> Note: The UPF_SHARE_IRQ flag is configured unconditionally. Therefore, the
> IRQF_SHARED flag can be set unconditionally as well.
>
> Example stack trace by performing `echo 1 > /dev/ttyS2` on a non-patched system:
>
> |irq 85: nobody cared (try booting with the "irqpoll" option)
> | [...]
> |handlers:
> |[<ffff0000080fc628>] irq_default_primary_handler threaded [<ffff00000855fbb8>] serial8250_interrupt
> |Disabling IRQ #85
>
> ---8<---8<---
>
> But unfortunately didn't fix the root cause. Let's try again here by moving
> IRQ flag assignment from serial_link_irq_chain() to serial8250_do_startup().
>
> This should fix the similar issue reported for 8250_pnp case.
>
> Since this change we don't need to have custom solutions in 8250_aspeed_vuart
> and 8250_of drivers, thus, drop them.
>
> Fixes: 1c2f04937b3e ("serial: 8250: add IRQ trigger support")
> Reported-by: Li RongQing <lirongqing@baidu.com>
> Cc: Kurt Kanzenbach <kurt@linutronix.de>
> Cc: Vikram Pandita <vikram.pandita@ti.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The code looks alright to me.

 Acked-by: Kurt Kanzenbach <kurt@linutronix.de>

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]