Re: [PATCH 1/2] PCI: host-common: Do not set drvdata in pci_host_common_init()

[Marc Zyngier <maz@kernel.org>]

On Wed, 19 Nov 2025 16:19:14 +0000,
Radu Rendec <rrendec@redhat.com> wrote:
> 
> On Wed, 2025-11-19 at 12:01 +0000, Marc Zyngier wrote:
> > On Tue, 18 Nov 2025 22:12:43 +0000,
> > Radu Rendec <rrendec@redhat.com> wrote:

[...]

> > My concern with this is two-fold:
> > 
> > - it is yet another obscure API change with odd side effects,
> >   requiring to track and understand the per-driver flow of information
> >   (and the apple pcie driver is a prime example of how hard this is)
> > 
> > - we can't directly associate the PCIe port data with the bridge like
> >   must drivers do, because the bridge allocation is done outside of
> >   the calling driver, and there is no link back to the bridge from
> >   pci_config_window.
> > 
> > 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.
> 
> 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.
> 
> 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.
> 
> 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/controller/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(struct device *dev,
 EXPORT_SYMBOL_GPL(pci_host_common_ecam_create);
 
 int pci_host_common_init(struct platform_device *pdev,
+			 struct pci_host_bridge *bridge,
 			 const struct pci_ecam_ops *ops)
 {
 	struct device *dev = &pdev->dev;
-	struct pci_host_bridge *bridge;
 	struct pci_config_window *cfg;
 
-	bridge = devm_pci_alloc_host_bridge(dev, 0);
-	if (!bridge)
-		return -ENOMEM;
-
 	of_pci_check_probe_only();
 
 	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;
 
 	ops = of_device_get_match_data(&pdev->dev);
 	if (!ops)
 		return -ENODEV;
 
-	return pci_host_common_init(pdev, ops);
+	bridge = 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);
 
diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/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;
 
 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);
 
diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-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;
 };
 
-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;
 }
 
-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 == dev)
-			return pcie;
-	}
+	struct pci_host_bridge *bridge;
 
-	return NULL;
+	bridge = dev_get_drvdata(dev);
+	return pci_host_bridge_priv(bridge);
 }
 
 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 = {
 static int apple_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct pci_host_bridge *bridge;
 	struct apple_pcie *pcie;
 	int ret;
 
-	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
-	if (!pcie)
+	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
+	if (!bridge)
 		return -ENOMEM;
 
+	pcie = pci_host_bridge_priv(bridge);
 	pcie->dev = dev;
 	pcie->hw = of_device_get_match_data(dev);
 	if (!pcie->hw)
@@ -897,12 +876,7 @@ static int apple_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	apple_pcie_register(pcie);
-
-	ret = pci_host_common_init(pdev, &apple_pcie_cfg_ecam_ops);
-	if (ret)
-		apple_pcie_unregister(pcie);
-
+	ret = pci_host_common_init(pdev, bridge, &apple_pcie_cfg_ecam_ops);
 	return ret;
 }
 

-- 
Without deviation from the norm, progress is not possible.