From patchwork Thu Jan 22 16:14:46 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Grant Likely X-Patchwork-Id: 43535 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-la0-f70.google.com (mail-la0-f70.google.com [209.85.215.70]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id EB4C1240DB for ; Thu, 22 Jan 2015 16:15:00 +0000 (UTC) Received: by mail-la0-f70.google.com with SMTP id hs14sf1586326lab.1 for ; Thu, 22 Jan 2015 08:14:59 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:delivered-to:from:subject:to:cc :in-reply-to:references:date:message-id:sender:precedence:list-id :x-original-sender:x-original-authentication-results:mailing-list :list-post:list-help:list-archive:list-unsubscribe; bh=Z0/+lllGUgnxxpZHKmkI4eP8yPtD1u9Qwbrpac4BY0o=; b=YrLtnz6dv/GTvo0NtinnhXdI7zTKmqTQADSGMeQYWjIwANcD41uLrV1qvdosoX+d4O pykFK5TBJmwhpBgaDIVaTFIUOAWvtzRXXlWHbmy7aUGRUtOLj3yvlPbpakkfhVKys2Po CIuDmO3+Ula1YWbZ9KEU6H+5o2z2Wl9Ckq28Q2TZZuj6Sdbn2ZGIolULmis1iQ1gcF7i Pm+xb1wsBOuj+3B/COmwPATHiWMBoMK2R1XSIiK312/GTst9GAt+pAfQvsY+r4QWciaD PIkOHlw2/ofIrGiObA1kjNHX+F3CLOFTthEJ/UoQuil1O4vTe+f+vUkZ33MQFSq9Vdg0 Bx2g== X-Gm-Message-State: ALoCoQmcGdp6GC0cxe69z8ovkkbrp4oHKTdhviqNSO4Iu/FBfHL6+lHmnjIUV9msWWbU3b8Le3W6 X-Received: by 10.152.2.40 with SMTP id 8mr334191lar.7.1421943299858; Thu, 22 Jan 2015 08:14:59 -0800 (PST) MIME-Version: 1.0 X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.29.69 with SMTP id i5ls23078lah.23.gmail; Thu, 22 Jan 2015 08:14:59 -0800 (PST) X-Received: by 10.152.29.6 with SMTP id f6mr2673111lah.32.1421943299676; Thu, 22 Jan 2015 08:14:59 -0800 (PST) Received: from mail-lb0-f174.google.com (mail-lb0-f174.google.com. [209.85.217.174]) by mx.google.com with ESMTPS id ld9si21094693lbc.77.2015.01.22.08.14.59 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 22 Jan 2015 08:14:59 -0800 (PST) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.174 as permitted sender) client-ip=209.85.217.174; Received: by mail-lb0-f174.google.com with SMTP id f15so2419463lbj.5 for ; Thu, 22 Jan 2015 08:14:59 -0800 (PST) X-Received: by 10.112.52.229 with SMTP id w5mr2641168lbo.52.1421943299540; Thu, 22 Jan 2015 08:14:59 -0800 (PST) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.9.200 with SMTP id c8csp313677lbb; Thu, 22 Jan 2015 08:14:58 -0800 (PST) X-Received: by 10.68.130.225 with SMTP id oh1mr3164098pbb.157.1421943297319; Thu, 22 Jan 2015 08:14:57 -0800 (PST) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id zd8si13191266pac.24.2015.01.22.08.14.56; Thu, 22 Jan 2015 08:14:57 -0800 (PST) Received-SPF: none (google.com: devicetree-owner@vger.kernel.org does not designate permitted sender hosts) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753808AbbAVQOv (ORCPT + 5 others); Thu, 22 Jan 2015 11:14:51 -0500 Received: from mail-we0-f176.google.com ([74.125.82.176]:35290 "EHLO mail-we0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753784AbbAVQOt (ORCPT ); Thu, 22 Jan 2015 11:14:49 -0500 Received: by mail-we0-f176.google.com with SMTP id w62so2686589wes.7 for ; Thu, 22 Jan 2015 08:14:48 -0800 (PST) X-Received: by 10.180.8.169 with SMTP id s9mr6771483wia.72.1421943288513; Thu, 22 Jan 2015 08:14:48 -0800 (PST) Received: from trevor.secretlab.ca (host81-159-185-229.range81-159.btcentralplus.com. [81.159.185.229]) by mx.google.com with ESMTPSA id jp3sm3614942wid.9.2015.01.22.08.14.47 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 22 Jan 2015 08:14:47 -0800 (PST) Received: by trevor.secretlab.ca (Postfix, from userid 1000) id 101EBC40A80; Thu, 22 Jan 2015 16:14:46 +0000 (GMT) From: Grant Likely Subject: Re: [PATCH] of: Add missing of_node_put() in of_find_node_by_path() To: Geert Uytterhoeven , Rob Herring Cc: Gaurav Minocha , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Geert Uytterhoeven In-Reply-To: <1421250356-16690-1-git-send-email-geert+renesas@glider.be> References: <1421250356-16690-1-git-send-email-geert+renesas@glider.be> Date: Thu, 22 Jan 2015 16:14:46 +0000 Message-Id: <20150122161446.101EBC40A80@trevor.secretlab.ca> Sender: devicetree-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: devicetree@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: grant.likely@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.174 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , On Wed, 14 Jan 2015 16:45:56 +0100 , Geert Uytterhoeven wrote: > When traversing all nodes and moving to a new path component, the old > one must be released by calling of_node_put(). Else the refcounts of the > parent node(s) will not be decremented. > > Signed-off-by: Geert Uytterhoeven > --- > Background. > > While investigating a reference count imbalance issue with > CONFIG_OF_DYNAMIC=y, I wrote the debug code below to validate the > reference counts of the nodes I was interested in. > During the first call of check_refcnts(), it gathers all reference > counts. During a subsequent call, it verifies that they are still the > same. > > Surprisingly, lots of reference counts were wrong, and kept incrementing > every time check_refcnts() was called. Yup, reference counting is a mess. We definitely need tests to make sure the core code does the right things with them. > I was just wondering whether it would be useful to have a reference > count test in OF_UNITTEST, but now I see the "select OF_DYNAMIC" will go > away? select OF_DYNAMIC is going away, but that only so that unittests work with both OF_DYNAMIC and !OF_DYANMIC. Instead there will be some unittests that are only run when OF_DYNAMIC is selected. > > Feel free to (ab)use the code below and derive a unittest from it... > > static struct to_check { > const char *path; > int refcnt; > } to_check[] = { > { "/" }, > { "/cpus/cpu@0" }, > { "/cpus/cpu@1" }, > /* ... other paths I was interested in ... */ > }; > > static void check_refcnts(void) > { > static bool called; > unsigned int i; > const char *path; > struct device_node *np; > int refcnt; > unsigned int errors = 0; > > pr_info("----- %s reference counts -----\n", > called ? "Checking" : "Saving"); > for (i = 0; i < ARRAY_SIZE(to_check); i++) { > path = to_check[i].path; > np = of_find_node_by_path(path); > if (!np) > continue; > > refcnt = atomic_read(&np->kobj.kref.refcount); > if (!called) { > pr_info("%s %d\n", path, refcnt); > to_check[i].refcnt = refcnt; > } else if (refcnt == to_check[i].refcnt) { > pr_info("%s %d (OK)\n", path, refcnt); > } else { > pr_info("%s %d (should be %d)\n", path, refcnt, > to_check[i].refcnt); > errors++; > } > > of_node_put(np); > } > > if (called) > pr_info("----- Checking done (%u errors) -----\n", errors); > else > pr_info("----- Saving done -----\n"); > > called = true; > } > --- > drivers/of/base.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index 36536b6a8834acd2..f3e346e19c69d1f2 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -791,8 +791,10 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt > if (!np) > np = of_node_get(of_root); > while (np && *path == '/') { > + struct device_node *parent = np; > path++; /* Increment past '/' delimiter */ > - np = __of_find_node_by_path(np, path); > + np = __of_find_node_by_path(parent, path); > + of_node_put(parent); > path = strchrnul(path, '/'); > } > raw_spin_unlock_irqrestore(&devtree_lock, flags); This doesn't look /quite/ the best. __for_each_child_of_node() is fiddling with refcounts, but the '__' of functions shouldn't need to do that since they are called under the spinlock (nothing is going to change while they are called). __of_find_all_nodes() for instance doesn't do refcounting, but of_find_all_nodes() does. Does the following also solve the problem? g. --- -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/of/base.c b/drivers/of/base.c index 36536b6a8834..0357b51a7440 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -626,9 +626,8 @@ static struct device_node *__of_get_next_child(const struct device_node *node, next = prev ? prev->sibling : node->child; for (; next; next = next->sibling) - if (of_node_get(next)) + if (next) break; - of_node_put(prev); return next; } #define __for_each_child_of_node(parent, child) \ @@ -650,7 +649,8 @@ struct device_node *of_get_next_child(const struct device_node *node, unsigned long flags; raw_spin_lock_irqsave(&devtree_lock, flags); - next = __of_get_next_child(node, prev); + next = of_node_get(__of_get_next_child(node, prev)); + of_node_put(prev); raw_spin_unlock_irqrestore(&devtree_lock, flags); return next; } @@ -789,12 +789,13 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt /* Step down the tree matching path components */ raw_spin_lock_irqsave(&devtree_lock, flags); if (!np) - np = of_node_get(of_root); + np = of_root; while (np && *path == '/') { path++; /* Increment past '/' delimiter */ np = __of_find_node_by_path(np, path); path = strchrnul(path, '/'); } + of_node_get(np); raw_spin_unlock_irqrestore(&devtree_lock, flags); return np; }