Re: [PATCH] powerpc/pci: Remove useless null check before call of_node_put()

[Tyrel Datwyler <tyreld@linux.ibm.com>]

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

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