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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86ldk2s1i7.wl-maz@kernel.org>
Date: Wed, 19 Nov 2025 11:30:40 +0000
From: Marc Zyngier <maz@...nel.org>
To: Radu Rendec <rrendec@...hat.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,	Manivannan Sadhasivam
 <mani@...nel.org>,	Will Deacon <will@...nel.org>,	Rob Herring
 <robh@...nel.org>,	Krzysztof Wilczyński
 <kwilczynski@...nel.org>,	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	linux-pci@...r.kernel.org,	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] PCI: apple: Store private pointer in platform device's drvdata

On Tue, 18 Nov 2025 22:12:44 +0000,
Radu Rendec <rrendec@...hat.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@...hat.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.