From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 D75FE2FBE1C; Thu, 20 Nov 2025 09:23:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763630590; cv=none; b=NNyw85XfDmodkLDPPXj5uARp86LijNYlzxcsY2O1ed/mWW3U4uFN6GduEoPF38UhTYwUH8D8xltUEJmQcvvysZCCIHIBiRFUrunOroJbyE0nefb9kRQqYIYFVG2ups/ylZOvtpMfsdxlzxbEH9KsW1jFRp+ACvgqtl17LFZl4m4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763630590; c=relaxed/simple; bh=6YLG0MvmJWlaFPyCww/YsMdQWP5HEihl/TAyJznr6aQ=; h=Date:Message-ID:From:To:Cc:Subject:In-Reply-To:References: MIME-Version:Content-Type; b=qjfsSnhPfNQhWK2J7uqT/67FV+T+EhcxxmLjOcLVAN98wh6CdKoEJL5vkn7lbcjsC0fqjLAXr58XUb3TcrUY2YhimChWASbigChFwOVR7Ksfm0odRd1bD43dnWPSmsklQQUK04Xyx/UGgESdaDg9ql41E0ifJK0VK4PnPTl8sqM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Czwx2cNc; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Czwx2cNc" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8F70EC4CEF1; Thu, 20 Nov 2025 09:23:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1763630590; bh=6YLG0MvmJWlaFPyCww/YsMdQWP5HEihl/TAyJznr6aQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Czwx2cNcq31Yz9RL+XetgpLq25vzp6DtVj5cTGYfrGk+GGUbwhXC7ZC32pCCo/iI3 vCDdYVrgWd1KNwyeARWMIenIgzNfQzr1rMCqr428mQrk5J7dyyokwBhIbNMs4UKjQ+ dh9qHB2WR+/qcNtAIdZzLqFn3xFqWDVqiIl/4y3SLGMnFgrevZSvnCRiVuGVoIJmTL m2eWEfh5JvEfOo+L0ARTglgAp4Hgjp8gZM5hbgwJJVMaOZFwMw3wKiFZXrV9ZPRJo1 2I8bdH0YSkKGDDnsKhml0P7Far7K8Ss6ojx9nRCmDHHCiJUpyTB7ZcEwShEC92v0++ 6c1agB0O73rZA== 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.98.2) (envelope-from ) id 1vM0se-00000006pJ7-0mds; Thu, 20 Nov 2025 09:23:08 +0000 Date: Thu, 20 Nov 2025 09:23:07 +0000 Message-ID: <868qg1rrb8.wl-maz@kernel.org> From: Marc Zyngier To: Radu Rendec Cc: Bjorn Helgaas , Manivannan Sadhasivam , Will Deacon , Rob Herring , Krzysztof =?UTF-8?B?V2lsY3p5xYRza2k=?= , Lorenzo Pieralisi , linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] PCI: host-common: Do not set drvdata in pci_host_common_init() In-Reply-To: References: <20251118221244.372423-1-rrendec@redhat.com> <20251118221244.372423-2-rrendec@redhat.com> <86jyzms036.wl-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) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: rrendec@redhat.com, bhelgaas@google.com, mani@kernel.org, will@kernel.org, robh@kernel.org, kwilczynski@kernel.org, lpieralisi@kernel.org, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Wed, 19 Nov 2025 16:19:14 +0000, Radu Rendec wrote: >=20 > On Wed, 2025-11-19 at 12:01 +0000, Marc Zyngier wrote: > > On Tue, 18 Nov 2025 22:12:43 +0000, > > Radu Rendec wrote: [...] > > My concern with this is two-fold: > >=20 > > - it is yet another obscure API change with odd side effects, > > =C2=A0 requiring to track and understand the per-driver flow of informa= tion > > =C2=A0 (and the apple pcie driver is a prime example of how hard this i= s) > >=20 > > - we can't directly associate the PCIe port data with the bridge like > > =C2=A0 must drivers do, because the bridge allocation is done outside of > > =C2=A0 the calling driver, and there is no link back to the bridge from > > =C2=A0 pci_config_window. > >=20 > > I'd rather that last point was addressed so that we could make the > > Apple driver behave similarly to other drivers, and let it use the > > bridge private data for its PCIe port information. >=20 > Using the bridge private data to store the driver's private data was my > first thought too. In fact, in the first version of my (local) changes > I added a "size" parameter to pci_host_common_init(), to pass it down > to devm_pci_alloc_host_bridge() as the priv size. >=20 > But there's another problem with this approach: pci_host_common_init() > also calls pci_host_common_ecam_create() -> pci_ecam_create() -> > ops->init(), so init() would be called with the bridge private area > allocated but not populated. >=20 > I don't see an elegant solution to this. The two options that crossed > my mind are these: > * Add yet another parameter to pci_host_common_init(), a void *, so it > takes a void *priv and a size_t size. The size would be passed down > to devm_pci_alloc_host_bridge(), then `size` bytes would be copied > from `priv` to the bridge private data area. What I don't like about > this is the extra memcpy() and the fact that the calling driver > would have to prepare an "offline" copy of its private data (likely > as a local variable) and pass it to pci_host_common_init(). > * Pass just the void *priv to pci_host_common_init(). By convention, > the bridge private data would be used to store that void *priv > itself, which is a pointer to the driver's private data. So, the 2nd > parameter to devm_pci_alloc_host_bridge() would be sizeof(void *). > The downside is that there's an extra pointer indirection. I don't think either method is appealing. I expect the correct way would be to let individual drivers allocate the bridge (something that is already pretty common), initialise it as required, and pass that to pci_host_common_init(). > In any case, your other concern about linking back to the bridge from > pci_config_window would be addressed because pci_config_window has a > pointer to the device (the `parent` field), which (in this scenario, by > convention) has the bridge pointer stored as drvdata. Ah, of course, thanks for pointing that out. > If any of the options above looks appealing, or if you have a better > idea, please let me know, and I can prepare a v2. See the patch below, which I have actually tested on an M2-pro box. M. diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/control= ler/pci-host-common.c index 810d1c8de24e9..c473e7c03baca 100644 --- a/drivers/pci/controller/pci-host-common.c +++ b/drivers/pci/controller/pci-host-common.c @@ -53,16 +53,12 @@ struct pci_config_window *pci_host_common_ecam_create(s= truct device *dev, EXPORT_SYMBOL_GPL(pci_host_common_ecam_create); =20 int pci_host_common_init(struct platform_device *pdev, + struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops) { struct device *dev =3D &pdev->dev; - struct pci_host_bridge *bridge; struct pci_config_window *cfg; =20 - bridge =3D devm_pci_alloc_host_bridge(dev, 0); - if (!bridge) - return -ENOMEM; - of_pci_check_probe_only(); =20 platform_set_drvdata(pdev, bridge); @@ -85,12 +81,17 @@ EXPORT_SYMBOL_GPL(pci_host_common_init); int pci_host_common_probe(struct platform_device *pdev) { const struct pci_ecam_ops *ops; + struct pci_host_bridge *bridge; =20 ops =3D of_device_get_match_data(&pdev->dev); if (!ops) return -ENODEV; =20 - return pci_host_common_init(pdev, ops); + bridge =3D devm_pci_alloc_host_bridge(&pdev->dev, 0); + if (!bridge) + return -ENOMEM; + + return pci_host_common_init(pdev, bridge, ops); } EXPORT_SYMBOL_GPL(pci_host_common_probe); =20 diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/control= ler/pci-host-common.h index 51c35ec0cf37d..b5075d4bd7eb3 100644 --- a/drivers/pci/controller/pci-host-common.h +++ b/drivers/pci/controller/pci-host-common.h @@ -14,6 +14,7 @@ struct pci_ecam_ops; =20 int pci_host_common_probe(struct platform_device *pdev); int pci_host_common_init(struct platform_device *pdev, + struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops); void pci_host_common_remove(struct platform_device *pdev); =20 diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/p= cie-apple.c index 0380d300adca6..701fba1e45db9 100644 --- a/drivers/pci/controller/pcie-apple.c +++ b/drivers/pci/controller/pcie-apple.c @@ -206,9 +206,6 @@ struct apple_pcie_port { int idx; }; =20 -static LIST_HEAD(pcie_list); -static DEFINE_MUTEX(pcie_list_lock); - static void rmw_set(u32 set, void __iomem *addr) { writel_relaxed(readl_relaxed(addr) | set, addr); @@ -724,32 +721,12 @@ static int apple_msi_init(struct apple_pcie *pcie) return 0; } =20 -static void apple_pcie_register(struct apple_pcie *pcie) -{ - guard(mutex)(&pcie_list_lock); - - list_add_tail(&pcie->entry, &pcie_list); -} - -static void apple_pcie_unregister(struct apple_pcie *pcie) -{ - guard(mutex)(&pcie_list_lock); - - list_del(&pcie->entry); -} - static struct apple_pcie *apple_pcie_lookup(struct device *dev) { - struct apple_pcie *pcie; - - guard(mutex)(&pcie_list_lock); - - list_for_each_entry(pcie, &pcie_list, entry) { - if (pcie->dev =3D=3D dev) - return pcie; - } + struct pci_host_bridge *bridge; =20 - return NULL; + bridge =3D dev_get_drvdata(dev); + return pci_host_bridge_priv(bridge); } =20 static struct apple_pcie_port *apple_pcie_get_port(struct pci_dev *pdev) @@ -875,13 +852,15 @@ static const struct pci_ecam_ops apple_pcie_cfg_ecam_= ops =3D { static int apple_pcie_probe(struct platform_device *pdev) { struct device *dev =3D &pdev->dev; + struct pci_host_bridge *bridge; struct apple_pcie *pcie; int ret; =20 - pcie =3D devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); - if (!pcie) + bridge =3D devm_pci_alloc_host_bridge(dev, sizeof(*pcie)); + if (!bridge) return -ENOMEM; =20 + pcie =3D pci_host_bridge_priv(bridge); pcie->dev =3D dev; pcie->hw =3D of_device_get_match_data(dev); if (!pcie->hw) @@ -897,12 +876,7 @@ static int apple_pcie_probe(struct platform_device *pd= ev) if (ret) return ret; =20 - apple_pcie_register(pcie); - - ret =3D pci_host_common_init(pdev, &apple_pcie_cfg_ecam_ops); - if (ret) - apple_pcie_unregister(pcie); - + ret =3D pci_host_common_init(pdev, bridge, &apple_pcie_cfg_ecam_ops); return ret; } =20 --=20 Without deviation from the norm, progress is not possible.