From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH] mmc: core: Cleanup unused OF nodes while parsing for child nodes Date: Tue, 31 Mar 2015 09:06:09 +0200 Message-ID: <551A4761.2070803@redhat.com> References: <1427707141-3152-1-git-send-email-ulf.hansson@linaro.org> <551951B0.7090704@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:60667 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750774AbbCaHGO (ORCPT ); Tue, 31 Mar 2015 03:06:14 -0400 In-Reply-To: Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Ulf Hansson Cc: linux-mmc Hi, On 30-03-15 17:09, Ulf Hansson wrote: > On 30 March 2015 at 15:37, Hans de Goede wrote: >> Hi, >> >> >> On 30-03-15 11:19, Ulf Hansson wrote: >>> >>> Signed-off-by: Ulf Hansson >>> --- >>> drivers/mmc/core/core.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >>> index c296bc0..e6b0bdb 100644 >>> --- a/drivers/mmc/core/core.c >>> +++ b/drivers/mmc/core/core.c >>> @@ -1291,6 +1291,8 @@ struct device_node *mmc_of_find_child_device(struct >>> mmc_host *host, >>> for_each_child_of_node(host->parent->of_node, node) { >>> if (mmc_of_get_func_num(node) == func_num) >>> return node; >>> + else >>> + of_node_put(node); >>> } >>> >>> return NULL; >>> >> >> I don't think this is right, non of the other users of >> for_each_child_of_node >> do this, I think that rather then doing this we should be changing the >> callers > > So, everybody don't follow the API. Cool. :-) Hmm, I don't know where (if anywhere) the API is specified, but if I look at the actual implementation in include/linux/of.h #define for_each_child_of_node(parent, child) \ for (child = of_get_next_child(parent, NULL); child != NULL; \ child = of_get_next_child(parent, child)) And in drivers/of/base.c: static struct device_node *__of_get_next_child(const struct device_node *node, struct device_node *prev) { struct device_node *next; if (!node) return NULL; next = prev ? prev->sibling : node->child; for (; next; next = next->sibling) if (of_node_get(next)) break; of_node_put(prev); return next; } (the non __ prefixed version takes a lock then calls into this one) Note the "of_node_put(prev);" in the of_get_next_child implementation, so yes we've a ref while going through the loop, but its gets freed on the "increment" part of the for. Also see e.g. include/linux/of.h : static inline int of_get_child_count(const struct device_node *np) { struct device_node *child; int num = 0; for_each_child_of_node(np, child) num++; return num; } Which I would expect to get things right. >> of mmc_of_find_child_device to do: of_node_get(), except for the call which >> my "mmc: Add support for marking hpi as broken through devicetree" patch >> adds, >> as that is intended to only take a temporary reference. >> > > In principle you are saying that the implementation of > for_each_child_of_node() API needs to be adopted for how users > actually use it, which means leave the of_node_get|put() to be done > entirely by the caller, right? What I'm saying is that, if I'm not reading the code the wrong way, that is already how the for_each_child_of_node() API works. As for the mmc subsys it seems that means that no changes are necessary, since we do: for_each_child_of_node(host->parent->of_node, node) { if (mmc_of_get_func_num(node) == func_num) return node; } So when we've found the right node, we jump out of the loop, returning the reference we have while in the loop. This does mean that my: "mmc: Add support for marking hpi as broken through devicetree" patch needs to be changed as I ended up calling mmc_of_find_child_device twice in there, since in that patch I need the of_node before mmc_add_card() gets called, so I'm leaking a reference there. I'll do a v2 fixing this. Regards, Hans