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 <[email protected]>
---
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);
/*
* At this point we *might* still have a pci_dev that was
--
2.7.4
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 <[email protected]>
> ---
> 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.
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.
-Tyrel
>
> /*
> * At this point we *might* still have a pci_dev that was
Tyrel Datwyler <[email protected]> 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 <[email protected]>
>> ---
>> 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
On 4/23/22 07:32, Michael Ellerman wrote:
> Tyrel Datwyler <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
Looks like we leak it. From pci_add_device_node_info() we clearly take a
reference we don't free:
/* Attach to parent node */
INIT_LIST_HEAD(&pdn->child_list);
INIT_LIST_HEAD(&pdn->list);
parent = of_get_parent(dn);
pdn->parent = parent ? PCI_DN(parent) : NULL;
if (pdn->parent)
list_add_tail(&pdn->list, &pdn->parent->child_list);
return pdn;
The question becomes whats the right fix. Doing a double put in the remove path
seems wrong, and looks gross. We no longer store a reference to the parent
device node in pci_dn::parent but instead a reference to the an actual pci_dn
struct. Seems to suggest we can drop the reference taken in
pci_add_device_node_info().
-Tyrel
>
> cheers