Re: [PATCH v3 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 1 potential issue(s) to consider:
- [High] irqchip: spurious interrupts due to handle_level_irq with latched status
--

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?

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