Re: [PATCH v2 3/3] xen/arm: handle irq_set_type() failures

[Mykola Kvach <mykola_kvach@xxxxxxxx>]

Hi Andrew,

Thank you for the review.

On Tue, Aug 11, 2026 at 02:01:17PM +0100, Andrew Cooper wrote:
> On 10/08/2026 7:38 pm, Mykola Kvach wrote:
> > Several Arm firmware initialization paths discard irq_set_type()'s return
> > value, violating MISRA C Rule 17.7. If trigger configuration fails,
> > initialization continues with an IRQ that was not configured as requested.
> >
> > Check the return value in the GTDT, MADT, SPCR, and FF-A paths. Store
> > timer INTIDs only after successful trigger configuration, make GTDT
> > parsing failure fatal, and stop UART or notification setup when trigger
> > configuration fails.
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > Changes in v2:
> > - new patch.
> > ---
> >  xen/arch/arm/gic-v2.c        |  8 ++++++--
> >  xen/arch/arm/gic-v3.c        |  8 ++++++--
> >  xen/arch/arm/tee/ffa_notif.c | 11 ++++++++++-
> >  xen/arch/arm/time.c          | 18 ++++++++++++++----
> >  xen/drivers/char/ns16550.c   |  5 ++++-
> >  xen/drivers/char/pl011.c     |  4 +++-
> >  6 files changed, 43 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 43a379fdda..b8dcbb0bb4 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -1157,6 +1157,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header 
> > *header,
> >                          const unsigned long end)
> >  {
> >      static int cpu_base_assigned = 0;
> > +    int rc;
> >      struct acpi_madt_generic_interrupt *processor =
> >                 container_of(header, struct acpi_madt_generic_interrupt, 
> > header);
> >  
> > @@ -1173,9 +1174,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header 
> > *header,
> >          gicv2_info.maintenance_irq = processor->vgic_interrupt;
> >  
> >          if ( processor->flags & ACPI_MADT_VGIC_IRQ_MODE )
> > -            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_EDGE_BOTH);
> > +            rc = irq_set_type(gicv2_info.maintenance_irq, 
> > IRQ_TYPE_EDGE_BOTH);
> >          else
> > -            irq_set_type(gicv2_info.maintenance_irq, IRQ_TYPE_LEVEL_MASK);
> > +            rc = irq_set_type(gicv2_info.maintenance_irq, 
> > IRQ_TYPE_LEVEL_MASK);
> 
> I know it was pre-existing, but this is an overly verbose way of writing:
> 
> rc = irq_set_type(gicv2_info.maintenance_irq,
>           (processor->flags & ACPI_MADT_VGIC_IRQ_MODE)
>           ? IRQ_TYPE_EDGE_BOTH
>           : IRQ_TYPE_LEVEL_MASK);
> 
> I expect the optimiser can transform behind the scenes, but it's better
> to make the C simpler for humans too.

Agreed. I'll use a single irq_set_type() call with a conditional
trigger type in both the GICv2 and GICv3 MADT paths.

Best regards,
Mykola