From patchwork Wed Aug 12 10:10:10 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 52332 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-lb0-f198.google.com (mail-lb0-f198.google.com [209.85.217.198]) by patches.linaro.org (Postfix) with ESMTPS id 44CAA22ED0 for ; Wed, 12 Aug 2015 10:10:23 +0000 (UTC) Received: by lbsm2 with SMTP id m2sf4115275lbs.1 for ; Wed, 12 Aug 2015 03:10:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:date:from:to:cc:subject:message-id :references:mime-version:content-type:content-disposition :in-reply-to:user-agent:sender:precedence:list-id:x-original-sender :x-original-authentication-results:mailing-list:list-post:list-help :list-archive:list-unsubscribe; bh=mqR+bEE9XJWWw/it+68qqeBysNn+mJw9gl0a8NnfLnk=; b=g688uMLN01jhVXQsKDVfi1yuyk6o+4aKkgpQb0PmX70vnBToPoZ3CxtGcyxBLGAnKw 0dWQ5nEKROXPKRN6W/3IGGlWi6ATC2HhcakYWd/FRzCjKexoufj5qs9sn2qQPMX+GBwY qIq2KC4adBtO86ZNMdtJ69W7NP1pmFI63culhLpiv6AhIkzHVkeGfarlre4obAeQTz4G xpptPxjmkBOPcTE1LODdSsgx/fj6FxQw0TqB4QVa7AsdwvjFbER5Nb6ucu7XcUvqGY6K OyGFAbKkY96YoaQkHnatEJ4kLZqKxe6LoNJa+HCcbLR95MzqDIUNQzQPthS+2BD5000o FzKw== X-Gm-Message-State: ALoCoQnB2OPJlTOVyFYJip6VGnx5/JYKh/c4vGl31u7LBM104Huig/qtpS0IkyG2gS1zJnEEbuOP X-Received: by 10.113.3.8 with SMTP id bs8mr9466712lbd.23.1439374222309; Wed, 12 Aug 2015 03:10:22 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.28.226 with SMTP id e2ls23497lah.14.gmail; Wed, 12 Aug 2015 03:10:22 -0700 (PDT) X-Received: by 10.112.210.6 with SMTP id mq6mr31383726lbc.83.1439374222161; Wed, 12 Aug 2015 03:10:22 -0700 (PDT) Received: from mail-lb0-f174.google.com (mail-lb0-f174.google.com. [209.85.217.174]) by mx.google.com with ESMTPS id ay10si3774375lab.9.2015.08.12.03.10.22 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 12 Aug 2015 03:10:22 -0700 (PDT) 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 lbcbn3 with SMTP id bn3so6622314lbc.2 for ; Wed, 12 Aug 2015 03:10:22 -0700 (PDT) X-Received: by 10.152.44.196 with SMTP id g4mr12960255lam.56.1439374222005; Wed, 12 Aug 2015 03:10:22 -0700 (PDT) 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.7.198 with SMTP id l6csp223369lba; Wed, 12 Aug 2015 03:10:20 -0700 (PDT) X-Received: by 10.70.38.10 with SMTP id c10mr66003714pdk.72.1439374219923; Wed, 12 Aug 2015 03:10:19 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id rb6si8759540pab.13.2015.08.12.03.10.19; Wed, 12 Aug 2015 03:10:19 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-pm-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754204AbbHLKKR (ORCPT + 12 others); Wed, 12 Aug 2015 06:10:17 -0400 Received: from mail-pa0-f49.google.com ([209.85.220.49]:33388 "EHLO mail-pa0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753881AbbHLKKQ (ORCPT ); Wed, 12 Aug 2015 06:10:16 -0400 Received: by pabyb7 with SMTP id yb7so11598177pab.0 for ; Wed, 12 Aug 2015 03:10:14 -0700 (PDT) X-Received: by 10.68.217.102 with SMTP id ox6mr66830203pbc.158.1439374214795; Wed, 12 Aug 2015 03:10:14 -0700 (PDT) Received: from localhost ([122.171.186.190]) by smtp.gmail.com with ESMTPSA id k10sm5810503pbq.63.2015.08.12.03.10.13 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 12 Aug 2015 03:10:14 -0700 (PDT) Date: Wed, 12 Aug 2015 15:40:10 +0530 From: Viresh Kumar To: Dan Carpenter Cc: Rafael Wysocki , nm@ti.com, sboyd@codeaurora.org, linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org, khilman@linaro.org, Greg Kroah-Hartman , Len Brown , open list , Pavel Machek Subject: Re: [PATCH V2 1/6] PM / OPP: Free resources and properly return error on failure Message-ID: <20150812101010.GA32432@linux> References: <334a9052264630b9157fa9bfc3d4efe945054c34.1439288881.git.viresh.kumar@linaro.org> <20150811144345.GN5180@mwanda> <20150811145938.GA32049@linux> <20150811171132.GA25166@mwanda> <20150812064309.GL32049@linux> <20150812081113.GC32040@mwanda> <20150812082302.GD16445@linux> <20150812090351.GE32040@mwanda> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150812090351.GE32040@mwanda> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pm-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: viresh.kumar@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.mailfrom=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 12-08-15, 12:03, Dan Carpenter wrote: > It's a lot better but it would be better yet with a return 0;. There is > an earlier goto put_opp_np on the success path, but that's fine. It's > not the normal success path so it's necessarily complicated. I see where you are coming from and it makes lot of sense. Thanks for the teaching part :) > Anyway, here is what I would suggest: > > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > index 51b220e..243317c 100644 > --- a/drivers/base/power/opp.c > +++ b/drivers/base/power/opp.c > @@ -1317,19 +1317,23 @@ static int _of_init_opp_table_v2(struct device *dev, > > /* We have opp-list node now, iterate over it and add OPPs */ > for_each_available_child_of_node(opp_np, np) { > - count++; > - > ret = _opp_add_static_v2(dev, np); > if (ret) { > dev_err(dev, "%s: Failed to add OPP, %d\n", __func__, > ret); > break; > } > + count++; > } > > /* There should be one of more OPP defined */ > - if (WARN_ON(!count)) > + if (WARN_ON(!count)) { This is wrong. _opp_add_static_v2() failed and so count was 0. And we aren't supposed to WARN() in this case. We only WARN if the user hasn't added any available nodes in the opp table. > + ret = -ENOENT; > goto put_opp_np; > + } > + if (ret) > + goto free_table; Also the 'put_opp_np' thing is getting removed, check 2/6 patch of this series. What about this: Message-Id: From: Viresh Kumar Date: Tue, 11 Aug 2015 14:23:34 +0530 Subject: [PATCH] PM / OPP: Free resources and properly return error on failure _of_init_opp_table_v2() isn't freeing up resources on some errors and the error values returned are also not correct always. This fixes following problems: - Return -ENOENT, if no entries are found in the table. - Use IS_ERR() to properly check return value of _find_device_opp(). - Return error value with PTR_ERR() in above case. - Free table if _find_device_opp() fails. Reported-by: Dan Carpenter Signed-off-by: Viresh Kumar --- drivers/base/power/opp.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pm" 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/base/power/opp.c b/drivers/base/power/opp.c index 204c6c945168..4d6c4576f7ae 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -1323,28 +1323,30 @@ static int _of_init_opp_table_v2(struct device *dev, if (ret) { dev_err(dev, "%s: Failed to add OPP, %d\n", __func__, ret); - break; + goto free_table; } } /* There should be one of more OPP defined */ - if (WARN_ON(!count)) + if (WARN_ON(!count)) { + ret = -ENOENT; goto put_opp_np; + } - if (!ret) { - if (!dev_opp) { - dev_opp = _find_device_opp(dev); - if (WARN_ON(!dev_opp)) - goto put_opp_np; - } - - dev_opp->np = opp_np; - dev_opp->shared_opp = of_property_read_bool(opp_np, - "opp-shared"); - } else { - of_free_opp_table(dev); + dev_opp = _find_device_opp(dev); + if (WARN_ON(IS_ERR(dev_opp))) { + ret = PTR_ERR(dev_opp); + goto free_table; } + dev_opp->np = opp_np; + dev_opp->shared_opp = of_property_read_bool(opp_np, "opp-shared"); + + of_node_put(opp_np); + return 0; + +free_table: + of_free_opp_table(dev); put_opp_np: of_node_put(opp_np);