From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932677AbcESRLv (ORCPT ); Thu, 19 May 2016 13:11:51 -0400 Received: from bear.ext.ti.com ([198.47.19.11]:59597 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932420AbcESRLt (ORCPT ); Thu, 19 May 2016 13:11:49 -0400 Subject: Re: [PATCH] PM / sleep: fix unbalanced pm runtime disable in __device_suspend_late() To: "Rafael J. Wysocki" References: <1463162628-16976-1-git-send-email-grygorii.strashko@ti.com> CC: "Rafael J. Wysocki" , "linux-pm@vger.kernel.org" , Len Brown , Pavel Machek , Greg Kroah-Hartman , Kevin Hilman , Ulf Hansson , Linux Kernel Mailing List From: Grygorii Strashko Message-ID: <573DF3C6.7080503@ti.com> Date: Thu, 19 May 2016 20:11:34 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/19/2016 04:38 PM, Rafael J. Wysocki wrote: > On Fri, May 13, 2016 at 8:03 PM, Grygorii Strashko > wrote: >> The PM runtime will be left disabled for the device if its .suspend_late() >> callback fails and async suspend is not allowed for this device. In >> this case device will not be added in dpm_late_early_list and >> dpm_resume_early() will ignore this device, as result PM runtime will >> be disabled for it forever (side effect: after 8 subsequent failures >> for the same device the PM runtime will be reenabled due to >> disable_depth overflow). >> >> Hence, re-enable PM runtime in __device_suspend_late() if >> .suspend_late() callback fails and async suspend is not allowed for >> this device. >> >> Signed-off-by: Grygorii Strashko >> --- >> drivers/base/power/main.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c >> index 6e7c3cc..9b266e5 100644 >> --- a/drivers/base/power/main.c >> +++ b/drivers/base/power/main.c >> @@ -1207,10 +1207,13 @@ static int __device_suspend_late(struct device *dev, pm_message_t state, bool as >> } >> >> error = dpm_run_callback(callback, dev, state, info); >> - if (!error) >> + if (!error) { >> dev->power.is_late_suspended = true; >> - else >> + } else { Point [1] >> async_error = error; >> + if (!is_async(dev)) > > Why is the is_async() check necessary here? A: deviceX is suspended *async* and reached point [1], in this case: - deviceX has been added in dpm_late_early_list already - dpm_suspend_late() will detect async_error and call dpm_resume_early() - dpm_resume_early() will call device_resume_early() for deviceX - device_resume_early() will re-enable PM runtime { ... if (!dev->power.is_late_suspended) goto Out; ... Out: TRACE_RESUME(error); pm_runtime_enable(dev); ^^^^^^^^^^^^ complete_all(&dev->power.completion); return error; } B: deviceX is suspended *sync* and reached point [1], in this case: - deviceX has not been added in dpm_late_early_list yet - dpm_suspend_late() will detect sync_error and call dpm_resume_early() - dpm_resume_early() will ignore deviceX if i'll not check for !is_async(dev) then pm_runtime_enable(dev) will be called twice for deviceX with this patch. > >> + pm_runtime_enable(dev); >> + } >> >> Complete: >> TRACE_SUSPEND(error); >> -- -- regards, -grygorii