From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julia Lawall Subject: Re: [PATCH] of: resolver: Add missing of_node_put Date: Thu, 28 Jan 2016 12:36:31 +0100 (CET) Message-ID: References: <20160127152017.GA16048@amitoj-Inspiron-3542> <20160127160531.GB17123@leverpostej> <20160127162153.GC17123@leverpostej> <20160128112810.GK17123@leverpostej> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Return-path: In-Reply-To: <20160128112810.GK17123@leverpostej> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Mark Rutland Cc: Pantelis Antoniou , Amitoj Kaur Chawla , Rob Herring , Frank Rowand , Grant Likely , Devicetree List , Linux Kernel Mailing List List-Id: devicetree@vger.kernel.org On Thu, 28 Jan 2016, Mark Rutland wrote: > On Wed, Jan 27, 2016 at 08:48:00PM +0100, Julia Lawall wrote: > > > > > > On Wed, 27 Jan 2016, Pantelis Antoniou wrote: > > > > > Hi Mark, > > > > > > > On Jan 27, 2016, at 18:21 , Mark Rutland wrote: > > > > > > > > On Wed, Jan 27, 2016 at 06:14:00PM +0200, Pantelis Antoniou wrote: > > > >> Hi Mark, > > > >> > > > >>> On Jan 27, 2016, at 18:05 , Mark Rutland wrote: > > > >>> > > > >>> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote: > > > >>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c > > > >>>> index 640eb4c..e2a0143 100644 > > > >>>> --- a/drivers/of/resolver.c > > > >>>> +++ b/drivers/of/resolver.c > > > >>>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node, > > > >>>> > > > >>>> for_each_child_of_node(node, child) { > > > >>>> found = __of_find_node_by_full_name(child, full_name); > > > >>>> - if (found != NULL) > > > >>>> + if (found != NULL) { > > > >>>> + of_node_put(child); > > > >>>> return found; > > > >>>> + } > > > >>>> } > > > >>>> > > > >>>> return NULL; > > > >>> > > > >>> I don't think this is quite right. When child == found, this change will > > > >>> leave it decremented. > > > >>> > > > >> > > > >> > > > >> This patch is bogus. > > > >> > > > >> __of_find_node_by_full_name() is not taking a reference on the node if found. > > > >> This method relies on keeping the reference taken by the loop. > > > > > > > > Sure. For the found node, that makes sense. > > > > > > > > However, it also increments the refcount of all the parents, which does > > > > not seem correct to me, given they're not put on the return path, and a > > > > put of the found node won't decrement its parents refcounts, unless I > > > > have missed something. > > > > > > > > > > Hmm, yes. The parent refcounts must be decremented. > > > > So there should be if (found != child) of_node_put(child); ? > > That would match the intended semantics, yes. I don't think so. I sent another mail with what seems like a better solution (upping the reference count of the child that is selected). julia -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html