Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <c4613523-de98-b824-175a-89fd66931bd6@linux.ibm.com>
Date:   Fri, 22 Apr 2022 13:05:51 -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:     Haowen Bai <baihaowen@meizu.com>,
        Michael Ellerman <mpe@ellerman.id.au>,
        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>
From:   Tyrel Datwyler <tyreld@linux.ibm.com>
In-Reply-To: <1650509529-27525-1-git-send-email-baihaowen@meizu.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

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