:p
atchew
Login
This series fixes a common bug pattern found in several Xen UART drivers: when setup_irq() fails during post-IRQ initialization, drivers log the error but continue executing and unconditionally unmask hardware interrupt lines with no handler registered. On platforms where the GIC receives these asserted lines, the result is either repeated spurious-interrupt warnings or an unhandled interrupt fault. Patch 1, originally submitted as a standalone fix in v1, addresses the issue in the SCIF driver and moves the hardware error-flag clearing sequence to before the setup_irq() call so that stale error bits are cleared unconditionally. Patches 2-4 apply the same early-return fix to the pl011, cadence-uart, and exynos4210-uart drivers. The pl011 and cadence-uart drivers additionally had a positive-condition IRQ guard (if uart->irq > 0) that could fall through to the interrupt unmask even when no valid IRQ was provided. Both are restructured to use the early-return idiom. Changes in v2: - Extend fix to pl011, cadence-uart and exynos4210 - fix typo in patch 1 description Oleksii Moisieiev (4): xen/drivers/char: fix SCIF IRQ registration failure propagation xen/drivers/char/pl011: fix IRQ registration failure propagation xen/drivers/char/cadence-uart: fix IRQ registration failure propagation xen/drivers/char: fix exynos4210 IRQ registration failure propagation xen/drivers/char/cadence-uart.c | 17 +++++++++++------ xen/drivers/char/exynos4210-uart.c | 4 ++++ xen/drivers/char/pl011.c | 17 +++++++++++------ xen/drivers/char/scif-uart.c | 16 ++++++++++++---- 4 files changed, 38 insertions(+), 16 deletions(-) -- 2.43.0 base-commit: 2ca756d39f59f834160263fd142ef91c6ca3dd90 branch: amoi_dfmea_scif
In scif_uart_init_postirq(), when setup_irq() returns an error the failure was only logged via dprintk() and execution continued, unconditionally writing TIE|RIE|REIE into the Serial Control Register (SCSCR). This armed all three hardware interrupt lines (TX FIFO empty, RX data ready, receive error) with no handler registered to service them. On platforms where the GIC receives these asserted lines, the result is either repeated spurious-interrupt warnings or an unhandled interrupt fault. The fix adds an early return inside the error branch. The interrupt-enable write to SCSCR is skipped entirely when no handler is registered. SCIF TX continues to operate correctly after this change. The Xen serial framework never calls serial_async_transmit() for SCIF, so port->txbuf is always NULL. This causes __serial_putc() to take the synchronous finite-capacity path, which polls the SCFSR_TDFE hardware flag directly and does not depend on the interrupt mechanism. RX wouldn't work if irq wasn't registered. As a secondary clean-up, the hardware error-flag clearing sequence is moved to before the setup_irq() call so that error bits accumulated since init_preirq() are cleared unconditionally, regardless of whether IRQ registration succeeds. Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> --- Changes in v2: - Extend fix to pl011, cadence-uart and exynos4210 - fix typo in patch 1 description xen/drivers/char/scif-uart.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/scif-uart.c +++ b/xen/drivers/char/scif-uart.c @@ -XXX,XX +XXX,XX @@ static void __init scif_uart_init_postirq(struct serial_port *port) uart->irqaction.name = "scif_uart"; uart->irqaction.dev_id = port; - if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) - dprintk(XENLOG_ERR, "Failed to allocated scif_uart IRQ %d\n", - uart->irq); - /* Clear all errors */ if ( scif_readw(uart, params->status_reg) & params->error_mask ) scif_writew(uart, params->status_reg, ~params->error_mask); if ( scif_readw(uart, params->overrun_reg) & params->overrun_mask ) scif_writew(uart, params->overrun_reg, ~params->overrun_mask); + if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) + { + dprintk(XENLOG_ERR, "Failed to allocated scif_uart IRQ %d\n", + uart->irq); + /* + * If the IRQ handler could not be installed (setup_irq failed), + * do not enable TX/RX or error interrupts. Serial transmit will + * fall back to polling mode. + */ + return; + } + /* Enable TX/RX and Error Interrupts */ scif_writew(uart, SCIF_SCSCR, scif_readw(uart, SCIF_SCSCR) | params->irq_flags); -- 2.43.0
In pl011_init_postirq(), two code paths could reach the interrupt-unmask write to IMSC without a handler being registered: - When no valid IRQ number was provided (uart->irq <= 0), the original positive-condition guard (if uart->irq > 0) skipped the irqaction setup but still fell through to the IMSC write, unmasking RTI|OEI|BEI|PEI|FEI|TXI|RXI with no handler installed. - When setup_irq() returned an error, only an error message was printed and execution continued to the IMSC write, arming all hardware interrupt lines with no handler to service them. On platforms where the GIC receives these asserted lines, the result is either repeated spurious-interrupt warnings or an unhandled interrupt fault. Restructure pl011_init_postirq() to use early returns: return immediately when no valid IRQ is provided, and return after logging the error when setup_irq() fails. The interrupt-enable write to IMSC is only reached when IRQ registration succeeds. Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> --- xen/drivers/char/pl011.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/pl011.c +++ b/xen/drivers/char/pl011.c @@ -XXX,XX +XXX,XX @@ static void __init pl011_init_postirq(struct serial_port *port) struct pl011 *uart = port->uart; int rc; - if ( uart->irq > 0 ) + /* Don't unmask interrupts if no valid irq was provided */ + if ( uart->irq <= 0 ) + return; + + uart->irqaction.handler = pl011_interrupt; + uart->irqaction.name = "pl011"; + uart->irqaction.dev_id = port; + if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) { - uart->irqaction.handler = pl011_interrupt; - uart->irqaction.name = "pl011"; - uart->irqaction.dev_id = port; - if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) - printk("ERROR: Failed to allocate pl011 IRQ %d\n", uart->irq); + printk("ERROR: Failed to allocate pl011 IRQ %d\n", uart->irq); + /* Do not unmask interrupts if irq handler wasn't set */ + return; } /* Clear pending error interrupts */ -- 2.43.0
In cuart_init_postirq(), two code paths could reach the interrupt-enable write to IER without a handler being registered: - When no valid IRQ number was provided (uart->irq <= 0), the original positive-condition guard (if uart->irq > 0) skipped the irqaction setup but still fell through to the IER write, enabling the receive data interrupt with no handler installed. - When setup_irq() returned an error, only an error message was printed and execution continued to the IER write, arming the receive hardware interrupt line with no handler to service it. On platforms where the GIC receives this asserted line, the result is either repeated spurious-interrupt warnings or an unhandled interrupt fault. Restructure cuart_init_postirq() to use early returns in both error paths. Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> --- xen/drivers/char/cadence-uart.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/xen/drivers/char/cadence-uart.c b/xen/drivers/char/cadence-uart.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/cadence-uart.c +++ b/xen/drivers/char/cadence-uart.c @@ -XXX,XX +XXX,XX @@ static void __init cuart_init_postirq(struct serial_port *port) struct cuart *uart = port->uart; int rc; - if ( uart->irq > 0 ) + /* Don't unmask interrupts if no valid irq was provided */ + if ( uart->irq <= 0 ) + return; + + uart->irqaction.handler = cuart_interrupt; + uart->irqaction.name = "cadence-uart"; + uart->irqaction.dev_id = port; + if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) { - uart->irqaction.handler = cuart_interrupt; - uart->irqaction.name = "cadence-uart"; - uart->irqaction.dev_id = port; - if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) - printk("ERROR: Failed to allocate cadence-uart IRQ %d\n", uart->irq); + printk("ERROR: Failed to allocate cadence-uart IRQ %d\n", uart->irq); + /* Do not unmask interrupts if irq handler wasn't set */ + return; } /* Clear pending error interrupts */ -- 2.43.0
In exynos4210_uart_init_postirq(), when setup_irq() returns an error the failure was only logged via dprintk() and execution continued, unconditionally clearing UINTM and setting UMCON_INT_EN. This enabled receive and transmit interrupt lines with no handler registered. On platforms where the GIC receives these asserted lines, the result is either repeated spurious-interrupt warnings or an unhandled interrupt fault. Add an early return in the setup_irq() error branch so that the interrupt-enable writes to UINTM and UMCON are skipped when IRQ registration fails. Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> --- xen/drivers/char/exynos4210-uart.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/xen/drivers/char/exynos4210-uart.c b/xen/drivers/char/exynos4210-uart.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/exynos4210-uart.c +++ b/xen/drivers/char/exynos4210-uart.c @@ -XXX,XX +XXX,XX @@ static void __init exynos4210_uart_init_postirq(struct serial_port *port) uart->irqaction.dev_id = port; if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) + { dprintk(XENLOG_ERR, "Failed to allocated exynos4210_uart IRQ %d\n", uart->irq); + /* Don't enable interrupts if irq setup was failed */ + return; + } /* Unmask interrupts */ exynos4210_write(uart, UINTM, ~UINTM_ALLI); -- 2.43.0
This series fixes a common bug pattern found in several Xen UART drivers: when setup_irq() fails during post-IRQ initialization, drivers log the error but continue executing and unconditionally unmask hardware interrupt lines with no handler registered. On platforms where the GIC receives these asserted lines, the result is either repeated spurious-interrupt warnings or an unhandled interrupt fault. Patch 1, originally submitted as a standalone fix in v1, addresses the issue in the SCIF driver and moves the hardware error-flag clearing sequence to before the setup_irq() call so that stale error bits are cleared unconditionally. Patches 2-4 apply the same early-return fix to the pl011, cadence-uart, and exynos4210-uart drivers. The pl011 and cadence-uart drivers additionally had a positive-condition IRQ guard (if uart->irq > 0) that could fall through to the interrupt unmask even when no valid IRQ was provided. Both are restructured to use the early-return idiom. Changes in v4: - update comment for setup_irq - add R-b - change %d to %u in printk since irq is unsigned - fix uart->irq <= 0 to uart->irq == 0 since it's unsigned - update %d to %u in printk since irq is unsigned - add r-b - fix %d to %u since irq is unsigned in cadence uart - add R-b - update comment on setup_irq - change %d to %u since irq is unsigned in exynos - add r-b Changes in v3: - clear pending error interrupts before setup_irq for pl011 - clear pending error interrupts before setup_irq call for cadence uart - change uart->irq <= 0 to uart->irq == 0 since irq is unsigned - skip clearing pending interrupts if setup_irq was failed because according to the 13.4.1.13 of the RM: it must be cleared after cleaning interrupt pending in INTC. Changes in v2: - Extend fix to pl011, cadence-uart and exynos4210 - fix typo in patch 1 description Oleksii Moisieiev (4): xen/drivers/char: fix SCIF IRQ registration failure propagation xen/drivers/char/pl011: fix IRQ registration failure propagation xen/drivers/char/cadence-uart: fix IRQ registration failure propagation xen/drivers/char: fix exynos4210 IRQ registration failure propagation xen/drivers/char/cadence-uart.c | 22 ++++++++++++++-------- xen/drivers/char/exynos4210-uart.c | 6 +++++- xen/drivers/char/pl011.c | 22 ++++++++++++++-------- xen/drivers/char/scif-uart.c | 12 ++++++++---- 4 files changed, 41 insertions(+), 21 deletions(-) -- 2.43.0 base-commit: 077dcf9841ad7df4e63c718249d8ac95f8a709ff branch: amoi_dfmea_scifv4
In scif_uart_init_postirq(), when setup_irq() returns an error the failure was only logged via dprintk() and execution continued, unconditionally writing TIE|RIE|REIE into the Serial Control Register (SCSCR). This armed all three hardware interrupt lines (TX FIFO empty, RX data ready, receive error) with no handler registered to service them. On platforms where the GIC receives these asserted lines, the result is either repeated spurious-interrupt warnings or an unhandled interrupt fault. The fix adds an early return inside the error branch. The interrupt-enable write to SCSCR is skipped entirely when no handler is registered. SCIF TX continues to operate correctly after this change. The Xen serial framework never calls serial_async_transmit() for SCIF, so port->txbuf is always NULL. This causes __serial_putc() to take the synchronous finite-capacity path, which polls the SCFSR_TDFE hardware flag directly and does not depend on the interrupt mechanism. RX wouldn't work if irq wasn't registered. As a secondary clean-up, the hardware error-flag clearing sequence is moved to before the setup_irq() call so that error bits accumulated since init_preirq() are cleared unconditionally, regardless of whether IRQ registration succeeds. Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> --- Changes in v4: - update comment for setup_irq - add R-b - change %d to %u in printk since irq is unsigned Changes in v2: - Extend fix to pl011, cadence-uart and exynos4210 - fix typo in patch 1 description xen/drivers/char/scif-uart.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/scif-uart.c +++ b/xen/drivers/char/scif-uart.c @@ -XXX,XX +XXX,XX @@ static void __init scif_uart_init_postirq(struct serial_port *port) uart->irqaction.name = "scif_uart"; uart->irqaction.dev_id = port; - if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) - dprintk(XENLOG_ERR, "Failed to allocated scif_uart IRQ %d\n", - uart->irq); - /* Clear all errors */ if ( scif_readw(uart, params->status_reg) & params->error_mask ) scif_writew(uart, params->status_reg, ~params->error_mask); if ( scif_readw(uart, params->overrun_reg) & params->overrun_mask ) scif_writew(uart, params->overrun_reg, ~params->overrun_mask); + if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) + { + dprintk(XENLOG_ERR, "Failed to allocated scif_uart IRQ %u\n", + uart->irq); + /* Do not unmask interrupts if irq handler wasn't set */ + return; + } + /* Enable TX/RX and Error Interrupts */ scif_writew(uart, SCIF_SCSCR, scif_readw(uart, SCIF_SCSCR) | params->irq_flags); -- 2.43.0
In pl011_init_postirq(), two code paths could reach the interrupt-unmask write to IMSC without a handler being registered: - When no valid IRQ number was provided (uart->irq <= 0), the original positive-condition guard (if uart->irq > 0) skipped the irqaction setup but still fell through to the IMSC write, unmasking RTI|OEI|BEI|PEI|FEI|TXI|RXI with no handler installed. - When setup_irq() returned an error, only an error message was printed and execution continued to the IMSC write, arming all hardware interrupt lines with no handler to service them. On platforms where the GIC receives these asserted lines, the result is either repeated spurious-interrupt warnings or an unhandled interrupt fault. Restructure pl011_init_postirq() to use early returns: return immediately when no valid IRQ is provided, and return after logging the error when setup_irq() fails. The interrupt-enable write to IMSC is only reached when IRQ registration succeeds. Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> --- Changes in v4: - fix uart->irq <= 0 to uart->irq == 0 since it's unsigned - update %d to %u in printk since irq is unsigned - add r-b Changes in v3: - clear pending error interrupts before setup_irq for pl011 xen/drivers/char/pl011.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/pl011.c +++ b/xen/drivers/char/pl011.c @@ -XXX,XX +XXX,XX @@ static void __init pl011_init_postirq(struct serial_port *port) struct pl011 *uart = port->uart; int rc; - if ( uart->irq > 0 ) - { - uart->irqaction.handler = pl011_interrupt; - uart->irqaction.name = "pl011"; - uart->irqaction.dev_id = port; - if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) - printk("ERROR: Failed to allocate pl011 IRQ %d\n", uart->irq); - } + /* Don't unmask interrupts if no valid irq was provided */ + if ( uart->irq == 0 ) + return; + + uart->irqaction.handler = pl011_interrupt; + uart->irqaction.name = "pl011"; + uart->irqaction.dev_id = port; /* Clear pending error interrupts */ pl011_write(uart, ICR, OEI|BEI|PEI|FEI); + if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) + { + printk("ERROR: Failed to allocate pl011 IRQ %u\n", uart->irq); + /* Do not unmask interrupts if irq handler wasn't set */ + return; + } + /* Unmask interrupts */ pl011_write(uart, IMSC, RTI|OEI|BEI|PEI|FEI|TXI|RXI); } -- 2.43.0
In cuart_init_postirq(), two code paths could reach the interrupt-enable write to IER without a handler being registered: - When no valid IRQ number was provided (uart->irq <= 0), the original positive-condition guard (if uart->irq > 0) skipped the irqaction setup but still fell through to the IER write, enabling the receive data interrupt with no handler installed. - When setup_irq() returned an error, only an error message was printed and execution continued to the IER write, arming the receive hardware interrupt line with no handler to service it. On platforms where the GIC receives this asserted line, the result is either repeated spurious-interrupt warnings or an unhandled interrupt fault. Restructure cuart_init_postirq() to use early returns in both error paths. Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> --- Changes in v4: - fix %d to %u since irq is unsigned in cadence uart - add R-b Changes in v3: - clear pending error interrupts before setup_irq call for cadence uart - change uart->irq <= 0 to uart->irq == 0 since irq is unsigned xen/drivers/char/cadence-uart.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/xen/drivers/char/cadence-uart.c b/xen/drivers/char/cadence-uart.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/cadence-uart.c +++ b/xen/drivers/char/cadence-uart.c @@ -XXX,XX +XXX,XX @@ static void __init cuart_init_postirq(struct serial_port *port) struct cuart *uart = port->uart; int rc; - if ( uart->irq > 0 ) - { - uart->irqaction.handler = cuart_interrupt; - uart->irqaction.name = "cadence-uart"; - uart->irqaction.dev_id = port; - if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) - printk("ERROR: Failed to allocate cadence-uart IRQ %d\n", uart->irq); - } + /* Don't unmask interrupts if no valid irq was provided */ + if ( uart->irq == 0 ) + return; + + uart->irqaction.handler = cuart_interrupt; + uart->irqaction.name = "cadence-uart"; + uart->irqaction.dev_id = port; /* Clear pending error interrupts */ cuart_write(uart, R_UART_RTRIG, 1); cuart_write(uart, R_UART_CISR, ~0); + if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) + { + printk("ERROR: Failed to allocate cadence-uart IRQ %u\n", uart->irq); + /* Do not unmask interrupts if irq handler wasn't set */ + return; + } + /* Unmask interrupts */ cuart_write(uart, R_UART_IDR, ~0); cuart_write(uart, R_UART_IER, UART_SR_INTR_RTRIG); -- 2.43.0
In exynos4210_uart_init_postirq(), when setup_irq() returns an error the failure was only logged via dprintk() and execution continued, unconditionally clearing UINTM and setting UMCON_INT_EN. This enabled receive and transmit interrupt lines with no handler registered. On platforms where the GIC receives these asserted lines, the result is either repeated spurious-interrupt warnings or an unhandled interrupt fault. Add an early return in the setup_irq() error branch so that the interrupt-enable writes to UINTM and UMCON are skipped when IRQ registration fails. Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> --- Changes in v4: - update comment on setup_irq - change %d to %u since irq is unsigned in exynos - add r-b Changes in v3: - skip clearing pending interrupts if setup_irq was failed because according to the 13.4.1.13 of the RM: it must be cleared after cleaning interrupt pending in INTC. xen/drivers/char/exynos4210-uart.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/xen/drivers/char/exynos4210-uart.c b/xen/drivers/char/exynos4210-uart.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/exynos4210-uart.c +++ b/xen/drivers/char/exynos4210-uart.c @@ -XXX,XX +XXX,XX @@ static void __init exynos4210_uart_init_postirq(struct serial_port *port) uart->irqaction.dev_id = port; if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) - dprintk(XENLOG_ERR, "Failed to allocated exynos4210_uart IRQ %d\n", + { + dprintk(XENLOG_ERR, "Failed to allocated exynos4210_uart IRQ %u\n", uart->irq); + /* Do not unmask interrupts if irq handler wasn't set */ + return; + } /* Unmask interrupts */ exynos4210_write(uart, UINTM, ~UINTM_ALLI); -- 2.43.0