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

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Aug 17 2026 at 16:22, Tushar Nimkar wrote:
> +/**
> + * struct xilinx_smmu_csr - SMMU CSR interrupt controller context
> + * @base: MMIO base address of the CSR registers
> + * @domain: IRQ domain for the child interrupts
> + * @parent_irq: parent (GIC) IRQ this block is chained to
> + * @lock: protects the SMMU_CSR_IER/IDR/ISR read and writes

Please make the member descriptions tabular aligned

    @base:	MMIO ...
    @domain:	Interrupt domain

And yes, use interrupt and not IRQ. This is not twitter.

> +static void xilinx_smmu_csr_irq_mask(struct irq_data *d)
> +{
> +	struct xilinx_smmu_csr *csr = irq_data_get_irq_chip_data(d);
> +	u32 mask = xilinx_smmu_csr_hwirq_mask(d->hwirq);
> +
> +	if (!mask)
> +		return;
> +
> +	raw_spin_lock(&csr->lock);

  guard(raw_spinlock)(&crs->lock);

> +	writel(mask, csr->base + SMMU_CSR_IDR);
> +	raw_spin_unlock(&csr->lock);
> +}
> +
> +static void xilinx_smmu_csr_irq_unmask(struct irq_data *d)
> +{
> +	struct xilinx_smmu_csr *csr = irq_data_get_irq_chip_data(d);
> +	u32 mask = xilinx_smmu_csr_hwirq_mask(d->hwirq);
> +
> +	if (!mask)
> +		return;
> +
> +	raw_spin_lock(&csr->lock);

Ditto

> +	writel(mask, csr->base + SMMU_CSR_IER);
> +	raw_spin_unlock(&csr->lock);
> +}
> +
> +static void xilinx_smmu_csr_irq_ack(struct irq_data *d)
> +{
> +	struct xilinx_smmu_csr *csr = irq_data_get_irq_chip_data(d);
> +	u32 mask = xilinx_smmu_csr_hwirq_mask(d->hwirq);
> +
> +	if (!mask)
> +		return;
> +
> +	raw_spin_lock(&csr->lock);

Ditto.

> +	writel(mask, csr->base + SMMU_CSR_ISR);
> +	raw_spin_unlock(&csr->lock);
> +}
> +
> +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 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);
> +	raw_spin_lock(&csr->lock);

scoped_guard() if you can explain what the lock is actually protecting
here ...

> +	status = readl(csr->base + SMMU_CSR_ISR);
> +	raw_spin_unlock(&csr->lock);
> +
> +	/* Only service sources we support; SMMU_CSR_ISR latches raw status */
> +	pending = status & SMMU_INTR_ALL;
> +
> +	while (pending) {
> +		irq_hw_number_t hwirq = __ffs(pending);
> +		int ret;
> +
> +		ret = generic_handle_domain_irq(csr->domain, hwirq);
> +		if (ret) {
> +			raw_spin_lock(&csr->lock);
> +			writel(BIT(hwirq), csr->base + SMMU_CSR_ISR);
> +			raw_spin_unlock(&csr->lock);

... and here. There is _ONE_ chained demultiplex handler per chip, so where
is the concurrency vs. the read and write from/to SMMU_CSR_ISR?

The irq_ack() callback of the demultiplexed interrupts cannot happen
concurrently because that happens in the context of the demultiplexed
handler invoked by generic_handle_domain_irq(). No?

Not that I care about the performance of your code, but I care about
code clarity. If there is a reason for this magic lock voodoo here, then
please explain it in a comment.

Also this write here wants a comment. Why is the pending bit written
back in the failure case? I assume to acknowlegde the interrupt. How are
the interrupts which are handled acknowledged?

Also if this happens, then this code should make sure to mask this
interrupt line because if something left it unmasked it will come back
forever.

> +			pr_err_ratelimited("xilinx-smmu-csr: Failed to handle domain IRQ %lu: %d\n",
> +					   hwirq, ret);
> +		}
> +
> +		pending &= ~BIT(hwirq);
> +	}
> +static int __init xilinx_smmu_csr_init(struct device_node *node,
> +				       struct device_node *parent)

No line break required. You have 100 characters. Please fix that up all over the place.

> +{
> +	struct xilinx_smmu_csr *csr;
> +	int ret;
> +
> +	if (WARN_ON_ONCE(!parent))
> +		return -EINVAL;
> +
> +	if (irq_find_matching_fwnode(of_fwnode_handle(node),
> +				     DOMAIN_BUS_ANY))
> +		return -ENODEV;
> +
> +	csr = kzalloc(sizeof(*csr), GFP_KERNEL);

devm_kzalloc()

> +	if (!csr)
> +		return -ENOMEM;
> +
> +	raw_spin_lock_init(&csr->lock);
> +
> +	csr->base = of_iomap(node, 0);

devm_of_iomap()

> +	if (!csr->base) {
> +		ret = -ENOMEM;
> +		goto free;
> +	}
> +
> +	/* 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);
> +
> +	csr->domain = irq_domain_create_linear(of_fwnode_handle(node), SMMU_CSR_IRQ_NR,
> +					       &xilinx_smmu_csr_domain_ops,
> +					       csr);

devm_irq_domain_instantiate() or use this one:

 https://lore.kernel.org/lkml/20260819090543.585131-2-Zhipeng.wang_1@oss.nxp.com/

It's not merged into tip yet, but it will be.

> +	if (!csr->domain) {
> +		pr_err("%pOF: failed to create irq domain\n", node);
> +		ret = -ENOMEM;
> +		goto unmap;

with that all these 'ret = -ERROR; goto foo;' go away.

Thanks,

        tglx