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

[Kurt Kanzenbach <kurt@linutronix.de>]

[-- 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 --]