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(-)
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);
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
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
On Sunday, June 19, 2011, Alan Stern wrote:
> 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. :-(
Well, that was kind of difficult to debug, but not impossible. :-)
The problem here turns out to be related to the SCSI subsystem.
Namely, when the AHCI controller is suspended, it uses the SCSI error
handling mechanism for scheduling the suspend operation (I'm still at a little
loss why that is necessary, but Tejun says it is :-)). This (after several
convoluted operations) causes scsi_error_handler() to be woken up and
it calls scsi_autopm_get_host(shost), which returns error code (-EAGAIN),
because the runtime PM has been disabled at the host level.
This happens because scsi_autopm_get_host() uses
pm_runtime_get_sync(&shost->shost_gendev) and returns error code when
shost_gendev.power.disable_depth is nonzero.
So, the problem is either in scsi_autopm_get_host() that should check the
error code returned by pm_runtime_get_sync(), or in rpm_suspend() that should
return 0 if RPM_GET_PUT is set in flags. I'm inclined to say that the
problem should be fixed in rpm_suspend() and hence the appended patch that
works (well, it probably should be split into three separate patches).
> > 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.
Certainly, although the situation is not as bad as I thought, because
__pm_generic_call() is executed after the patch below disables runtime PM
during system suspend. Still, the pm_runtime_suspended() check in there is
pointless.
Thanks,
Rafael
---
drivers/base/power/main.c | 9 ++++++++-
drivers/base/power/runtime.c | 41 ++++++++++++++++++++++++-----------------
include/linux/pm_runtime.h | 13 ++++++++++---
3 files changed, 42 insertions(+), 21 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
@@ -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, PRD_DEPTH);
+ }
Unlock:
device_unlock(dev);
Index: linux-2.6/drivers/base/power/runtime.c
===================================================================
--- linux-2.6.orig/drivers/base/power/runtime.c
+++ linux-2.6/drivers/base/power/runtime.c
@@ -455,12 +455,14 @@ static int rpm_resume(struct device *dev
dev_dbg(dev, "%s flags 0x%x\n", __func__, rpmflags);
repeat:
- if (dev->power.runtime_error)
+ if (dev->power.runtime_error) {
retval = -EINVAL;
- else if (dev->power.disable_depth > 0)
- retval = -EAGAIN;
- if (retval)
goto out;
+ } else if (dev->power.disable_depth > 0) {
+ if (!(rpmflags & RPM_GET_PUT))
+ retval = -EAGAIN;
+ goto out;
+ }
/*
* Other scheduled or pending requests need to be canceled. Small
@@ -965,18 +967,23 @@ EXPORT_SYMBOL_GPL(pm_runtime_barrier);
/**
* __pm_runtime_disable - Disable run-time PM of a device.
* @dev: Device to handle.
- * @check_resume: If set, check if there's a resume request for the device.
+ * @level: How much to do.
+ *
+ * Increment power.disable_depth for the device and if was zero previously.
+ *
+ * If @level is at least PRD_BARRIER, additionally cancel all pending run-time
+ * PM requests for the device and wait for all operations in progress to
+ * complete.
+ *
+ * If @level is at least PRD_CHECK_RESUME and there's a resume request pending
+ * when this function is called, and power.disable_depth is zero, the device
+ * will be woken up before disabling its run-time PM.
+ *
+ * The device can be either active or suspended after its run-time PM has been
+ * disabled.
*
- * Increment power.disable_depth for the device and if was zero previously,
- * cancel all pending run-time PM requests for the device and wait for all
- * operations in progress to complete. The device can be either active or
- * suspended after its run-time PM has been disabled.
- *
- * If @check_resume is set and there's a resume request pending when
- * __pm_runtime_disable() is called and power.disable_depth is zero, the
- * function will wake up the device before disabling its run-time PM.
*/
-void __pm_runtime_disable(struct device *dev, bool check_resume)
+void __pm_runtime_disable(struct device *dev, enum prd_level level)
{
spin_lock_irq(&dev->power.lock);
@@ -990,7 +997,7 @@ void __pm_runtime_disable(struct device
* means there probably is some I/O to process and disabling run-time PM
* shouldn't prevent the device from processing the I/O.
*/
- if (check_resume && dev->power.request_pending
+ if (level >= PRD_CHECK_RESUME && dev->power.request_pending
&& dev->power.request == RPM_REQ_RESUME) {
/*
* Prevent suspends and idle notifications from being carried
@@ -1003,7 +1010,7 @@ void __pm_runtime_disable(struct device
pm_runtime_put_noidle(dev);
}
- if (!dev->power.disable_depth++)
+ if (!dev->power.disable_depth++ && level >= PRD_BARRIER)
__pm_runtime_barrier(dev);
out:
@@ -1230,7 +1237,7 @@ void pm_runtime_init(struct device *dev)
*/
void pm_runtime_remove(struct device *dev)
{
- __pm_runtime_disable(dev, false);
+ __pm_runtime_disable(dev, PRD_BARRIER);
/* Change the status back to 'suspended' to match the initial status. */
if (dev->power.runtime_status == RPM_ACTIVE)
Index: linux-2.6/include/linux/pm_runtime.h
===================================================================
--- linux-2.6.orig/include/linux/pm_runtime.h
+++ linux-2.6/include/linux/pm_runtime.h
@@ -22,6 +22,13 @@
usage_count */
#define RPM_AUTO 0x08 /* Use autosuspend_delay */
+/* Runtime PM disable levels */
+enum prd_level {
+ PRD_DEPTH = 0, /* Only increment disable depth */
+ PRD_BARRIER, /* Additionally, act as a runtime PM barrier */
+ PRD_CHECK_RESUME, /* Additionally, check if resume is pending */
+};
+
#ifdef CONFIG_PM_RUNTIME
extern struct workqueue_struct *pm_wq;
@@ -33,7 +40,7 @@ extern int pm_schedule_suspend(struct de
extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
extern int pm_runtime_barrier(struct device *dev);
extern void pm_runtime_enable(struct device *dev);
-extern void __pm_runtime_disable(struct device *dev, bool check_resume);
+extern void __pm_runtime_disable(struct device *dev, enum prd_level level);
extern void pm_runtime_allow(struct device *dev);
extern void pm_runtime_forbid(struct device *dev);
extern int pm_generic_runtime_idle(struct device *dev);
@@ -119,7 +126,7 @@ static inline int __pm_runtime_set_statu
unsigned int status) { return 0; }
static inline int pm_runtime_barrier(struct device *dev) { return 0; }
static inline void pm_runtime_enable(struct device *dev) {}
-static inline void __pm_runtime_disable(struct device *dev, bool c) {}
+static inline void __pm_runtime_disable(struct device *dev, enum prd_level l) {}
static inline void pm_runtime_allow(struct device *dev) {}
static inline void pm_runtime_forbid(struct device *dev) {}
@@ -232,7 +239,7 @@ static inline void pm_runtime_set_suspen
static inline void pm_runtime_disable(struct device *dev)
{
- __pm_runtime_disable(dev, true);
+ __pm_runtime_disable(dev, PRD_CHECK_RESUME);
}
static inline void pm_runtime_use_autosuspend(struct device *dev)
On Sun, 19 Jun 2011, Rafael J. Wysocki wrote:
> Well, that was kind of difficult to debug, but not impossible. :-)
>
> The problem here turns out to be related to the SCSI subsystem.
> Namely, when the AHCI controller is suspended, it uses the SCSI error
> handling mechanism for scheduling the suspend operation (I'm still at a little
> loss why that is necessary, but Tejun says it is :-)). This (after several
> convoluted operations) causes scsi_error_handler() to be woken up and
> it calls scsi_autopm_get_host(shost), which returns error code (-EAGAIN),
> because the runtime PM has been disabled at the host level.
Oh no. I was afraid something like this was going to happen
eventually.
It's clear that we don't want runtime PM kicking in while the SCSI
error handler is running. That's why I added the
scsi_autopm_get_host(). But this also means we will run into trouble
if the error handler needs to be used during a power transition.
> This happens because scsi_autopm_get_host() uses
> pm_runtime_get_sync(&shost->shost_gendev) and returns error code when
> shost_gendev.power.disable_depth is nonzero.
Maybe get_sync doesn't need to return an error if the runtime status is
already ACTIVE. I'm not sure about this; it's just an idea...
> So, the problem is either in scsi_autopm_get_host() that should check the
> error code returned by pm_runtime_get_sync(), or in rpm_suspend() that should
> return 0 if RPM_GET_PUT is set in flags. I'm inclined to say that the
> problem should be fixed in rpm_suspend() and hence the appended patch that
> works (well, it probably should be split into three separate patches).
Maybe it would be good enough if the error handler ended up doing a
get_noresume instead of get_sync? Although I could be wrong, I don't
think scsi_error_handler() will ever run in a situation where the host
adapter is not runtime-active.
Tejun, does that sound right to you?
Alan Stern
On Sunday, June 19, 2011, Alan Stern wrote:
> On Sun, 19 Jun 2011, Rafael J. Wysocki wrote:
>
> > Well, that was kind of difficult to debug, but not impossible. :-)
> >
> > The problem here turns out to be related to the SCSI subsystem.
> > Namely, when the AHCI controller is suspended, it uses the SCSI error
> > handling mechanism for scheduling the suspend operation (I'm still at a little
> > loss why that is necessary, but Tejun says it is :-)). This (after several
> > convoluted operations) causes scsi_error_handler() to be woken up and
> > it calls scsi_autopm_get_host(shost), which returns error code (-EAGAIN),
> > because the runtime PM has been disabled at the host level.
>
> Oh no. I was afraid something like this was going to happen
> eventually.
>
> It's clear that we don't want runtime PM kicking in while the SCSI
> error handler is running. That's why I added the
> scsi_autopm_get_host(). But this also means we will run into trouble
> if the error handler needs to be used during a power transition.
>
> > This happens because scsi_autopm_get_host() uses
> > pm_runtime_get_sync(&shost->shost_gendev) and returns error code when
> > shost_gendev.power.disable_depth is nonzero.
>
> Maybe get_sync doesn't need to return an error if the runtime status is
> already ACTIVE. I'm not sure about this; it's just an idea...
Well, if disable_depth > 0, ACTIVE isn't really well defined. As I said,
though, I think it makes sense for pm_runtime_get_sync() to return 0 when
disable_depth > 0, because the grabbing of a reference is successful anyway and
the caller may assume that the device is accessible in that case.
In the meantime I rethought the __pm_runtime_disable() part of my previous
patch and I now think it's not necessary to complicate it any more. Of course,
we need not check if runtime resume is pending in __device_suspend(), because
we've done it already in dpm_prepare(), but the barrier part should better be
done in there too.
Updated patch is appended.
Thanks,
Rafael
---
drivers/base/power/main.c | 6 ++++++
drivers/base/power/runtime.c | 10 ++++++----
2 files changed, 12 insertions(+), 4 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
@@ -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);
@@ -896,6 +900,8 @@ static int __device_suspend(struct devic
if (error)
async_error = error;
+ else if (dev->power.is_suspended)
+ __pm_runtime_disable(dev, false);
return error;
}
Index: linux-2.6/drivers/base/power/runtime.c
===================================================================
--- linux-2.6.orig/drivers/base/power/runtime.c
+++ linux-2.6/drivers/base/power/runtime.c
@@ -455,12 +455,14 @@ static int rpm_resume(struct device *dev
dev_dbg(dev, "%s flags 0x%x\n", __func__, rpmflags);
repeat:
- if (dev->power.runtime_error)
+ if (dev->power.runtime_error) {
retval = -EINVAL;
- else if (dev->power.disable_depth > 0)
- retval = -EAGAIN;
- if (retval)
goto out;
+ } else if (dev->power.disable_depth > 0) {
+ if (!(rpmflags & RPM_GET_PUT))
+ retval = -EAGAIN;
+ goto out;
+ }
/*
* Other scheduled or pending requests need to be canceled. Small
On Sun, 19 Jun 2011, Rafael J. Wysocki wrote:
> In the meantime I rethought the __pm_runtime_disable() part of my previous
> patch and I now think it's not necessary to complicate it any more. Of course,
> we need not check if runtime resume is pending in __device_suspend(), because
> we've done it already in dpm_prepare(), but the barrier part should better be
> done in there too.
Does this really make sense? What use is a barrier in dpm_prepare() if
runtime PM is allowed to continue functioning up to the
suspend callback?
As I see it, we never want a suspend or suspend_noirq callback to call
pm_runtime_suspend(). However it's okay for the suspend callback to
invoke pm_runtime_resume(), as long as this is all done in subsystem
code.
And in between the prepare and suspend callbacks, runtime PM should be
more or less fully functional, right? For most devices it will never
be triggered, because it has to run in process context and both
userspace and pm_wq are frozen. It may trigger for devices marked as
IRQ-safe, though.
Maybe the barrier should be moved into __device_suspend().
Alan Stern
On Monday, June 20, 2011, Alan Stern wrote:
> On Sun, 19 Jun 2011, Rafael J. Wysocki wrote:
>
> > In the meantime I rethought the __pm_runtime_disable() part of my previous
> > patch and I now think it's not necessary to complicate it any more. Of course,
> > we need not check if runtime resume is pending in __device_suspend(), because
> > we've done it already in dpm_prepare(), but the barrier part should better be
> > done in there too.
>
> Does this really make sense? What use is a barrier in dpm_prepare() if
> runtime PM is allowed to continue functioning up to the
> suspend callback?
It checks if a resume request is pending and executes runtime resume in that
case.
> As I see it, we never want a suspend or suspend_noirq callback to call
> pm_runtime_suspend(). However it's okay for the suspend callback to
> invoke pm_runtime_resume(), as long as this is all done in subsystem
> code.
First off, I don't really see a reason for a subsystem to call
pm_runtime_resume() from its .suspend_noirq() callback. Now, if
pm_runtime_resume() is to be called concurrently with the subsystem's
.suspend_noirq() callback, I'd rather won't let that happen. :-)
> And in between the prepare and suspend callbacks, runtime PM should be
> more or less fully functional, right? For most devices it will never
> be triggered, because it has to run in process context and both
> userspace and pm_wq are frozen. It may trigger for devices marked as
> IRQ-safe, though.
It also may trigger for drivers using non-freezable workqueues and calling
runtime PM synchronously from there.
> Maybe the barrier should be moved into __device_suspend().
I _really_ think that the initial approach, i.e. before commit
e8665002477f0278f84f898145b1f141ba26ee26, made the most sense. It didn't
cover the "pm_runtime_resume() called during system suspend" case, but
it did cover everything else.
So, I think there are serious technical arguments for reverting that commit.
I think we went really far trying to avoid that, but I'm not sure I want to go
any further.
Thanks,
Rafael