Re: [PATCH v3 1/4] xen/drivers/char: fix SCIF IRQ registration failure propagation

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 22/04/2026 11:33, Oleksii Moisieiev wrote:
> 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@xxxxxxxx>
> ---
> 
> (no changes since v2)
> 
> 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 888821a3b8..673a2d3800 100644
> --- a/xen/drivers/char/scif-uart.c
> +++ b/xen/drivers/char/scif-uart.c
> @@ -187,16 +187,24 @@ 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.
> +         */
As mentioned before, why do these comments differ depending on the patch. I
would suggest to just add:
/* Don't enable interrupts if irq handler was not set. Fall back to polling */

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal