From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8445DC83F09 for ; Tue, 8 Jul 2025 14:59:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: References:In-Reply-To:Subject:Cc:To:From:Message-ID:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Lu067gzfX8Y3GAfmHSnHoOdlmM/liVd5i64a52xtjiM=; b=K+c6Gh3IFXEmnlkA6VzfWx0p66 kasuLnnH+ELeOq5nc2SkD6rUtmaed//KfX1W0umXlOXvgOcEefINBrz5vtMyKqdBnHXjNLVG1CzX1 ZDArye/FA4cUFtVBGJXjxeul8VOxckq9r4MGfMIIZfOot2+vES/8AZV+YDDRDT1112+x/3kIHKXN+ AAv3PkSKeyFHe00WVdWh6+Bs18s75gVKPcjF+TShI2tXss1Tqzm/o8BisMM+8slf3JVlj0A54kYFZ a0QhIfhlnB3vpGjmn4lil7Qz080KPIzcFMVP9VCIH7dLu6WM62hxMVTVPAXhytHJeph6n5GakM/Wz /xIhjPxw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1uZ9nD-00000005gtn-3Vp7; Tue, 08 Jul 2025 14:59:35 +0000 Received: from sea.source.kernel.org ([2600:3c0a:e001:78e:0:1991:8:25]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1uZ9aw-00000005ebQ-1xsT for linux-arm-kernel@lists.infradead.org; Tue, 08 Jul 2025 14:46:55 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sea.source.kernel.org (Postfix) with ESMTP id C3AD4453E6; Tue, 8 Jul 2025 14:46:53 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 998FCC4CEED; Tue, 8 Jul 2025 14:46:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1751986013; bh=8VDA1hhdNP0DvXhtKa1xQX/6//M/u2gJCpKHUwbE8rE=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=eBexEpwpcZoo15tmiOa+fHLZtIIup6t7ZstZvno914Fm8b9WDRcTOHfgVLTOGNhaY Cd03QHHSu0iJ+WkK1uJee2fjYr03DqRjwsWIfSXe3uziHv0jZ/0B18ide3lfYj8pHQ rrTryzT15LP86d1TUMZh0TABx4DOIX5Z6EwfO2ASIyQEXHAG4ai4RsRU9AUNQBMHoI 2NHcanp3vnarWx3HczXOS6YsLDBlcPPXfApYQ22yfcM7F2ROyyre91pVMbP0f1HigR Mz8/S8XBctflYVDxlkKvbLR95p3ijVopVHdiXt0Jo+UFh/cPfyoHillOgJON6/9vQm clHZl90jdsaQA== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1uZ9at-00DnQF-D8; Tue, 08 Jul 2025 15:46:51 +0100 Date: Tue, 08 Jul 2025 15:46:51 +0100 Message-ID: <86pleaagf8.wl-maz@kernel.org> From: Marc Zyngier To: Lorenzo Pieralisi Cc: linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Toan Le , Krzysztof =?UTF-8?B?V2lsY3p5xYRza2k=?= , Manivannan Sadhasivam , Rob Herring , Bjorn Helgaas , Thomas Gleixner Subject: Re: [PATCH 08/12] PCI: xgene-msi: Get rid of intermediate tracking structure In-Reply-To: References: <20250628173005.445013-1-maz@kernel.org> <20250628173005.445013-9-maz@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/30.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: lpieralisi@kernel.org, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, toan@os.amperecomputing.com, kwilczynski@kernel.org, mani@kernel.org, robh@kernel.org, bhelgaas@google.com, tglx@linutronix.de X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250708_074654_579578_97ECF403 X-CRM114-Status: GOOD ( 38.05 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 07 Jul 2025 16:22:43 +0100, Lorenzo Pieralisi wrote: > > On Sat, Jun 28, 2025 at 06:30:01PM +0100, Marc Zyngier wrote: > > The xgene-msi driver uses an odd construct in the form of an > > intermediate tracking structure, evidently designed to deal with > > multiple instances of the MSI widget. However, the existing HW > > only has one set, and it is obvious that there won't be new HW > > coming down that particular line. > > > > Simplify the driver by using a bit of pointer arithmetic instead, > > directly tracking the interrupt and avoiding extra memory allocation. > > A couple of nits, nothing else. > > > Signed-off-by: Marc Zyngier > > --- > > drivers/pci/controller/pci-xgene-msi.c | 58 ++++++++------------------ > > 1 file changed, 17 insertions(+), 41 deletions(-) > > > > diff --git a/drivers/pci/controller/pci-xgene-msi.c b/drivers/pci/controller/pci-xgene-msi.c > > index b3ac0125b3b40..4be79b9ff80df 100644 > > --- a/drivers/pci/controller/pci-xgene-msi.c > > +++ b/drivers/pci/controller/pci-xgene-msi.c > > @@ -24,19 +24,13 @@ > > #define NR_HW_IRQS 16 > > #define NR_MSI_VEC (IDX_PER_GROUP * IRQS_PER_IDX * NR_HW_IRQS) > > > > -struct xgene_msi_group { > > - struct xgene_msi *msi; > > - int gic_irq; > > - u32 msi_grp; > > -}; > > - > > struct xgene_msi { > > struct irq_domain *inner_domain; > > u64 msi_addr; > > void __iomem *msi_regs; > > unsigned long *bitmap; > > struct mutex bitmap_lock; > > - struct xgene_msi_group *msi_groups; > > + unsigned int gic_irq[NR_HW_IRQS]; > > }; > > > > /* Global data */ > > @@ -261,27 +255,20 @@ static int xgene_msi_init_allocator(struct device *dev) > > > > mutex_init(&xgene_msi_ctrl->bitmap_lock); > > > > - xgene_msi_ctrl->msi_groups = devm_kcalloc(dev, NR_HW_IRQS, > > - sizeof(struct xgene_msi_group), > > - GFP_KERNEL); > > - if (!xgene_msi_ctrl->msi_groups) > > - return -ENOMEM; > > - > > return 0; > > } > > > > static void xgene_msi_isr(struct irq_desc *desc) > > { > > + unsigned int *irqp = irq_desc_get_handler_data(desc); > > struct irq_chip *chip = irq_desc_get_chip(desc); > > struct xgene_msi *xgene_msi = xgene_msi_ctrl; > > - struct xgene_msi_group *msi_groups; > > int msir_index, msir_val, hw_irq, ret; > > u32 intr_index, grp_select, msi_grp; > > > > chained_irq_enter(chip, desc); > > > > - msi_groups = irq_desc_get_handler_data(desc); > > - msi_grp = msi_groups->msi_grp; > > + msi_grp = irqp - xgene_msi->gic_irq; > > > > /* > > * MSIINTn (n is 0..F) indicates if there is a pending MSI interrupt > > @@ -341,35 +328,31 @@ static void xgene_msi_remove(struct platform_device *pdev) > > cpuhp_remove_state(pci_xgene_online); > > cpuhp_remove_state(CPUHP_PCI_XGENE_DEAD); > > > > - kfree(msi->msi_groups); > > - > > xgene_free_domains(msi); > > } > > > > static int xgene_msi_hwirq_alloc(unsigned int cpu) > > { > > - struct xgene_msi *msi = xgene_msi_ctrl; > > - struct xgene_msi_group *msi_group; > > int i; > > int err; > > > > for (i = cpu; i < NR_HW_IRQS; i += num_possible_cpus()) { > > - msi_group = &msi->msi_groups[i]; > > + unsigned int irq = xgene_msi_ctrl->gic_irq[i]; > > > > /* > > * Statically allocate MSI GIC IRQs to each CPU core. > > * With 8-core X-Gene v1, 2 MSI GIC IRQs are allocated > > * to each core. > > */ > > - irq_set_status_flags(msi_group->gic_irq, IRQ_NO_BALANCING); > > - err = irq_set_affinity(msi_group->gic_irq, cpumask_of(cpu)); > > + irq_set_status_flags(irq, IRQ_NO_BALANCING); > > + err = irq_set_affinity(irq, cpumask_of(cpu)); > > if (err) { > > pr_err("failed to set affinity for GIC IRQ"); > > return err; > > } > > > > - irq_set_chained_handler_and_data(msi_group->gic_irq, > > - xgene_msi_isr, msi_group); > > + irq_set_chained_handler_and_data(irq, xgene_msi_isr, > > + &xgene_msi_ctrl->gic_irq[i]); > > } > > > > return 0; > > @@ -378,15 +361,12 @@ static int xgene_msi_hwirq_alloc(unsigned int cpu) > > static int xgene_msi_hwirq_free(unsigned int cpu) > > { > > struct xgene_msi *msi = xgene_msi_ctrl; > > - struct xgene_msi_group *msi_group; > > int i; > > > > for (i = cpu; i < NR_HW_IRQS; i += num_possible_cpus()) { > > - msi_group = &msi->msi_groups[i]; > > - if (!msi_group->gic_irq) > > + if (!msi->gic_irq[i]) > > In patch 5 we removed this check in xgene_msi_hwirq_alloc(), if it > superfluous there it should be here too. Hmmm, good point. I'll get rid of that one too. > > > continue; > > - irq_set_chained_handler_and_data(msi_group->gic_irq, NULL, > > - NULL); > > + irq_set_chained_handler_and_data(msi->gic_irq[i], NULL, NULL); > > } > > return 0; > > } > > @@ -399,10 +379,9 @@ static const struct of_device_id xgene_msi_match_table[] = { > > static int xgene_msi_probe(struct platform_device *pdev) > > { > > struct resource *res; > > - int rc, irq_index; > > Just noticed, insignificant nit: don't see why moving irq_index to a > local loop variable is required in this patch - fine to leave the > code in the patch as-is - reporting it to make sure I have not > missed anything. Not required, just my own obsession with scope reduction of local variables. I thought that given the magnitude of the changes, I might as well give myself some artistic license! ;-) Thanks, M. -- Without deviation from the norm, progress is not possible.