Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7426c4c7-b2f2-5453-bdab-a88c7308b212@linux.ibm.com>
Date:   Mon, 25 Apr 2022 12:57:00 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH] powerpc/pci: Remove useless null check before call
 of_node_put()
Content-Language: en-US
To:     Michael Ellerman <mpe@ellerman.id.au>,
        Haowen Bai <baihaowen@meizu.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>
Cc:     linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
References: <1650509529-27525-1-git-send-email-baihaowen@meizu.com>
 <c4613523-de98-b824-175a-89fd66931bd6@linux.ibm.com>
 <87levv98fa.fsf@mpe.ellerman.id.au>
From:   Tyrel Datwyler <tyreld@linux.ibm.com>
In-Reply-To: <87levv98fa.fsf@mpe.ellerman.id.au>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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