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

[Tushar Nimkar]

Hi Thomas,

thanks for reviewing.

On 8/20/2026 1:03 AM, Thomas Gleixner wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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.
sure.
> > +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);

okay

> > +     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
okay
> > +     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.
okay
> > +     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 ...
will remove seems not needed.
> > +     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?
I agree! Locking part will be removed.
> 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?

The original intention was to handle cases where an interrupt source is 
asserted but the ARM SMMUv3 driver has not registered a corresponding 
handler yet. In that case generic_handle_domain_irq() returns an error 
and the interrupt remains pending, which can lead to an interrupt storm.
The write-back was added to clear the pending status in that failure path.
However, with the current implementation we only process interrupt 
sources covered by SMMU_INTR_ALL:
...

        /* Only service sources we support; SMMU_CSR_ISR latches raw 
status */
        pending = status & SMMU_INTR_ALL;
...

and those are expected to have registered handlers. Therefore the 
failure case should not be reachable today. Given that, I agree the 
extra locking/acknowledgement logic is not justified and can be removed 
for clarity.
We could always reintroduce it if support for additional interrupt 
sources is added in the future.

> 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.
Shall we mask still ?
> > +                     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.
okay
> > +{
> > +     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()
okay
> > +     if (!csr)
> > +             return -ENOMEM;
> > +
> > +     raw_spin_lock_init(&csr->lock);
> > +
> > +     csr->base = of_iomap(node, 0);
> devm_of_iomap()
okay
> > +     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@xxxxxxxxxxx/
>
> It's not merged into tip yet, but it will be.
okay sure,  let me re-base.
> > +     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.
did not get you here,
> Thanks,
>
>          tglx

Thanks,
Tushar