diff mbox

calling runtime PM from system PM methods

Message ID 201106182301.04090.rjw@sisk.pl (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Rafael Wysocki June 18, 2011, 9:01 p.m. UTC 
On Saturday, June 18, 2011, Alan Stern wrote:
> On Sat, 18 Jun 2011, Rafael J. Wysocki wrote:
> 
> > On Friday, June 17, 2011, Rafael J. Wysocki wrote:
> > > On Friday, June 17, 2011, Alan Stern wrote:
> > > > On Fri, 17 Jun 2011, Rafael J. Wysocki wrote:
> > > > 
> > > > > Having considered that a bit more I see that, in fact, commit
> > > > > e8665002477f0278f84f898145b1f141ba26ee26 (PM: Allow pm_runtime_suspend() to
> > > > > succeed during system suspend) has introduced at least one regression.
> > > > > Namely, the PCI bus type runs pm_runtime_resume() in its .prepare()
> > > > > callback to guarantee that devices will be in a well known state before
> > > > > the PCI .suspend() and .suspend_noirq() callbacks are executed.
> > > > > Unfortunately, after commit e8665002477f0278f84f898145b1f141ba26ee26 this
> > > > > isn't valid any more, because devices can be runtime-suspend after the
> > > > > pm_runtime_resume() in .prepare() has run.
> > > > > 
> > > > > USB seems to do something similar in choose_wakeup().
> > > > > 
> > > > > So, either the both of these subsystems should be modified to use
> > > > > pm_runtime_get_sync() and then pm_runtime_put_<something>() some time
> > > > > during resume, or we should revert commit e8665002477f0278f84f898145b1f141ba26ee26.
> > > > 
> > > > pm_runtime_put_noidle would be appropriate.
> > > > 
> > > > > Quite frankly, which shouldn't be a surprise to anyone at this point, I'd
> > > > > prefer to revert that commit for 3.0.
> > > > 
> > > > Maybe we can compromise.  Instead of reverting that commit outright,
> > > > put the get_noresume just before the suspend callback and put the
> > > > put_sync just after the resume callback.
> > > 
> > > That wouldn't fix the PCI problem, though, because it would leave a small
> > > window in which the device could be suspended after the pm_runtime_resume()
> > > in pci_pm_prepare() had run.
> > 
> > That said, the PCI case can be solved with a separate patch and if the other
> > subsystems are not affected, perhaps that's the best approach.
> 
> Yes, it would be a simple change.
> 
> > Still, I'd like to make sure that there won't be any races between runtime
> > PM and .suspend_noirq() and .resume_noirq() callbacks, so I'd like to apply
> > the patch below.
> > 
> > Thanks,
> > Rafael
> > 
> > ---
> >  drivers/base/power/main.c |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > 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
> > @@ -591,6 +591,8 @@ void dpm_resume(pm_message_t state)
> >  	async_error = 0;
> >  
> >  	list_for_each_entry(dev, &dpm_suspended_list, power.entry) {
> > +		pm_runtime_get_noresume(dev);
> > +		pm_runtime_enable(dev);
> >  		INIT_COMPLETION(dev->power.completion);
> >  		if (is_async(dev)) {
> >  			get_device(dev);
> > @@ -614,6 +616,7 @@ void dpm_resume(pm_message_t state)
> >  		}
> >  		if (!list_empty(&dev->power.entry))
> >  			list_move_tail(&dev->power.entry, &dpm_prepared_list);
> > +		pm_runtime_put_noidle(dev);
> >  		put_device(dev);
> >  	}
> >  	mutex_unlock(&dpm_list_mtx);
> > @@ -939,8 +942,10 @@ int dpm_suspend(pm_message_t state)
> >  			put_device(dev);
> >  			break;
> >  		}
> > -		if (!list_empty(&dev->power.entry))
> > +		if (!list_empty(&dev->power.entry)) {
> >  			list_move(&dev->power.entry, &dpm_suspended_list);
> > +			pm_runtime_disable(dev);
> > +		}
> 
> The put_noidle is in the wrong place for async resumes.  Likewise for
> the pm_runtime_disable() and async suspends.  Also this runs into
> problems if a device is never suspended (i.e., if the sleep transition
> aborts before suspending that device).

I overlooked that, thanks for pointing it out.

Well, assuming that https://patchwork.kernel.org/patch/893722/ is applied,
which is going to be, I think we can put

+       pm_runtime_get_noresume(dev);
+       pm_runtime_enable(dev);

in device_resume() after the dev->power.is_suspended check and
pm_runtime_put_noidle() under the End label.  That cause them to
be called under the device lock, but that shouldn't be a big deal.

Accordingly, we can call pm_runtime_disable(dev) in __device_suspend(),
right next to the setting of power.is_suspended.

This is implemented by the patch below.

Thanks,
Rafael

---
 drivers/base/power/main.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Rafael Wysocki June 18, 2011, 11:57 p.m. UTC | #1 
On Saturday, June 18, 2011, Rafael J. Wysocki wrote:
> On Saturday, June 18, 2011, Alan Stern wrote:
> > On Sat, 18 Jun 2011, Rafael J. Wysocki wrote:
...
> 
> Well, assuming that https://patchwork.kernel.org/patch/893722/ is applied,
> which is going to be, I think we can put
> 
> +       pm_runtime_get_noresume(dev);
> +       pm_runtime_enable(dev);
> 
> in device_resume() after the dev->power.is_suspended check and
> pm_runtime_put_noidle() under the End label.  That cause them to
> be called under the device lock, but that shouldn't be a big deal.
> 
> Accordingly, we can call pm_runtime_disable(dev) in __device_suspend(),
> right next to the setting of power.is_suspended.
> 
> This is implemented by the patch below.

Well, it hangs suspend on my Toshiba test box, I'm not sure why exactly.

This happens even if the pm_runtime_disable() is replaced with a version
that only increments the disable depth, so it looks like something down
the road relies on disable_depth being zero.  Which is worrisome.

Trying to figure out what the problem is I noticed that, for example,
the generic PM operations use pm_runtime_suspended() to decide whether or
not to execute system suspend callbacks, so the patch below would break it.

Also, after commit e8665002477f0278f84f898145b1f141ba26ee26 the
pm_runtime_suspended() check in __pm_generic_call() doesn't really make
sense.

Thanks,
Rafael


> ---
>  drivers/base/power/main.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> 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
> @@ -521,6 +521,9 @@ static int device_resume(struct device *
>  	if (!dev->power.is_suspended)
>  		goto Unlock;
>  
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
> +
>  	if (dev->pwr_domain) {
>  		pm_dev_dbg(dev, state, "power domain ");
>  		error = pm_op(dev, &dev->pwr_domain->ops, state);
> @@ -557,6 +560,7 @@ static int device_resume(struct device *
>  
>   End:
>  	dev->power.is_suspended = false;
> +	pm_runtime_put_noidle(dev);
>  
>   Unlock:
>  	device_unlock(dev);
> @@ -888,7 +892,10 @@ static int __device_suspend(struct devic
>  	}
>  
>   End:
> -	dev->power.is_suspended = !error;
> +	if (!error) {
> +		dev->power.is_suspended = true;
> +		pm_runtime_disable(dev);
> +	}
>  
>   Unlock:
>  	device_unlock(dev);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
>
Alan Stern June 19, 2011, 1:42 a.m. UTC | #2 
On Sun, 19 Jun 2011, Rafael J. Wysocki wrote:

> On Saturday, June 18, 2011, Rafael J. Wysocki wrote:
> > On Saturday, June 18, 2011, Alan Stern wrote:
> > > On Sat, 18 Jun 2011, Rafael J. Wysocki wrote:
> ...
> > 
> > Well, assuming that https://patchwork.kernel.org/patch/893722/ is applied,
> > which is going to be, I think we can put
> > 
> > +       pm_runtime_get_noresume(dev);
> > +       pm_runtime_enable(dev);
> > 
> > in device_resume() after the dev->power.is_suspended check and
> > pm_runtime_put_noidle() under the End label.  That cause them to
> > be called under the device lock, but that shouldn't be a big deal.
> > 
> > Accordingly, we can call pm_runtime_disable(dev) in __device_suspend(),
> > right next to the setting of power.is_suspended.
> > 
> > This is implemented by the patch below.
> 
> Well, it hangs suspend on my Toshiba test box, I'm not sure why exactly.
> 
> This happens even if the pm_runtime_disable() is replaced with a version
> that only increments the disable depth, so it looks like something down
> the road relies on disable_depth being zero.  Which is worrisome.

This is a sign that the PM subsystem is getting a little too 
complicated.  :-(

> Trying to figure out what the problem is I noticed that, for example,
> the generic PM operations use pm_runtime_suspended() to decide whether or
> not to execute system suspend callbacks, so the patch below would break it.
> 
> Also, after commit e8665002477f0278f84f898145b1f141ba26ee26 the
> pm_runtime_suspended() check in __pm_generic_call() doesn't really make
> sense.

In light of the recent changes, we should revisit the decisions behind 
the generic PM operations.

Alan Stern
diff mbox

Patch

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
@@ -521,6 +521,9 @@  static int device_resume(struct device *
 	if (!dev->power.is_suspended)
 		goto Unlock;
 
+	pm_runtime_get_noresume(dev);
+	pm_runtime_enable(dev);
+
 	if (dev->pwr_domain) {
 		pm_dev_dbg(dev, state, "power domain ");
 		error = pm_op(dev, &dev->pwr_domain->ops, state);
@@ -557,6 +560,7 @@  static int device_resume(struct device *
 
  End:
 	dev->power.is_suspended = false;
+	pm_runtime_put_noidle(dev);
 
  Unlock:
 	device_unlock(dev);
@@ -888,7 +892,10 @@  static int __device_suspend(struct devic
 	}
 
  End:
-	dev->power.is_suspended = !error;
+	if (!error) {
+		dev->power.is_suspended = true;
+		pm_runtime_disable(dev);
+	}
 
  Unlock:
 	device_unlock(dev);