On 18 November 2017 at 15:41, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > 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@xxxxxxxxx> > --- > > 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). > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
