Re: [PATCH] PCI: host-generic: Move bridge allocation outside of pci_host_common_init()

[Radu Rendec <rrendec@redhat.com>]

On Thu, 2025-11-20 at 11:36 +0000, Marc Zyngier wrote:
> Having the host bridge allocation inside pci_host_common_init() results
> in a lot of complexity in the pcie-apple driver (the only direct user
> of this function outside of code PCI code).
                              ^^^^
nit: s/code/core :)

> It forces the allocation of driver-specific tracking structures outside
> of the bridge allocation, which in turns requires it to use inneficient
> data structures to match the bridge and the private structre as needed.
> 
> Instead, let the bridge structure be passed to pci_host_common_init(),
> allowing the driver to allocate it together with the private data,
> as it is usually intended. The driver can then retrieve the bridge
> via the owning device attached to the PCI config window structure.
> This allows the pcie-apple driver to be significantly simplified.
> 
> Both core and driver code are changed in one go to avoid going via
> a transitional interface.
> 
> Link: https://lore.kernel.org/r/86jyzms036.wl-maz@kernel.org
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: Radu Rendec <rrendec@redhat.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Krzysztof Wilczyński <kwilczynski@kernel.org>
> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>
> ---
>  drivers/pci/controller/pci-host-common.c | 13 ++++----
>  drivers/pci/controller/pci-host-common.h |  1 +
>  drivers/pci/controller/pcie-apple.c      | 42 ++++--------------------
>  3 files changed, 14 insertions(+), 42 deletions(-)
> 
> 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..a902aa81a497e 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,9 @@ 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;
> +	return pci_host_bridge_priv(dev_get_drvdata(dev));
>  }
> 
> 

You forgot to remove the `entry` field from struct apple_pcie. It's no
longer needed now that pcie_list is gone.

>  
>  static struct apple_pcie_port *apple_pcie_get_port(struct pci_dev *pdev)
> @@ -875,13 +849,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,13 +873,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);
> -
> -	return ret;
> +	return pci_host_common_init(pdev, bridge, &apple_pcie_cfg_ecam_ops);
>  }
>  
>  static const struct of_device_id apple_pcie_of_match[] = {

With those two nitpicks addressed,

Reviewed-by: Radu Rendec <rrendec@redhat.com>

And thanks again for spending time on this and creating the patch.

-- 
Radu