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

[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

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 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 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

[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

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 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

[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