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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 4/9/26 16:50, 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>
> ---
>
>
>
>   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 b2f379833f..a63dc4adb2 100644
> --- a/xen/drivers/char/cadence-uart.c
> +++ b/xen/drivers/char/cadence-uart.c
> @@ -72,13 +72,18 @@ 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 )
But irq field is defined as an unsigned int. By definition, an unsigned 
int can never be less than zero.
> +        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);
NIT: The format specifier should be %u

> +        /* Do not unmask interrupts if irq handler wasn't set */
> +        return;
>       }
>   
>       /* Clear pending error interrupts */

I notice that with this change, the block that clears pending error 
interrupts is now skipped if setup_irq() fails or if no valid IRQ is 
provided. Is this intentional / OK?
In "[PATCH v2 1/4] xen/drivers/char: fix SCIF IRQ registration failure 
propagation", the error status registers are always cleared, even on 
failure.