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

[sashiko-bot@kernel.org]

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
--

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?

[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? 

> +
> +	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?

> +
> +		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?

> +
> +	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