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

[Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>]

Hi Michal,

I'll prepare v2 with the fixes.

--

Oleksii

On 09/04/2026 11:17, Orzel, Michal wrote:
> On 08/04/2026 19:03, 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.
> NIT: It would be nice to at least mention that there will be no serial RX
> without interrupts.
>
> > 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>
> That's a good catch. I can see most of our drivers already use that.
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>
> That said, I can see that we have exactly the same issue for pl011, cadence and
> exynos. I can either take your patch as is (no more work for you) and submit the
> patch fixing remaining drivers or you can send a v2 fixing all at once. It's up
> to you.
>
> ~Michal
>
> > ---
> >
> >   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.
> > +         */
> > +        return;
> > +    }
> > +
> >       /* Enable TX/RX and Error Interrupts  */
> >       scif_writew(uart, SCIF_SCSCR, scif_readw(uart, SCIF_SCSCR) |
> >                   params->irq_flags);