Re: [PATCH v3 3/4] xen/drivers/char/cadence-uart: fix IRQ registration failure propagation

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 4/22/26 12:33, Oleksii Moisieiev wrote:

Hello Oleksii

> 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@xxxxxxxx>
> ---
>
> 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
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
with NIT (could probably be fixed on commit) ...

>   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 b2f379833f..8961d39de1 100644
> --- a/xen/drivers/char/cadence-uart.c
> +++ b/xen/drivers/char/cadence-uart.c
> @@ -72,19 +72,25 @@ 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 %d\n", uart->irq);
   ... the format specifier should be %u instead of %d since the value 
is unsigned.
> +        /* 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);