From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2A043221D96 for ; Wed, 19 Aug 2026 19:33:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=193.142.43.55 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787167995; cv=none; b=bcLp+f0F/XaLtfeeteo9LI6jsKSQf2I59/kZOMJi6nilcKQ9PwCqW5NdCcTCQRZzx4FH5KqKjAjsK3UuLPQ5Rz1860uEsvHhN1FYKD6gmFfIP15Kdb4sx+39QO68bSyCyQz+XujzSA2A5n8ZhqPtYad9Yz8CUmS1PsJ1tbr+tl8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787167995; c=relaxed/simple; bh=mYzlopCswFmFWbnfQq5x4lcPGCt5+nDtOQ+YkDaDJsk=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=lpd7o0Od4f8xA98z8h6c9cBhuIl+EnDMXZ/X7ncJerRSYWq2eXjyL48Usjf6/0lfNhhEuIS3mHP9WY1ijLI9YQUZEIa/wYlQyVvIk0gLY5DQoZ5/0GIBA66qhmOmU73C2Pvxh62zw1d+CqXiJXd1wqUHhqx6O3fC+YvoJTp9fNk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linutronix.de; spf=pass smtp.mailfrom=linutronix.de; dkim=pass (2048-bit key) header.d=linutronix.de header.i=@linutronix.de header.b=TfftlYHa; dkim=permerror (0-bit key) header.d=linutronix.de header.i=@linutronix.de header.b=wVzU0ySr; arc=none smtp.client-ip=193.142.43.55 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linutronix.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linutronix.de Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linutronix.de header.i=@linutronix.de header.b="TfftlYHa"; dkim=permerror (0-bit key) header.d=linutronix.de header.i=@linutronix.de header.b="wVzU0ySr" From: Thomas Gleixner DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1787167991; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=e22S2Y9/EK7c6rFE1KVoXB90JmbtRnFORvBglPDPLsA=; b=TfftlYHaSsJWJa0wh07u57mBK6QGrkKCSS/vJ6CMV8HGjWEHLHxLaO31xAS7KmYRcdzg8m rajGKi/S7JlPQkUlcVSmC9Wb5TCIWEPujTbtx4I9G+JhZZb2g5m+YV0+MmhYsVaZSeoA1S f3inu8mE356lhEfd7c5YZVVCGoTaqRTKo2iE3B5XblVQH1zHxxZygun+FuOgnxyr0eJgsF a4gIfMvN8P/NnyUaMPa+FLWsLMjM661aGXpxuz7/aQI78vU7UaA5Z+MFsDH4xJCJvs2HQJ +85ohrEczzEHynqoLAaaoWY2MRtcVRPox5Dbgna7Q0TQ+9QwxkAZ1+ZQJORkPA== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1787167991; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=e22S2Y9/EK7c6rFE1KVoXB90JmbtRnFORvBglPDPLsA=; b=wVzU0ySrUHCpDnIdbOekvDw7Ci8LLrP6Obq/7zEnUIdYwMy4HdtdPodoM1INRErKccdA7G fNFRR3m4Gvq/g1Dw== To: Tushar Nimkar , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Cc: Michal Simek , Anirudha Sarangi , git-dev@amd.com, Tushar Nimkar Subject: Re: [PATCH 2/2] irqchip: Add Xilinx Versal NET SMMU CSR interrupt controller driver In-Reply-To: <20260817105251.1557770-1-tushar.nimkar@amd.com> References: <20260817105251.1557770-1-tushar.nimkar@amd.com> Date: Wed, 19 Aug 2026 21:33:10 +0200 Message-ID: <87ik55kirt.ffs@fw13> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain 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