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=-2.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 D3225C3A5A6 for ; Thu, 19 Sep 2019 19:25:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A05522053B for ; Thu, 19 Sep 2019 19:25:10 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="Wtb8HIE8" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2404416AbfISTZK (ORCPT ); Thu, 19 Sep 2019 15:25:10 -0400 Received: from mail-pg1-f193.google.com ([209.85.215.193]:42379 "EHLO mail-pg1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2404328AbfISTZK (ORCPT ); Thu, 19 Sep 2019 15:25:10 -0400 Received: by mail-pg1-f193.google.com with SMTP id z12so2427802pgp.9 for ; Thu, 19 Sep 2019 12:25:09 -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=cRUM2Bgk+uo7XLX/Jy2FrsXWAee8Of1P+PViX2GKNZY=; b=Wtb8HIE8vSdrRJNHZ2wmfbgGrAtk3ivSxMNFGnSByX+FxJcPGP4rAB6aVWjW89hdFe pYYji/ebkFmwElkLjlB5gfhwZshuG6GyHgpQ6OUxqgFbFCH2I40jlTNCFNzbNl1Ikf9u xEI7catyJ1zqMSoXBGHLZqRSe/5bskK0AKwiQ= 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=cRUM2Bgk+uo7XLX/Jy2FrsXWAee8Of1P+PViX2GKNZY=; b=hf5Z5oR6gf9Ed4zwcvjdY3uyqklu5yb1gpFehN+Vxreq9j10Z5cSBzeK9E7ob5NXlo Gm9mSyeY3rN4Os9DIW87uEqHWI/R5LFKDn2WTkDmBCEgkQny/gL5LYOwsaBRjGWbb4K8 WHvOaGNkEj5eb9lXxle3lMgLw6HfquhaL7sSv6hn/GxGUvbCcMQoy6e7as1e0xLC4FYD tRFmj209SOqyzuhyeW39Ouse8Rd2zGsHlaOIP+6vcZsa2ftNFXi1fxc49vuVUgkmaGie dDxw+W0fS9cOPp8v6cKu6VPGvxcFXhkpmOHfM9o8viEoqphJoQDx7qHD/7j/FAqx4vtY O8KQ== X-Gm-Message-State: APjAAAU71Ux7EM5XZOpvLapsnVu9nOGedhfZsSmdAepwVTs6SklMRfOu 4Sf6Z06uTHUn1C0o1UMBJ/lw4A== X-Google-Smtp-Source: APXvYqwngcFum1vlZgnTY7YySXFB/HJZmBru5y/kt5s7NFfKVBnC1yOjxATiB3VpQrksdB/U2eFy0w== X-Received: by 2002:a63:515a:: with SMTP id r26mr10663846pgl.121.1568921109183; Thu, 19 Sep 2019 12:25:09 -0700 (PDT) Received: from localhost ([2620:15c:202:1:75a:3f6e:21d:9374]) by smtp.gmail.com with ESMTPSA id g4sm14154791pfo.33.2019.09.19.12.25.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 19 Sep 2019 12:25:08 -0700 (PDT) Date: Thu, 19 Sep 2019 12:25:07 -0700 From: Matthias Kaehlcke To: Leonard Crestez Cc: MyungJoo Ham , Kyungmin Park , Chanwoo Choi , Artur =?utf-8?B?xZp3aWdvxYQ=?= , Saravana Kannan , Krzysztof Kozlowski , Alexandre Bailon , Georgi Djakov , Abel Vesa , Jacky Bai , Viresh Kumar , "linux-pm@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH 1/8] PM / devfreq: Lock devfreq in trans_stat_show Message-ID: <20190919192507.GW133864@google.com> References: <7d8f4d5c608d45ba19cdd52068fe6ffe30de67c1.1568764439.git.leonard.crestez@nxp.com> <20190918212836.GN133864@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org On Thu, Sep 19, 2019 at 06:42:22PM +0000, Leonard Crestez wrote: > On 19.09.2019 00:28, Matthias Kaehlcke wrote: > > Hi Leonard, > > > > this series doesn't indicate the version, from the change history in > > the cover letter I suppose it is v5. > > Sorry about that, I forgot --subject-prefix. It is indeed v5 > > > On Wed, Sep 18, 2019 at 03:18:20AM +0300, Leonard Crestez wrote: > >> There is no locking in this sysfs show function so stats printing can > >> race with a devfreq_update_status called as part of freq switching or > >> with initialization. > >> > >> Also add an assert in devfreq_update_status to make it clear that lock > >> must be held by caller. > > > > This and some other patches look like generic improvements and not > > directly related to the series "PM / devfreq: Add dev_pm_qos > > support". If there are no dependencies I think it is usually better to > > send the improvements separately, it keeps the series more focussed > > and might reduce version churn. Just my POV though ;-) > > The locking cleanups are required in order to initialize pm_qos request > and notifiers without introducing lockdep warnings. > > pm_qos calls notifiers under dev_pm_qos_mtx and those notifiers needs to > take &devfreq->lock. This means initializing pm_qos notifiers and > requests must be done outside &devfreq->lock which needs some cleanups > in devfreq_add_device. Thanks for the clarification! > This particular patch is a more loosely related bugfix. Devfreq > maintainers: would it help to post it separately? If it's just this single patch it probably isn't a problem, I'd be more concerned about multiple unrelated patches or if the changes are complex. > >> @@ -1415,15 +1416,20 @@ static ssize_t trans_stat_show(struct device *dev, > >> struct devfreq *devfreq = to_devfreq(dev); > >> ssize_t len; > >> int i, j; > >> unsigned int max_state = devfreq->profile->max_state; > >> > >> + mutex_lock(&devfreq->lock); > >> if (!devfreq->stop_polling && > >> - devfreq_update_status(devfreq, devfreq->previous_freq)) > >> - return 0; > >> - if (max_state == 0) > >> - return sprintf(buf, "Not Supported.\n"); > >> + devfreq_update_status(devfreq, devfreq->previous_freq)) { > >> + len = 0; > > > > you could assign 'len' in the declaration instead, but it's just > > another option, it'ss fine as is > >> + goto out; > >> + } > >> + if (max_state == 0) { > >> + len = sprintf(buf, "Not Supported.\n"); > >> + goto out; > >> + } > > > > This leaves the general structure of the code as is, which is great, > > but since you are already touching this part you can consider to > > improve it: 'max_state' is constant after device creation, hence the > > check could be done at the beginning, which IMO would be clearer, it > > could also save an unnecessary devfreq_update_status() call and it > > wouldn't be necessary to hold the lock (one goto less). > > Now that I look at this more closely &devfreq->lock only really needs to > be held during the stats update, it can be released during sprintf. right, another simplification :) 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=-2.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 0940BC3A5A6 for ; Thu, 19 Sep 2019 19:25:23 +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 CE05A2053B for ; Thu, 19 Sep 2019 19:25:22 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="eYxyaOJn"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="Wtb8HIE8" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CE05A2053B 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=VMKLr43Z8wtaLDFimJahrueA46tdsH+deFm+2C3BK00=; b=eYxyaOJng+POzh N5ZEU3hBFJu2mR26XbCqXoHgWM4vigvqfzTBODsOfG0teyUGoVZ4bexMlbzuKggJhycV8I9aoptbl UQIm+RNjw2MCHn2zfNbZhHvt54uMdtDyBSbkGeAL9mZ+VYFnVQRlvn6S6cF1/KDvLLfKgfWfZ1qCO Y/vYx0nS7vDiHOHseIL9hFyhXy9okSqa68j79NB+KAgSR+iu9RKkv6gPiYcBDGu0IPm8ZAqjFDlR2 s35l5YSF2rl9FXnOCymVchVw2mOMiQx+bjkdKArbmsjGVtR0/sg7ByFrhjciZ6sV+t1Y2gXqR5VNm 42PRmGcG5RIMM/nmPwUw==; 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 1iB23G-0008Ru-VK; Thu, 19 Sep 2019 19:25:14 +0000 Received: from mail-pg1-x544.google.com ([2607:f8b0:4864:20::544]) by bombadil.infradead.org with esmtps (Exim 4.92.2 #3 (Red Hat Linux)) id 1iB23D-0008RH-L6 for linux-arm-kernel@lists.infradead.org; Thu, 19 Sep 2019 19:25:13 +0000 Received: by mail-pg1-x544.google.com with SMTP id g3so860044pgs.11 for ; Thu, 19 Sep 2019 12:25:09 -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=cRUM2Bgk+uo7XLX/Jy2FrsXWAee8Of1P+PViX2GKNZY=; b=Wtb8HIE8vSdrRJNHZ2wmfbgGrAtk3ivSxMNFGnSByX+FxJcPGP4rAB6aVWjW89hdFe pYYji/ebkFmwElkLjlB5gfhwZshuG6GyHgpQ6OUxqgFbFCH2I40jlTNCFNzbNl1Ikf9u xEI7catyJ1zqMSoXBGHLZqRSe/5bskK0AKwiQ= 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=cRUM2Bgk+uo7XLX/Jy2FrsXWAee8Of1P+PViX2GKNZY=; b=qkizTsrQ7W/tEg4MWypUeIhWm1sUNf04M1fMC00Jn1pDqnKdDj8iO1ex/04LGPvNOL L+Yng7N624r3V3/OFjDGaDr31gyCCaM9osgWVCxoZuHQBByZQPeHXAKM5s/XklqnjX4T EVh9pCNiqSrIXgIRPwtM7vfyVef1A0run7Tpr0CFxu9t9ME4WFUnWNBuqPSDpS1Rhv4b 0iyn1+dCb+G9q0WY/pSULVoZKZKGvJGfWLy4j9kcW8OqGK6CyILoQ9RIo157PlQMbnQU Zeqvhob8oYPt7MmtvcxbIvRDBAi0lGifVppFJQ/f1FyRN4VBB53bNuMw9eYAd9oQcI6X VIMg== X-Gm-Message-State: APjAAAXqAFx896LRB5aC8977VuS3GiAp8cK+7kJQlH1eckxw0VUtsJTi B33mO8vPZCYBBBsfQhT0yyU8tg== X-Google-Smtp-Source: APXvYqwngcFum1vlZgnTY7YySXFB/HJZmBru5y/kt5s7NFfKVBnC1yOjxATiB3VpQrksdB/U2eFy0w== X-Received: by 2002:a63:515a:: with SMTP id r26mr10663846pgl.121.1568921109183; Thu, 19 Sep 2019 12:25:09 -0700 (PDT) Received: from localhost ([2620:15c:202:1:75a:3f6e:21d:9374]) by smtp.gmail.com with ESMTPSA id g4sm14154791pfo.33.2019.09.19.12.25.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 19 Sep 2019 12:25:08 -0700 (PDT) Date: Thu, 19 Sep 2019 12:25:07 -0700 From: Matthias Kaehlcke To: Leonard Crestez Subject: Re: [PATCH 1/8] PM / devfreq: Lock devfreq in trans_stat_show Message-ID: <20190919192507.GW133864@google.com> References: <7d8f4d5c608d45ba19cdd52068fe6ffe30de67c1.1568764439.git.leonard.crestez@nxp.com> <20190918212836.GN133864@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: 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-20190919_122511_731538_4D68074C X-CRM114-Status: GOOD ( 27.20 ) 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 On Thu, Sep 19, 2019 at 06:42:22PM +0000, Leonard Crestez wrote: > On 19.09.2019 00:28, Matthias Kaehlcke wrote: > > Hi Leonard, > > > > this series doesn't indicate the version, from the change history in > > the cover letter I suppose it is v5. > > Sorry about that, I forgot --subject-prefix. It is indeed v5 > > > On Wed, Sep 18, 2019 at 03:18:20AM +0300, Leonard Crestez wrote: > >> There is no locking in this sysfs show function so stats printing can > >> race with a devfreq_update_status called as part of freq switching or > >> with initialization. > >> > >> Also add an assert in devfreq_update_status to make it clear that lock > >> must be held by caller. > > > > This and some other patches look like generic improvements and not > > directly related to the series "PM / devfreq: Add dev_pm_qos > > support". If there are no dependencies I think it is usually better to > > send the improvements separately, it keeps the series more focussed > > and might reduce version churn. Just my POV though ;-) > > The locking cleanups are required in order to initialize pm_qos request > and notifiers without introducing lockdep warnings. > > pm_qos calls notifiers under dev_pm_qos_mtx and those notifiers needs to > take &devfreq->lock. This means initializing pm_qos notifiers and > requests must be done outside &devfreq->lock which needs some cleanups > in devfreq_add_device. Thanks for the clarification! > This particular patch is a more loosely related bugfix. Devfreq > maintainers: would it help to post it separately? If it's just this single patch it probably isn't a problem, I'd be more concerned about multiple unrelated patches or if the changes are complex. > >> @@ -1415,15 +1416,20 @@ static ssize_t trans_stat_show(struct device *dev, > >> struct devfreq *devfreq = to_devfreq(dev); > >> ssize_t len; > >> int i, j; > >> unsigned int max_state = devfreq->profile->max_state; > >> > >> + mutex_lock(&devfreq->lock); > >> if (!devfreq->stop_polling && > >> - devfreq_update_status(devfreq, devfreq->previous_freq)) > >> - return 0; > >> - if (max_state == 0) > >> - return sprintf(buf, "Not Supported.\n"); > >> + devfreq_update_status(devfreq, devfreq->previous_freq)) { > >> + len = 0; > > > > you could assign 'len' in the declaration instead, but it's just > > another option, it'ss fine as is > >> + goto out; > >> + } > >> + if (max_state == 0) { > >> + len = sprintf(buf, "Not Supported.\n"); > >> + goto out; > >> + } > > > > This leaves the general structure of the code as is, which is great, > > but since you are already touching this part you can consider to > > improve it: 'max_state' is constant after device creation, hence the > > check could be done at the beginning, which IMO would be clearer, it > > could also save an unnecessary devfreq_update_status() call and it > > wouldn't be necessary to hold the lock (one goto less). > > Now that I look at this more closely &devfreq->lock only really needs to > be held during the stats update, it can be released during sprintf. right, another simplification :) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel