On 4/23/22 07:32, Michael Ellerman wrote:
> Tyrel Datwyler <tyr...@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 <baihao...@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.
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
