All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
To: linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
Cc: Alan Stern
	<stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>,
	linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Kevin Hilman <khilman-l0cyMroinI0@public.gmane.org>
Subject: [PATCH] PM: Allow pm_runtime_suspend() to succeed during system suspend
Date: Fri, 11 Feb 2011 21:00:23 +0100	[thread overview]
Message-ID: <201102112100.23996.rjw@sisk.pl> (raw)
In-Reply-To: <201101311919.49225.rjw-KKrjLPT3xs0@public.gmane.org>

On Monday, January 31, 2011, Rafael J. Wysocki wrote:
> On Monday, January 31, 2011, Alan Stern wrote:
> > On Mon, 31 Jan 2011, Kevin Hilman wrote:
> > 
> > > I understand how this works, but frankly I'm still a bit fuzzy on why.
> > > 
> > > I guess I'm still missing a good understanding of what "interfering with a
> > > system power transition" means, and why a runtime suspend qualifies as
> > > interfering but not a runtime resume.
> > 
> > These are good questions.  Rafael implemented this design originally; 
> > my contribution was only to warn him of the potential for problems.  
> > Therefore he should explain the rationale for the design.
> 
> The reason why runtime resume is allowed during system power transitions is
> because in some cases during system suspend we simply have to resume devices
> that were previously runtime-suspended (for example, the PCI bus type does
> that).
> 
> The reason why runtime suspend is not allowed during system power transitions
> if the following race:
> 
> - A device has been suspended via a system suspend callback.
> - The runtime PM framework executes a (scheduled) suspend on that device,
>   not knowing that it's already been suspended, which potentially results in
>   accessing the device's registers in a low-power state.
> 
> Now, it can be avoided if every driver does the right thing and checks whether
> the device is already suspended in its runtime suspend callback, but that would
> kind of defeat the purpose of the runtime PM framework, at least partially.

In fact, I've just realized that the above race cannot really occur, because
pm_wq is freezable, so I'm proposing the following change.

Of course, it still doesn't prevent user space from disabling the runtime PM
framework's helpers via /sys/devices/.../power/control.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
Subject: 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 <rjw-KKrjLPT3xs0@public.gmane.org>
---
 drivers/base/power/main.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -669,7 +669,6 @@ static void dpm_complete(pm_message_t st
 		mutex_unlock(&dpm_list_mtx);
 
 		device_complete(dev, state);
-		pm_runtime_put_sync(dev);
 
 		mutex_lock(&dpm_list_mtx);
 		put_device(dev);
@@ -1005,12 +1004,9 @@ static int dpm_prepare(pm_message_t stat
 		if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
 			pm_wakeup_event(dev, 0);
 
-		if (pm_wakeup_pending()) {
-			pm_runtime_put_sync(dev);
-			error = -EBUSY;
-		} else {
-			error = device_prepare(dev, state);
-		}
+		pm_runtime_put_sync(dev);
+		error = pm_wakeup_pending() ?
+				-EBUSY : device_prepare(dev, state);
 
 		mutex_lock(&dpm_list_mtx);
 		if (error) {

WARNING: multiple messages have this Message-ID (diff)
From: rjw@sisk.pl (Rafael J. Wysocki)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] PM: Allow pm_runtime_suspend() to succeed during system suspend
Date: Fri, 11 Feb 2011 21:00:23 +0100	[thread overview]
Message-ID: <201102112100.23996.rjw@sisk.pl> (raw)
In-Reply-To: <201101311919.49225.rjw@sisk.pl>

On Monday, January 31, 2011, Rafael J. Wysocki wrote:
> On Monday, January 31, 2011, Alan Stern wrote:
> > On Mon, 31 Jan 2011, Kevin Hilman wrote:
> > 
> > > I understand how this works, but frankly I'm still a bit fuzzy on why.
> > > 
> > > I guess I'm still missing a good understanding of what "interfering with a
> > > system power transition" means, and why a runtime suspend qualifies as
> > > interfering but not a runtime resume.
> > 
> > These are good questions.  Rafael implemented this design originally; 
> > my contribution was only to warn him of the potential for problems.  
> > Therefore he should explain the rationale for the design.
> 
> The reason why runtime resume is allowed during system power transitions is
> because in some cases during system suspend we simply have to resume devices
> that were previously runtime-suspended (for example, the PCI bus type does
> that).
> 
> The reason why runtime suspend is not allowed during system power transitions
> if the following race:
> 
> - A device has been suspended via a system suspend callback.
> - The runtime PM framework executes a (scheduled) suspend on that device,
>   not knowing that it's already been suspended, which potentially results in
>   accessing the device's registers in a low-power state.
> 
> Now, it can be avoided if every driver does the right thing and checks whether
> the device is already suspended in its runtime suspend callback, but that would
> kind of defeat the purpose of the runtime PM framework, at least partially.

In fact, I've just realized that the above race cannot really occur, because
pm_wq is freezable, so I'm proposing the following change.

Of course, it still doesn't prevent user space from disabling the runtime PM
framework's helpers via /sys/devices/.../power/control.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: 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 <rjw@sisk.pl>
---
 drivers/base/power/main.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -669,7 +669,6 @@ static void dpm_complete(pm_message_t st
 		mutex_unlock(&dpm_list_mtx);
 
 		device_complete(dev, state);
-		pm_runtime_put_sync(dev);
 
 		mutex_lock(&dpm_list_mtx);
 		put_device(dev);
@@ -1005,12 +1004,9 @@ static int dpm_prepare(pm_message_t stat
 		if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
 			pm_wakeup_event(dev, 0);
 
-		if (pm_wakeup_pending()) {
-			pm_runtime_put_sync(dev);
-			error = -EBUSY;
-		} else {
-			error = device_prepare(dev, state);
-		}
+		pm_runtime_put_sync(dev);
+		error = pm_wakeup_pending() ?
+				-EBUSY : device_prepare(dev, state);
 
 		mutex_lock(&dpm_list_mtx);
 		if (error) {

  parent reply	other threads:[~2011-02-11 20:00 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-01-28  0:18 [PATCH] i2c: OMAP: fix static suspend vs. runtime suspend Kevin Hilman
2011-01-28  0:18 ` Kevin Hilman
2011-01-31 11:28 ` Rajendra Nayak
2011-01-31 11:28 ` Rajendra Nayak
2011-01-31 11:28   ` Rajendra Nayak
2011-01-31 15:13   ` [linux-pm] " Alan Stern
2011-01-31 15:13     ` Alan Stern
     [not found]     ` <Pine.LNX.4.44L0.1101311010580.1931-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
2011-01-31 15:28       ` Rajendra Nayak
2011-01-31 15:28         ` Rajendra Nayak
2011-01-31 15:28     ` Rajendra Nayak
2011-01-31 16:09     ` [linux-pm] " Kevin Hilman
2011-01-31 16:09       ` Kevin Hilman
     [not found]       ` <877hdl9hsn.fsf-l0cyMroinI0@public.gmane.org>
2011-01-31 16:22         ` Alan Stern
2011-01-31 16:22           ` Alan Stern
2011-01-31 18:19           ` Rafael J. Wysocki
     [not found]           ` <Pine.LNX.4.44L0.1101311119190.1931-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
2011-01-31 18:19             ` [linux-pm] " Rafael J. Wysocki
2011-01-31 18:19               ` Rafael J. Wysocki
2011-02-11 20:00               ` [PATCH] PM: Allow pm_runtime_suspend() to succeed during system suspend Rafael J. Wysocki
     [not found]               ` <201101311919.49225.rjw-KKrjLPT3xs0@public.gmane.org>
2011-02-11 20:00                 ` Rafael J. Wysocki [this message]
2011-02-11 20:00                   ` Rafael J. Wysocki
     [not found]                   ` <201102112100.23996.rjw-KKrjLPT3xs0@public.gmane.org>
2011-02-11 20:36                     ` Alan Stern
2011-02-11 20:36                       ` Alan Stern
2011-02-11 20:38                     ` Kevin Hilman
2011-02-11 20:38                       ` Kevin Hilman
     [not found]                       ` <87ei7e9uhy.fsf-l0cyMroinI0@public.gmane.org>
2011-02-11 21:25                         ` Rafael J. Wysocki
2011-02-11 21:25                           ` Rafael J. Wysocki
2011-02-11 23:45                           ` Kevin Hilman
2011-02-11 23:45                             ` Kevin Hilman
2011-02-12  0:00                             ` Rafael J. Wysocki
     [not found]                             ` <87aai26sq4.fsf-l0cyMroinI0@public.gmane.org>
2011-02-12  0:00                               ` Rafael J. Wysocki
2011-02-12  0:00                                 ` Rafael J. Wysocki
2011-02-11 23:45                           ` Kevin Hilman
2011-02-11 21:25                       ` Rafael J. Wysocki
2011-02-11 20:36                   ` Alan Stern
2011-02-11 20:38                   ` Kevin Hilman
2011-01-31 16:22       ` [PATCH] i2c: OMAP: fix static suspend vs. runtime suspend Alan Stern
2011-01-31 16:09     ` Kevin Hilman
2011-01-31 15:13   ` Alan Stern
2011-02-05 16:08 ` Ben Dooks
     [not found] ` <1296173921-4832-1-git-send-email-khilman-l0cyMroinI0@public.gmane.org>
2011-02-05 16:08   ` Ben Dooks
2011-02-05 16:08     ` Ben Dooks
     [not found]     ` <20110205160843.GD15795-SMNkleLxa3Z6Wcw2j4pizdi2O/JbrIOy@public.gmane.org>
2011-02-08 18:31       ` Kevin Hilman
2011-02-08 18:31         ` Kevin Hilman
2011-02-08 18:31     ` Kevin Hilman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201102112100.23996.rjw@sisk.pl \
    --to=rjw-kkrjlpt3xs0@public.gmane.org \
    --cc=ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org \
    --cc=khilman-l0cyMroinI0@public.gmane.org \
    --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
    --cc=linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org \
    --cc=stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.