FAILED: patch "[PATCH] serial: 8250: Check UPF_IRQ_SHARED in advance" failed to apply to 4.14-stable tree
The patch below does not apply to the 4.14-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to stable@vger.kernel.org.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 7febbcbc48fc92e3f33863b32ed715ba4aff18c4 Mon Sep 17 00:00:00 2001
From: Andy Shevchenko andriy.shevchenko@linux.intel.com Date: Tue, 11 Feb 2020 15:55:59 +0200 Subject: [PATCH] serial: 8250: Check UPF_IRQ_SHARED in advance
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 Cc: stable stable@vger.kernel.org Acked-by: Kurt Kanzenbach kurt@linutronix.de Link: https://lore.kernel.org/r/20200211135559.85960-1-andriy.shevchenko@linux.int... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
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 0894a22fd702..f2a33c9082a6 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 430e3467aff7..0325f2e53b74 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -2177,6 +2177,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; /*
On Wed, Feb 26, 2020 at 11:36:04AM +0100, gregkh@linuxfoundation.org wrote:
The patch below does not apply to the 4.14-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to stable@vger.kernel.org.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 7febbcbc48fc92e3f33863b32ed715ba4aff18c4 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko andriy.shevchenko@linux.intel.com Date: Tue, 11 Feb 2020 15:55:59 +0200 Subject: [PATCH] serial: 8250: Check UPF_IRQ_SHARED in advance
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 Cc: stable stable@vger.kernel.org Acked-by: Kurt Kanzenbach kurt@linutronix.de Link: https://lore.kernel.org/r/20200211135559.85960-1-andriy.shevchenko@linux.int... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
For 4.14, I've worked around these missing commits:
5909c0bf9c7a ("serial/aspeed-vuart: Implement quick throttle mechanism") 989983ea849d ("serial/aspeed-vuart: Implement rx throttling") 54e53b2e8081 ("tty: serial: 8250: pass IRQ shared flag to UART ports")
And queued up a backport. Older kernels are a bit trickier than that.
On Wed, Feb 26, 2020 at 06:39:49PM -0500, Sasha Levin wrote:
On Wed, Feb 26, 2020 at 11:36:04AM +0100, gregkh@linuxfoundation.org wrote:
...
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 Cc: stable stable@vger.kernel.org Acked-by: Kurt Kanzenbach kurt@linutronix.de Link: https://lore.kernel.org/r/20200211135559.85960-1-andriy.shevchenko@linux.int... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
For 4.14, I've worked around these missing commits:
5909c0bf9c7a ("serial/aspeed-vuart: Implement quick throttle mechanism") 989983ea849d ("serial/aspeed-vuart: Implement rx throttling") 54e53b2e8081 ("tty: serial: 8250: pass IRQ shared flag to UART ports")
Thanks!
And queued up a backport. Older kernels are a bit trickier than that.
Since it's quite old bug and not many reports so far I guess we are not in hurry to fix that old kernels.
Hi,
On Thu Feb 27 2020, Andy Shevchenko wrote:
On Wed, Feb 26, 2020 at 06:39:49PM -0500, Sasha Levin wrote:
On Wed, Feb 26, 2020 at 11:36:04AM +0100, gregkh@linuxfoundation.org wrote:
...
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 Cc: stable stable@vger.kernel.org Acked-by: Kurt Kanzenbach kurt@linutronix.de Link: https://lore.kernel.org/r/20200211135559.85960-1-andriy.shevchenko@linux.int... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
For 4.14, I've worked around these missing commits:
5909c0bf9c7a ("serial/aspeed-vuart: Implement quick throttle mechanism") 989983ea849d ("serial/aspeed-vuart: Implement rx throttling") 54e53b2e8081 ("tty: serial: 8250: pass IRQ shared flag to UART ports")
Thanks!
And queued up a backport. Older kernels are a bit trickier than that.
Thanks for backporting. Why is it trickier?
Since it's quite old bug and not many reports so far I guess we are not in hurry to fix that old kernels.
It would be quite nice to have that in v4.9 as well. My original patch was made against that tree and tested.
Thanks, Kurt
On Fri, Feb 28, 2020 at 09:47:16AM +0100, Kurt Kanzenbach wrote:
Hi,
On Thu Feb 27 2020, Andy Shevchenko wrote:
On Wed, Feb 26, 2020 at 06:39:49PM -0500, Sasha Levin wrote:
On Wed, Feb 26, 2020 at 11:36:04AM +0100, gregkh@linuxfoundation.org wrote:
...
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 Cc: stable stable@vger.kernel.org Acked-by: Kurt Kanzenbach kurt@linutronix.de Link: https://lore.kernel.org/r/20200211135559.85960-1-andriy.shevchenko@linux.int... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
For 4.14, I've worked around these missing commits:
5909c0bf9c7a ("serial/aspeed-vuart: Implement quick throttle mechanism") 989983ea849d ("serial/aspeed-vuart: Implement rx throttling") 54e53b2e8081 ("tty: serial: 8250: pass IRQ shared flag to UART ports")
Thanks!
And queued up a backport. Older kernels are a bit trickier than that.
Thanks for backporting. Why is it trickier?
Since it's quite old bug and not many reports so far I guess we are not in hurry to fix that old kernels.
It would be quite nice to have that in v4.9 as well. My original patch was made against that tree and tested.
Hm, yes, looking at it now it doesn't look too tricky, I'm not sure what happened a few days ago :)
If you want to send me a backport I'll be happy to queue it up.
Hi Sasha,
On Fri Feb 28 2020, Sasha Levin wrote:
Hm, yes, looking at it now it doesn't look too tricky, I'm not sure what happened a few days ago :)
If you want to send me a backport I'll be happy to queue it up.
Sure. In v4.9 and older kernels the bug exists, but the workarounds aren't applied. So, Andy's patch becomes shorter.
Here's a backport for v4.9 (compile tested only):
====================================================================== From e819a733cc31d0a6b598aafafc8230fefa37f34a Mon Sep 17 00:00:00 2001 From: Andy Shevchenko andriy.shevchenko@linux.intel.com Date: Tue, 3 Mar 2020 08:49:55 +0100 Subject: [PATCH] serial: 8250: Check UPF_IRQ_SHARED in advance
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 [Kurt: Backport to v4.9] Signed-off-by: Kurt Kanzenbach kurt@linutronix.de --- drivers/tty/serial/8250/8250_core.c | 5 ++--- drivers/tty/serial/8250/8250_port.c | 4 ++++ 2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c index e8819aa20415..c4e9eba36023 100644 --- a/drivers/tty/serial/8250/8250_core.c +++ b/drivers/tty/serial/8250/8250_core.c @@ -181,7 +181,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);
@@ -216,9 +216,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, "serial", i); + up->port.irqflags, "serial", i); if (ret < 0) serial_do_unlink(i, up); } diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index 8f1233324586..c7a7574172fa 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -2199,6 +2199,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) { unsigned char iir1; /*
On Tue, Mar 03, 2020 at 09:01:23AM +0100, Kurt Kanzenbach wrote:
Hi Sasha,
On Fri Feb 28 2020, Sasha Levin wrote:
Hm, yes, looking at it now it doesn't look too tricky, I'm not sure what happened a few days ago :)
If you want to send me a backport I'll be happy to queue it up.
Sure. In v4.9 and older kernels the bug exists, but the workarounds aren't applied. So, Andy's patch becomes shorter.
Here's a backport for v4.9 (compile tested only):
Now applied, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
Greg KH -
gregkh＠linuxfoundation.org
-
Kurt Kanzenbach -
Sasha Levin