From patchwork Wed Aug 12 09:03:51 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dan Carpenter X-Patchwork-Id: 6998181 Return-Path: X-Original-To: patchwork-linux-pm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id DC841C05AC for ; Wed, 12 Aug 2015 09:04:57 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id ED58420670 for ; Wed, 12 Aug 2015 09:04:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id F210720668 for ; Wed, 12 Aug 2015 09:04:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965193AbbHLJEd (ORCPT ); Wed, 12 Aug 2015 05:04:33 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:29975 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965037AbbHLJE3 (ORCPT ); Wed, 12 Aug 2015 05:04:29 -0400 Received: from userv0021.oracle.com (userv0021.oracle.com [156.151.31.71]) by userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id t7C94CuT023419 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 12 Aug 2015 09:04:13 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userv0021.oracle.com (8.13.8/8.13.8) with ESMTP id t7C94Cbj010874 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL); Wed, 12 Aug 2015 09:04:12 GMT Received: from abhmp0002.oracle.com (abhmp0002.oracle.com [141.146.116.8]) by aserv0121.oracle.com (8.13.8/8.13.8) with ESMTP id t7C94C9p025506; Wed, 12 Aug 2015 09:04:12 GMT Received: from mwanda (/154.0.139.178) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 12 Aug 2015 02:04:01 -0700 Date: Wed, 12 Aug 2015 12:03:51 +0300 From: Dan Carpenter To: Viresh Kumar 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: <20150812090351.GE32040@mwanda> 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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150812082302.GD16445@linux> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Aug 12, 2015 at 01:53:02PM +0530, Viresh Kumar wrote: > On 12-08-15, 11:11, Dan Carpenter wrote: > > If it doesn't WARN() then it's not buggy, but it's still ugly. We > > should not call of_free_opp_table() because we *tried* to add an OPP, we > > should only call it if we *succeeded*. > > This is done in order to write lesser code. I have been trying to discourage you from focusing so much on the writing fewer lines. That little bunny hop "goto put_opp_np;" at the end of the success path should be replaced with: of_node_put(opp_np); return 0; It's one extra line of code, but "return 0;" is so much more clear. A lot of my focus for the past few years has been on error handling so I am perhaps more sensitive than many devs. But here is what I like: success_path(); if (fail) fail_path(); success_path(); success_path(); success_path(); if (fail) fail_path(); success_path(); return 0; more_fail: fail_path(); fail: fail_path(); return ret; The success path is a list of commands indented one tab. The fail path is intended two tabs or in reverse order at the bottom of the function indented one tab. This uses the minimum amount of if statements and indenting. In the original code, we had: success_path(); if (fail) fail_path(); success_path(); success_path(); success_path(); if (success) { success_path(); if (fail) fail_path(); } else { fail_path(); } free: success_and_fail_path(); return ret; The original code was confusing and, as a direct result, buggy. Now we have improved the code so it looks something like this: success_path(); if (fail) fail_path(); success_path(); success_path(); success_path(); if (fail) fail_path(); goto free; more_fail: fail_path(); free: success_and_fail_path(); return ret; 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. Anyway, here is what I would suggest: --- 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 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)) { + ret = -ENOENT; goto put_opp_np; + } + if (ret) + goto free_table; regards, dan carpenter