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

[Tushar Nimkar <tunimkar@amd.com>]

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@oss.nxp.com/
>
> 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