From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ulf Hansson Subject: Re: [PATCH V3] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume() Date: Fri, 21 Oct 2016 10:26:14 +0200 Message-ID: References: <1476370734-23168-1-git-send-email-ulf.hansson@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Received: from mail-lf0-f45.google.com ([209.85.215.45]:35312 "EHLO mail-lf0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753294AbcJUI0S (ORCPT ); Fri, 21 Oct 2016 04:26:18 -0400 Received: by mail-lf0-f45.google.com with SMTP id l131so127025599lfl.2 for ; Fri, 21 Oct 2016 01:26:18 -0700 (PDT) In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Marek Szyprowski Cc: "Rafael J. Wysocki" , Alan Stern , "linux-pm@vger.kernel.org" , Len Brown , Pavel Machek , Kevin Hilman , Geert Uytterhoeven , Lina Iyer , Jon Hunter , Andy Gross , Laurent Pinchart , Linus Walleij , Bartlomiej Zolnierkiewicz On 18 October 2016 at 12:32, Marek Szyprowski wrote: > Hi Ulf, > > > On 2016-10-13 16:58, Ulf Hansson wrote: >> >> When the pm_runtime_force_suspend|resume() helpers were invented, we still >> had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options. >> >> To make sure these helpers worked for all combinations and without >> introducing too much of complexity, the device was always resumed in >> pm_runtime_force_resume(). >> >> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was >> unset, we needed to resume the device as the subsystem/driver couldn't >> rely on using runtime PM to do it. >> >> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it >> removed this combination, of using CONFIG_PM_SLEEP without the earlier >> CONFIG_PM_RUNTIME. >> >> For this reason we can now rely on the subsystem/driver to use runtime PM >> to resume the device, instead of forcing that to be done in all cases. In >> other words, let's defer the runtime resume to a later point when it's >> actually needed. >> >> Signed-off-by: Ulf Hansson > > > I wasn't aware that You are working on this feature. I had it on my todo > list, > because it was a waste of power to always wake up all devices in resume from > sleep. Your change solves this issue and it works fine with my use cases. :) > You can add my: > > Tested-by: Marek Szyprowski > > I've tested it together with my Exynos IOMMU runtime pm rework and Rafael's > device dependency patchset (needed for my rework). Great news, thanks for helping out testing! Kind regards Uffe > > >> --- >> >> Changes in v3: >> - Updated to take care of parent-child relations. >> - Improved comment in the code and updated text in a function >> header >> to better describe the changes. >> >> This patch has earlier been sent standalone, but also as a part of series. >> In >> the end it turned out the solution needed some improvement to take care of >> parent-child relations, as reported by Geert [1]. >> >> Geert, I would really appreciate if you could help out testing to make >> sure the >> reported issue is fixed. >> >> [1] >> https://patches.linaro.org/patch/67940/ >> >> --- >> >> drivers/base/power/runtime.c | 35 ++++++++++++++++++++++++++++++----- >> 1 file changed, 30 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c >> index e097d35..f662267 100644 >> --- a/drivers/base/power/runtime.c >> +++ b/drivers/base/power/runtime.c >> @@ -1478,6 +1478,16 @@ int pm_runtime_force_suspend(struct device *dev) >> if (ret) >> goto err; >> + /* >> + * Increase the runtime PM usage count for the device's parent, in >> case >> + * when we find the device being used when system suspend was >> invoked. >> + * This informs pm_runtime_force_resume() to resume the parent >> + * immediately, which is needed to be able to resume its children, >> + * when not deferring the resume to be managed via runtime PM. >> + */ >> + if (dev->parent && atomic_read(&dev->power.usage_count) > 1) >> + pm_runtime_get_noresume(dev->parent); >> + >> pm_runtime_set_suspended(dev); >> return 0; >> err: >> @@ -1487,16 +1497,20 @@ err: >> EXPORT_SYMBOL_GPL(pm_runtime_force_suspend); >> /** >> - * pm_runtime_force_resume - Force a device into resume state. >> + * pm_runtime_force_resume - Force a device into resume state if needed. >> * @dev: Device to resume. >> * >> * Prior invoking this function we expect the user to have brought the >> device >> * into low power state by a call to pm_runtime_force_suspend(). Here we >> reverse >> - * those actions and brings the device into full power. We update the >> runtime PM >> - * status and re-enables runtime PM. >> + * those actions and brings the device into full power, if it is expected >> to be >> + * used on system resume. To distinguish that, we check whether the >> runtime PM >> + * usage count is greater than 1 (the PM core increases the usage count >> in the >> + * system PM prepare phase), as that indicates a real user (such as a >> subsystem, >> + * driver, userspace, etc.) is using it. If that is the case, the device >> is >> + * expected to be used on system resume as well, so then we resume it. In >> the >> + * other case, we defer the resume to be managed via runtime PM. >> * >> - * Typically this function may be invoked from a system resume callback >> to make >> - * sure the device is put into full power state. >> + * Typically this function may be invoked from a system resume callback. >> */ >> int pm_runtime_force_resume(struct device *dev) >> { >> @@ -1513,6 +1527,17 @@ int pm_runtime_force_resume(struct device *dev) >> if (!pm_runtime_status_suspended(dev)) >> goto out; >> + /* >> + * Decrease the parent's runtime PM usage count, if we increased >> it >> + * during system suspend in pm_runtime_force_suspend(). >> + */ >> + if (atomic_read(&dev->power.usage_count) > 1) { >> + if (dev->parent) >> + pm_runtime_put_noidle(dev->parent); >> + } else { >> + goto out; >> + } >> + >> ret = pm_runtime_set_active(dev); >> if (ret) >> goto out; > > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland >