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

[Tushar Nimkar <tunimkar@amd.com>]

On 9/18/2026 12:18 PM, sashiko-bot@kernel.org wrote:
> [You don't often get email from sashiko-bot@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> 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 2 potential issue(s) to consider:
> - [High] irqchip: unrecoverable interrupt storm due to unhandled CMDQ_SYNC
> - [Medium] irqchip: spurious handling of disabled interrupts causes log spam
Thanks Sashiko for the review,  all issues addressed in v3 and validated.
> --
>
> commit bfa978fa90b50ee4257046a8002784c0f7fc45f7
> 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 to demultiplex these
> interrupts and chain them 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..d9b575c6c58b0
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-xilinx-smmu-csr.c
> [ ... ]
>> +static void xilinx_smmu_csr_irq_handler(struct irq_desc *desc)
>> +{
>> +     struct xilinx_smmu_csr *csr = irq_desc_get_handler_data(desc);
>> +     struct irq_chip *chip = irq_desc_get_chip(desc);
>> +     u32 status, pending;
>> +
>> +     chained_irq_enter(chip, desc);
>> +
>> +     status = readl(csr->base + SMMU_CSR_ISR);
>> +
>> +     /* Only service sources we support; SMMU_CSR_ISR latches raw status */
>> +     pending = status & SMMU_INTR_ALL;
> [Severity: High]
> If the CMDQ_SYNC interrupt (bit 1) was enabled or asserted by a previous boot
> stage like u-boot or kexec, it is filtered out of this pending mask. Because
> it is never cleared or acknowledged, will this cause an interrupt storm on the
> parent GIC line and a system hard lockup?
Yes — fixed in v3. The handler now gates on the hardware mask register 
and defensively masks+clears any active source it doesn't demultiplex 
(e.g. CMDQ_SYNC), so it can't hold the GIC
line asserted.
> [Severity: Medium]
> Additionally, since SMMU_CSR_ISR latches the raw status, does masking against
> SMMU_INTR_ALL instead of a shadow mask of actually enabled interrupts lead to
> processing disabled or unmapped events?
Fixed in v3 - dispatch is now gated on active = status & ~mask using the 
hardware mask register (SMMU_CSR_MASK), so masked/disabled-but-latched 
sources are never processed.
>> +
>> +     while (pending) {
>> +             irq_hw_number_t hwirq = __ffs(pending);
>> +             int ret;
>> +
>> +             ret = generic_handle_domain_irq(csr->domain, hwirq);
>> +             if (ret)
>> +                     pr_err_ratelimited("xilinx-smmu-csr: Failed to handle domain IRQ %lu: %d\n",
>> +                                        hwirq, ret);
> If an unmapped interrupt fires, generic_handle_domain_irq() will fail with
> -EINVAL and log this error. Because the driver cannot ACK an unmapped
> interrupt, the bit remains set in the ISR. Does this result in permanent
> ratelimited log spam and wasted CPU cycles on every subsequent valid
> interrupt?
Fixed in v3 -  On an error return the handler now masks+clears that 
source instead of logging, so the bit can't stay set
>> +
>> +             pending &= ~BIT(hwirq);
>> +     }
>> +
>> +     chained_irq_exit(chip, desc);
>> +}
> [ ... ]
>> +static int __init xilinx_smmu_csr_init(struct device_node *node, struct device_node *parent)
>> +{
> [ ... ]
>> +     /* Start from a known state: all sources disabled, latches cleared. */
>> +     writel(SMMU_INTR_ALL, csr->base + SMMU_CSR_IDR);
>> +     writel(SMMU_INTR_ALL, csr->base + SMMU_CSR_ISR);
> [Severity: High]
> SMMU_INTR_ALL omits bit 1 (CMDQ_SYNC). If a prior boot stage left CMDQ_SYNC
> enabled or pending, doesn't writing only SMMU_INTR_ALL fail to disable and
> clear it, setting up the interrupt storm described above?
Correct — fixed in v3. Init now masks and clears every source the block 
implements (SMMU_INTR_HW_ALL = GENMASK(3,0)), including CMDQ_SYNC:

   writel(SMMU_INTR_HW_ALL, csr->base + SMMU_CSR_IDR); /* mask all  */
   writel(SMMU_INTR_HW_ALL, csr->base + SMMU_CSR_ISR); /* clear all */

so an inherited enabled/pending CMDQ_SYNC is disabled and cleared before 
the parent line is enabled.

>> +
>> +     csr->domain = irq_domain_create_linear(of_fwnode_handle(node), SMMU_CSR_IRQ_NR,
>> +                                            &xilinx_smmu_csr_domain_ops,
>> +                                            csr);
> [ ... ]
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260918-smmu-changes-v2-v2-0-adda681ab983@amd.com?part=2
Thanks, Tushar Nimkar