Re: [PATCH v3 2/2] irqchip: Add Xilinx Versal NET SMMU CSR interrupt controller driver

[Tushar Nimkar <tunimkar@amd.com>]

Thanks for the review,

On 9/23/2026 12:56 PM, sashiko-bot@kernel.org wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] irqchip: spurious interrupts due to handle_level_irq with latched status
> --
will fix in v4
>
> commit 2d37d393fff3fd3914b9e3ef7cecbd4d6f1c364c
> Author: Anirudha Sarangi <anirudha.sarangi@amd.com>
>
> irqchip: Add Xilinx Versal NET SMMU CSR interrupt controller driver
>
> The Xilinx Versal NET SoC implements a Control and Status Register (CSR) block
> in front of the ARM SMMUv3 to manage EVENTQ, PRIQ and GLOBAL interrupts. This
> commit adds an irqchip driver for the CSR block that registers as an interrupt
> controller and chains to the upstream SMMUv3 driver.
>
>> diff --git a/drivers/irqchip/irq-xilinx-smmu-csr.c b/drivers/irqchip/irq-xilinx-smmu-csr.c
>> new file mode 100644
>> index 0000000000000..b29aafd01dbeb
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-xilinx-smmu-csr.c
> [ ... ]
>
>> +static struct irq_chip xilinx_smmu_csr_chip = {
>> +     .name           = "xlnx-smmu-csr",
>> +     .irq_mask       = xilinx_smmu_csr_irq_mask,
>> +     .irq_unmask     = xilinx_smmu_csr_irq_unmask,
>> +     .irq_ack        = xilinx_smmu_csr_irq_ack,
>> +};
> [ ... ]
>
>> +static int xilinx_smmu_csr_domain_map(struct irq_domain *d, unsigned int virq,
>> +                                   irq_hw_number_t hwirq)
>> +{
>> +     struct xilinx_smmu_csr *csr = d->host_data;
>> +
>> +     if (!xilinx_smmu_csr_hwirq_mask(hwirq))
>> +             return -EINVAL;
>> +
>> +     irq_set_chip_and_handler(virq, &xilinx_smmu_csr_chip, handle_level_irq);
> [Severity: High]
> Will using handle_level_irq() cause spurious interrupts due to the latched
> status in the SMMU_CSR_ISR register?
>
> Since handle_level_irq() acknowledges the interrupt by calling the .irq_ack
> callback (which clears the W1C SMMU_CSR_ISR register) before invoking the
> SMMUv3 device handler, the CSR block clears its status while the underlying
> SMMU line is still asserted.
>
> Because the source isn't deasserted yet, the CSR block will immediately
> re-latch the status. When handle_level_irq() unmasks the interrupt after the
> SMMUv3 handler finishes, this re-latched status will trigger a second
> interrupt. The SMMUv3 driver will find no actual events and return IRQ_NONE,
> eventually causing the kernel's spurious interrupt detector to disable the GIC
> line.
>
> Should this driver use handle_fasteoi_irq() and implement the .irq_eoi
> callback instead, so the CSR status is only cleared after the underlying
> SMMUv3 driver has successfully deasserted the source?
Good catch, agreed !
SMMU_CSR_ISR is sticky W1C, so clearing it in .irq_ack before the SMMUv3 
handler
drains the source lets the CSR re-latch, producing a spurious IRQ_NONE 
that trips
the spurious detector.

Fixed in v4: switched to handle_fasteoi_irq() with the W1C clear moved to
.irq_eoi, plus IRQCHIP_EOI_THREADED so the clear is deferred until the
threaded EVENTQ/PRIQ handler has actually drained the queue.

Thanks,
Tushar
>> +     irq_set_chip_data(virq, csr);
>> +     irq_set_status_flags(virq, IRQ_LEVEL);
>> +
>> +     return 0;
>> +}
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260923-smmu-changes-v2-v3-0-87bd09ba9605@amd.com?part=2