Received: by 10.223.176.5 with SMTP id f5csp1231819wra;
        Fri, 2 Feb 2018 13:36:52 -0800 (PST)
X-Google-Smtp-Source: AH8x227UW0uHTDuHYJFF5603HX126RQs4R1SAEe+8KmrEIowAPJoe3LiN9EdzKDr0YfQi5OnPPGg
X-Received: by 10.101.78.139 with SMTP id b11mr14726100pgs.229.1517607411889;
        Fri, 02 Feb 2018 13:36:51 -0800 (PST)
ARC-Seal: i=1; a=rsa-sha256; t=1517607411; cv=none;
        d=google.com; s=arc-20160816;
        b=OOGYDEVBmY/tpFkvHUbAFiAOKZCj/7i89MTb7jfIYhE+zwjPZraEN01HEy4+g/EMNj
         ezC1Bwji5sWv0+Ar+GnRCq8ZIGGpCUekf5/3Jx0qFOMqAn7RIHg1oUvMgeiM/Ef7zzjt
         D8Wo4wHp3NsQdvEY3P3QPSrvgx4LZLIRyXcJhYr7HBKEMwjAUVd6n9/JdW0NAOu48T8Y
         zWUG+MLRUyy6tdvM1nAdmMo5D0vWXihe258USvjKJgAj2h9Q3h1ReQ57/xHlmXFmbLKC
         Nvpem8PgNqUXgxKYsA5ELPk0Wcp0+KuCHAH8TQQjNGaRmEVJkotT0HlmlIRGE7nl5rZS
         4AHg==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816;
        h=list-id:precedence:sender:content-language
         :content-transfer-encoding:in-reply-to:mime-version:user-agent:date
         :message-id:from:references:cc:to:subject:arc-authentication-results;
        bh=JfiLXfaKMNd4497EhWGps/TvX1Sh3xsftwDltONhSNg=;
        b=QtcjNdVMghf0Cs3JTWWskSjBLvFvKJH9bjGSud26Ul9sEsf1c4bpg6FDA2pPaL4yHZ
         aOAmVBNBF8PMohciJyE9ApENUU1plJBSO/t8ylXu6wX0gHRKtWz1qX1kMaTPfDFdiTh0
         8MdNDLK5L7rowA9mQbKDljKsTUo6jKPan29rmHh9XwtnTouq5wppe9TNAWGhVOc9AWBD
         q5W0KvA6jsZlHJSfO1qReeBDYlQZhMGDfxShSPxU3w8HWh7jIFUph0J162NgWfV7rqLt
         nXlWnz3tX0acNKsXES0jOHdeJ4S1FDv/8o/HJxwSiOHfLLHH9FK4GGrGlrDIxqeb9GQ5
         ywwA==
ARC-Authentication-Results: i=1; mx.google.com;
       spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
       dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=nvidia.com
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67])
        by mx.google.com with ESMTP id 31-v6si2359658plk.532.2018.02.02.13.36.35;
        Fri, 02 Feb 2018 13:36:51 -0800 (PST)
Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Authentication-Results: mx.google.com;
       spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
       dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=nvidia.com
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751881AbeBBV2Y convert rfc822-to-8bit (ORCPT
        <rfc822;huangweiredhat@gmail.com> + 99 others);
        Fri, 2 Feb 2018 16:28:24 -0500
Received: from hqemgate15.nvidia.com ([216.228.121.64]:14217 "EHLO
        hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751699AbeBBV2R (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 2 Feb 2018 16:28:17 -0500
Received: from hqpgpgate101.nvidia.com (Not Verified[216.228.121.13]) by hqemgate15.nvidia.com
        id <B5a74d7f30000>; Fri, 02 Feb 2018 13:28:20 -0800
Received: from HQMAIL108.nvidia.com ([172.20.161.6])
  by hqpgpgate101.nvidia.com (PGP Universal service);
  Fri, 02 Feb 2018 13:28:16 -0800
X-PGP-Universal: processed;
        by hqpgpgate101.nvidia.com on Fri, 02 Feb 2018 13:28:16 -0800
Received: from [172.17.136.14] (172.17.136.14) by HQMAIL108.nvidia.com
 (172.18.146.13) with Microsoft SMTP Server (TLS) id 15.0.1347.2; Fri, 2 Feb
 2018 21:28:15 +0000
Subject: Re: [PATCH] cpufreq: skip cpufreq resume if it's not suspended
To:     Saravana Kannan <skannan@codeaurora.org>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>
CC:     <viresh.kumar@linaro.org>, <sgurrappadi@nvidia.com>,
        <linux-pm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <1516744675-21233-1-git-send-email-byan@nvidia.com>
 <1744712.rO4QOLozun@aspire.rjw.lan>
 <913f1715-bdd0-1c03-ad76-38be9d3d2298@nvidia.com>
 <17447147.z6jfkRxuEB@aspire.rjw.lan> <5A74BD55.5000402@codeaurora.org>
X-Nvconfidentiality: public
From:   Bo Yan <byan@nvidia.com>
Message-ID: <dc33ef16-9684-3d8d-f4cc-6e24697f007f@nvidia.com>
Date:   Fri, 2 Feb 2018 13:28:15 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <5A74BD55.5000402@codeaurora.org>
X-Originating-IP: [172.17.136.14]
X-ClientProxiedBy: HQMAIL104.nvidia.com (172.18.146.11) To
 HQMAIL108.nvidia.com (172.18.146.13)
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8BIT
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 02/02/2018 11:34 AM, Saravana Kannan wrote:
> On 02/02/2018 03:54 AM, Rafael J. Wysocki wrote:
>> On Wednesday, January 24, 2018 9:53:14 PM CET Bo Yan wrote:
>>>
>>> On 01/23/2018 06:02 PM, Rafael J. Wysocki wrote:
>>>> On Tuesday, January 23, 2018 10:57:55 PM CET Bo Yan wrote:
>>>>>    drivers/cpufreq/cpufreq.c | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>>>> index 41d148af7748..95b1c4afe14e 100644
>>>>> --- a/drivers/cpufreq/cpufreq.c
>>>>> +++ b/drivers/cpufreq/cpufreq.c
>>>>> @@ -1680,6 +1680,10 @@ void cpufreq_resume(void)
>>>>>        if (!cpufreq_driver)
>>>>>            return;
>>>>>
>>>>> +    if (unlikely(!cpufreq_suspended)) {
>>>>> +        pr_warn("%s: resume after failing suspend\n", __func__);
>>>>> +        return;
>>>>> +    }
>>>>>        cpufreq_suspended = false;
>>>>>
>>>>>        if (!has_target() && !cpufreq_driver->resume)
>>>>>
>>>> Good catch, but rather than doing this it would be better to avoid
>>>> calling cpufreq_resume() at all if cpufreq_suspend() has not been 
>>>> called.
>>> Yes, I thought about that, but there is no good way to skip over it
>>> without introducing another flag. cpufreq_resume is called by
>>> dpm_resume, cpufreq_suspend is called by dpm_suspend. In the failure
>>> case, dpm_resume is called, but dpm_suspend is not. So on a higher 
>>> level
>>> it's already unbalanced.
>>>
>>> One possibility is to rely on the pm_transition flag. So something 
>>> like:
>>>
>>>
>>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>>> index dc259d20c967..8469e6fc2b2c 100644
>>> --- a/drivers/base/power/main.c
>>> +++ b/drivers/base/power/main.c
>>> @@ -842,6 +842,7 @@ static void async_resume(void *data, async_cookie_t
>>> cookie)
>>>    void dpm_resume(pm_message_t state)
>>>    {
>>>           struct device *dev;
>>> +       bool suspended = (pm_transition.event != PM_EVENT_ON);
>>>           ktime_t starttime = ktime_get();
>>>
>>>           trace_suspend_resume(TPS("dpm_resume"), state.event, true);
>>> @@ -885,7 +886,8 @@ void dpm_resume(pm_message_t state)
>>>           async_synchronize_full();
>>>           dpm_show_time(starttime, state, NULL);
>>>
>>> -       cpufreq_resume();
>>> +       if (likely(suspended))
>>> +               cpufreq_resume();
>>>           trace_suspend_resume(TPS("dpm_resume"), state.event, false);
>>>    }
>>
>> I was thinking about something else.
>>
>> Anyway, I think your original patch is OK too, but without printing the
>> message.  Just combine the cpufreq_suspended check with the 
>> cpufreq_driver
>> one and the unlikely() thing is not necessary.
>>
>
> I rather have this fixed in the dpm_suspend/resume() code. This is 
> just masking the first issue that's being caused by unbalanced error 
> handling. If that means adding flags in dpm_suspend/resume() then 
> that's what we should do right now and clean it up later if it can be 
> improved. Making cpufreq more messy doesn't seem like the right answer.
>
> Thanks,
> Saravana
>
>
dpm_suspend and dpm_resume by themselves are not balanced in this 
particular case. As it's currently structured, dpm_resume can't be 
omitted even if dpm_suspend is skipped due to earlier failure.  I think 
checking cpufreq_suspended flag is a reasonable compromise. If we can 
find a way to make dpm_suspend/dpm_resume also balanced, that will be best.