From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A5E4BC4CEC9 for ; Wed, 18 Sep 2019 23:29:21 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 77F5B218AE for ; Wed, 18 Sep 2019 23:29:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="XnlF1Snd"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="hAauSUP5" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 77F5B218AE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=HKA2LUzVnTyHjBBCz15i/YDcqVa0bIHJIW9PG3FJLy8=; b=XnlF1SndGGGfs/ UOMBu8EZr1VGDE1qsr4+f3APU112zxL4WqQ3mo+0+tyBnBup/IuRA2+SKjD0//fxuJKYNP02nuyY8 4rdPUV3jsnxyG2tqXn6ii7zA89ZrG/OPnM9SXJeeVcsgK86pBhdJq0cVASEA+x+Kt+Cfu2yFaC0dh cVHzY9nq5YCTifoD5F69Xjtao4LJ0HWdMGy4VzA8D7gg6xAjvJXvHOj2VDDqYV/tjf3HfGMfaruvr F8HTQ4oMY9JKKuepLjTQjfBlfj4DUTJzVJ58YGYHDD39eBKQpyBUm6uQveZBAGD/TLVIuxhtUVCWg lwFmvv8Pot6Ih+R9DOSw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.2 #3 (Red Hat Linux)) id 1iAjNn-0001dF-CU; Wed, 18 Sep 2019 23:29:11 +0000 Received: from mail-pg1-x542.google.com ([2607:f8b0:4864:20::542]) by bombadil.infradead.org with esmtps (Exim 4.92.2 #3 (Red Hat Linux)) id 1iAjNk-0001ck-Eq for linux-arm-kernel@lists.infradead.org; Wed, 18 Sep 2019 23:29:09 +0000 Received: by mail-pg1-x542.google.com with SMTP id a24so757786pgj.2 for ; Wed, 18 Sep 2019 16:29:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=xkSAHLojjpoJjVoNc1BW0mr4HJS6Pbtg1jwjCnh3QCI=; b=hAauSUP5UaViSzCRq0EKIC4YzU2nskkSrg4ZGnVbrYp/XXBzJ46HAMnj9EAwu4YtKp E4KIFQ2BKRpP3DO3CyNDa365WBPB47eW6TCcZz3bN4tW+duJrvt3NZtzScga2H9iReAX EGYcbu3JXWAkSMh6SwY7BnXff/BrhjvptGwvM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=xkSAHLojjpoJjVoNc1BW0mr4HJS6Pbtg1jwjCnh3QCI=; b=IYSEpOIwcM8miTr9GTLxjH+MT5pZWAEIb6AYaWdj/cclw+z00gJ+aSbhRMBSqghRKF g3HyDZPDzYTO+mNv59MIXadlAPD2JGC92fPNYTD+LcvjpJ87rnF9iEW55uJzkhhhBFHO kwzs0VImRikf8tP1Mg8QZ6Lac9q+SMq0HivwC8QgG4kFpaMjpuQXIYTds/WwrbUQLSPH ywxi+Wz1pr+f5acKKfl0ABXskw/EZWqvU2FmGZFBjIWWxQfBEWnnrF6MwKK5WlurlZe3 HnFaPXS+Y9KqEqEBS90TzuF6rAX1K7exZO1YF2S0DjsCsblPBOHmITwenHYla4MozZlq Cecg== X-Gm-Message-State: APjAAAUTh5rf2sZt0BQ1vDAo8YmUhJw5p9/Hh3VslOkfEpS+AWZ7z9aM SsL7MJVAxsEqQzdHNi1CIP58bOLsmfw= X-Google-Smtp-Source: APXvYqwCuTM1Y/zo1nM7wFA2UvVoZjRZ/YbI8o0M/84ds1jVpWuIesSxUztoUh6l6JOlD1ZVb8OAyA== X-Received: by 2002:a63:211c:: with SMTP id h28mr6301450pgh.372.1568849347203; Wed, 18 Sep 2019 16:29:07 -0700 (PDT) Received: from localhost ([2620:15c:202:1:75a:3f6e:21d:9374]) by smtp.gmail.com with ESMTPSA id k13sm7533807pfa.138.2019.09.18.16.29.06 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 18 Sep 2019 16:29:06 -0700 (PDT) Date: Wed, 18 Sep 2019 16:29:04 -0700 From: Matthias Kaehlcke To: Leonard Crestez Subject: Re: [PATCH 3/8] PM / devfreq: Move more initialization before registration Message-ID: <20190918232904.GP133864@google.com> References: <59bd0d871fad520eb417ca46943fa7f86ef9186a.1568764439.git.leonard.crestez@nxp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <59bd0d871fad520eb417ca46943fa7f86ef9186a.1568764439.git.leonard.crestez@nxp.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190918_162908_520684_A9FDA50B X-CRM114-Status: GOOD ( 20.60 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Artur =?utf-8?B?xZp3aWdvxYQ=?= , Abel Vesa , Saravana Kannan , linux-pm@vger.kernel.org, Viresh Kumar , Krzysztof Kozlowski , Chanwoo Choi , Kyungmin Park , MyungJoo Ham , Alexandre Bailon , Georgi Djakov , linux-arm-kernel@lists.infradead.org, Jacky Bai Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Leonard, On Wed, Sep 18, 2019 at 03:18:22AM +0300, Leonard Crestez wrote: > In general it is a better to initialize an object before making it > accessible externally (through device_register). > > This make it possible to avoid relying on locking a partially > initialized object. > > Signed-off-by: Leonard Crestez > --- > drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index a715f27f35fd..57a217fc92de 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev) > > if (devfreq->profile->exit) > devfreq->profile->exit(devfreq->dev.parent); > > mutex_destroy(&devfreq->lock); > + kfree(devfreq->time_in_state); > + kfree(devfreq->trans_table); > kfree(devfreq); > } > > /** > * devfreq_add_device() - Add devfreq feature to the device > @@ -671,44 +673,43 @@ struct devfreq *devfreq_add_device(struct device *dev, > devfreq->max_freq = devfreq->scaling_max_freq; > > devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); > atomic_set(&devfreq->suspend_count, 0); > > - dev_set_name(&devfreq->dev, "devfreq%d", > - atomic_inc_return(&devfreq_no)); > - err = device_register(&devfreq->dev); > - if (err) { > - mutex_unlock(&devfreq->lock); > - put_device(&devfreq->dev); > - goto err_out; > - } > - > - devfreq->trans_table = devm_kzalloc(&devfreq->dev, > + devfreq->trans_table = kzalloc( > array3_size(sizeof(unsigned int), > devfreq->profile->max_state, > devfreq->profile->max_state), > GFP_KERNEL); > if (!devfreq->trans_table) { > mutex_unlock(&devfreq->lock); > err = -ENOMEM; > - goto err_devfreq; > + goto err_dev; > } > > - devfreq->time_in_state = devm_kcalloc(&devfreq->dev, > - devfreq->profile->max_state, > - sizeof(unsigned long), > - GFP_KERNEL); > + devfreq->time_in_state = kcalloc(devfreq->profile->max_state, > + sizeof(unsigned long), > + GFP_KERNEL); > if (!devfreq->time_in_state) { > mutex_unlock(&devfreq->lock); > err = -ENOMEM; > - goto err_devfreq; > + goto err_dev; > } > > devfreq->last_stat_updated = jiffies; > > srcu_init_notifier_head(&devfreq->transition_notifier_list); > > + dev_set_name(&devfreq->dev, "devfreq%d", > + atomic_inc_return(&devfreq_no)); > + err = device_register(&devfreq->dev); > + if (err) { > + mutex_unlock(&devfreq->lock); > + put_device(&devfreq->dev); > + goto err_out; goto err_dev; > + } > + > mutex_unlock(&devfreq->lock); > > mutex_lock(&devfreq_list_lock); > > governor = try_then_request_governor(devfreq->governor_name); > @@ -734,14 +735,15 @@ struct devfreq *devfreq_add_device(struct device *dev, > > return devfreq; > > err_init: > mutex_unlock(&devfreq_list_lock); > -err_devfreq: > devfreq_remove_device(devfreq); > - devfreq = NULL; > + return ERR_PTR(err); The two return paths in the unwind part are unorthodox, but I see why they are needed. Maybe add an empty line between the two paths to make it a bit more evident that they are separate. > err_dev: This code path should include mutex_destroy(&devfreq->lock); That was already missing in the original code though. Actually with the later device registration the mutex could be initialized later and doesn't need to be held. This would obsolete the mutex_unlock() calls in the error paths. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel