PCI/pwrctrl: Fix device and OF node leaks
Subsystems (2)
cover PCI/pwrctrl: Fix device and OF node leaks
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(-)
Signature · 1 line
2.49.1
PATCH 1/3 PCI/pwrctrl: Fix device leak at registration
Make sure to drop the reference to the pwrctrl device taken by of_find_device_by_node() when registering a PCI device.
--- drivers/pci/bus.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
drivers/pci/bus.c +9 -5
@@ -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))
Signature · 1 line
2.49.1
Re: [PATCH 1/3] PCI/pwrctrl: Fix device leak at registration
Quoted 7 lines · On Mon, 21 Jul 2025 17:36:07 +0200 Johan Hovold <[email protected]> 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 [email protected] # 6.13Cc Manivannan Sadhasivam <[email protected]>Signed-off-by Johan Hovold <[email protected]>
Hi Johan,
Perhaps time forDEFINE_FREE(put_pdev, struct platform_device *, if (_T) put_device(&_T->dev));
then...
Quoted 9 lines
--- drivers/pci/bus.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)drivers/pci/bus.c +0
-0
drivers/pci/bus.c void pci_bus_add_device(struct pci_dev *dev) · line 369
Quoted 8 lines
drivers/pci/bus.c +0
-5@@ -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);
drivers/pci/bus.c void pci_bus_add_device(struct pci_dev *dev) · line 373
Quoted 9 lines
drivers/pci/bus.c +9
-0+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.
Quoted 3 lines
drivers/pci/bus.c +0
-0}if (!dn ||of_device_is_available (dn))
Re: [PATCH 1/3] PCI/pwrctrl: Fix device leak at registration
Quoted 40 lines · Jonathan Cameron
On Mon, 21 Jul 2025 17:36:07 +0200
Johan Hovold <[email protected]> wrote:Perhaps time for
DEFINE_FREE(put_pdev, struct platform_device *, if (_T) put_device(&_T->dev));then...
Quoted 9 lines
--- drivers/pci/bus.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)drivers/pci/bus.c +0
-0drivers/pci/bus.c void pci_bus_add_device(struct pci_dev *dev) · line 369
Quoted 8 lines
drivers/pci/bus.c +0
-5@@ -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);drivers/pci/bus.c void pci_bus_add_device(struct pci_dev *dev) · line 373
Quoted 9 lines
drivers/pci/bus.c +9
-0+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.
Quoted 2 lines
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
In reply to Johan Hovold…
Quoted 2 lines
+++
b/drivers/pci/bus.c(quoted diff) +0
-0@@ -362,11 +362,15 @@ void pci_bus_add_device(struct pci_dev *dev)
…
drivers/pci/bus.c void pci_bus_add_device(struct pci_dev *dev) · line 372
Quoted 7 lines
drivers/pci/bus.c +7
-0+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
In reply to Johan HovoldMake 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.
--- drivers/pci/probe.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
drivers/pci/probe.c +16 -3
@@ -2515,9 +2515,15 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, instruct 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 ; }/*
Signature · 1 line
2.49.1
Re: [PATCH 2/3] PCI/pwrctrl: Fix device and OF node leak at bus scan
Quoted 8 lines · On Mon, 21 Jul 2025 17:36:08 +0200 Johan Hovold <[email protected]> wrote:
Make sure to drop the references to the pwrctrl OF node and device taken by
of_pci_find_child_device()andof_find_device_by_node()respectively when scanning the bus.Fixes 957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")Cc [email protected] # 6.15Cc Manivannan Sadhasivam <[email protected]>Signed-off-by Johan Hovold <[email protected]>
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.
Quoted 9 lines
--- drivers/pci/probe.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)drivers/pci/probe.c +0
-0
drivers/pci/probe.c static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in · line 2521
Quoted 8 lines
drivers/pci/probe.c +2
-1@@ -2515,9 +2515,15 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, instruct 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.
Quoted 35 lines
drivers/pci/probe.c +14
-2+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
Quoted 11 lines · Jonathan Cameron
Quoted 8 lines · On Mon, 21 Jul 2025 17:36:08 +0200 Johan Hovold <[email protected]> wrote:
drivers/pci/probe.c +1
-1@@ -2515,9 +2515,15 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, instruct 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 ;
Quoted 4 lines
drivers/pci/probe.c static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in · line 2521
Quoted 1 line
drivers/pci/probe.c +1
-0+ 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
In reply to Johan HovoldMake sure to drop the reference to the pwrctrl device taken by of_find_device_by_node() when stopping a PCI device.
--- drivers/pci/remove.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/pci/remove.c +2 -0
@@ -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 ); }
Signature · 1 line
2.49.1
Quoted 4 lines · 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.
- Mani
Quoted 16 lines
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 stopdrivers/pci/bus.c | 14 +++++++++----- drivers/pci/probe.c | 19 ++++++++++++++++--- drivers/pci/remove.c | 2 ++ 3 files changed, 27 insertions(+), 8 deletions(-)Signature · 1 line
2.49.1
Signature · 1 line
மணிவண்ணன் சதாசிவம்
Quoted 8 lines · 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 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
Bjorn Helgaas
AppliedQuoted 12 lines · 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
Quoted 4 lines
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!