| Message ID | 5730411.DndCTChqpV@aspire.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 18 November 2017 at 15:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Make the PM core handle DPM_FLAG_LEAVE_SUSPENDED directly for > devices whose "noirq", "late" and "early" driver callbacks are > invoked directly by it. This indicates that your target for this particular change isn't ACPI/PCI, but instead this aims to be a more generic solution to be able to optimize the resume path for devices. Assuming, this is the case, I don't think this is good enough as I pointed out [1] earlier. Simply because it isn't as flexible as is required - to really be able cover generic cases. > > Namely, make it skip all of the system-wide resume callbacks for > such devices with DPM_FLAG_LEAVE_SUSPENDED set if they are in > runtime suspend during the "noirq" phase of system-wide suspend > (or analogous) transitions or the system transition under way is > a proper suspend (rather than anything related to hibernation) and > the device's wakeup settings are compatible with runtime PM (that > is, the device cannot generate wakeup signals at all or it is > allowed to wake up the system from sleep). As I pointed out by submitting another patch [2], device_may_wakeup() doesn't really tell whether the wakeup is configured as "in-band" or "out-of-band". That knowledge is known by the driver and the subsystem layer - and for that reason I don't think the PM core shall base generic decisions like this on it. No comments on the code, so far. :-) Kind regards Uffe [1] https://www.spinics.net/lists/linux-pci/msg66502.html [2] https://patchwork.kernel.org/patch/10056323/ > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v3 -> v4: Rebase on the v4 of patch [1/6], add a forward declaration of > dpm_subsys_suspend_late_cb() dropped from the [4/6]. > > v2 -> v3: Rebase on the v3 of patch [1/6]. > > --- > Documentation/driver-api/pm/devices.rst | 9 +++++ > drivers/base/power/main.c | 51 ++++++++++++++++++++++++++++---- > 2 files changed, 55 insertions(+), 5 deletions(-) > > Index: linux-pm/drivers/base/power/main.c > =================================================================== > --- linux-pm.orig/drivers/base/power/main.c > +++ linux-pm/drivers/base/power/main.c > @@ -525,6 +525,10 @@ static void dpm_watchdog_clear(struct dp > #define dpm_watchdog_clear(x) > #endif > > +static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev, > + pm_message_t state, > + const char **info_p); > + > /*------------------------- Resume routines -------------------------*/ > > /** > @@ -581,6 +585,7 @@ static int device_resume_noirq(struct de > { > pm_callback_t callback; > const char *info; > + bool skip_resume; > int error = 0; > > TRACE_DEVICE(dev); > @@ -594,17 +599,27 @@ static int device_resume_noirq(struct de > > dpm_wait_for_superior(dev, async); > > + skip_resume = dev_pm_may_skip_resume(dev); > + > callback = dpm_subsys_resume_noirq_cb(dev, state, &info); > + if (callback) > + goto Run; > + > + if (skip_resume) > + goto Skip; > > if (!callback && dev->driver && dev->driver->pm) { > info = "noirq driver "; > callback = pm_noirq_op(dev->driver->pm, state); > } > > +Run: > error = dpm_run_callback(callback, dev, state, info); > + > +Skip: > dev->power.is_noirq_suspended = false; > > - if (dev_pm_may_skip_resume(dev)) { > + if (skip_resume) { > /* > * The device is going to be left in suspend, but it might not > * have been in runtime suspend before the system suspended, so > @@ -617,7 +632,7 @@ static int device_resume_noirq(struct de > dev->power.is_suspended = false; > } > > - Out: > +Out: > complete_all(&dev->power.completion); > TRACE_RESUME(error); > return error; > @@ -1193,6 +1208,7 @@ static int __device_suspend_noirq(struct > { > pm_callback_t callback; > const char *info; > + bool direct_cb = false; > int error = 0; > > TRACE_DEVICE(dev); > @@ -1212,12 +1228,17 @@ static int __device_suspend_noirq(struct > goto Complete; > > callback = dpm_subsys_suspend_noirq_cb(dev, state, &info); > + if (callback) > + goto Run; > > - if (!callback && dev->driver && dev->driver->pm) { > + direct_cb = true; > + > + if (dev->driver && dev->driver->pm) { > info = "noirq driver "; > callback = pm_noirq_op(dev->driver->pm, state); > } > > +Run: > error = dpm_run_callback(callback, dev, state, info); > if (error) { > async_error = error; > @@ -1227,13 +1248,33 @@ static int __device_suspend_noirq(struct > dev->power.is_noirq_suspended = true; > > if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) { > + pm_message_t resume_msg = resume_event(state); > + bool skip_resume; > + > + if (direct_cb && > + !dpm_subsys_suspend_late_cb(dev, state, NULL) && > + !dpm_subsys_resume_early_cb(dev, resume_msg, NULL) && > + !dpm_subsys_resume_noirq_cb(dev, resume_msg, NULL)) { > + /* > + * If all of the device driver's "noirq", "late" and > + * "early" callbacks are invoked directly by the core, > + * the decision to allow the device to stay in suspend > + * can be based on its current runtime PM status and its > + * wakeup settings. > + */ > + skip_resume = pm_runtime_status_suspended(dev) || > + (resume_msg.event == PM_EVENT_RESUME && > + (!device_can_wakeup(dev) || > + device_may_wakeup(dev))); > + } else { > + skip_resume = dev->power.may_skip_resume; > + } > /* > * The only safe strategy here is to require that if the device > * may not be left in suspend, resume callbacks must be invoked > * for it. > */ > - dev->power.must_resume = dev->power.must_resume || > - !dev->power.may_skip_resume || > + dev->power.must_resume = dev->power.must_resume || !skip_resume || > atomic_read(&dev->power.usage_count) > 1; > } else { > dev->power.must_resume = true; > Index: linux-pm/Documentation/driver-api/pm/devices.rst > =================================================================== > --- linux-pm.orig/Documentation/driver-api/pm/devices.rst > +++ linux-pm/Documentation/driver-api/pm/devices.rst > @@ -814,3 +814,12 @@ middle layer is then responsible for han > whether or not the device is left suspended, but the other resume callbacks > (except for ``->complete``) will be skipped automatically by the PM core if the > device really can be left in suspend. > + > +For devices whose "noirq", "late" and "early" driver callbacks are invoked > +directly by the PM core, all of the system-wide resume callbacks are skipped if > +``DPM_FLAG_LEAVE_SUSPENDED`` is set and the device is in runtime suspend during > +the ``suspend_noirq`` (or analogous) phase or the transition under way is a > +proper system suspend (rather than anything related to hibernation) and the > +device's wakeup settings are suitable for runtime PM (that is, it cannot > +generate wakeup signals at all or it is allowed to wake up the system from > +sleep). >
On Monday, November 20, 2017 2:42:26 PM CET Ulf Hansson wrote: > On 18 November 2017 at 15:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Make the PM core handle DPM_FLAG_LEAVE_SUSPENDED directly for > > devices whose "noirq", "late" and "early" driver callbacks are > > invoked directly by it. > > This indicates that your target for this particular change isn't > ACPI/PCI, but instead this aims to be a more generic solution to be > able to optimize the resume path for devices. > > Assuming, this is the case, I don't think this is good enough as I > pointed out [1] earlier. Simply because it isn't as flexible as is > required - to really be able cover generic cases. I'll go back to that message, but nothing so far has been flexible enough to cover everything you can imagine. The case this and the next patch cover really is to allow drivers that can be used with or without a PM domain to avoid doing any "are we suspended?" type of checks in their callbacks. Actually, the [6/6] is more important from that standpoint, but this one also may play the role because of the dependencies between devices involved in the handling of LEAVE_SUSPENDED (eg. say a PCI parent has a child platform or I2C or similar devices without a PM domain and what happens to the child affects the parent). > > > > Namely, make it skip all of the system-wide resume callbacks for > > such devices with DPM_FLAG_LEAVE_SUSPENDED set if they are in > > runtime suspend during the "noirq" phase of system-wide suspend > > (or analogous) transitions or the system transition under way is > > a proper suspend (rather than anything related to hibernation) and > > the device's wakeup settings are compatible with runtime PM (that > > is, the device cannot generate wakeup signals at all or it is > > allowed to wake up the system from sleep). > > As I pointed out by submitting another patch [2], device_may_wakeup() > doesn't really tell whether the wakeup is configured as "in-band" or > "out-of-band". That knowledge is known by the driver and the subsystem > layer - and for that reason I don't think the PM core shall base > generic decisions like this on it. The "or it is allowed to wake up the system from sleep" case may be overly optimistic, but the remaining two (runtime-suspended devices and devices that can't generate wakeup signals at all) are quite straightforward to me. Thanks, Rafael
On Wednesday, November 22, 2017 2:10:51 AM CET Rafael J. Wysocki wrote: > On Monday, November 20, 2017 2:42:26 PM CET Ulf Hansson wrote: > > On 18 November 2017 at 15:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Make the PM core handle DPM_FLAG_LEAVE_SUSPENDED directly for > > > devices whose "noirq", "late" and "early" driver callbacks are > > > invoked directly by it. > > > > This indicates that your target for this particular change isn't > > ACPI/PCI, but instead this aims to be a more generic solution to be > > able to optimize the resume path for devices. > > > > Assuming, this is the case, I don't think this is good enough as I > > pointed out [1] earlier. Simply because it isn't as flexible as is > > required - to really be able cover generic cases. > > I'll go back to that message, but nothing so far has been flexible enough to > cover everything you can imagine. > > The case this and the next patch cover really is to allow drivers that can be > used with or without a PM domain to avoid doing any "are we suspended?" type > of checks in their callbacks. Actually, the [6/6] is more important from that > standpoint, but this one also may play the role because of the dependencies > between devices involved in the handling of LEAVE_SUSPENDED (eg. say a PCI > parent has a child platform or I2C or similar devices without a PM domain > and what happens to the child affects the parent). > > > > > > > Namely, make it skip all of the system-wide resume callbacks for > > > such devices with DPM_FLAG_LEAVE_SUSPENDED set if they are in > > > runtime suspend during the "noirq" phase of system-wide suspend > > > (or analogous) transitions or the system transition under way is > > > a proper suspend (rather than anything related to hibernation) and > > > the device's wakeup settings are compatible with runtime PM (that > > > is, the device cannot generate wakeup signals at all or it is > > > allowed to wake up the system from sleep). > > > > As I pointed out by submitting another patch [2], device_may_wakeup() > > doesn't really tell whether the wakeup is configured as "in-band" or > > "out-of-band". That knowledge is known by the driver and the subsystem > > layer - and for that reason I don't think the PM core shall base > > generic decisions like this on it. > > The "or it is allowed to wake up the system from sleep" case may be overly > optimistic, but the remaining two (runtime-suspended devices and devices > that can't generate wakeup signals at all) are quite straightforward to me. BTW, I'm not sure if the device_may_wakeup() check is really insufficient in this particular case. Say the device was not in runtime suspend before, but device_may_wakeup() returns "true" for it and the system is resuming from suspend. The device's state should be suitable to wake up the system in any case, so the "in-band" vs "out-of-band" difference has had to be taken care of already during system suspend. Thanks, Rafael
Index: linux-pm/drivers/base/power/main.c =================================================================== --- linux-pm.orig/drivers/base/power/main.c +++ linux-pm/drivers/base/power/main.c @@ -525,6 +525,10 @@ static void dpm_watchdog_clear(struct dp #define dpm_watchdog_clear(x) #endif +static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev, + pm_message_t state, + const char **info_p); + /*------------------------- Resume routines -------------------------*/ /** @@ -581,6 +585,7 @@ static int device_resume_noirq(struct de { pm_callback_t callback; const char *info; + bool skip_resume; int error = 0; TRACE_DEVICE(dev); @@ -594,17 +599,27 @@ static int device_resume_noirq(struct de dpm_wait_for_superior(dev, async); + skip_resume = dev_pm_may_skip_resume(dev); + callback = dpm_subsys_resume_noirq_cb(dev, state, &info); + if (callback) + goto Run; + + if (skip_resume) + goto Skip; if (!callback && dev->driver && dev->driver->pm) { info = "noirq driver "; callback = pm_noirq_op(dev->driver->pm, state); } +Run: error = dpm_run_callback(callback, dev, state, info); + +Skip: dev->power.is_noirq_suspended = false; - if (dev_pm_may_skip_resume(dev)) { + if (skip_resume) { /* * The device is going to be left in suspend, but it might not * have been in runtime suspend before the system suspended, so @@ -617,7 +632,7 @@ static int device_resume_noirq(struct de dev->power.is_suspended = false; } - Out: +Out: complete_all(&dev->power.completion); TRACE_RESUME(error); return error; @@ -1193,6 +1208,7 @@ static int __device_suspend_noirq(struct { pm_callback_t callback; const char *info; + bool direct_cb = false; int error = 0; TRACE_DEVICE(dev); @@ -1212,12 +1228,17 @@ static int __device_suspend_noirq(struct goto Complete; callback = dpm_subsys_suspend_noirq_cb(dev, state, &info); + if (callback) + goto Run; - if (!callback && dev->driver && dev->driver->pm) { + direct_cb = true; + + if (dev->driver && dev->driver->pm) { info = "noirq driver "; callback = pm_noirq_op(dev->driver->pm, state); } +Run: error = dpm_run_callback(callback, dev, state, info); if (error) { async_error = error; @@ -1227,13 +1248,33 @@ static int __device_suspend_noirq(struct dev->power.is_noirq_suspended = true; if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) { + pm_message_t resume_msg = resume_event(state); + bool skip_resume; + + if (direct_cb && + !dpm_subsys_suspend_late_cb(dev, state, NULL) && + !dpm_subsys_resume_early_cb(dev, resume_msg, NULL) && + !dpm_subsys_resume_noirq_cb(dev, resume_msg, NULL)) { + /* + * If all of the device driver's "noirq", "late" and + * "early" callbacks are invoked directly by the core, + * the decision to allow the device to stay in suspend + * can be based on its current runtime PM status and its + * wakeup settings. + */ + skip_resume = pm_runtime_status_suspended(dev) || + (resume_msg.event == PM_EVENT_RESUME && + (!device_can_wakeup(dev) || + device_may_wakeup(dev))); + } else { + skip_resume = dev->power.may_skip_resume; + } /* * The only safe strategy here is to require that if the device * may not be left in suspend, resume callbacks must be invoked * for it. */ - dev->power.must_resume = dev->power.must_resume || - !dev->power.may_skip_resume || + dev->power.must_resume = dev->power.must_resume || !skip_resume || atomic_read(&dev->power.usage_count) > 1; } else { dev->power.must_resume = true; Index: linux-pm/Documentation/driver-api/pm/devices.rst =================================================================== --- linux-pm.orig/Documentation/driver-api/pm/devices.rst +++ linux-pm/Documentation/driver-api/pm/devices.rst @@ -814,3 +814,12 @@ middle layer is then responsible for han whether or not the device is left suspended, but the other resume callbacks (except for ``->complete``) will be skipped automatically by the PM core if the device really can be left in suspend. + +For devices whose "noirq", "late" and "early" driver callbacks are invoked +directly by the PM core, all of the system-wide resume callbacks are skipped if +``DPM_FLAG_LEAVE_SUSPENDED`` is set and the device is in runtime suspend during +the ``suspend_noirq`` (or analogous) phase or the transition under way is a +proper system suspend (rather than anything related to hibernation) and the +device's wakeup settings are suitable for runtime PM (that is, it cannot +generate wakeup signals at all or it is allowed to wake up the system from +sleep).