Re: [PATCH v2 2/4] xen/drivers/char/pl011: fix IRQ registration failure propagation

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

On 09/04/2026 15:50, Oleksii Moisieiev wrote:
> 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@xxxxxxxx>
> ---
> 
> 
> 
>  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 5f9913367d..918b9d4d4a 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -150,13 +150,18 @@ 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 */
I think we should clear pending errors every time. Other than that, the patch is
ok. Provided the remark is addressed:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal