[PATCH 0/2] PCI: host-common: Allow drivers to use the device's drvdata pointer

[PATCH 0/2] PCI: host-common: Allow drivers to use the device's drvdata pointer

[Radu Rendec]

Currently there's only one user of pci_host_common_init(), the
pcie-apple driver, and it goes to great lengths to store its own
per-device private pointer, because the device's drvdata is used by
pci_host_common_init() to store a pointer to struct pci_host_bridge.
See commit 643c0c9d0496 ("PCI: apple: Add tracking of probed root
ports").

However, it looks like this is necessary only for simple drivers that
use pci_host_common_{probe,remove}() directly as their platform
probe/remove functions. More complex drivers that allocate their own
private structure, like pcie-apple, can store a pointer to that instead;
the pointer to struct pci_host_bridge can be encapsulated in the private
structure if it's needed.

The first patch changes the pci-host-common library to free up drvdata
for drivers that do not use pci_host_common_{probe,remove}() directly.

The second patch uses the changes in the first patch to simplify the
pointer storage logic. This patch is *untested* because I don't have
access to the hardware.

Radu Rendec (2):
  PCI: host-common: Do not set drvdata in pci_host_common_init()
  PCI: apple: Store private pointer in platform device's drvdata

 drivers/pci/controller/pci-host-common.c | 36 +++++++++++-----
 drivers/pci/controller/pci-host-common.h |  6 ++-
 drivers/pci/controller/pcie-apple.c      | 53 ++++--------------------
 3 files changed, 36 insertions(+), 59 deletions(-)

-- 
2.51.1

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

[Radu Rendec]

Currently pci_host_common_init() uses the platform device's drvdata to
store the pointer to the allocated struct pci_host_bridge. This makes
sense for drivers that use pci_host_common_{probe,remove}() directly as
the platform probe/remove functions, but leaves no option for more
complex drivers to store a pointer to their own private data.

Change pci_host_common_init() to return the pointer to the allocated
struct pci_host_bridge, and move the platform_set_drvdata() call to
pci_host_common_probe(). This way, drivers that implement their own
probe function can still use pci_host_common_init() but store their own
pointer in the platform device's drvdata.

For symmetry, move the release code to a new function that takes a
pointer to struct pci_host_bridge, and make pci_host_common_release() a
wrapper to it that extracts the pointer from the platform device's
drvdata. This way, drivers that store their own private pointer in the
platform device's drvdata can still use the library release code.

No functional change to the existing users of pci-host-common is
intended, with the exception of the pcie-apple driver, which is modified
in a subsequent patch.

Signed-off-by: Radu Rendec <rrendec@redhat.com>
---
 drivers/pci/controller/pci-host-common.c | 36 ++++++++++++++++--------
 drivers/pci/controller/pci-host-common.h |  6 ++--
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index 810d1c8de24e9..86002195c93ac 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -52,25 +52,24 @@ 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,
-			 const struct pci_ecam_ops *ops)
+struct pci_host_bridge *pci_host_common_init(struct platform_device *pdev,
+					     const struct pci_ecam_ops *ops)
 {
 	struct device *dev = &pdev->dev;
 	struct pci_host_bridge *bridge;
 	struct pci_config_window *cfg;
+	int rc;
 
 	bridge = devm_pci_alloc_host_bridge(dev, 0);
 	if (!bridge)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	of_pci_check_probe_only();
 
-	platform_set_drvdata(pdev, bridge);
-
 	/* Parse and map our Configuration Space windows */
 	cfg = pci_host_common_ecam_create(dev, bridge, ops);
 	if (IS_ERR(cfg))
-		return PTR_ERR(cfg);
+		return (struct pci_host_bridge *)cfg;
 
 	bridge->sysdata = cfg;
 	bridge->ops = (struct pci_ops *)&ops->pci_ops;
@@ -78,31 +77,46 @@ int pci_host_common_init(struct platform_device *pdev,
 	bridge->disable_device = ops->disable_device;
 	bridge->msi_domain = true;
 
-	return pci_host_probe(bridge);
+	rc = pci_host_probe(bridge);
+	if (rc)
+		return ERR_PTR(rc);
+
+	return bridge;
 }
 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 = pci_host_common_init(pdev, ops);
+	if (IS_ERR(bridge))
+		return PTR_ERR(bridge);
+
+	platform_set_drvdata(pdev, bridge);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_host_common_probe);
 
-void pci_host_common_remove(struct platform_device *pdev)
+void pci_host_common_release(struct pci_host_bridge *bridge)
 {
-	struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
-
 	pci_lock_rescan_remove();
 	pci_stop_root_bus(bridge->bus);
 	pci_remove_root_bus(bridge->bus);
 	pci_unlock_rescan_remove();
 }
+EXPORT_SYMBOL_GPL(pci_host_common_release);
+
+void pci_host_common_remove(struct platform_device *pdev)
+{
+	pci_host_common_release(platform_get_drvdata(pdev));
+}
 EXPORT_SYMBOL_GPL(pci_host_common_remove);
 
 MODULE_DESCRIPTION("Common library for PCI host controller drivers");
diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
index 51c35ec0cf37d..018e593bafe47 100644
--- a/drivers/pci/controller/pci-host-common.h
+++ b/drivers/pci/controller/pci-host-common.h
@@ -11,11 +11,13 @@
 #define _PCI_HOST_COMMON_H
 
 struct pci_ecam_ops;
+struct pci_host_bridge;
 
 int pci_host_common_probe(struct platform_device *pdev);
-int pci_host_common_init(struct platform_device *pdev,
-			 const struct pci_ecam_ops *ops);
+struct pci_host_bridge *pci_host_common_init(struct platform_device *pdev,
+					     const struct pci_ecam_ops *ops);
 void pci_host_common_remove(struct platform_device *pdev);
+void pci_host_common_release(struct pci_host_bridge *bridge);
 
 struct pci_config_window *pci_host_common_ecam_create(struct device *dev,
 	struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
-- 
2.51.1

[PATCH 2/2] PCI: apple: Store private pointer in platform device's drvdata

[Radu Rendec]

Now pci_host_common_init() no longer uses the platform device's drvdata
to store the pointer to the allocated struct pci_host_bridge. Use the
drvdata to store the pointer to the driver's private data, which is the
struct apple_pcie allocated in apple_pcie_probe(). This eliminates the
need to store these pointers in a separate mapping table.

This reverts most of the changes in commit 643c0c9d0496 ("PCI: apple:
Add tracking of probed root ports"). No "Fixes" tag is added because
nothing is broken in that commit. This is just a simplification.

Signed-off-by: Radu Rendec <rrendec@redhat.com>
---
 drivers/pci/controller/pcie-apple.c | 53 ++++-------------------------
 1 file changed, 7 insertions(+), 46 deletions(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 0380d300adca6..75bb6d51d31c2 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -187,7 +187,6 @@ struct apple_pcie {
 	const struct hw_info	*hw;
 	unsigned long		*bitmap;
 	struct list_head	ports;
-	struct list_head	entry;
 	struct completion	event;
 	struct irq_fwspec	fwspec;
 	u32			nvecs;
@@ -206,9 +205,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,45 +720,13 @@ 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;
-	}
-
-	return NULL;
-}
-
 static struct apple_pcie_port *apple_pcie_get_port(struct pci_dev *pdev)
 {
 	struct pci_config_window *cfg = pdev->sysdata;
-	struct apple_pcie *pcie;
+	struct apple_pcie *pcie = platform_get_drvdata(to_platform_device(cfg->parent));
 	struct pci_dev *port_pdev;
 	struct apple_pcie_port *port;
 
-	pcie = apple_pcie_lookup(cfg->parent);
-	if (WARN_ON(!pcie))
-		return NULL;
-
 	/* Find the root port this device is on */
 	port_pdev = pcie_find_root_port(pdev);
 
@@ -843,13 +807,9 @@ static void apple_pcie_disable_device(struct pci_host_bridge *bridge, struct pci
 static int apple_pcie_init(struct pci_config_window *cfg)
 {
 	struct device *dev = cfg->parent;
-	struct apple_pcie *pcie;
+	struct apple_pcie *pcie = platform_get_drvdata(to_platform_device(dev));
 	int ret;
 
-	pcie = apple_pcie_lookup(dev);
-	if (WARN_ON(!pcie))
-		return -ENOENT;
-
 	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
 		ret = apple_pcie_setup_port(pcie, of_port);
 		if (ret) {
@@ -876,6 +836,7 @@ static int apple_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct apple_pcie *pcie;
+	struct pci_host_bridge *bridge;
 	int ret;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
@@ -897,11 +858,11 @@ static int apple_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	apple_pcie_register(pcie);
+	bridge = pci_host_common_init(pdev, &apple_pcie_cfg_ecam_ops);
+	if (IS_ERR(bridge))
+		return PTR_ERR(bridge);
 
-	ret = pci_host_common_init(pdev, &apple_pcie_cfg_ecam_ops);
-	if (ret)
-		apple_pcie_unregister(pcie);
+	platform_set_drvdata(pdev, pcie);
 
 	return ret;
 }
-- 
2.51.1

Re: [PATCH 2/2] PCI: apple: Store private pointer in platform device's drvdata

[Marc Zyngier]

On Tue, 18 Nov 2025 22:12:44 +0000,
Radu Rendec <rrendec@redhat.com> wrote:
> 
> Now pci_host_common_init() no longer uses the platform device's drvdata
> to store the pointer to the allocated struct pci_host_bridge. Use the
> drvdata to store the pointer to the driver's private data, which is the
> struct apple_pcie allocated in apple_pcie_probe(). This eliminates the
> need to store these pointers in a separate mapping table.
> 
> This reverts most of the changes in commit 643c0c9d0496 ("PCI: apple:
> Add tracking of probed root ports"). No "Fixes" tag is added because
> nothing is broken in that commit. This is just a simplification.
> 
> Signed-off-by: Radu Rendec <rrendec@redhat.com>
> ---
>  drivers/pci/controller/pcie-apple.c | 53 ++++-------------------------
>  1 file changed, 7 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 0380d300adca6..75bb6d51d31c2 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -187,7 +187,6 @@ struct apple_pcie {
>  	const struct hw_info	*hw;
>  	unsigned long		*bitmap;
>  	struct list_head	ports;
> -	struct list_head	entry;
>  	struct completion	event;
>  	struct irq_fwspec	fwspec;
>  	u32			nvecs;
> @@ -206,9 +205,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,45 +720,13 @@ 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;
> -	}
> -
> -	return NULL;
> -}
> -
>  static struct apple_pcie_port *apple_pcie_get_port(struct pci_dev *pdev)
>  {
>  	struct pci_config_window *cfg = pdev->sysdata;
> -	struct apple_pcie *pcie;
> +	struct apple_pcie *pcie = platform_get_drvdata(to_platform_device(cfg->parent));

Isn't that an overly complicated way to write:

	struct apple_pcie *pcie = dev_get_drvdata(cfg->parent);

>  	struct pci_dev *port_pdev;
>  	struct apple_pcie_port *port;
>  
> -	pcie = apple_pcie_lookup(cfg->parent);
> -	if (WARN_ON(!pcie))
> -		return NULL;
> -
>  	/* Find the root port this device is on */
>  	port_pdev = pcie_find_root_port(pdev);
>  
> @@ -843,13 +807,9 @@ static void apple_pcie_disable_device(struct pci_host_bridge *bridge, struct pci
>  static int apple_pcie_init(struct pci_config_window *cfg)
>  {
>  	struct device *dev = cfg->parent;
> -	struct apple_pcie *pcie;
> +	struct apple_pcie *pcie = platform_get_drvdata(to_platform_device(dev));
>  	int ret;
>  
> -	pcie = apple_pcie_lookup(dev);
> -	if (WARN_ON(!pcie))
> -		return -ENOENT;
> -
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>  		ret = apple_pcie_setup_port(pcie, of_port);
>  		if (ret) {
> @@ -876,6 +836,7 @@ static int apple_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct apple_pcie *pcie;
> +	struct pci_host_bridge *bridge;
>  	int ret;
>  
>  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> @@ -897,11 +858,11 @@ static int apple_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	apple_pcie_register(pcie);
> +	bridge = pci_host_common_init(pdev, &apple_pcie_cfg_ecam_ops);
> +	if (IS_ERR(bridge))
> +		return PTR_ERR(bridge);
>  
> -	ret = pci_host_common_init(pdev, &apple_pcie_cfg_ecam_ops);
> -	if (ret)
> -		apple_pcie_unregister(pcie);
> +	platform_set_drvdata(pdev, pcie);

Not quite. You probably want to look at:

- when struct pci_ecam_ops::init() is called...
- ... compared to when you populate the driver data.

Thanks,

	M.

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

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

[Marc Zyngier]

On Tue, 18 Nov 2025 22:12:43 +0000,
Radu Rendec <rrendec@redhat.com> wrote:
> 
> Currently pci_host_common_init() uses the platform device's drvdata to
> store the pointer to the allocated struct pci_host_bridge. This makes
> sense for drivers that use pci_host_common_{probe,remove}() directly as
> the platform probe/remove functions, but leaves no option for more
> complex drivers to store a pointer to their own private data.
> 
> Change pci_host_common_init() to return the pointer to the allocated
> struct pci_host_bridge, and move the platform_set_drvdata() call to
> pci_host_common_probe(). This way, drivers that implement their own
> probe function can still use pci_host_common_init() but store their own
> pointer in the platform device's drvdata.
> 
> For symmetry, move the release code to a new function that takes a
> pointer to struct pci_host_bridge, and make pci_host_common_release() a
> wrapper to it that extracts the pointer from the platform device's
> drvdata. This way, drivers that store their own private pointer in the
> platform device's drvdata can still use the library release code.
> 
> No functional change to the existing users of pci-host-common is
> intended, with the exception of the pcie-apple driver, which is modified
> in a subsequent patch.

This paragraph doesn't belong here. Maybe as a note, but not in the
commit message.

> 
> Signed-off-by: Radu Rendec <rrendec@redhat.com>
> ---
>  drivers/pci/controller/pci-host-common.c | 36 ++++++++++++++++--------
>  drivers/pci/controller/pci-host-common.h |  6 ++--
>  2 files changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> index 810d1c8de24e9..86002195c93ac 100644
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c
> @@ -52,25 +52,24 @@ 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,
> -			 const struct pci_ecam_ops *ops)
> +struct pci_host_bridge *pci_host_common_init(struct platform_device *pdev,
> +					     const struct pci_ecam_ops *ops)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct pci_host_bridge *bridge;
>  	struct pci_config_window *cfg;
> +	int rc;
>  
>  	bridge = devm_pci_alloc_host_bridge(dev, 0);
>  	if (!bridge)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	of_pci_check_probe_only();
>  
> -	platform_set_drvdata(pdev, bridge);
> -
>  	/* Parse and map our Configuration Space windows */
>  	cfg = pci_host_common_ecam_create(dev, bridge, ops);
>  	if (IS_ERR(cfg))
> -		return PTR_ERR(cfg);
> +		return (struct pci_host_bridge *)cfg;
>  
>  	bridge->sysdata = cfg;
>  	bridge->ops = (struct pci_ops *)&ops->pci_ops;
> @@ -78,31 +77,46 @@ int pci_host_common_init(struct platform_device *pdev,
>  	bridge->disable_device = ops->disable_device;
>  	bridge->msi_domain = true;
>  
> -	return pci_host_probe(bridge);
> +	rc = pci_host_probe(bridge);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	return bridge;
>  }
>  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 = pci_host_common_init(pdev, ops);
> +	if (IS_ERR(bridge))
> +		return PTR_ERR(bridge);
> +
> +	platform_set_drvdata(pdev, bridge);

Congratulations, you just broke pcie-microchip-host.c again.

Yes, I did that myself in afc0a570bb613871 ("PCI: host-generic:
Extract an ECAM bridge creation helper from pci_host_common_probe()"),
and it was fixed by Geert in bdb32a0f67806 ("PCI: host-generic: Set
driver_data before calling gen_pci_init()").

> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(pci_host_common_probe);
>  
> -void pci_host_common_remove(struct platform_device *pdev)
> +void pci_host_common_release(struct pci_host_bridge *bridge)
>  {
> -	struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
> -
>  	pci_lock_rescan_remove();
>  	pci_stop_root_bus(bridge->bus);
>  	pci_remove_root_bus(bridge->bus);
>  	pci_unlock_rescan_remove();
>  }
> +EXPORT_SYMBOL_GPL(pci_host_common_release);

Even with the pcie-apple.c driver change, this is never called. I'd
refrain from adding an export until we actually have an identified
user.

> +
> +void pci_host_common_remove(struct platform_device *pdev)
> +{
> +	pci_host_common_release(platform_get_drvdata(pdev));
> +}
>  EXPORT_SYMBOL_GPL(pci_host_common_remove);
>  
>  MODULE_DESCRIPTION("Common library for PCI host controller drivers");
> diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
> index 51c35ec0cf37d..018e593bafe47 100644
> --- a/drivers/pci/controller/pci-host-common.h
> +++ b/drivers/pci/controller/pci-host-common.h
> @@ -11,11 +11,13 @@
>  #define _PCI_HOST_COMMON_H
>  
>  struct pci_ecam_ops;
> +struct pci_host_bridge;
>  
>  int pci_host_common_probe(struct platform_device *pdev);
> -int pci_host_common_init(struct platform_device *pdev,
> -			 const struct pci_ecam_ops *ops);
> +struct pci_host_bridge *pci_host_common_init(struct platform_device *pdev,
> +					     const struct pci_ecam_ops *ops);
>  void pci_host_common_remove(struct platform_device *pdev);
> +void pci_host_common_release(struct pci_host_bridge *bridge);
>  
>  struct pci_config_window *pci_host_common_ecam_create(struct device *dev,
>  	struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);

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.

Thanks,

	M.

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

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

[Radu Rendec]

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:
> > 
> > Currently pci_host_common_init() uses the platform device's drvdata to
> > store the pointer to the allocated struct pci_host_bridge. This makes
> > sense for drivers that use pci_host_common_{probe,remove}() directly as
> > the platform probe/remove functions, but leaves no option for more
> > complex drivers to store a pointer to their own private data.
> > 
> > Change pci_host_common_init() to return the pointer to the allocated
> > struct pci_host_bridge, and move the platform_set_drvdata() call to
> > pci_host_common_probe(). This way, drivers that implement their own
> > probe function can still use pci_host_common_init() but store their own
> > pointer in the platform device's drvdata.
> > 
> > For symmetry, move the release code to a new function that takes a
> > pointer to struct pci_host_bridge, and make pci_host_common_release() a
> > wrapper to it that extracts the pointer from the platform device's
> > drvdata. This way, drivers that store their own private pointer in the
> > platform device's drvdata can still use the library release code.
> > 
> > No functional change to the existing users of pci-host-common is
> > intended, with the exception of the pcie-apple driver, which is modified
> > in a subsequent patch.
> 
> This paragraph doesn't belong here. Maybe as a note, but not in the
> commit message.

Ack. And thanks for reviewing! More comments below.

> > 
> > Signed-off-by: Radu Rendec <rrendec@redhat.com>
> > ---
> >  drivers/pci/controller/pci-host-common.c | 36 ++++++++++++++++--------
> >  drivers/pci/controller/pci-host-common.h |  6 ++--
> >  2 files changed, 29 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> > index 810d1c8de24e9..86002195c93ac 100644
> > --- a/drivers/pci/controller/pci-host-common.c
> > +++ b/drivers/pci/controller/pci-host-common.c
> > @@ -52,25 +52,24 @@ 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,
> > - const struct pci_ecam_ops *ops)
> > +struct pci_host_bridge *pci_host_common_init(struct platform_device *pdev,
> > +      const struct pci_ecam_ops *ops)
> >  {
> >   struct device *dev = &pdev->dev;
> >   struct pci_host_bridge *bridge;
> >   struct pci_config_window *cfg;
> > + int rc;
> >  
> >   bridge = devm_pci_alloc_host_bridge(dev, 0);
> >   if (!bridge)
> > - return -ENOMEM;
> > + return ERR_PTR(-ENOMEM);
> >  
> >   of_pci_check_probe_only();
> >  
> > - platform_set_drvdata(pdev, bridge);
> > -
> >   /* Parse and map our Configuration Space windows */
> >   cfg = pci_host_common_ecam_create(dev, bridge, ops);
> >   if (IS_ERR(cfg))
> > - return PTR_ERR(cfg);
> > + return (struct pci_host_bridge *)cfg;
> >  
> >   bridge->sysdata = cfg;
> >   bridge->ops = (struct pci_ops *)&ops->pci_ops;
> > @@ -78,31 +77,46 @@ int pci_host_common_init(struct platform_device *pdev,
> >   bridge->disable_device = ops->disable_device;
> >   bridge->msi_domain = true;
> >  
> > - return pci_host_probe(bridge);
> > + rc = pci_host_probe(bridge);
> > + if (rc)
> > + return ERR_PTR(rc);
> > +
> > + return bridge;
> >  }
> >  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 = pci_host_common_init(pdev, ops);
> > + if (IS_ERR(bridge))
> > + return PTR_ERR(bridge);
> > +
> > + platform_set_drvdata(pdev, bridge);
> 
> Congratulations, you just broke pcie-microchip-host.c again.
> 
> Yes, I did that myself in afc0a570bb613871 ("PCI: host-generic:
> Extract an ECAM bridge creation helper from pci_host_common_probe()"),
> and it was fixed by Geert in bdb32a0f67806 ("PCI: host-generic: Set
> driver_data before calling gen_pci_init()").

Right. Which means there is an assumption (outside of
pci_host_common_probe() and pci_host_common_remove()) that the device's
drvdata is a pointer to the bridge. This is what I had feared but it
wasn't obvious and I failed to check thoroughly. Sorry!

> > +
> > + return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(pci_host_common_probe);
> >  
> > -void pci_host_common_remove(struct platform_device *pdev)
> > +void pci_host_common_release(struct pci_host_bridge *bridge)
> >  {
> > - struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
> > -
> >   pci_lock_rescan_remove();
> >   pci_stop_root_bus(bridge->bus);
> >   pci_remove_root_bus(bridge->bus);
> >   pci_unlock_rescan_remove();
> >  }
> > +EXPORT_SYMBOL_GPL(pci_host_common_release);
> 
> Even with the pcie-apple.c driver change, this is never called. I'd
> refrain from adding an export until we actually have an identified
> user.

That's fair. I just wanted to provide it for symmetry. But it's not
even needed if we change the approach (see below).

> > +
> > +void pci_host_common_remove(struct platform_device *pdev)
> > +{
> > + pci_host_common_release(platform_get_drvdata(pdev));
> > +}
> >  EXPORT_SYMBOL_GPL(pci_host_common_remove);
> >  
> >  MODULE_DESCRIPTION("Common library for PCI host controller drivers");
> > diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
> > index 51c35ec0cf37d..018e593bafe47 100644
> > --- a/drivers/pci/controller/pci-host-common.h
> > +++ b/drivers/pci/controller/pci-host-common.h
> > @@ -11,11 +11,13 @@
> >  #define _PCI_HOST_COMMON_H
> >  
> >  struct pci_ecam_ops;
> > +struct pci_host_bridge;
> >  
> >  int pci_host_common_probe(struct platform_device *pdev);
> > -int pci_host_common_init(struct platform_device *pdev,
> > - const struct pci_ecam_ops *ops);
> > +struct pci_host_bridge *pci_host_common_init(struct platform_device *pdev,
> > +      const struct pci_ecam_ops *ops);
> >  void pci_host_common_remove(struct platform_device *pdev);
> > +void pci_host_common_release(struct pci_host_bridge *bridge);
> >  
> >  struct pci_config_window *pci_host_common_ecam_create(struct device *dev,
> >   struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
> 
> 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.

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.

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.

-- 
Thanks,
Radu

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

[Marc Zyngier]

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.