[PATCH 0/3] PCI/pwrctrl: Fix device and OF node leaks

[PATCH 0/3] PCI/pwrctrl: Fix device and OF node leaks

[Johan Hovold]

This series fixes some pwrctrl device and OF node leaks spotted while
discussing the recent pwrctrl-related (ASPM and probe) regressions on
Qualcomm platforms.

Johan


Johan Hovold (3):
  PCI/pwrctrl: Fix device leak at registration
  PCI/pwrctrl: Fix device and OF node leak at bus scan
  PCI/pwrctrl: Fix device leak at device stop

 drivers/pci/bus.c    | 14 +++++++++-----
 drivers/pci/probe.c  | 19 ++++++++++++++++---
 drivers/pci/remove.c |  2 ++
 3 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.49.1

[PATCH 1/3] PCI/pwrctrl: Fix device leak at registration

[Johan Hovold]

Make sure to drop the reference to the pwrctrl device taken by
of_find_device_by_node() when registering a PCI device.

Fixes: b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers are probed before PCI client drivers")
Cc: stable@vger.kernel.org	# 6.13
Cc: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pci/bus.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 69048869ef1c..0394a9c77b38 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -362,11 +362,15 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * before PCI client drivers.
 	 */
 	pdev = of_find_device_by_node(dn);
-	if (pdev && of_pci_supply_present(dn)) {
-		if (!device_link_add(&dev->dev, &pdev->dev,
-				     DL_FLAG_AUTOREMOVE_CONSUMER))
-			pci_err(dev, "failed to add device link to power control device %s\n",
-				pdev->name);
+	if (pdev) {
+		if (of_pci_supply_present(dn)) {
+			if (!device_link_add(&dev->dev, &pdev->dev,
+					     DL_FLAG_AUTOREMOVE_CONSUMER)) {
+				pci_err(dev, "failed to add device link to power control device %s\n",
+					pdev->name);
+			}
+		}
+		put_device(&pdev->dev);
 	}
 
 	if (!dn || of_device_is_available(dn))
-- 
2.49.1

[PATCH 2/3] PCI/pwrctrl: Fix device and OF node leak at bus scan

[Johan Hovold]

Make sure to drop the references to the pwrctrl OF node and device taken
by of_pci_find_child_device() and of_find_device_by_node() respectively
when scanning the bus.

Fixes: 957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")
Cc: stable@vger.kernel.org	# 6.15
Cc: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pci/probe.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4b8693ec9e4c..c5f59de790c7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2515,9 +2515,15 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
 	struct device_node *np;
 
 	np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
-	if (!np || of_find_device_by_node(np))
+	if (!np)
 		return NULL;
 
+	pdev = of_find_device_by_node(np);
+	if (pdev) {
+		put_device(&pdev->dev);
+		goto err_put_of_node;
+	}
+
 	/*
 	 * First check whether the pwrctrl device really needs to be created or
 	 * not. This is decided based on at least one of the power supplies
@@ -2525,17 +2531,24 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
 	 */
 	if (!of_pci_supply_present(np)) {
 		pr_debug("PCI/pwrctrl: Skipping OF node: %s\n", np->name);
-		return NULL;
+		goto err_put_of_node;
 	}
 
 	/* Now create the pwrctrl device */
 	pdev = of_platform_device_create(np, NULL, &host->dev);
 	if (!pdev) {
 		pr_err("PCI/pwrctrl: Failed to create pwrctrl device for node: %s\n", np->name);
-		return NULL;
+		goto err_put_of_node;
 	}
 
+	of_node_put(np);
+
 	return pdev;
+
+err_put_of_node:
+	of_node_put(np);
+
+	return NULL;
 }
 
 /*
-- 
2.49.1

[PATCH 3/3] PCI/pwrctrl: Fix device leak at device stop

[Johan Hovold]

Make sure to drop the reference to the pwrctrl device taken by
of_find_device_by_node() when stopping a PCI device.

Fixes: 681725afb6b9 ("PCI/pwrctl: Remove pwrctl device without iterating over all children of pwrctl parent")
Cc: stable@vger.kernel.org	# 6.13
Cc: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pci/remove.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 445afdfa6498..16f21edbc29d 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -31,6 +31,8 @@ static void pci_pwrctrl_unregister(struct device *dev)
 		return;
 
 	of_device_unregister(pdev);
+	put_device(&pdev->dev);
+
 	of_node_clear_flag(np, OF_POPULATED);
 }
 
-- 
2.49.1

Re: [PATCH 1/3] PCI/pwrctrl: Fix device leak at registration

[Jonathan Cameron]

On Mon, 21 Jul 2025 17:36:07 +0200
Johan Hovold <johan+linaro@kernel.org> wrote:

> Make sure to drop the reference to the pwrctrl device taken by
> of_find_device_by_node() when registering a PCI device.
> 
> Fixes: b458ff7e8176 ("PCI/pwrctl: Ensure that pwrctl drivers are probed before PCI client drivers")
> Cc: stable@vger.kernel.org	# 6.13
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Hi Johan,

Perhaps time for 
DEFINE_FREE(put_pdev, struct platform_device *, if (_T) put_device(&_T->dev));

then...

> ---
>  drivers/pci/bus.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 69048869ef1c..0394a9c77b38 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -362,11 +362,15 @@ void pci_bus_add_device(struct pci_dev *dev)
>  	 * before PCI client drivers.
>  	 */
>  	pdev = of_find_device_by_node(dn);
> -	if (pdev && of_pci_supply_present(dn)) {
> -		if (!device_link_add(&dev->dev, &pdev->dev,
> -				     DL_FLAG_AUTOREMOVE_CONSUMER))
> -			pci_err(dev, "failed to add device link to power control device %s\n",
> -				pdev->name);

	struct platform_device *pdev __free(put_pdev) =
		of_find_device_by_node(dn);
> +	if (pdev) {
> +		if (of_pci_supply_present(dn)) {
> +			if (!device_link_add(&dev->dev, &pdev->dev,
> +					     DL_FLAG_AUTOREMOVE_CONSUMER)) {
> +				pci_err(dev, "failed to add device link to power control device %s\n",
> +					pdev->name);
> +			}
> +		}
> +		put_device(&pdev->dev);

and no need for any explicit put.

We already do this extensively in some subsystems (e.g. CXL) and it
greatly simplifies code.

>  	}
>  
>  	if (!dn || of_device_is_available(dn))

Re: [PATCH 2/3] PCI/pwrctrl: Fix device and OF node leak at bus scan

[Jonathan Cameron]

On Mon, 21 Jul 2025 17:36:08 +0200
Johan Hovold <johan+linaro@kernel.org> wrote:

> Make sure to drop the references to the pwrctrl OF node and device taken
> by of_pci_find_child_device() and of_find_device_by_node() respectively
> when scanning the bus.
> 
> Fixes: 957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")
> Cc: stable@vger.kernel.org	# 6.15
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Similar to previous, I'd use a new DEFINE_FREE for the platform device and
the infrastructure is already in place for the for the of_node
(and used a lot in the core DT code).

One other comment inline.


> ---
>  drivers/pci/probe.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4b8693ec9e4c..c5f59de790c7 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2515,9 +2515,15 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
>  	struct device_node *np;
>  
>  	np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
> -	if (!np || of_find_device_by_node(np))
> +	if (!np)
>  		return NULL;
>  
> +	pdev = of_find_device_by_node(np);
Given we have two entirely different pdevs in here, I'd use an extra
local variable to indicate what this one is the pwctrl one created below.

> +	if (pdev) {
> +		put_device(&pdev->dev);
> +		goto err_put_of_node;
> +	}
> +
>  	/*
>  	 * First check whether the pwrctrl device really needs to be created or
>  	 * not. This is decided based on at least one of the power supplies
> @@ -2525,17 +2531,24 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
>  	 */
>  	if (!of_pci_supply_present(np)) {
>  		pr_debug("PCI/pwrctrl: Skipping OF node: %s\n", np->name);
> -		return NULL;
> +		goto err_put_of_node;
>  	}
>  
>  	/* Now create the pwrctrl device */
>  	pdev = of_platform_device_create(np, NULL, &host->dev);
>  	if (!pdev) {
>  		pr_err("PCI/pwrctrl: Failed to create pwrctrl device for node: %s\n", np->name);
> -		return NULL;
> +		goto err_put_of_node;
>  	}
>  
> +	of_node_put(np);
> +
>  	return pdev;
> +
> +err_put_of_node:
> +	of_node_put(np);
> +
> +	return NULL;
>  }
>  
>  /*

Re: [PATCH 1/3] PCI/pwrctrl: Fix device leak at registration

[Johan Hovold]

On Tue, Jul 22, 2025 at 11:05:26AM +0100, Jonathan Cameron wrote:
> On Mon, 21 Jul 2025 17:36:07 +0200
> Johan Hovold <johan+linaro@kernel.org> wrote:

> Perhaps time for 
> DEFINE_FREE(put_pdev, struct platform_device *, if (_T) put_device(&_T->dev));
> 
> then...
> 
> > ---
> >  drivers/pci/bus.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 69048869ef1c..0394a9c77b38 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -362,11 +362,15 @@ void pci_bus_add_device(struct pci_dev *dev)
> >  	 * before PCI client drivers.
> >  	 */
> >  	pdev = of_find_device_by_node(dn);
> > -	if (pdev && of_pci_supply_present(dn)) {
> > -		if (!device_link_add(&dev->dev, &pdev->dev,
> > -				     DL_FLAG_AUTOREMOVE_CONSUMER))
> > -			pci_err(dev, "failed to add device link to power control device %s\n",
> > -				pdev->name);
> 
> 	struct platform_device *pdev __free(put_pdev) =
> 		of_find_device_by_node(dn);
> > +	if (pdev) {
> > +		if (of_pci_supply_present(dn)) {
> > +			if (!device_link_add(&dev->dev, &pdev->dev,
> > +					     DL_FLAG_AUTOREMOVE_CONSUMER)) {
> > +				pci_err(dev, "failed to add device link to power control device %s\n",
> > +					pdev->name);
> > +			}
> > +		}
> > +		put_device(&pdev->dev);
> 
> and no need for any explicit put.
> 
> We already do this extensively in some subsystems (e.g. CXL) and it
> greatly simplifies code.

No, I'm no fan of those kind of changes which I find leads to less
readable code (e.g. with those in-code declarations).

Johan

Re: [PATCH 2/3] PCI/pwrctrl: Fix device and OF node leak at bus scan

[Johan Hovold]

On Tue, Jul 22, 2025 at 11:08:33AM +0100, Jonathan Cameron wrote:
> On Mon, 21 Jul 2025 17:36:08 +0200
> Johan Hovold <johan+linaro@kernel.org> wrote:

> > @@ -2515,9 +2515,15 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
> >  	struct device_node *np;
> >  
> >  	np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
> > -	if (!np || of_find_device_by_node(np))
> > +	if (!np)
> >  		return NULL;
> >  
> > +	pdev = of_find_device_by_node(np);

> Given we have two entirely different pdevs in here, I'd use an extra
> local variable to indicate what this one is the pwctrl one created below.

It's actually the same pwrctrl platform device (which may have been
created in an earlier call to the function) so using the same variable
should be fine.

Johan

Re: [PATCH 1/3] PCI/pwrctrl: Fix device leak at registration

[Markus Elfring]

…
> +++ b/drivers/pci/bus.c
> @@ -362,11 +362,15 @@ void pci_bus_add_device(struct pci_dev *dev)
…
> +		if (of_pci_supply_present(dn)) {
> +			if (!device_link_add(&dev->dev, &pdev->dev,
> +					     DL_FLAG_AUTOREMOVE_CONSUMER)) {
> +				pci_err(dev, "failed to add device link to power control device %s\n",
> +					pdev->name);
> +			}
> +		}
…

How do you think about to reconsider the usage of any curly brackets
once more?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.16-rc7#n197

Regards,
Markus

Re: [PATCH 0/3] PCI/pwrctrl: Fix device and OF node leaks

[Manivannan Sadhasivam]

On Mon, Jul 21, 2025 at 05:36:06PM GMT, Johan Hovold wrote:
> This series fixes some pwrctrl device and OF node leaks spotted while
> discussing the recent pwrctrl-related (ASPM and probe) regressions on
> Qualcomm platforms.
> 

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

- Mani

> Johan
> 
> 
> Johan Hovold (3):
>   PCI/pwrctrl: Fix device leak at registration
>   PCI/pwrctrl: Fix device and OF node leak at bus scan
>   PCI/pwrctrl: Fix device leak at device stop
> 
>  drivers/pci/bus.c    | 14 +++++++++-----
>  drivers/pci/probe.c  | 19 ++++++++++++++++---
>  drivers/pci/remove.c |  2 ++
>  3 files changed, 27 insertions(+), 8 deletions(-)
> 
> -- 
> 2.49.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 0/3] PCI/pwrctrl: Fix device and OF node leaks

[Johan Hovold]

On Mon, Jul 21, 2025 at 05:36:06PM +0200, Johan Hovold wrote:
> This series fixes some pwrctrl device and OF node leaks spotted while
> discussing the recent pwrctrl-related (ASPM and probe) regressions on
> Qualcomm platforms.

> Johan Hovold (3):
>   PCI/pwrctrl: Fix device leak at registration
>   PCI/pwrctrl: Fix device and OF node leak at bus scan
>   PCI/pwrctrl: Fix device leak at device stop

Can these be picked up?

Johan

Re: [PATCH 0/3] PCI/pwrctrl: Fix device and OF node leaks

[Bjorn Helgaas]

On Mon, Jul 21, 2025 at 05:36:06PM +0200, Johan Hovold wrote:
> This series fixes some pwrctrl device and OF node leaks spotted while
> discussing the recent pwrctrl-related (ASPM and probe) regressions on
> Qualcomm platforms.
> 
> Johan
> 
> 
> Johan Hovold (3):
>   PCI/pwrctrl: Fix device leak at registration
>   PCI/pwrctrl: Fix device and OF node leak at bus scan
>   PCI/pwrctrl: Fix device leak at device stop
> 
>  drivers/pci/bus.c    | 14 +++++++++-----
>  drivers/pci/probe.c  | 19 ++++++++++++++++---
>  drivers/pci/remove.c |  2 ++
>  3 files changed, 27 insertions(+), 8 deletions(-)

Applied to pci/pwrctrl for v6.18, thanks, Johan!