From patchwork Fri Jun 10 23:14:36 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Hilman X-Patchwork-Id: 871052 Received: from smtp1.linux-foundation.org (smtp1.linux-foundation.org [140.211.169.13]) by demeter2.kernel.org (8.14.4/8.14.4) with ESMTP id p5ANGEFR009140 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL) for ; Fri, 10 Jun 2011 23:16:34 GMT Received: from daredevil.linux-foundation.org (localhost [127.0.0.1]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id p5ANEj79031845; Fri, 10 Jun 2011 16:14:46 -0700 Received: from na3sys009aog111.obsmtp.com (na3sys009aog111.obsmtp.com [74.125.149.205]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with SMTP id p5ANEerE031832 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Fri, 10 Jun 2011 16:14:42 -0700 Received: from mail-pz0-f42.google.com ([209.85.210.42]) (using TLSv1) by na3sys009aob111.postini.com ([74.125.148.12]) with SMTP ID DSNKTfKlX7YFI9yL/NkXUqsyvaGcaHDIBkeI@postini.com; Fri, 10 Jun 2011 16:14:42 PDT Received: by pzk37 with SMTP id 37so2851985pzk.1 for ; Fri, 10 Jun 2011 16:14:39 -0700 (PDT) Received: by 10.68.19.101 with SMTP id d5mr1187942pbe.82.1307747679170; Fri, 10 Jun 2011 16:14:39 -0700 (PDT) Received: from localhost (c-24-19-7-36.hsd1.wa.comcast.net [24.19.7.36]) by mx.google.com with ESMTPS id i7sm2674082pbj.26.2011.06.10.16.14.37 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 10 Jun 2011 16:14:38 -0700 (PDT) From: Kevin Hilman To: "Rafael J. Wysocki" Organization: Texas Instruments, Inc. References: <201106062029.36389.rjw@sisk.pl> <87zkluwqab.fsf@ti.com> <201106072332.30464.rjw@sisk.pl> Date: Fri, 10 Jun 2011 16:14:36 -0700 In-Reply-To: <201106072332.30464.rjw@sisk.pl> (Rafael J. Wysocki's message of "Tue, 7 Jun 2011 23:32:30 +0200") Message-ID: <8739jhs2hv.fsf@ti.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1.50 (gnu/linux) MIME-Version: 1.0 Received-SPF: pass (localhost is always allowed.) X-Spam-Status: No, hits=-104.243 required=5 tests=AWL, BAYES_00, OSDL_HEADER_SPF_PASS, OSDL_HEADER_SUBJECT_BRACKETED, USER_IN_WHITELIST X-Spam-Checker-Version: SpamAssassin 3.2.4-osdl_revision__1.47__ X-MIMEDefang-Filter: lf$Revision: 1.188 $ X-Scanned-By: MIMEDefang 2.63 on 140.211.169.21 Cc: Linux-pm mailing list , linux-omap@vger.kernel.org Subject: Re: [linux-pm] calling runtime PM from system PM methods X-BeenThere: linux-pm@lists.linux-foundation.org X-Mailman-Version: 2.1.9 Precedence: list List-Id: Linux power management List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-pm-bounces@lists.linux-foundation.org Errors-To: linux-pm-bounces@lists.linux-foundation.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter2.kernel.org [140.211.167.43]); Fri, 10 Jun 2011 23:16:34 +0000 (UTC) "Rafael J. Wysocki" writes: [...] > Whether or not user space has disabled runtime PM _doesn't_ _matter_ for > system suspend, because _you_ _can't_ call pm_runtime_suspend(), or > pm_runtime_put_sunc(), from a driver's .suspend() callback _anyway_. > The reason is that doing that would cause the subsystem's (or power > domain's in this case) .runtime_suspend() callback to be invoked and > that's incorrect. Namely, it would require the subsystem (power domain) > to expect that its .runtime_suspend() would always be executed indirectly > as a result of calling its .suspend() (through the driver's callback) > and that expectation may or may not be met (depending on the driver's > design). So here's an interesting scenario which I think it triggers the same problem as you highlight above. Assume you have a driver that's using runtime PM on a per-xfer basis. Before each xfer, it does a pm_runtime_get_sync(), after each xfer it does a pm_runtime_put_sync() (for this example, it's important that it's a _put_sync()). The _put_sync() might happen in an ISR, or possibly in a thread waiting on a completion which is awoken by the ISR, etc. etc. (the runtime PM callbacks are IRQ safe, and device is marked as such.) The driver is in the middle of an xfer and a system suspend request happens. The driver's ->suspend() callback happens, and the driver - enables/disables wakeups based on device_may_wakeup() - prevents future xfers - waits for current xfer to finish As soon as the xfer finishes, the driver gets notified (completion, callback, IRQ, whatever) and calls pm_runtime_put_sync(), which triggers subsys->runtime_suspend --> driver->runtime_suspend. While the driver's ->suspend() callback doesn't directly call pm_runtime_put_sync(), the act of waiting for the xfer to finish causes the subsystem/driver->runtime_suspend callbacks to be called during the subsytem/driver->suspend callback, which is the same problem as you highlight above. Based on your commit that removed incrementing the usage count across suspend[1], you mentioned "we can rely on subsystems and device drivers to avoid doing that unnecessarily." The above example shows that this type of thing might not be that obvious to detect and thus avoid. I suspect the solution to the above will be to add back the usage count increment across system suspend, but I'm hoping not. IMO, it would be more flexible to allow the subsystems to decide. The subsystems could provide locking (or manage dev->power.usage_count) themselves if necessary. For example, leave it to the subsystem->prepare() to pm_runtime_get_noresume() if it wants to avoid the "nesting" of callbacks. A related question: does the pm_wq need to be freezable? From Documentation/power/runtime_pm.txt: * The power management workqueue pm_wq in which bus types and device drivers can put their PM-related work items. It is strongly recommended that pm_wq be used for queuing all work items related to run-time PM, because this allows them to be synchronized with system-wide power transitions (suspend to RAM, hibernation and resume from system sleep states). pm_wq is declared in include/linux/pm_runtime.h and defined in kernel/power/main.c. Is "synchronized with system-wide power transistions" correct here? Rather than synchronize, using a freezable workqueue actually _prevents_ runtime PM events (at least async ones.) Again, proper locking (or management of dev->power.usage_count) at the subsystem level would get you the same effect, but still leave flexibility to the subsystem/pwr_domain layer. Kevin P.S. the commit below[1] removed the usage count increment/decrement across system suspend/resume, but Documentation/power/runtime_pm.txt still refers to it. Patch below[2] removes it, ssuming you're not planning on adding it back. ;) [1] commit e8665002477f0278f84f898145b1f141ba26ee26 Author: Rafael J. Wysocki Date: Sat Feb 12 01:42:41 2011 +0100 PM: Allow pm_runtime_suspend() to succeed during system suspend The dpm_prepare() function increments the runtime PM reference counters of all devices to prevent pm_runtime_suspend() from executing subsystem-level callbacks. However, this was supposed to guard against a specific race condition that cannot happen, because the power management workqueue is freezable, so pm_runtime_suspend() can only be called synchronously during system suspend and we can rely on subsystems and device drivers to avoid doing that unnecessarily. Make dpm_prepare() drop the runtime PM reference to each device after making sure that runtime resume is not pending for it. Signed-off-by: Rafael J. Wysocki Acked-by: Kevin Hilman [2] From 8968e3e41d785e7e5ce7584d64f6a55b303e7060 Mon Sep 17 00:00:00 2001 From: Kevin Hilman Date: Fri, 10 Jun 2011 16:05:51 -0700 Subject: [PATCH] PM / Runtime: update doc: usage count no longer incremented across system PM commit e8665002477f0278f84f898145b1f141ba26ee26 (PM: Allow pm_runtime_suspend() to succeed during system suspend) removed usage count increment across system PM. Update doc to reflect this. Signed-off-by: Kevin Hilman --- Applies on v3.0-rc2 Documentation/power/runtime_pm.txt | 5 ----- 1 files changed, 0 insertions(+), 5 deletions(-) diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt index 654097b..22accb3 100644 --- a/Documentation/power/runtime_pm.txt +++ b/Documentation/power/runtime_pm.txt @@ -566,11 +566,6 @@ to do this is: pm_runtime_set_active(dev); pm_runtime_enable(dev); -The PM core always increments the run-time usage counter before calling the -->prepare() callback and decrements it after calling the ->complete() callback. -Hence disabling run-time PM temporarily like this will not cause any run-time -suspend callbacks to be lost. - 7. Generic subsystem callbacks Subsystems may wish to conserve code space by using the set of generic power