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

[Marc Zyngier <maz@kernel.org>]

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.