Messages in this thread |  | | From | Michael Ellerman <> | Subject | Re: [PATCH] powerpc/pci: Remove useless null check before call of_node_put() | Date | Sun, 24 Apr 2022 00:32:57 +1000 |
| |
Tyrel Datwyler <tyreld@linux.ibm.com> writes: > On 4/20/22 19:52, Haowen Bai wrote: >> No need to add null check before call of_node_put(), since the >> implementation of of_node_put() has done it. >> >> Signed-off-by: Haowen Bai <baihaowen@meizu.com> >> --- >> arch/powerpc/kernel/pci_dn.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c >> index 61571ae23953..ba3bbc9bec2d 100644 >> --- a/arch/powerpc/kernel/pci_dn.c >> +++ b/arch/powerpc/kernel/pci_dn.c >> @@ -357,8 +357,8 @@ void pci_remove_device_node_info(struct device_node *dn) >> >> /* Drop the parent pci_dn's ref to our backing dt node */ >> parent = of_get_parent(dn); >> - if (parent) >> - of_node_put(parent); >> + >> + of_node_put(parent); > > This whole block of code looks useless, or suspect. Examining the rest of the > code for this function this is the only place that parent is referenced. The > of_get_parent() call returns the parent with its refcount incremented, and then > we turn around and call of_node_put() which drops that reference we just took. > The comment doesn't do what it says it does. If we really need to drop a > previous reference to the parent device node this code block would need to call > of_node_put() twice on parent to accomplish that.
Yeah good analysis.
It used to use pdn->parent, which didn't grab an extra reference, see commit 14db3d52d3a2 ("powerpc/eeh: Reduce use of pci_dn::node").
The old code was:
if (pdn->parent) of_node_put(pdn->parent->node);
> A closer examination is required to determine if what the comment says we need > to do is required. If it is then the code as it exists today is leaking that > reference AFAICS.
Yeah. This function is only called from pnv_php.c, ie. powernv PCI hotplug, which I think gets less testing than pseries hotplug. So possibly we are leaking references and haven't noticed, or maybe the comment is out of date.
cheers
|  |