[PATCH 0/4] PM / core: Direct handling of DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED

[PATCH 0/4] PM / core: Direct handling of DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED

[Rafael J. Wysocki]

Hi All,

This series is a follow-up for

https://marc.info/?l=linux-doc&m=151101644105835&w=2

Patches[1-3/6] from the above have been reviewed and agreed on, so
they are in linux-next now and here's a next version of the rest.

Patches [1-2/4] are preparatory.  The first one is just really small
code duplication avoidance on top of this recent fix:

https://patchwork.kernel.org/patch/10097563/

and the second one simply moves some code to separate functions.

Patch [3/4] causes the PM core to carry out some optimizations for
drivers of devices with DPM_FLAG_SMART_SUSPEND set whose "late"
and "noirq" suspend (or equivalent) driver callbacks are invoked
directly by the core.

The underlying observation is that if the device is suspended (via
runtime PM) during the "late suspend" phase of a system transition,
invoking the "late" and "noirq" callbacks from the driver for it is not
going to make it more suspended, so to speak, so it doesn't make sense to
invoke them at all.

[That optimization is only done for devices with DPM_FLAG_SMART_SUSPEND
set, because drivers setting that flag are expected to be prepared for
skipping their "late" and "noirq" callbacks if the device is already
suspended.]

Patch [4/4] makes the core do an analogous thing for devices with
DPM_FLAG_LEAVE_SUSPENDED set whose "noirq" and "early" resume (or
equivalent) driver callbacks are directly invoked by the core.

In that case the observation is that if such devices can be left in
suspend after the system transition to the working state, running
resume callbacks from their drivers is simply not necessary.

Pathes [3-4/4] have been reoredered and reworked a bit since the last
iteration, so they are regarded as new.

The series is on top of the linux-next branch of the linux-pm.git tree
that should be merged into linux-next on Monday.

[I have developed debug bus type and driver modules to test that code,
but they are not ready to be made available at this point.]

Thanks,
Rafael

[PATCH 1/4] PM / core: Use dev_pm_skip_next_resume_phases() internally

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make the PM core call dev_pm_skip_next_resume_phases() to skip the
"early resume" and "resume" phases of system-wide transitions to the
working state for a given device instead of clearing the relevant
status bits for it directly.

No intentional changes in functionality.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -609,8 +609,7 @@ static int device_resume_noirq(struct de
 		 * device again.
 		 */
 		pm_runtime_set_suspended(dev);
-		dev->power.is_late_suspended = false;
-		dev->power.is_suspended = false;
+		dev_pm_skip_next_resume_phases(dev);
 	}
 
  Out:

[PATCH 2/4] PM / core: Add helpers for subsystem callback selection

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Add helper routines to find and return a suitable subsystem callback
during the "noirq" phases of system suspend/resume (or analogous)
transitions as well as during the "late" phase of system suspend and
the "early" phase of system resume (or analogous) transitions.

The helpers will be called from additional sites going forward.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/main.c |  188 +++++++++++++++++++++++++++++++---------------
 1 file changed, 128 insertions(+), 60 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -552,6 +552,35 @@ bool dev_pm_may_skip_resume(struct devic
 	return !dev->power.must_resume && pm_transition.event != PM_EVENT_RESTORE;
 }
 
+static pm_callback_t dpm_subsys_resume_noirq_cb(struct device *dev,
+						pm_message_t state,
+						const char **info_p)
+{
+	pm_callback_t callback;
+	const char *info;
+
+	if (dev->pm_domain) {
+		info = "noirq power domain ";
+		callback = pm_noirq_op(&dev->pm_domain->ops, state);
+	} else if (dev->type && dev->type->pm) {
+		info = "noirq type ";
+		callback = pm_noirq_op(dev->type->pm, state);
+	} else if (dev->class && dev->class->pm) {
+		info = "noirq class ";
+		callback = pm_noirq_op(dev->class->pm, state);
+	} else if (dev->bus && dev->bus->pm) {
+		info = "noirq bus ";
+		callback = pm_noirq_op(dev->bus->pm, state);
+	} else {
+		return NULL;
+	}
+
+	if (info_p)
+		*info_p = info;
+
+	return callback;
+}
+
 /**
  * device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
@@ -563,8 +592,8 @@ bool dev_pm_may_skip_resume(struct devic
  */
 static int device_resume_noirq(struct device *dev, pm_message_t state, bool async)
 {
-	pm_callback_t callback = NULL;
-	const char *info = NULL;
+	pm_callback_t callback;
+	const char *info;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -578,19 +607,7 @@ static int device_resume_noirq(struct de
 
 	dpm_wait_for_superior(dev, async);
 
-	if (dev->pm_domain) {
-		info = "noirq power domain ";
-		callback = pm_noirq_op(&dev->pm_domain->ops, state);
-	} else if (dev->type && dev->type->pm) {
-		info = "noirq type ";
-		callback = pm_noirq_op(dev->type->pm, state);
-	} else if (dev->class && dev->class->pm) {
-		info = "noirq class ";
-		callback = pm_noirq_op(dev->class->pm, state);
-	} else if (dev->bus && dev->bus->pm) {
-		info = "noirq bus ";
-		callback = pm_noirq_op(dev->bus->pm, state);
-	}
+	callback = dpm_subsys_resume_noirq_cb(dev, state, &info);
 
 	if (!callback && dev->driver && dev->driver->pm) {
 		info = "noirq driver ";
@@ -705,6 +722,35 @@ void dpm_resume_noirq(pm_message_t state
 	dpm_noirq_end();
 }
 
+static pm_callback_t dpm_subsys_resume_early_cb(struct device *dev,
+						pm_message_t state,
+						const char **info_p)
+{
+	pm_callback_t callback;
+	const char *info;
+
+	if (dev->pm_domain) {
+		info = "early power domain ";
+		callback = pm_late_early_op(&dev->pm_domain->ops, state);
+	} else if (dev->type && dev->type->pm) {
+		info = "early type ";
+		callback = pm_late_early_op(dev->type->pm, state);
+	} else if (dev->class && dev->class->pm) {
+		info = "early class ";
+		callback = pm_late_early_op(dev->class->pm, state);
+	} else if (dev->bus && dev->bus->pm) {
+		info = "early bus ";
+		callback = pm_late_early_op(dev->bus->pm, state);
+	} else {
+		return NULL;
+	}
+
+	if (info_p)
+		*info_p = info;
+
+	return callback;
+}
+
 /**
  * device_resume_early - Execute an "early resume" callback for given device.
  * @dev: Device to handle.
@@ -715,8 +761,8 @@ void dpm_resume_noirq(pm_message_t state
  */
 static int device_resume_early(struct device *dev, pm_message_t state, bool async)
 {
-	pm_callback_t callback = NULL;
-	const char *info = NULL;
+	pm_callback_t callback;
+	const char *info;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -730,19 +776,7 @@ static int device_resume_early(struct de
 
 	dpm_wait_for_superior(dev, async);
 
-	if (dev->pm_domain) {
-		info = "early power domain ";
-		callback = pm_late_early_op(&dev->pm_domain->ops, state);
-	} else if (dev->type && dev->type->pm) {
-		info = "early type ";
-		callback = pm_late_early_op(dev->type->pm, state);
-	} else if (dev->class && dev->class->pm) {
-		info = "early class ";
-		callback = pm_late_early_op(dev->class->pm, state);
-	} else if (dev->bus && dev->bus->pm) {
-		info = "early bus ";
-		callback = pm_late_early_op(dev->bus->pm, state);
-	}
+	callback = dpm_subsys_resume_early_cb(dev, state, &info);
 
 	if (!callback && dev->driver && dev->driver->pm) {
 		info = "early driver ";
@@ -1129,6 +1163,35 @@ static void dpm_superior_set_must_resume
 	device_links_read_unlock(idx);
 }
 
+static pm_callback_t dpm_subsys_suspend_noirq_cb(struct device *dev,
+						 pm_message_t state,
+						 const char **info_p)
+{
+	pm_callback_t callback;
+	const char *info;
+
+	if (dev->pm_domain) {
+		info = "noirq power domain ";
+		callback = pm_noirq_op(&dev->pm_domain->ops, state);
+	} else if (dev->type && dev->type->pm) {
+		info = "noirq type ";
+		callback = pm_noirq_op(dev->type->pm, state);
+	} else if (dev->class && dev->class->pm) {
+		info = "noirq class ";
+		callback = pm_noirq_op(dev->class->pm, state);
+	} else if (dev->bus && dev->bus->pm) {
+		info = "noirq bus ";
+		callback = pm_noirq_op(dev->bus->pm, state);
+	} else {
+		return NULL;
+	}
+
+	if (info_p)
+		*info_p = info;
+
+	return callback;
+}
+
 /**
  * __device_suspend_noirq - Execute a "noirq suspend" callback for given device.
  * @dev: Device to handle.
@@ -1140,8 +1203,8 @@ static void dpm_superior_set_must_resume
  */
 static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool async)
 {
-	pm_callback_t callback = NULL;
-	const char *info = NULL;
+	pm_callback_t callback;
+	const char *info;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -1160,19 +1223,7 @@ static int __device_suspend_noirq(struct
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
-	if (dev->pm_domain) {
-		info = "noirq power domain ";
-		callback = pm_noirq_op(&dev->pm_domain->ops, state);
-	} else if (dev->type && dev->type->pm) {
-		info = "noirq type ";
-		callback = pm_noirq_op(dev->type->pm, state);
-	} else if (dev->class && dev->class->pm) {
-		info = "noirq class ";
-		callback = pm_noirq_op(dev->class->pm, state);
-	} else if (dev->bus && dev->bus->pm) {
-		info = "noirq bus ";
-		callback = pm_noirq_op(dev->bus->pm, state);
-	}
+	callback = dpm_subsys_suspend_noirq_cb(dev, state, &info);
 
 	if (!callback && dev->driver && dev->driver->pm) {
 		info = "noirq driver ";
@@ -1307,6 +1358,35 @@ int dpm_suspend_noirq(pm_message_t state
 	return ret;
 }
 
+static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
+						pm_message_t state,
+						const char **info_p)
+{
+	pm_callback_t callback;
+	const char *info;
+
+	if (dev->pm_domain) {
+		info = "late power domain ";
+		callback = pm_late_early_op(&dev->pm_domain->ops, state);
+	} else if (dev->type && dev->type->pm) {
+		info = "late type ";
+		callback = pm_late_early_op(dev->type->pm, state);
+	} else if (dev->class && dev->class->pm) {
+		info = "late class ";
+		callback = pm_late_early_op(dev->class->pm, state);
+	} else if (dev->bus && dev->bus->pm) {
+		info = "late bus ";
+		callback = pm_late_early_op(dev->bus->pm, state);
+	} else {
+		return NULL;
+	}
+
+	if (info_p)
+		*info_p = info;
+
+	return callback;
+}
+
 /**
  * __device_suspend_late - Execute a "late suspend" callback for given device.
  * @dev: Device to handle.
@@ -1317,8 +1397,8 @@ int dpm_suspend_noirq(pm_message_t state
  */
 static int __device_suspend_late(struct device *dev, pm_message_t state, bool async)
 {
-	pm_callback_t callback = NULL;
-	const char *info = NULL;
+	pm_callback_t callback;
+	const char *info;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -1339,19 +1419,7 @@ static int __device_suspend_late(struct
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
-	if (dev->pm_domain) {
-		info = "late power domain ";
-		callback = pm_late_early_op(&dev->pm_domain->ops, state);
-	} else if (dev->type && dev->type->pm) {
-		info = "late type ";
-		callback = pm_late_early_op(dev->type->pm, state);
-	} else if (dev->class && dev->class->pm) {
-		info = "late class ";
-		callback = pm_late_early_op(dev->class->pm, state);
-	} else if (dev->bus && dev->bus->pm) {
-		info = "late bus ";
-		callback = pm_late_early_op(dev->bus->pm, state);
-	}
+	callback = dpm_subsys_suspend_late_cb(dev, state, &info);
 
 	if (!callback && dev->driver && dev->driver->pm) {
 		info = "late driver ";

[PATCH 3/4] PM / core: Direct DPM_FLAG_SMART_SUSPEND optimization

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make the PM core avoid invoking the "late" and "noirq" system-wide
suspend (or analogous) callbacks provided by device drivers directly
for devices with DPM_FLAG_SMART_SUSPEND set that are in runtime
suspend during the "late" and "noirq" phases of system-wide suspend
(or analogous) transitions.  That is only done for devices without
any middle-layer "late" and "noirq" suspend callbacks (to avoid
confusing the middle layer if there is one).

The underlying observation is that runtime PM is disabled for devices
during the "late" and "noirq" system-wide suspend phases, so if they
remain in runtime suspend from the "late" phase forward, it doesn't
make sense to invoke the "late" and "noirq" callbacks provided by
the drivers for them (arguably, the device is already suspended and
in the right state).  Thus, if the remaining driver suspend callbacks
are to be invoked directly by the core, they can be skipped.

This change really makes it possible for, say, platform device
drivers to re-use runtime PM suspend and resume callbacks by
pointing ->suspend_late and ->resume_early, respectively (and
possibly the analogous hibernation-related callback pointers too),
to them without adding any extra "is the device already suspended?"
type of checks to the callback routines, as long as they will be
invoked directly by the core.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/driver-api/pm/devices.rst |   18 +++---
 drivers/base/power/main.c               |   85 +++++++++++++++++++++++++++++---
 2 files changed, 88 insertions(+), 15 deletions(-)

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
@@ -777,14 +777,16 @@ The driver can indicate that by setting
 runtime suspend at the beginning of the ``suspend_late`` phase of system-wide
 suspend (or in the ``poweroff_late`` phase of hibernation), when runtime PM
 has been disabled for it, under the assumption that its state should not change
-after that point until the system-wide transition is over.  If that happens, the
-driver's system-wide resume callbacks, if present, may still be invoked during
-the subsequent system-wide resume transition and the device's runtime power
-management status may be set to "active" before enabling runtime PM for it,
-so the driver must be prepared to cope with the invocation of its system-wide
-resume callbacks back-to-back with its ``->runtime_suspend`` one (without the
-intervening ``->runtime_resume`` and so on) and the final state of the device
-must reflect the "active" status for runtime PM in that case.
+after that point until the system-wide transition is over (the PM core itself
+does that for devices whose "noirq", "late" and "early" system-wide PM callbacks
+are executed directly by it).  If that happens, the driver's system-wide resume
+callbacks, if present, may still be invoked during the subsequent system-wide
+resume transition and the device's runtime power management status may be set
+to "active" before enabling runtime PM for it, so the driver must be prepared to
+cope with the invocation of its system-wide resume callbacks back-to-back with
+its ``->runtime_suspend`` one (without the intervening ``->runtime_resume`` and
+so on) and the final state of the device must reflect the "active" runtime PM
+status in that case.
 
 During system-wide resume from a sleep state it's easiest to put devices into
 the full-power state, as explained in :file:`Documentation/power/runtime_pm.txt`.
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -541,6 +541,24 @@ void dev_pm_skip_next_resume_phases(stru
 }
 
 /**
+ * suspend_event - Return a "suspend" message for given "resume" one.
+ * @resume_msg: PM message representing a system-wide resume transition.
+ */
+static pm_message_t suspend_event(pm_message_t resume_msg)
+{
+	switch (resume_msg.event) {
+	case PM_EVENT_RESUME:
+		return PMSG_SUSPEND;
+	case PM_EVENT_THAW:
+	case PM_EVENT_RESTORE:
+		return PMSG_FREEZE;
+	case PM_EVENT_RECOVER:
+		return PMSG_HIBERNATE;
+	}
+	return PMSG_ON;
+}
+
+/**
  * dev_pm_may_skip_resume - System-wide device resume optimization check.
  * @dev: Target device.
  *
@@ -581,6 +599,14 @@ static pm_callback_t dpm_subsys_resume_n
 	return callback;
 }
 
+static pm_callback_t dpm_subsys_suspend_noirq_cb(struct device *dev,
+						 pm_message_t state,
+						 const char **info_p);
+
+static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
+						pm_message_t state,
+						const char **info_p);
+
 /**
  * device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
@@ -608,13 +634,40 @@ static int device_resume_noirq(struct de
 	dpm_wait_for_superior(dev, async);
 
 	callback = dpm_subsys_resume_noirq_cb(dev, state, &info);
+	if (callback)
+		goto Run;
 
-	if (!callback && dev->driver && dev->driver->pm) {
+	if (dev_pm_smart_suspend_and_suspended(dev)) {
+		pm_message_t suspend_msg = suspend_event(state);
+
+		/*
+		 * If "freeze" callbacks have been skipped during a transition
+		 * related to hibernation, the subsequent "thaw" callbacks must
+		 * be skipped too or bad things may happen.  Otherwise, resume
+		 * callbacks are going to be run for the device, so its runtime
+		 * PM status must be changed to reflect the new state after the
+		 * transition under way.
+		 */
+		if (!dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) &&
+		    !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL)) {
+			if (state.event == PM_EVENT_THAW) {
+				dev_pm_skip_next_resume_phases(dev);
+				goto Skip;
+			} else {
+				pm_runtime_set_active(dev);
+			}
+		}
+	}
+
+	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);
+
+Skip:
 	dev->power.is_noirq_suspended = false;
 
 	if (dev_pm_may_skip_resume(dev)) {
@@ -629,7 +682,7 @@ static int device_resume_noirq(struct de
 		dev_pm_skip_next_resume_phases(dev);
 	}
 
- Out:
+Out:
 	complete_all(&dev->power.completion);
 	TRACE_RESUME(error);
 	return error;
@@ -1224,18 +1277,26 @@ 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) {
+	if (dev_pm_smart_suspend_and_suspended(dev) &&
+	    !dpm_subsys_suspend_late_cb(dev, state, NULL))
+		goto Skip;
+
+	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;
 		goto Complete;
 	}
 
+Skip:
 	dev->power.is_noirq_suspended = true;
 
 	if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) {
@@ -1420,17 +1481,27 @@ static int __device_suspend_late(struct
 		goto Complete;
 
 	callback = dpm_subsys_suspend_late_cb(dev, state, &info);
+	if (callback)
+		goto Run;
 
-	if (!callback && dev->driver && dev->driver->pm) {
+	if (dev_pm_smart_suspend_and_suspended(dev) &&
+	    !dpm_subsys_suspend_noirq_cb(dev, state, NULL))
+		goto Skip;
+
+	if (dev->driver && dev->driver->pm) {
 		info = "late driver ";
 		callback = pm_late_early_op(dev->driver->pm, state);
 	}
 
+Run:
 	error = dpm_run_callback(callback, dev, state, info);
-	if (!error)
-		dev->power.is_late_suspended = true;
-	else
+	if (error) {
 		async_error = error;
+		goto Complete;
+	}
+
+Skip:
+	dev->power.is_late_suspended = true;
 
 Complete:
 	TRACE_SUSPEND(error);

[PATCH 4/4] PM / core: Direct DPM_FLAG_LEAVE_SUSPENDED handling

[Rafael J. Wysocki]

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.

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).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/driver-api/pm/devices.rst |    9 +++++
 drivers/base/power/main.c               |   51 +++++++++++++++++++++++++-------
 2 files changed, 49 insertions(+), 11 deletions(-)

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
@@ -816,3 +816,12 @@ appropriate in its "noirq" resume callba
 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).
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -620,6 +620,7 @@ static int device_resume_noirq(struct de
 {
 	pm_callback_t callback;
 	const char *info;
+	bool skip_resume;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -633,10 +634,15 @@ 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 (dev_pm_smart_suspend_and_suspended(dev)) {
 		pm_message_t suspend_msg = suspend_event(state);
 
@@ -651,7 +657,7 @@ static int device_resume_noirq(struct de
 		if (!dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) &&
 		    !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL)) {
 			if (state.event == PM_EVENT_THAW) {
-				dev_pm_skip_next_resume_phases(dev);
+				skip_resume = true;
 				goto Skip;
 			} else {
 				pm_runtime_set_active(dev);
@@ -670,7 +676,7 @@ Run:
 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
@@ -1245,6 +1251,32 @@ static pm_callback_t dpm_subsys_suspend_
 	return callback;
 }
 
+static bool device_must_resume(struct device *dev, pm_message_t state,
+			       bool no_subsys_suspend_noirq)
+{
+	pm_message_t resume_msg = resume_event(state);
+
+	/*
+	 * 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.
+	 */
+	if (no_subsys_suspend_noirq &&
+	    !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))
+		return !pm_runtime_status_suspended(dev) &&
+			(resume_msg.event != PM_EVENT_RESUME ||
+			 (device_can_wakeup(dev) && !device_may_wakeup(dev)));
+
+	/*
+	 * 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.
+	 */
+	return !dev->power.may_skip_resume;
+}
+
 /**
  * __device_suspend_noirq - Execute a "noirq suspend" callback for given device.
  * @dev: Device to handle.
@@ -1258,6 +1290,7 @@ static int __device_suspend_noirq(struct
 {
 	pm_callback_t callback;
 	const char *info;
+	bool no_subsys_cb = false;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -1280,8 +1313,9 @@ static int __device_suspend_noirq(struct
 	if (callback)
 		goto Run;
 
-	if (dev_pm_smart_suspend_and_suspended(dev) &&
-	    !dpm_subsys_suspend_late_cb(dev, state, NULL))
+	no_subsys_cb = !dpm_subsys_suspend_late_cb(dev, state, NULL);
+
+	if (dev_pm_smart_suspend_and_suspended(dev) && no_subsys_cb)
 		goto Skip;
 
 	if (dev->driver && dev->driver->pm) {
@@ -1300,14 +1334,9 @@ Skip:
 	dev->power.is_noirq_suspended = true;
 
 	if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) {
-		/*
-		 * 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 ||
-					atomic_read(&dev->power.usage_count) > 1;
+				atomic_read(&dev->power.usage_count) > 1 ||
+				device_must_resume(dev, state, no_subsys_cb);
 	} else {
 		dev->power.must_resume = true;
 	}

Re: [PATCH 1/4] PM / core: Use dev_pm_skip_next_resume_phases() internally

[Geert Uytterhoeven]

On Sun, Dec 10, 2017 at 12:56 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Make the PM core call dev_pm_skip_next_resume_phases() to skip the
> "early resume" and "resume" phases of system-wide transitions to the
> working state for a given device instead of clearing the relevant
> status bits for it directly.
>
> No intentional changes in functionality.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] PM / core: Add helpers for subsystem callback selection

[Geert Uytterhoeven]

On Sun, Dec 10, 2017 at 12:58 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Add helper routines to find and return a suitable subsystem callback
> during the "noirq" phases of system suspend/resume (or analogous)
> transitions as well as during the "late" phase of system suspend and
> the "early" phase of system resume (or analogous) transitions.
>
> The helpers will be called from additional sites going forward.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] PM / core: Use dev_pm_skip_next_resume_phases() internally

[Ulf Hansson]

On 10 December 2017 at 00:56, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Make the PM core call dev_pm_skip_next_resume_phases() to skip the
> "early resume" and "resume" phases of system-wide transitions to the
> working state for a given device instead of clearing the relevant
> status bits for it directly.
>
> No intentional changes in functionality.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

[...]

Kind regards
Uffe

Re: [PATCH 3/4] PM / core: Direct DPM_FLAG_SMART_SUSPEND optimization

[Ulf Hansson]

On 10 December 2017 at 01:00, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Make the PM core avoid invoking the "late" and "noirq" system-wide
> suspend (or analogous) callbacks provided by device drivers directly
> for devices with DPM_FLAG_SMART_SUSPEND set that are in runtime
> suspend during the "late" and "noirq" phases of system-wide suspend
> (or analogous) transitions.  That is only done for devices without
> any middle-layer "late" and "noirq" suspend callbacks (to avoid
> confusing the middle layer if there is one).
>
> The underlying observation is that runtime PM is disabled for devices
> during the "late" and "noirq" system-wide suspend phases, so if they
> remain in runtime suspend from the "late" phase forward, it doesn't
> make sense to invoke the "late" and "noirq" callbacks provided by
> the drivers for them (arguably, the device is already suspended and
> in the right state).  Thus, if the remaining driver suspend callbacks
> are to be invoked directly by the core, they can be skipped.
>

As I have stated earlier, this isn't going to solve the general case,
as the above change log seems to state. I think we really need to do
that, before adding yet another system suspend/resume optimization
path in the PM core.

The main reason is that lots of drivers have additional operations to
perform, besides making sure that its device is put into a "runtime
suspended state" during system suspend. In other words, skipping
system suspend callbacks (and likewise for system resume) is to me the
wrong solution to mitigate these problems.

> This change really makes it possible for, say, platform device
> drivers to re-use runtime PM suspend and resume callbacks by
> pointing ->suspend_late and ->resume_early, respectively (and
> possibly the analogous hibernation-related callback pointers too),
> to them without adding any extra "is the device already suspended?"
> type of checks to the callback routines, as long as they will be
> invoked directly by the core.

Certainly there are drivers that could start to deploying this
solution, because at the moment they don't have any additional
operations to perform at system suspend. But what about the rest,
don't we care about them?

Moreover, what happens when/if a driver that has deployed this
solution, starts being used on a new SoC and then additional
operations during system suspend becomes required (for example
pinctrls that needs to be put in a system sleep state)? Then
everything falls apart, doesn't it?

Kind regards
Uffe

>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  Documentation/driver-api/pm/devices.rst |   18 +++---
>  drivers/base/power/main.c               |   85 +++++++++++++++++++++++++++++---
>  2 files changed, 88 insertions(+), 15 deletions(-)
>
> 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
> @@ -777,14 +777,16 @@ The driver can indicate that by setting
>  runtime suspend at the beginning of the ``suspend_late`` phase of system-wide
>  suspend (or in the ``poweroff_late`` phase of hibernation), when runtime PM
>  has been disabled for it, under the assumption that its state should not change
> -after that point until the system-wide transition is over.  If that happens, the
> -driver's system-wide resume callbacks, if present, may still be invoked during
> -the subsequent system-wide resume transition and the device's runtime power
> -management status may be set to "active" before enabling runtime PM for it,
> -so the driver must be prepared to cope with the invocation of its system-wide
> -resume callbacks back-to-back with its ``->runtime_suspend`` one (without the
> -intervening ``->runtime_resume`` and so on) and the final state of the device
> -must reflect the "active" status for runtime PM in that case.
> +after that point until the system-wide transition is over (the PM core itself
> +does that for devices whose "noirq", "late" and "early" system-wide PM callbacks
> +are executed directly by it).  If that happens, the driver's system-wide resume
> +callbacks, if present, may still be invoked during the subsequent system-wide
> +resume transition and the device's runtime power management status may be set
> +to "active" before enabling runtime PM for it, so the driver must be prepared to
> +cope with the invocation of its system-wide resume callbacks back-to-back with
> +its ``->runtime_suspend`` one (without the intervening ``->runtime_resume`` and
> +so on) and the final state of the device must reflect the "active" runtime PM
> +status in that case.
>
>  During system-wide resume from a sleep state it's easiest to put devices into
>  the full-power state, as explained in :file:`Documentation/power/runtime_pm.txt`.
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -541,6 +541,24 @@ void dev_pm_skip_next_resume_phases(stru
>  }
>
>  /**
> + * suspend_event - Return a "suspend" message for given "resume" one.
> + * @resume_msg: PM message representing a system-wide resume transition.
> + */
> +static pm_message_t suspend_event(pm_message_t resume_msg)
> +{
> +       switch (resume_msg.event) {
> +       case PM_EVENT_RESUME:
> +               return PMSG_SUSPEND;
> +       case PM_EVENT_THAW:
> +       case PM_EVENT_RESTORE:
> +               return PMSG_FREEZE;
> +       case PM_EVENT_RECOVER:
> +               return PMSG_HIBERNATE;
> +       }
> +       return PMSG_ON;
> +}
> +
> +/**
>   * dev_pm_may_skip_resume - System-wide device resume optimization check.
>   * @dev: Target device.
>   *
> @@ -581,6 +599,14 @@ static pm_callback_t dpm_subsys_resume_n
>         return callback;
>  }
>
> +static pm_callback_t dpm_subsys_suspend_noirq_cb(struct device *dev,
> +                                                pm_message_t state,
> +                                                const char **info_p);
> +
> +static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
> +                                               pm_message_t state,
> +                                               const char **info_p);
> +
>  /**
>   * device_resume_noirq - Execute a "noirq resume" callback for given device.
>   * @dev: Device to handle.
> @@ -608,13 +634,40 @@ static int device_resume_noirq(struct de
>         dpm_wait_for_superior(dev, async);
>
>         callback = dpm_subsys_resume_noirq_cb(dev, state, &info);
> +       if (callback)
> +               goto Run;
>
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       if (dev_pm_smart_suspend_and_suspended(dev)) {
> +               pm_message_t suspend_msg = suspend_event(state);
> +
> +               /*
> +                * If "freeze" callbacks have been skipped during a transition
> +                * related to hibernation, the subsequent "thaw" callbacks must
> +                * be skipped too or bad things may happen.  Otherwise, resume
> +                * callbacks are going to be run for the device, so its runtime
> +                * PM status must be changed to reflect the new state after the
> +                * transition under way.
> +                */
> +               if (!dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) &&
> +                   !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL)) {
> +                       if (state.event == PM_EVENT_THAW) {
> +                               dev_pm_skip_next_resume_phases(dev);
> +                               goto Skip;
> +                       } else {
> +                               pm_runtime_set_active(dev);
> +                       }
> +               }
> +       }
> +
> +       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);
> +
> +Skip:
>         dev->power.is_noirq_suspended = false;
>
>         if (dev_pm_may_skip_resume(dev)) {
> @@ -629,7 +682,7 @@ static int device_resume_noirq(struct de
>                 dev_pm_skip_next_resume_phases(dev);
>         }
>
> - Out:
> +Out:
>         complete_all(&dev->power.completion);
>         TRACE_RESUME(error);
>         return error;
> @@ -1224,18 +1277,26 @@ 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) {
> +       if (dev_pm_smart_suspend_and_suspended(dev) &&
> +           !dpm_subsys_suspend_late_cb(dev, state, NULL))
> +               goto Skip;
> +
> +       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;
>                 goto Complete;
>         }
>
> +Skip:
>         dev->power.is_noirq_suspended = true;
>
>         if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) {
> @@ -1420,17 +1481,27 @@ static int __device_suspend_late(struct
>                 goto Complete;
>
>         callback = dpm_subsys_suspend_late_cb(dev, state, &info);
> +       if (callback)
> +               goto Run;
>
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       if (dev_pm_smart_suspend_and_suspended(dev) &&
> +           !dpm_subsys_suspend_noirq_cb(dev, state, NULL))
> +               goto Skip;
> +
> +       if (dev->driver && dev->driver->pm) {
>                 info = "late driver ";
>                 callback = pm_late_early_op(dev->driver->pm, state);
>         }
>
> +Run:
>         error = dpm_run_callback(callback, dev, state, info);
> -       if (!error)
> -               dev->power.is_late_suspended = true;
> -       else
> +       if (error) {
>                 async_error = error;
> +               goto Complete;
> +       }
> +
> +Skip:
> +       dev->power.is_late_suspended = true;
>
>  Complete:
>         TRACE_SUSPEND(error);
>

Re: [PATCH 3/4] PM / core: Direct DPM_FLAG_SMART_SUSPEND optimization

[Rafael J. Wysocki]

On Tue, Dec 19, 2017 at 8:38 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 10 December 2017 at 01:00, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Make the PM core avoid invoking the "late" and "noirq" system-wide
>> suspend (or analogous) callbacks provided by device drivers directly
>> for devices with DPM_FLAG_SMART_SUSPEND set that are in runtime
>> suspend during the "late" and "noirq" phases of system-wide suspend
>> (or analogous) transitions.  That is only done for devices without
>> any middle-layer "late" and "noirq" suspend callbacks (to avoid
>> confusing the middle layer if there is one).
>>
>> The underlying observation is that runtime PM is disabled for devices
>> during the "late" and "noirq" system-wide suspend phases, so if they
>> remain in runtime suspend from the "late" phase forward, it doesn't
>> make sense to invoke the "late" and "noirq" callbacks provided by
>> the drivers for them (arguably, the device is already suspended and
>> in the right state).  Thus, if the remaining driver suspend callbacks
>> are to be invoked directly by the core, they can be skipped.
>>

It looks like I'm consistently failing to explain my point clearly enough. :-)

> As I have stated earlier, this isn't going to solve the general case,
> as the above change log seems to state.

Well, it doesn't say that or anything similar, at least to my eyes.

The observation is that if you have set DPM_FLAG_SMART_SUSPEND, then
you need to be prepared for your ->suspend_late and ->suspend_noirq to
be skipped (because the ACPI PM domain does that and you may happen to
work with it, for example) if the device is already suspended at the
beginning of the "late suspend" phase.  That's already documented.

Given the above, and the fact that there is not much to be done for a
suspended device in ->suspend_late and ->suspend_noirq, why can't the
core skip these callbacks too if there's no middle layer?

But the reason why I really need this is because
i2c-designware-platdrv can work both with the ACPI PM domain and
standalone and I need it to be handled consistently in both cases.

> I think we really need to do that, before adding yet another system suspend/resume optimization
> path in the PM core.

So what exactly is the technical argument in the above?

> The main reason is that lots of drivers have additional operations to
> perform, besides making sure that its device is put into a "runtime
> suspended state" during system suspend. In other words, skipping
> system suspend callbacks (and likewise for system resume) is to me the
> wrong solution to mitigate these problems.
>
>> This change really makes it possible for, say, platform device
>> drivers to re-use runtime PM suspend and resume callbacks by
>> pointing ->suspend_late and ->resume_early, respectively (and
>> possibly the analogous hibernation-related callback pointers too),
>> to them without adding any extra "is the device already suspended?"
>> type of checks to the callback routines, as long as they will be
>> invoked directly by the core.
>
> Certainly there are drivers that could start to deploying this
> solution, because at the moment they don't have any additional
> operations to perform at system suspend. But what about the rest,
> don't we care about them?

We do, somewhat. :-)

> Moreover, what happens when/if a driver that has deployed this
> solution, starts being used on a new SoC and then additional
> operations during system suspend becomes required (for example
> pinctrls that needs to be put in a system sleep state)? Then
> everything falls apart, doesn't it?

Then you runtime-resume the device in ->suspend() and the remaining
callbacks will run for you just fine.

And IMO running "late" and "noirq" system suspend callbacks on a
suspended device is super-fragile anyway as they generally need to
distinguish between the "suspended" and "not suspended" cases
consistently and what they do may affect the children or the parent of
the device in ways that are difficult to predict in general.  So, I'd
rather not do that in any case.

Thanks,
Rafael

Re: [PATCH 3/4] PM / core: Direct DPM_FLAG_SMART_SUSPEND optimization

[Rafael J. Wysocki]

On Tue, Dec 19, 2017 at 12:13 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Dec 19, 2017 at 8:38 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 10 December 2017 at 01:00, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>

[cut]

>
>> Moreover, what happens when/if a driver that has deployed this
>> solution, starts being used on a new SoC and then additional
>> operations during system suspend becomes required (for example
>> pinctrls that needs to be put in a system sleep state)? Then
>> everything falls apart, doesn't it?
>
> Then you runtime-resume the device in ->suspend() and the remaining
> callbacks will run for you just fine.

BTW, I guess you'll need a middle layer in that case anyway so that
the driver doesn't have to distinguish between platforms it has to
work with which makes the argument somewhat moot.

Thanks,
Rafael

Re: [PATCH 3/4] PM / core: Direct DPM_FLAG_SMART_SUSPEND optimization

[Ulf Hansson]

On 19 December 2017 at 12:13, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Dec 19, 2017 at 8:38 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 10 December 2017 at 01:00, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Make the PM core avoid invoking the "late" and "noirq" system-wide
>>> suspend (or analogous) callbacks provided by device drivers directly
>>> for devices with DPM_FLAG_SMART_SUSPEND set that are in runtime
>>> suspend during the "late" and "noirq" phases of system-wide suspend
>>> (or analogous) transitions.  That is only done for devices without
>>> any middle-layer "late" and "noirq" suspend callbacks (to avoid
>>> confusing the middle layer if there is one).
>>>
>>> The underlying observation is that runtime PM is disabled for devices
>>> during the "late" and "noirq" system-wide suspend phases, so if they
>>> remain in runtime suspend from the "late" phase forward, it doesn't
>>> make sense to invoke the "late" and "noirq" callbacks provided by
>>> the drivers for them (arguably, the device is already suspended and
>>> in the right state).  Thus, if the remaining driver suspend callbacks
>>> are to be invoked directly by the core, they can be skipped.
>>>
>
> It looks like I'm consistently failing to explain my point clearly enough. :-)
>
>> As I have stated earlier, this isn't going to solve the general case,
>> as the above change log seems to state.
>
> Well, it doesn't say that or anything similar, at least to my eyes.
>
> The observation is that if you have set DPM_FLAG_SMART_SUSPEND, then
> you need to be prepared for your ->suspend_late and ->suspend_noirq to
> be skipped (because the ACPI PM domain does that and you may happen to
> work with it, for example) if the device is already suspended at the
> beginning of the "late suspend" phase.  That's already documented.
>
> Given the above, and the fact that there is not much to be done for a
> suspended device in ->suspend_late and ->suspend_noirq, why can't the
> core skip these callbacks too if there's no middle layer?
>
> But the reason why I really need this is because
> i2c-designware-platdrv can work both with the ACPI PM domain and
> standalone and I need it to be handled consistently in both cases.

Yeah, I understand that.

>
>> I think we really need to do that, before adding yet another system suspend/resume optimization
>> path in the PM core.
>
> So what exactly is the technical argument in the above?

As stated, when the driver needs additional operations to be done, it
all falls a part.

>
>> The main reason is that lots of drivers have additional operations to
>> perform, besides making sure that its device is put into a "runtime
>> suspended state" during system suspend. In other words, skipping
>> system suspend callbacks (and likewise for system resume) is to me the
>> wrong solution to mitigate these problems.
>>
>>> This change really makes it possible for, say, platform device
>>> drivers to re-use runtime PM suspend and resume callbacks by
>>> pointing ->suspend_late and ->resume_early, respectively (and
>>> possibly the analogous hibernation-related callback pointers too),
>>> to them without adding any extra "is the device already suspended?"
>>> type of checks to the callback routines, as long as they will be
>>> invoked directly by the core.
>>
>> Certainly there are drivers that could start to deploying this
>> solution, because at the moment they don't have any additional
>> operations to perform at system suspend. But what about the rest,
>> don't we care about them?
>
> We do, somewhat. :-)
>
>> Moreover, what happens when/if a driver that has deployed this
>> solution, starts being used on a new SoC and then additional
>> operations during system suspend becomes required (for example
>> pinctrls that needs to be put in a system sleep state)? Then
>> everything falls apart, doesn't it?
>
> Then you runtime-resume the device in ->suspend() and the remaining
> callbacks will run for you just fine.

Well, this proves my concern.

Even if the driver has additional operations to perform, why should it
have to runtime resume its device to have the callbacks to be invoked?
That may be a waste in both time and power.

No, this isn't good enough, sorry.

>
> And IMO running "late" and "noirq" system suspend callbacks on a
> suspended device is super-fragile anyway as they generally need to
> distinguish between the "suspended" and "not suspended" cases
> consistently and what they do may affect the children or the parent of
> the device in ways that are difficult to predict in general.  So, I'd
> rather not do that in any case.

That may be the case for the ACPI PM domain, but I don't see an issue
when devices are being attached to a more trivial middle layer/PM
domain.

Kind regards
Uffe

Re: [PATCH 3/4] PM / core: Direct DPM_FLAG_SMART_SUSPEND optimization

[Ulf Hansson]

On 19 December 2017 at 12:19, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Dec 19, 2017 at 12:13 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Tue, Dec 19, 2017 at 8:38 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 10 December 2017 at 01:00, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>
> [cut]
>
>>
>>> Moreover, what happens when/if a driver that has deployed this
>>> solution, starts being used on a new SoC and then additional
>>> operations during system suspend becomes required (for example
>>> pinctrls that needs to be put in a system sleep state)? Then
>>> everything falls apart, doesn't it?
>>
>> Then you runtime-resume the device in ->suspend() and the remaining
>> callbacks will run for you just fine.
>
> BTW, I guess you'll need a middle layer in that case anyway so that
> the driver doesn't have to distinguish between platforms it has to
> work with which makes the argument somewhat moot.

No, this isn't the case. Let's take the pinctrl as an example.

The pinctrl system sleep state could be optionally defined in the DT,
depending on the platform.

All the driver shall do, is to try to set the state, if it has been defined.

Kind regards
Uffe

Re: [PATCH 3/4] PM / core: Direct DPM_FLAG_SMART_SUSPEND optimization

[Rafael J. Wysocki]

On Tue, Dec 19, 2017 at 2:10 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 19 December 2017 at 12:13, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Tue, Dec 19, 2017 at 8:38 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 10 December 2017 at 01:00, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> Make the PM core avoid invoking the "late" and "noirq" system-wide
>>>> suspend (or analogous) callbacks provided by device drivers directly
>>>> for devices with DPM_FLAG_SMART_SUSPEND set that are in runtime
>>>> suspend during the "late" and "noirq" phases of system-wide suspend
>>>> (or analogous) transitions.  That is only done for devices without
>>>> any middle-layer "late" and "noirq" suspend callbacks (to avoid
>>>> confusing the middle layer if there is one).
>>>>
>>>> The underlying observation is that runtime PM is disabled for devices
>>>> during the "late" and "noirq" system-wide suspend phases, so if they
>>>> remain in runtime suspend from the "late" phase forward, it doesn't
>>>> make sense to invoke the "late" and "noirq" callbacks provided by
>>>> the drivers for them (arguably, the device is already suspended and
>>>> in the right state).  Thus, if the remaining driver suspend callbacks
>>>> are to be invoked directly by the core, they can be skipped.
>>>>
>>
>> It looks like I'm consistently failing to explain my point clearly enough. :-)
>>
>>> As I have stated earlier, this isn't going to solve the general case,
>>> as the above change log seems to state.

No, it doesn't, as long as drivers follow the documentation.

So your concern seems to be "What if I don't follow the
documentation?" which, honestly, is not something I can address. :-)

>> Well, it doesn't say that or anything similar, at least to my eyes.
>>
>> The observation is that if you have set DPM_FLAG_SMART_SUSPEND, then
>> you need to be prepared for your ->suspend_late and ->suspend_noirq to
>> be skipped (because the ACPI PM domain does that and you may happen to
>> work with it, for example) if the device is already suspended at the
>> beginning of the "late suspend" phase.  That's already documented.
>>
>> Given the above, and the fact that there is not much to be done for a
>> suspended device in ->suspend_late and ->suspend_noirq, why can't the
>> core skip these callbacks too if there's no middle layer?
>>
>> But the reason why I really need this is because
>> i2c-designware-platdrv can work both with the ACPI PM domain and
>> standalone and I need it to be handled consistently in both cases.
>
> Yeah, I understand that.
>
>>
>>> I think we really need to do that, before adding yet another system suspend/resume optimization
>>> path in the PM core.
>>
>> So what exactly is the technical argument in the above?
>
> As stated, when the driver needs additional operations to be done, it
> all falls a part.

If the driver *needs* such operations to be done, then it *should*
*not* set DPM_FLAG_SMART_SUSPEND as per the existing documentation.

>
>>
>>> The main reason is that lots of drivers have additional operations to
>>> perform, besides making sure that its device is put into a "runtime
>>> suspended state" during system suspend. In other words, skipping
>>> system suspend callbacks (and likewise for system resume) is to me the
>>> wrong solution to mitigate these problems.
>>>
>>>> This change really makes it possible for, say, platform device
>>>> drivers to re-use runtime PM suspend and resume callbacks by
>>>> pointing ->suspend_late and ->resume_early, respectively (and
>>>> possibly the analogous hibernation-related callback pointers too),
>>>> to them without adding any extra "is the device already suspended?"
>>>> type of checks to the callback routines, as long as they will be
>>>> invoked directly by the core.
>>>
>>> Certainly there are drivers that could start to deploying this
>>> solution, because at the moment they don't have any additional
>>> operations to perform at system suspend. But what about the rest,
>>> don't we care about them?
>>
>> We do, somewhat. :-)
>>
>>> Moreover, what happens when/if a driver that has deployed this
>>> solution, starts being used on a new SoC and then additional
>>> operations during system suspend becomes required (for example
>>> pinctrls that needs to be put in a system sleep state)? Then
>>> everything falls apart, doesn't it?
>>
>> Then you runtime-resume the device in ->suspend() and the remaining
>> callbacks will run for you just fine.
>
> Well, this proves my concern.
>
> Even if the driver has additional operations to perform, why should it
> have to runtime resume its device to have the callbacks to be invoked?
> That may be a waste in both time and power.
>
> No, this isn't good enough, sorry.

What isn't good enough for what?

>>
>> And IMO running "late" and "noirq" system suspend callbacks on a
>> suspended device is super-fragile anyway as they generally need to
>> distinguish between the "suspended" and "not suspended" cases
>> consistently and what they do may affect the children or the parent of
>> the device in ways that are difficult to predict in general.  So, I'd
>> rather not do that in any case.
>
> That may be the case for the ACPI PM domain, but I don't see an issue
> when devices are being attached to a more trivial middle layer/PM
> domain.

There always is a problem.

Yes, you can ignore it, but it doesn't make it automatically go away.

Also, what forces you to set DPM_FLAG_SMART_SUSPEND anyway?

Thanks,
Rafael

Re: [PATCH 3/4] PM / core: Direct DPM_FLAG_SMART_SUSPEND optimization

[Rafael J. Wysocki]

On Tue, Dec 19, 2017 at 2:15 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 19 December 2017 at 12:19, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Tue, Dec 19, 2017 at 12:13 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Tue, Dec 19, 2017 at 8:38 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 10 December 2017 at 01:00, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>
>> [cut]
>>
>>>
>>>> Moreover, what happens when/if a driver that has deployed this
>>>> solution, starts being used on a new SoC and then additional
>>>> operations during system suspend becomes required (for example
>>>> pinctrls that needs to be put in a system sleep state)? Then
>>>> everything falls apart, doesn't it?
>>>
>>> Then you runtime-resume the device in ->suspend() and the remaining
>>> callbacks will run for you just fine.
>>
>> BTW, I guess you'll need a middle layer in that case anyway so that
>> the driver doesn't have to distinguish between platforms it has to
>> work with which makes the argument somewhat moot.
>
> No, this isn't the case. Let's take the pinctrl as an example.
>
> The pinctrl system sleep state could be optionally defined in the DT,
> depending on the platform.
>
> All the driver shall do, is to try to set the state, if it has been defined.

And how exactly does the driver know what state should be set during
system suspend, say?

Anyway, though, as I said, nothing forces you to set
DPM_FLAG_SMART_SUSPEND.  Don't set it if it is not suitable for you
and no one else is going to be affected.

Thanks,
Rafael

Re: [PATCH 3/4] PM / core: Direct DPM_FLAG_SMART_SUSPEND optimization

[Rafael J. Wysocki]

On Tue, Dec 19, 2017 at 5:29 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Dec 19, 2017 at 2:10 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 19 December 2017 at 12:13, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Tue, Dec 19, 2017 at 8:38 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 10 December 2017 at 01:00, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>>>> Make the PM core avoid invoking the "late" and "noirq" system-wide
>>>>> suspend (or analogous) callbacks provided by device drivers directly
>>>>> for devices with DPM_FLAG_SMART_SUSPEND set that are in runtime
>>>>> suspend during the "late" and "noirq" phases of system-wide suspend
>>>>> (or analogous) transitions.  That is only done for devices without
>>>>> any middle-layer "late" and "noirq" suspend callbacks (to avoid
>>>>> confusing the middle layer if there is one).
>>>>>
>>>>> The underlying observation is that runtime PM is disabled for devices
>>>>> during the "late" and "noirq" system-wide suspend phases, so if they
>>>>> remain in runtime suspend from the "late" phase forward, it doesn't
>>>>> make sense to invoke the "late" and "noirq" callbacks provided by
>>>>> the drivers for them (arguably, the device is already suspended and
>>>>> in the right state).  Thus, if the remaining driver suspend callbacks
>>>>> are to be invoked directly by the core, they can be skipped.
>>>>>
>>>
>>> It looks like I'm consistently failing to explain my point clearly enough. :-)
>>>
>>>> As I have stated earlier, this isn't going to solve the general case,
>>>> as the above change log seems to state.
>
> No, it doesn't, as long as drivers follow the documentation.
>
> So your concern seems to be "What if I don't follow the
> documentation?" which, honestly, is not something I can address. :-)

I'm not sure why this ended up in this particular place ...

I must have messed up something.

>>> Well, it doesn't say that or anything similar, at least to my eyes.
>>>
>>> The observation is that if you have set DPM_FLAG_SMART_SUSPEND, then
>>> you need to be prepared for your ->suspend_late and ->suspend_noirq to
>>> be skipped (because the ACPI PM domain does that and you may happen to
>>> work with it, for example) if the device is already suspended at the
>>> beginning of the "late suspend" phase.  That's already documented.
>>>
>>> Given the above, and the fact that there is not much to be done for a
>>> suspended device in ->suspend_late and ->suspend_noirq, why can't the
>>> core skip these callbacks too if there's no middle layer?
>>>
>>> But the reason why I really need this is because
>>> i2c-designware-platdrv can work both with the ACPI PM domain and
>>> standalone and I need it to be handled consistently in both cases.
>>
>> Yeah, I understand that.
>>
>>>
>>>> I think we really need to do that, before adding yet another system suspend/resume optimization
>>>> path in the PM core.
>>>
>>> So what exactly is the technical argument in the above?
>>
>> As stated, when the driver needs additional operations to be done, it
>> all falls a part.

So I wanted to say the above thing here:

+ No, it doesn't, as long as drivers follow the documentation.
+
+ So your concern seems to be "What if I don't follow the
+ documentation?" which, honestly, is not something I can address. :-)

> If the driver *needs* such operations to be done, then it *should*
> *not* set DPM_FLAG_SMART_SUSPEND as per the existing documentation.

Thanks,
Rafael

Re: [PATCH 0/4] PM / core: Direct handling of DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED

[Rafael J. Wysocki]

On Sunday, December 10, 2017 12:55:23 AM CET Rafael J. Wysocki wrote:
> Hi All,
> 
> This series is a follow-up for
> 
> https://marc.info/?l=linux-doc&m=151101644105835&w=2
> 
> Patches[1-3/6] from the above have been reviewed and agreed on, so
> they are in linux-next now and here's a next version of the rest.
> 
> Patches [1-2/4] are preparatory.  The first one is just really small
> code duplication avoidance on top of this recent fix:
> 
> https://patchwork.kernel.org/patch/10097563/
> 
> and the second one simply moves some code to separate functions.
> 
> Patch [3/4] causes the PM core to carry out some optimizations for
> drivers of devices with DPM_FLAG_SMART_SUSPEND set whose "late"
> and "noirq" suspend (or equivalent) driver callbacks are invoked
> directly by the core.
> 
> The underlying observation is that if the device is suspended (via
> runtime PM) during the "late suspend" phase of a system transition,
> invoking the "late" and "noirq" callbacks from the driver for it is not
> going to make it more suspended, so to speak, so it doesn't make sense to
> invoke them at all.
> 
> [That optimization is only done for devices with DPM_FLAG_SMART_SUSPEND
> set, because drivers setting that flag are expected to be prepared for
> skipping their "late" and "noirq" callbacks if the device is already
> suspended.]
> 
> Patch [4/4] makes the core do an analogous thing for devices with
> DPM_FLAG_LEAVE_SUSPENDED set whose "noirq" and "early" resume (or
> equivalent) driver callbacks are directly invoked by the core.
> 
> In that case the observation is that if such devices can be left in
> suspend after the system transition to the working state, running
> resume callbacks from their drivers is simply not necessary.
> 
> Pathes [3-4/4] have been reoredered and reworked a bit since the last
> iteration, so they are regarded as new.
> 
> The series is on top of the linux-next branch of the linux-pm.git tree
> that should be merged into linux-next on Monday.
> 
> [I have developed debug bus type and driver modules to test that code,
> but they are not ready to be made available at this point.]

While I acknowledge that Ulf doesn't appear to be convinced by my
arguments, I also see no technical reason why this cannot go in.

As I said during the discussion, I have tested it and it works for me
as expected.  I also need it to make progress on the drivers front.

Moreover, it should not matter for any drivers that don't set the flags
in question, so the optimizations introduced here are super-easy to avoid
by leaving those flags unset.

So I'm going to apply the series.

Greg, please let me know if you have objections.

Thanks,
Rafael

Re: [PATCH 0/4] PM / core: Direct handling of DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED

[Ulf Hansson]

On 2 January 2018 at 12:32, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Sunday, December 10, 2017 12:55:23 AM CET Rafael J. Wysocki wrote:
>> Hi All,
>>
>> This series is a follow-up for
>>
>> https://marc.info/?l=linux-doc&m=151101644105835&w=2
>>
>> Patches[1-3/6] from the above have been reviewed and agreed on, so
>> they are in linux-next now and here's a next version of the rest.
>>
>> Patches [1-2/4] are preparatory.  The first one is just really small
>> code duplication avoidance on top of this recent fix:
>>
>> https://patchwork.kernel.org/patch/10097563/
>>
>> and the second one simply moves some code to separate functions.
>>
>> Patch [3/4] causes the PM core to carry out some optimizations for
>> drivers of devices with DPM_FLAG_SMART_SUSPEND set whose "late"
>> and "noirq" suspend (or equivalent) driver callbacks are invoked
>> directly by the core.
>>
>> The underlying observation is that if the device is suspended (via
>> runtime PM) during the "late suspend" phase of a system transition,
>> invoking the "late" and "noirq" callbacks from the driver for it is not
>> going to make it more suspended, so to speak, so it doesn't make sense to
>> invoke them at all.
>>
>> [That optimization is only done for devices with DPM_FLAG_SMART_SUSPEND
>> set, because drivers setting that flag are expected to be prepared for
>> skipping their "late" and "noirq" callbacks if the device is already
>> suspended.]
>>
>> Patch [4/4] makes the core do an analogous thing for devices with
>> DPM_FLAG_LEAVE_SUSPENDED set whose "noirq" and "early" resume (or
>> equivalent) driver callbacks are directly invoked by the core.
>>
>> In that case the observation is that if such devices can be left in
>> suspend after the system transition to the working state, running
>> resume callbacks from their drivers is simply not necessary.
>>
>> Pathes [3-4/4] have been reoredered and reworked a bit since the last
>> iteration, so they are regarded as new.
>>
>> The series is on top of the linux-next branch of the linux-pm.git tree
>> that should be merged into linux-next on Monday.
>>
>> [I have developed debug bus type and driver modules to test that code,
>> but they are not ready to be made available at this point.]
>
> While I acknowledge that Ulf doesn't appear to be convinced by my
> arguments, I also see no technical reason why this cannot go in.

Correct, I am not convinced this is the right path as a general
optimization, at least in it's current form. The main argument is
about skipping invoking callbacks, as I have stated.

Moreover, I think we are lacking important input from some more
experienced PM core code contributors, like Alan, Kevin etc. If any of
those guys would give an ack, that would also make me more comfortable
with this.

On the other hand, I realize that we can't wait forever for that to happen.

>
> As I said during the discussion, I have tested it and it works for me
> as expected.  I also need it to make progress on the drivers front.
>
> Moreover, it should not matter for any drivers that don't set the flags
> in question, so the optimizations introduced here are super-easy to avoid
> by leaving those flags unset.

What prevents you from folding in some changes to a few drivers as
apart of the $subject series?

I have asked for that, as to get a better picture of how this is going
to work in the end.

[...]

Kind regards
Uffe

Re: [PATCH 0/4] PM / core: Direct handling of DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED

[Rafael J. Wysocki]

On Tue, Jan 2, 2018 at 1:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 2 January 2018 at 12:32, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Sunday, December 10, 2017 12:55:23 AM CET Rafael J. Wysocki wrote:
>>> Hi All,
>>>
>>> This series is a follow-up for
>>>
>>> https://marc.info/?l=linux-doc&m=151101644105835&w=2
>>>
>>> Patches[1-3/6] from the above have been reviewed and agreed on, so
>>> they are in linux-next now and here's a next version of the rest.
>>>
>>> Patches [1-2/4] are preparatory.  The first one is just really small
>>> code duplication avoidance on top of this recent fix:
>>>
>>> https://patchwork.kernel.org/patch/10097563/
>>>
>>> and the second one simply moves some code to separate functions.
>>>
>>> Patch [3/4] causes the PM core to carry out some optimizations for
>>> drivers of devices with DPM_FLAG_SMART_SUSPEND set whose "late"
>>> and "noirq" suspend (or equivalent) driver callbacks are invoked
>>> directly by the core.
>>>
>>> The underlying observation is that if the device is suspended (via
>>> runtime PM) during the "late suspend" phase of a system transition,
>>> invoking the "late" and "noirq" callbacks from the driver for it is not
>>> going to make it more suspended, so to speak, so it doesn't make sense to
>>> invoke them at all.
>>>
>>> [That optimization is only done for devices with DPM_FLAG_SMART_SUSPEND
>>> set, because drivers setting that flag are expected to be prepared for
>>> skipping their "late" and "noirq" callbacks if the device is already
>>> suspended.]
>>>
>>> Patch [4/4] makes the core do an analogous thing for devices with
>>> DPM_FLAG_LEAVE_SUSPENDED set whose "noirq" and "early" resume (or
>>> equivalent) driver callbacks are directly invoked by the core.
>>>
>>> In that case the observation is that if such devices can be left in
>>> suspend after the system transition to the working state, running
>>> resume callbacks from their drivers is simply not necessary.
>>>
>>> Pathes [3-4/4] have been reoredered and reworked a bit since the last
>>> iteration, so they are regarded as new.
>>>
>>> The series is on top of the linux-next branch of the linux-pm.git tree
>>> that should be merged into linux-next on Monday.
>>>
>>> [I have developed debug bus type and driver modules to test that code,
>>> but they are not ready to be made available at this point.]
>>
>> While I acknowledge that Ulf doesn't appear to be convinced by my
>> arguments, I also see no technical reason why this cannot go in.
>
> Correct, I am not convinced this is the right path as a general
> optimization, at least in it's current form. The main argument is
> about skipping invoking callbacks, as I have stated.
>
> Moreover, I think we are lacking important input from some more
> experienced PM core code contributors, like Alan, Kevin etc. If any of
> those guys would give an ack, that would also make me more comfortable
> with this.
>
> On the other hand, I realize that we can't wait forever for that to happen.
>
>>
>> As I said during the discussion, I have tested it and it works for me
>> as expected.  I also need it to make progress on the drivers front.
>>
>> Moreover, it should not matter for any drivers that don't set the flags
>> in question, so the optimizations introduced here are super-easy to avoid
>> by leaving those flags unset.
>
> What prevents you from folding in some changes to a few drivers as
> apart of the $subject series?
>
> I have asked for that, as to get a better picture of how this is going
> to work in the end.

I can repost this along with the driver changes, but I don't think
that will make much of a difference honestly and I really don't want
to defer this series any more, so I will do as follows.

Consider this series as queued up unless Greg speaks up and I will
post patches [2-4/4] (the first one is in linux-next already) again
with the drivers stuff later today.

Thanks,
Rafael

Re: [PATCH 0/4] PM / core: Direct handling of DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED

[Greg Kroah-Hartman]

On Tue, Jan 02, 2018 at 12:32:55PM +0100, Rafael J. Wysocki wrote:
> On Sunday, December 10, 2017 12:55:23 AM CET Rafael J. Wysocki wrote:
> > Hi All,
> > 
> > This series is a follow-up for
> > 
> > https://marc.info/?l=linux-doc&m=151101644105835&w=2
> > 
> > Patches[1-3/6] from the above have been reviewed and agreed on, so
> > they are in linux-next now and here's a next version of the rest.
> > 
> > Patches [1-2/4] are preparatory.  The first one is just really small
> > code duplication avoidance on top of this recent fix:
> > 
> > https://patchwork.kernel.org/patch/10097563/
> > 
> > and the second one simply moves some code to separate functions.
> > 
> > Patch [3/4] causes the PM core to carry out some optimizations for
> > drivers of devices with DPM_FLAG_SMART_SUSPEND set whose "late"
> > and "noirq" suspend (or equivalent) driver callbacks are invoked
> > directly by the core.
> > 
> > The underlying observation is that if the device is suspended (via
> > runtime PM) during the "late suspend" phase of a system transition,
> > invoking the "late" and "noirq" callbacks from the driver for it is not
> > going to make it more suspended, so to speak, so it doesn't make sense to
> > invoke them at all.
> > 
> > [That optimization is only done for devices with DPM_FLAG_SMART_SUSPEND
> > set, because drivers setting that flag are expected to be prepared for
> > skipping their "late" and "noirq" callbacks if the device is already
> > suspended.]
> > 
> > Patch [4/4] makes the core do an analogous thing for devices with
> > DPM_FLAG_LEAVE_SUSPENDED set whose "noirq" and "early" resume (or
> > equivalent) driver callbacks are directly invoked by the core.
> > 
> > In that case the observation is that if such devices can be left in
> > suspend after the system transition to the working state, running
> > resume callbacks from their drivers is simply not necessary.
> > 
> > Pathes [3-4/4] have been reoredered and reworked a bit since the last
> > iteration, so they are regarded as new.
> > 
> > The series is on top of the linux-next branch of the linux-pm.git tree
> > that should be merged into linux-next on Monday.
> > 
> > [I have developed debug bus type and driver modules to test that code,
> > but they are not ready to be made available at this point.]
> 
> While I acknowledge that Ulf doesn't appear to be convinced by my
> arguments, I also see no technical reason why this cannot go in.
> 
> As I said during the discussion, I have tested it and it works for me
> as expected.  I also need it to make progress on the drivers front.
> 
> Moreover, it should not matter for any drivers that don't set the flags
> in question, so the optimizations introduced here are super-easy to avoid
> by leaving those flags unset.
> 
> So I'm going to apply the series.
> 
> Greg, please let me know if you have objections.

No objection from me at all, please apply.

thanks,

greg k-h

Re: [PATCH 0/4] PM / core: Direct handling of DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED

[Alan Stern]

On Tue, 2 Jan 2018, Ulf Hansson wrote:

> > While I acknowledge that Ulf doesn't appear to be convinced by my
> > arguments, I also see no technical reason why this cannot go in.
> 
> Correct, I am not convinced this is the right path as a general
> optimization, at least in it's current form. The main argument is
> about skipping invoking callbacks, as I have stated.
> 
> Moreover, I think we are lacking important input from some more
> experienced PM core code contributors, like Alan, Kevin etc. If any of
> those guys would give an ack, that would also make me more comfortable
> with this.
> 
> On the other hand, I realize that we can't wait forever for that to happen.

Sad to say, I have not been paying any significant attention to this 
patch series.  The major points I have gleaned are that these are all 
relatively small optimizations, and that by default the patches do not 
change existing behavior (they are opt-in).

So at first glance there's no pressing reason not to apply them.  But 
admittedly, this viewpoint ignores the larger picture.

Alan Stern

[PATCH 0/7] PM / core: Direct handling of DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED

[Rafael J. Wysocki]

On Sunday, December 10, 2017 12:55:23 AM CET Rafael J. Wysocki wrote:
> Hi All,
> 
> This series is a follow-up for
> 
> https://marc.info/?l=linux-doc&m=151101644105835&w=2
> 
> Patches[1-3/6] from the above have been reviewed and agreed on, so
> they are in linux-next now and here's a next version of the rest.
> 
> Patches [1-2/4] are preparatory.  The first one is just really small
> code duplication avoidance on top of this recent fix:
> 
> https://patchwork.kernel.org/patch/10097563/
> 
> and the second one simply moves some code to separate functions.
> 
> Patch [3/4] causes the PM core to carry out some optimizations for
> drivers of devices with DPM_FLAG_SMART_SUSPEND set whose "late"
> and "noirq" suspend (or equivalent) driver callbacks are invoked
> directly by the core.
> 
> The underlying observation is that if the device is suspended (via
> runtime PM) during the "late suspend" phase of a system transition,
> invoking the "late" and "noirq" callbacks from the driver for it is not
> going to make it more suspended, so to speak, so it doesn't make sense to
> invoke them at all.
> 
> [That optimization is only done for devices with DPM_FLAG_SMART_SUSPEND
> set, because drivers setting that flag are expected to be prepared for
> skipping their "late" and "noirq" callbacks if the device is already
> suspended.]
> 
> Patch [4/4] makes the core do an analogous thing for devices with
> DPM_FLAG_LEAVE_SUSPENDED set whose "noirq" and "early" resume (or
> equivalent) driver callbacks are directly invoked by the core.
> 
> In that case the observation is that if such devices can be left in
> suspend after the system transition to the working state, running
> resume callbacks from their drivers is simply not necessary.
> 
> Pathes [3-4/4] have been reoredered and reworked a bit since the last
> iteration, so they are regarded as new.
> 
> The series is on top of the linux-next branch of the linux-pm.git tree
> that should be merged into linux-next on Monday.
> 
> [I have developed debug bus type and driver modules to test that code,
> but they are not ready to be made available at this point.]

Resending the original series (except for the first patch that has been
applied already) along with some driver code changes based on them as
requested by Ulf.

The driver patches are an intel-lpss change (already reviewed), two
modifications of i2c-designware-platdrv (posted previously but slightly
changed since then) and a PCIe port driver change.

At this point patches [1-3/7] are pretty much on the way in and the driver
material depends on review comments (it is pointless to apply [4/7] without
[5-6/7], so it depends on them in my view).

I'm sending this from a system running with all of the series applied, although
admittedly not using i2c-designware-platdrv.  However, one of my test machines
has this one and I haven't seen any adverse effects of these changes on it so
far.

Thanks,
Rafael

[PATCH 0/7] PM / core: Direct handling of DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED

[Rafael J. Wysocki]

On Sunday, December 10, 2017 12:55:23 AM CET Rafael J. Wysocki wrote:
> Hi All,
> 
> This series is a follow-up for
> 
> https://marc.info/?l=linux-doc&m=151101644105835&w=2
> 
> Patches[1-3/6] from the above have been reviewed and agreed on, so
> they are in linux-next now and here's a next version of the rest.
> 
> Patches [1-2/4] are preparatory.  The first one is just really small
> code duplication avoidance on top of this recent fix:
> 
> https://patchwork.kernel.org/patch/10097563/
> 
> and the second one simply moves some code to separate functions.
> 
> Patch [3/4] causes the PM core to carry out some optimizations for
> drivers of devices with DPM_FLAG_SMART_SUSPEND set whose "late"
> and "noirq" suspend (or equivalent) driver callbacks are invoked
> directly by the core.
> 
> The underlying observation is that if the device is suspended (via
> runtime PM) during the "late suspend" phase of a system transition,
> invoking the "late" and "noirq" callbacks from the driver for it is not
> going to make it more suspended, so to speak, so it doesn't make sense to
> invoke them at all.
> 
> [That optimization is only done for devices with DPM_FLAG_SMART_SUSPEND
> set, because drivers setting that flag are expected to be prepared for
> skipping their "late" and "noirq" callbacks if the device is already
> suspended.]
> 
> Patch [4/4] makes the core do an analogous thing for devices with
> DPM_FLAG_LEAVE_SUSPENDED set whose "noirq" and "early" resume (or
> equivalent) driver callbacks are directly invoked by the core.
> 
> In that case the observation is that if such devices can be left in
> suspend after the system transition to the working state, running
> resume callbacks from their drivers is simply not necessary.
> 
> Pathes [3-4/4] have been reoredered and reworked a bit since the last
> iteration, so they are regarded as new.
> 
> The series is on top of the linux-next branch of the linux-pm.git tree
> that should be merged into linux-next on Monday.
> 
> [I have developed debug bus type and driver modules to test that code,
> but they are not ready to be made available at this point.]

Resending the original series (except for the first patch that has been
applied already) along with some driver code changes based on them as
requested by Ulf.

The driver patches are an intel-lpss change (already reviewed), two
modifications of i2c-designware-platdrv (posted previously but slightly
changed since then) and a PCIe port driver change.

At this point patches [1-3/7] are pretty much on the way in and the driver
material depends on review comments (it is pointless to apply [4/7] without
[5-6/7], so it depends on them in my view).

I'm sending this from a system running with all of the series applied, although
admittedly not using i2c-designware-platdrv.  However, one of my test machines
has this one and I haven't seen any adverse effects of these changes on it so
far.

Thanks,
Rafael

[PATCH 1/7] PM / core: Add helpers for subsystem callback selection

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Add helper routines to find and return a suitable subsystem callback
during the "noirq" phases of system suspend/resume (or analogous)
transitions as well as during the "late" phase of system suspend and
the "early" phase of system resume (or analogous) transitions.

The helpers will be called from additional sites going forward.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/main.c |  188 +++++++++++++++++++++++++++++++---------------
 1 file changed, 128 insertions(+), 60 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -551,6 +551,35 @@ bool dev_pm_may_skip_resume(struct devic
 	return !dev->power.must_resume && pm_transition.event != PM_EVENT_RESTORE;
 }
 
+static pm_callback_t dpm_subsys_resume_noirq_cb(struct device *dev,
+						pm_message_t state,
+						const char **info_p)
+{
+	pm_callback_t callback;
+	const char *info;
+
+	if (dev->pm_domain) {
+		info = "noirq power domain ";
+		callback = pm_noirq_op(&dev->pm_domain->ops, state);
+	} else if (dev->type && dev->type->pm) {
+		info = "noirq type ";
+		callback = pm_noirq_op(dev->type->pm, state);
+	} else if (dev->class && dev->class->pm) {
+		info = "noirq class ";
+		callback = pm_noirq_op(dev->class->pm, state);
+	} else if (dev->bus && dev->bus->pm) {
+		info = "noirq bus ";
+		callback = pm_noirq_op(dev->bus->pm, state);
+	} else {
+		return NULL;
+	}
+
+	if (info_p)
+		*info_p = info;
+
+	return callback;
+}
+
 /**
  * device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
@@ -562,8 +591,8 @@ bool dev_pm_may_skip_resume(struct devic
  */
 static int device_resume_noirq(struct device *dev, pm_message_t state, bool async)
 {
-	pm_callback_t callback = NULL;
-	const char *info = NULL;
+	pm_callback_t callback;
+	const char *info;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -577,19 +606,7 @@ static int device_resume_noirq(struct de
 
 	dpm_wait_for_superior(dev, async);
 
-	if (dev->pm_domain) {
-		info = "noirq power domain ";
-		callback = pm_noirq_op(&dev->pm_domain->ops, state);
-	} else if (dev->type && dev->type->pm) {
-		info = "noirq type ";
-		callback = pm_noirq_op(dev->type->pm, state);
-	} else if (dev->class && dev->class->pm) {
-		info = "noirq class ";
-		callback = pm_noirq_op(dev->class->pm, state);
-	} else if (dev->bus && dev->bus->pm) {
-		info = "noirq bus ";
-		callback = pm_noirq_op(dev->bus->pm, state);
-	}
+	callback = dpm_subsys_resume_noirq_cb(dev, state, &info);
 
 	if (!callback && dev->driver && dev->driver->pm) {
 		info = "noirq driver ";
@@ -704,6 +721,35 @@ void dpm_resume_noirq(pm_message_t state
 	dpm_noirq_end();
 }
 
+static pm_callback_t dpm_subsys_resume_early_cb(struct device *dev,
+						pm_message_t state,
+						const char **info_p)
+{
+	pm_callback_t callback;
+	const char *info;
+
+	if (dev->pm_domain) {
+		info = "early power domain ";
+		callback = pm_late_early_op(&dev->pm_domain->ops, state);
+	} else if (dev->type && dev->type->pm) {
+		info = "early type ";
+		callback = pm_late_early_op(dev->type->pm, state);
+	} else if (dev->class && dev->class->pm) {
+		info = "early class ";
+		callback = pm_late_early_op(dev->class->pm, state);
+	} else if (dev->bus && dev->bus->pm) {
+		info = "early bus ";
+		callback = pm_late_early_op(dev->bus->pm, state);
+	} else {
+		return NULL;
+	}
+
+	if (info_p)
+		*info_p = info;
+
+	return callback;
+}
+
 /**
  * device_resume_early - Execute an "early resume" callback for given device.
  * @dev: Device to handle.
@@ -714,8 +760,8 @@ void dpm_resume_noirq(pm_message_t state
  */
 static int device_resume_early(struct device *dev, pm_message_t state, bool async)
 {
-	pm_callback_t callback = NULL;
-	const char *info = NULL;
+	pm_callback_t callback;
+	const char *info;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -729,19 +775,7 @@ static int device_resume_early(struct de
 
 	dpm_wait_for_superior(dev, async);
 
-	if (dev->pm_domain) {
-		info = "early power domain ";
-		callback = pm_late_early_op(&dev->pm_domain->ops, state);
-	} else if (dev->type && dev->type->pm) {
-		info = "early type ";
-		callback = pm_late_early_op(dev->type->pm, state);
-	} else if (dev->class && dev->class->pm) {
-		info = "early class ";
-		callback = pm_late_early_op(dev->class->pm, state);
-	} else if (dev->bus && dev->bus->pm) {
-		info = "early bus ";
-		callback = pm_late_early_op(dev->bus->pm, state);
-	}
+	callback = dpm_subsys_resume_early_cb(dev, state, &info);
 
 	if (!callback && dev->driver && dev->driver->pm) {
 		info = "early driver ";
@@ -1128,6 +1162,35 @@ static void dpm_superior_set_must_resume
 	device_links_read_unlock(idx);
 }
 
+static pm_callback_t dpm_subsys_suspend_noirq_cb(struct device *dev,
+						 pm_message_t state,
+						 const char **info_p)
+{
+	pm_callback_t callback;
+	const char *info;
+
+	if (dev->pm_domain) {
+		info = "noirq power domain ";
+		callback = pm_noirq_op(&dev->pm_domain->ops, state);
+	} else if (dev->type && dev->type->pm) {
+		info = "noirq type ";
+		callback = pm_noirq_op(dev->type->pm, state);
+	} else if (dev->class && dev->class->pm) {
+		info = "noirq class ";
+		callback = pm_noirq_op(dev->class->pm, state);
+	} else if (dev->bus && dev->bus->pm) {
+		info = "noirq bus ";
+		callback = pm_noirq_op(dev->bus->pm, state);
+	} else {
+		return NULL;
+	}
+
+	if (info_p)
+		*info_p = info;
+
+	return callback;
+}
+
 /**
  * __device_suspend_noirq - Execute a "noirq suspend" callback for given device.
  * @dev: Device to handle.
@@ -1139,8 +1202,8 @@ static void dpm_superior_set_must_resume
  */
 static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool async)
 {
-	pm_callback_t callback = NULL;
-	const char *info = NULL;
+	pm_callback_t callback;
+	const char *info;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -1159,19 +1222,7 @@ static int __device_suspend_noirq(struct
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
-	if (dev->pm_domain) {
-		info = "noirq power domain ";
-		callback = pm_noirq_op(&dev->pm_domain->ops, state);
-	} else if (dev->type && dev->type->pm) {
-		info = "noirq type ";
-		callback = pm_noirq_op(dev->type->pm, state);
-	} else if (dev->class && dev->class->pm) {
-		info = "noirq class ";
-		callback = pm_noirq_op(dev->class->pm, state);
-	} else if (dev->bus && dev->bus->pm) {
-		info = "noirq bus ";
-		callback = pm_noirq_op(dev->bus->pm, state);
-	}
+	callback = dpm_subsys_suspend_noirq_cb(dev, state, &info);
 
 	if (!callback && dev->driver && dev->driver->pm) {
 		info = "noirq driver ";
@@ -1306,6 +1357,35 @@ int dpm_suspend_noirq(pm_message_t state
 	return ret;
 }
 
+static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
+						pm_message_t state,
+						const char **info_p)
+{
+	pm_callback_t callback;
+	const char *info;
+
+	if (dev->pm_domain) {
+		info = "late power domain ";
+		callback = pm_late_early_op(&dev->pm_domain->ops, state);
+	} else if (dev->type && dev->type->pm) {
+		info = "late type ";
+		callback = pm_late_early_op(dev->type->pm, state);
+	} else if (dev->class && dev->class->pm) {
+		info = "late class ";
+		callback = pm_late_early_op(dev->class->pm, state);
+	} else if (dev->bus && dev->bus->pm) {
+		info = "late bus ";
+		callback = pm_late_early_op(dev->bus->pm, state);
+	} else {
+		return NULL;
+	}
+
+	if (info_p)
+		*info_p = info;
+
+	return callback;
+}
+
 /**
  * __device_suspend_late - Execute a "late suspend" callback for given device.
  * @dev: Device to handle.
@@ -1316,8 +1396,8 @@ int dpm_suspend_noirq(pm_message_t state
  */
 static int __device_suspend_late(struct device *dev, pm_message_t state, bool async)
 {
-	pm_callback_t callback = NULL;
-	const char *info = NULL;
+	pm_callback_t callback;
+	const char *info;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -1338,19 +1418,7 @@ static int __device_suspend_late(struct
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
-	if (dev->pm_domain) {
-		info = "late power domain ";
-		callback = pm_late_early_op(&dev->pm_domain->ops, state);
-	} else if (dev->type && dev->type->pm) {
-		info = "late type ";
-		callback = pm_late_early_op(dev->type->pm, state);
-	} else if (dev->class && dev->class->pm) {
-		info = "late class ";
-		callback = pm_late_early_op(dev->class->pm, state);
-	} else if (dev->bus && dev->bus->pm) {
-		info = "late bus ";
-		callback = pm_late_early_op(dev->bus->pm, state);
-	}
+	callback = dpm_subsys_suspend_late_cb(dev, state, &info);
 
 	if (!callback && dev->driver && dev->driver->pm) {
 		info = "late driver ";

[PATCH 2/7] PM / core: Direct DPM_FLAG_SMART_SUSPEND optimization

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make the PM core avoid invoking the "late" and "noirq" system-wide
suspend (or analogous) callbacks provided by device drivers directly
for devices with DPM_FLAG_SMART_SUSPEND set that are in runtime
suspend during the "late" and "noirq" phases of system-wide suspend
(or analogous) transitions.  That is only done for devices without
any middle-layer "late" and "noirq" suspend callbacks (to avoid
confusing the middle layer if there is one).

The underlying observation is that runtime PM is disabled for devices
during the "late" and "noirq" system-wide suspend phases, so if they
remain in runtime suspend from the "late" phase forward, it doesn't
make sense to invoke the "late" and "noirq" callbacks provided by
the drivers for them (arguably, the device is already suspended and
in the right state).  Thus, if the remaining driver suspend callbacks
are to be invoked directly by the core, they can be skipped.

This change really makes it possible for, say, platform device
drivers to re-use runtime PM suspend and resume callbacks by
pointing ->suspend_late and ->resume_early, respectively (and
possibly the analogous hibernation-related callback pointers too),
to them without adding any extra "is the device already suspended?"
type of checks to the callback routines, as long as they will be
invoked directly by the core.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/driver-api/pm/devices.rst |   18 +++---
 drivers/base/power/main.c               |   85 +++++++++++++++++++++++++++++---
 2 files changed, 88 insertions(+), 15 deletions(-)

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
@@ -777,14 +777,16 @@ The driver can indicate that by setting
 runtime suspend at the beginning of the ``suspend_late`` phase of system-wide
 suspend (or in the ``poweroff_late`` phase of hibernation), when runtime PM
 has been disabled for it, under the assumption that its state should not change
-after that point until the system-wide transition is over.  If that happens, the
-driver's system-wide resume callbacks, if present, may still be invoked during
-the subsequent system-wide resume transition and the device's runtime power
-management status may be set to "active" before enabling runtime PM for it,
-so the driver must be prepared to cope with the invocation of its system-wide
-resume callbacks back-to-back with its ``->runtime_suspend`` one (without the
-intervening ``->runtime_resume`` and so on) and the final state of the device
-must reflect the "active" status for runtime PM in that case.
+after that point until the system-wide transition is over (the PM core itself
+does that for devices whose "noirq", "late" and "early" system-wide PM callbacks
+are executed directly by it).  If that happens, the driver's system-wide resume
+callbacks, if present, may still be invoked during the subsequent system-wide
+resume transition and the device's runtime power management status may be set
+to "active" before enabling runtime PM for it, so the driver must be prepared to
+cope with the invocation of its system-wide resume callbacks back-to-back with
+its ``->runtime_suspend`` one (without the intervening ``->runtime_resume`` and
+so on) and the final state of the device must reflect the "active" runtime PM
+status in that case.
 
 During system-wide resume from a sleep state it's easiest to put devices into
 the full-power state, as explained in :file:`Documentation/power/runtime_pm.txt`.
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -540,6 +540,24 @@ void dev_pm_skip_next_resume_phases(stru
 }
 
 /**
+ * suspend_event - Return a "suspend" message for given "resume" one.
+ * @resume_msg: PM message representing a system-wide resume transition.
+ */
+static pm_message_t suspend_event(pm_message_t resume_msg)
+{
+	switch (resume_msg.event) {
+	case PM_EVENT_RESUME:
+		return PMSG_SUSPEND;
+	case PM_EVENT_THAW:
+	case PM_EVENT_RESTORE:
+		return PMSG_FREEZE;
+	case PM_EVENT_RECOVER:
+		return PMSG_HIBERNATE;
+	}
+	return PMSG_ON;
+}
+
+/**
  * dev_pm_may_skip_resume - System-wide device resume optimization check.
  * @dev: Target device.
  *
@@ -580,6 +598,14 @@ static pm_callback_t dpm_subsys_resume_n
 	return callback;
 }
 
+static pm_callback_t dpm_subsys_suspend_noirq_cb(struct device *dev,
+						 pm_message_t state,
+						 const char **info_p);
+
+static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
+						pm_message_t state,
+						const char **info_p);
+
 /**
  * device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
@@ -607,13 +633,40 @@ static int device_resume_noirq(struct de
 	dpm_wait_for_superior(dev, async);
 
 	callback = dpm_subsys_resume_noirq_cb(dev, state, &info);
+	if (callback)
+		goto Run;
 
-	if (!callback && dev->driver && dev->driver->pm) {
+	if (dev_pm_smart_suspend_and_suspended(dev)) {
+		pm_message_t suspend_msg = suspend_event(state);
+
+		/*
+		 * If "freeze" callbacks have been skipped during a transition
+		 * related to hibernation, the subsequent "thaw" callbacks must
+		 * be skipped too or bad things may happen.  Otherwise, resume
+		 * callbacks are going to be run for the device, so its runtime
+		 * PM status must be changed to reflect the new state after the
+		 * transition under way.
+		 */
+		if (!dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) &&
+		    !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL)) {
+			if (state.event == PM_EVENT_THAW) {
+				dev_pm_skip_next_resume_phases(dev);
+				goto Skip;
+			} else {
+				pm_runtime_set_active(dev);
+			}
+		}
+	}
+
+	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);
+
+Skip:
 	dev->power.is_noirq_suspended = false;
 
 	if (dev_pm_may_skip_resume(dev)) {
@@ -628,7 +681,7 @@ static int device_resume_noirq(struct de
 		dev_pm_skip_next_resume_phases(dev);
 	}
 
- Out:
+Out:
 	complete_all(&dev->power.completion);
 	TRACE_RESUME(error);
 	return error;
@@ -1223,18 +1276,26 @@ 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) {
+	if (dev_pm_smart_suspend_and_suspended(dev) &&
+	    !dpm_subsys_suspend_late_cb(dev, state, NULL))
+		goto Skip;
+
+	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;
 		goto Complete;
 	}
 
+Skip:
 	dev->power.is_noirq_suspended = true;
 
 	if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) {
@@ -1419,17 +1480,27 @@ static int __device_suspend_late(struct
 		goto Complete;
 
 	callback = dpm_subsys_suspend_late_cb(dev, state, &info);
+	if (callback)
+		goto Run;
 
-	if (!callback && dev->driver && dev->driver->pm) {
+	if (dev_pm_smart_suspend_and_suspended(dev) &&
+	    !dpm_subsys_suspend_noirq_cb(dev, state, NULL))
+		goto Skip;
+
+	if (dev->driver && dev->driver->pm) {
 		info = "late driver ";
 		callback = pm_late_early_op(dev->driver->pm, state);
 	}
 
+Run:
 	error = dpm_run_callback(callback, dev, state, info);
-	if (!error)
-		dev->power.is_late_suspended = true;
-	else
+	if (error) {
 		async_error = error;
+		goto Complete;
+	}
+
+Skip:
+	dev->power.is_late_suspended = true;
 
 Complete:
 	TRACE_SUSPEND(error);

[PATCH 3/7] PM / core: Direct DPM_FLAG_LEAVE_SUSPENDED handling

[Rafael J. Wysocki]

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.

Namely, make it skip all of the system-wide resume callbacks for
such devices with DPM_FLAG_LEAVE_SUSPENDED set if (1) they are in
runtime suspend during the "noirq" phase of system-wide suspend
(or analogous) transitions or (2) 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).

The underlying observation is that if either (1) or (2) takes place,
it simply is not necessary to invoke any resume callbacks for the
device that can be left in suspend.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/driver-api/pm/devices.rst |    9 +++++
 drivers/base/power/main.c               |   51 +++++++++++++++++++++++++-------
 2 files changed, 49 insertions(+), 11 deletions(-)

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
@@ -816,3 +816,12 @@ appropriate in its "noirq" resume callba
 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).
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -619,6 +619,7 @@ static int device_resume_noirq(struct de
 {
 	pm_callback_t callback;
 	const char *info;
+	bool skip_resume;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -632,10 +633,15 @@ 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 (dev_pm_smart_suspend_and_suspended(dev)) {
 		pm_message_t suspend_msg = suspend_event(state);
 
@@ -650,7 +656,7 @@ static int device_resume_noirq(struct de
 		if (!dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) &&
 		    !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL)) {
 			if (state.event == PM_EVENT_THAW) {
-				dev_pm_skip_next_resume_phases(dev);
+				skip_resume = true;
 				goto Skip;
 			} else {
 				pm_runtime_set_active(dev);
@@ -669,7 +675,7 @@ Run:
 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
@@ -1244,6 +1250,32 @@ static pm_callback_t dpm_subsys_suspend_
 	return callback;
 }
 
+static bool device_must_resume(struct device *dev, pm_message_t state,
+			       bool no_subsys_suspend_noirq)
+{
+	pm_message_t resume_msg = resume_event(state);
+
+	/*
+	 * 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.
+	 */
+	if (no_subsys_suspend_noirq &&
+	    !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))
+		return !pm_runtime_status_suspended(dev) &&
+			(resume_msg.event != PM_EVENT_RESUME ||
+			 (device_can_wakeup(dev) && !device_may_wakeup(dev)));
+
+	/*
+	 * 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.
+	 */
+	return !dev->power.may_skip_resume;
+}
+
 /**
  * __device_suspend_noirq - Execute a "noirq suspend" callback for given device.
  * @dev: Device to handle.
@@ -1257,6 +1289,7 @@ static int __device_suspend_noirq(struct
 {
 	pm_callback_t callback;
 	const char *info;
+	bool no_subsys_cb = false;
 	int error = 0;
 
 	TRACE_DEVICE(dev);
@@ -1279,8 +1312,9 @@ static int __device_suspend_noirq(struct
 	if (callback)
 		goto Run;
 
-	if (dev_pm_smart_suspend_and_suspended(dev) &&
-	    !dpm_subsys_suspend_late_cb(dev, state, NULL))
+	no_subsys_cb = !dpm_subsys_suspend_late_cb(dev, state, NULL);
+
+	if (dev_pm_smart_suspend_and_suspended(dev) && no_subsys_cb)
 		goto Skip;
 
 	if (dev->driver && dev->driver->pm) {
@@ -1299,14 +1333,9 @@ Skip:
 	dev->power.is_noirq_suspended = true;
 
 	if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) {
-		/*
-		 * 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 ||
-					atomic_read(&dev->power.usage_count) > 1;
+				atomic_read(&dev->power.usage_count) > 1 ||
+				device_must_resume(dev, state, no_subsys_cb);
 	} else {
 		dev->power.must_resume = true;
 	}

[PATCH 4/7] PM / mfd: intel-lpss: Use DPM_FLAG_SMART_SUSPEND

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make the intel-lpss driver set DPM_FLAG_SMART_SUSPEND for its
devices which will allow them to stay in runtime suspend during
system suspend unless they need to be reconfigured for some reason.

Also make it avoid resuming its child devices if they have
DPM_FLAG_SMART_SUSPEND set to allow them to remain in runtime
suspend during system suspend.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/intel-lpss.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/mfd/intel-lpss.c
===================================================================
--- linux-pm.orig/drivers/mfd/intel-lpss.c
+++ linux-pm/drivers/mfd/intel-lpss.c
@@ -450,6 +450,8 @@ int intel_lpss_probe(struct device *dev,
 	if (ret)
 		goto err_remove_ltr;
 
+	dev_pm_set_driver_flags(dev, DPM_FLAG_SMART_SUSPEND);
+
 	return 0;
 
 err_remove_ltr:
@@ -478,7 +480,9 @@ EXPORT_SYMBOL_GPL(intel_lpss_remove);
 
 static int resume_lpss_device(struct device *dev, void *data)
 {
-	pm_runtime_resume(dev);
+	if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND))
+		pm_runtime_resume(dev);
+
 	return 0;
 }
 

[PATCH 5/7] PM: i2c-designware-platdrv: Use DPM_FLAG_SMART_PREPARE

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify i2c-designware-platdrv to set DPM_FLAG_SMART_PREPARE for its
devices and return 0 from the system suspend ->prepare callback
if the device has an ACPI companion object in order to tell the PM
core and middle layers to avoid skipping system suspend/resume
callbacks for the device in that case (which may be problematic,
because the device may be accessed during suspend and resume of
other devices via I2C operation regions then).

Also the pm_runtime_suspended() check in dw_i2c_plat_prepare()
is not necessary any more, because the core does it when setting
power.direct_complete for the device, so drop it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
===================================================================
--- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c
+++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -372,6 +372,8 @@ static int dw_i2c_plat_probe(struct plat
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
 
+	dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_SMART_PREPARE);
+
 	/* The code below assumes runtime PM to be disabled. */
 	WARN_ON(pm_runtime_enabled(&pdev->dev));
 
@@ -435,7 +437,13 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match)
 #ifdef CONFIG_PM_SLEEP
 static int dw_i2c_plat_prepare(struct device *dev)
 {
-	return pm_runtime_suspended(dev);
+	/*
+	 * If the ACPI companion device object is present for this device, it
+	 * may be accessed during suspend and resume of other devices via I2C
+	 * operation regions, so tell the PM core and middle layers to avoid
+	 * skipping system suspend/resume callbacks for it in that case.
+	 */
+	return !has_acpi_companion(dev);
 }
 
 static void dw_i2c_plat_complete(struct device *dev)

[PATCH 6/7] PM: i2c-designware-platdrv: Optimize power management

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Optimize the power management in i2c-designware-platdrv by making it
set the DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED which
allows some code to be dropped from its PM callbacks.

First, setting DPM_FLAG_SMART_SUSPEND causes the intel-lpss driver
to avoid resuming i2c-designware-platdrv devices in its ->prepare
callback, so they can stay in runtime suspend after that point even
if the direct-complete feature is not used for them.

It also causes the ACPI PM domain and the PM core to avoid invoking
"late" and "noirq" suspend callbacks for these devices if they are
in runtime suspend at the beginning of the "late" phase of device
suspend during system suspend.  That guarantees dw_i2c_plat_suspend()
to be called for a device only if it is not in runtime suspend.

Moreover, it causes the device's runtime PM status to be set to
"active" after calling dw_i2c_plat_resume() for it, so the
driver doesn't need internal flags to avoid invoking either
dw_i2c_plat_suspend() or dw_i2c_plat_resume() twice in a row.

Second, setting DPM_FLAG_LEAVE_SUSPENDED enables the optimization
allowing the device to stay suspended after system resume under
suitable conditions, so again the driver doesn't need to take
care of that by itself.

Accordingly, the internal "suspended" and "skip_resume" flags
used by the driver are not necessary any more, so drop them and
simplify the driver's PM callbacks.

Additionally, notice that dw_i2c_plat_complete() only needs to
schedule runtime PM resume for the device if platform firmware
has been involved in resuming the system, so make it call
pm_resume_via_firmware() to check that.  Also make it check the
runtime PM status of the device instead of its direct_complete
flag which also works if the device remained suspended due to
the DPM_FLAG_LEAVE_SUSPENDED driver flag.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.h    |    2 -
 drivers/i2c/busses/i2c-designware-platdrv.c |   31 ++++++++++------------------
 2 files changed, 12 insertions(+), 21 deletions(-)

Index: linux-pm/drivers/i2c/busses/i2c-designware-core.h
===================================================================
--- linux-pm.orig/drivers/i2c/busses/i2c-designware-core.h
+++ linux-pm/drivers/i2c/busses/i2c-designware-core.h
@@ -280,8 +280,6 @@ struct dw_i2c_dev {
 	int			(*acquire_lock)(struct dw_i2c_dev *dev);
 	void			(*release_lock)(struct dw_i2c_dev *dev);
 	bool			pm_disabled;
-	bool			suspended;
-	bool			skip_resume;
 	void			(*disable)(struct dw_i2c_dev *dev);
 	void			(*disable_int)(struct dw_i2c_dev *dev);
 	int			(*init)(struct dw_i2c_dev *dev);
Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
===================================================================
--- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c
+++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -42,6 +42,7 @@
 #include <linux/reset.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/suspend.h>
 
 #include "i2c-designware-core.h"
 
@@ -372,7 +373,10 @@ static int dw_i2c_plat_probe(struct plat
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
 
-	dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_SMART_PREPARE);
+	dev_pm_set_driver_flags(&pdev->dev,
+				DPM_FLAG_SMART_PREPARE |
+				DPM_FLAG_SMART_SUSPEND |
+				DPM_FLAG_LEAVE_SUSPENDED);
 
 	/* The code below assumes runtime PM to be disabled. */
 	WARN_ON(pm_runtime_enabled(&pdev->dev));
@@ -448,7 +452,13 @@ static int dw_i2c_plat_prepare(struct de
 
 static void dw_i2c_plat_complete(struct device *dev)
 {
-	if (dev->power.direct_complete)
+	/*
+	 * The device can only be in runtime suspend at this point if it has not
+	 * been resumed throughout the ending system suspend/resume cycle, so if
+	 * the platform firmware might mess up with it, request the runtime PM
+	 * framework to resume it.
+	 */
+	if (pm_runtime_suspended(dev) && pm_resume_via_firmware())
 		pm_request_resume(dev);
 }
 #else
@@ -461,16 +471,9 @@ static int dw_i2c_plat_suspend(struct de
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
-	if (i_dev->suspended) {
-		i_dev->skip_resume = true;
-		return 0;
-	}
-
 	i_dev->disable(i_dev);
 	i2c_dw_plat_prepare_clk(i_dev, false);
 
-	i_dev->suspended = true;
-
 	return 0;
 }
 
@@ -478,19 +481,9 @@ static int dw_i2c_plat_resume(struct dev
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
-	if (!i_dev->suspended)
-		return 0;
-
-	if (i_dev->skip_resume) {
-		i_dev->skip_resume = false;
-		return 0;
-	}
-
 	i2c_dw_plat_prepare_clk(i_dev, true);
 	i_dev->init(i_dev);
 
-	i_dev->suspended = false;
-
 	return 0;
 }
 

[PATCH 7/7] PCI / PM: Use SMART_SUSPEND and LEAVE_SUSPENDED flags for PCIe ports

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make the PCIe port driver set DPM_FLAG_SMART_SUSPEND and
DPM_FLAG_LEAVE_SUSPENDED for the devices handled by it to benefit
from the opportunistic optimizations in the PCI layer enabled by
these flags.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pcie/portdrv_pci.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-pm/drivers/pci/pcie/portdrv_pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pcie/portdrv_pci.c
+++ linux-pm/drivers/pci/pcie/portdrv_pci.c
@@ -150,6 +150,9 @@ static int pcie_portdrv_probe(struct pci
 
 	pci_save_state(dev);
 
+	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_SMART_SUSPEND |
+					   DPM_FLAG_LEAVE_SUSPENDED);
+
 	if (pci_bridge_d3_possible(dev)) {
 		/*
 		 * Keep the port resumed 100ms to make sure things like

Re: [PATCH 7/7] PCI / PM: Use SMART_SUSPEND and LEAVE_SUSPENDED flags for PCIe ports

[Bjorn Helgaas]

On Wed, Jan 03, 2018 at 01:38:27AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make the PCIe port driver set DPM_FLAG_SMART_SUSPEND and
> DPM_FLAG_LEAVE_SUSPENDED for the devices handled by it to benefit
> from the opportunistic optimizations in the PCI layer enabled by
> these flags.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Please merge this along with the rest of the series.

> ---
>  drivers/pci/pcie/portdrv_pci.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> Index: linux-pm/drivers/pci/pcie/portdrv_pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pcie/portdrv_pci.c
> +++ linux-pm/drivers/pci/pcie/portdrv_pci.c
> @@ -150,6 +150,9 @@ static int pcie_portdrv_probe(struct pci
>  
>  	pci_save_state(dev);
>  
> +	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_SMART_SUSPEND |
> +					   DPM_FLAG_LEAVE_SUSPENDED);
> +
>  	if (pci_bridge_d3_possible(dev)) {
>  		/*
>  		 * Keep the port resumed 100ms to make sure things like
> 

Re: [PATCH 7/7] PCI / PM: Use SMART_SUSPEND and LEAVE_SUSPENDED flags for PCIe ports

[Rafael J. Wysocki]

On Thu, Jan 4, 2018 at 11:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Wed, Jan 03, 2018 at 01:38:27AM +0100, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Make the PCIe port driver set DPM_FLAG_SMART_SUSPEND and
>> DPM_FLAG_LEAVE_SUSPENDED for the devices handled by it to benefit
>> from the opportunistic optimizations in the PCI layer enabled by
>> these flags.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Please merge this along with the rest of the series.

I will, thank you!

Re: [PATCH 5/7] PM: i2c-designware-platdrv: Use DPM_FLAG_SMART_PREPARE

[Jarkko Nikula]

On 01/03/2018 02:35 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Modify i2c-designware-platdrv to set DPM_FLAG_SMART_PREPARE for its
> devices and return 0 from the system suspend ->prepare callback
> if the device has an ACPI companion object in order to tell the PM
> core and middle layers to avoid skipping system suspend/resume
> callbacks for the device in that case (which may be problematic,
> because the device may be accessed during suspend and resume of
> other devices via I2C operation regions then).
> 
> Also the pm_runtime_suspended() check in dw_i2c_plat_prepare()
> is not necessary any more, because the core does it when setting
> power.direct_complete for the device, so drop it.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c |   10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH 6/7] PM: i2c-designware-platdrv: Optimize power management

[Jarkko Nikula]

On 01/03/2018 02:37 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Optimize the power management in i2c-designware-platdrv by making it
> set the DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED which
> allows some code to be dropped from its PM callbacks.
> 
> First, setting DPM_FLAG_SMART_SUSPEND causes the intel-lpss driver
> to avoid resuming i2c-designware-platdrv devices in its ->prepare
> callback, so they can stay in runtime suspend after that point even
> if the direct-complete feature is not used for them.
> 
> It also causes the ACPI PM domain and the PM core to avoid invoking
> "late" and "noirq" suspend callbacks for these devices if they are
> in runtime suspend at the beginning of the "late" phase of device
> suspend during system suspend.  That guarantees dw_i2c_plat_suspend()
> to be called for a device only if it is not in runtime suspend.
> 
> Moreover, it causes the device's runtime PM status to be set to
> "active" after calling dw_i2c_plat_resume() for it, so the
> driver doesn't need internal flags to avoid invoking either
> dw_i2c_plat_suspend() or dw_i2c_plat_resume() twice in a row.
> 
> Second, setting DPM_FLAG_LEAVE_SUSPENDED enables the optimization
> allowing the device to stay suspended after system resume under
> suitable conditions, so again the driver doesn't need to take
> care of that by itself.
> 
> Accordingly, the internal "suspended" and "skip_resume" flags
> used by the driver are not necessary any more, so drop them and
> simplify the driver's PM callbacks.
> 
> Additionally, notice that dw_i2c_plat_complete() only needs to
> schedule runtime PM resume for the device if platform firmware
> has been involved in resuming the system, so make it call
> pm_resume_via_firmware() to check that.  Also make it check the
> runtime PM status of the device instead of its direct_complete
> flag which also works if the device remained suspended due to
> the DPM_FLAG_LEAVE_SUSPENDED driver flag.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/i2c/busses/i2c-designware-core.h    |    2 -
>   drivers/i2c/busses/i2c-designware-platdrv.c |   31 ++++++++++------------------
>   2 files changed, 12 insertions(+), 21 deletions(-)
> 
This doesn't apply to linux-next due 0326f9f801b2 ("i2c: designware: 
rename i2c_dw_plat_prepare_clk to i2c_dw_prepare_clk"). It was trivial 
to fix which I did locally for testing.

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH 0/7] PM / core: Direct handling of DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED

[Jarkko Nikula]

On 01/03/2018 02:29 AM, Rafael J. Wysocki wrote:
> Resending the original series (except for the first patch that has been
> applied already) along with some driver code changes based on them as
> requested by Ulf.
> 
> The driver patches are an intel-lpss change (already reviewed), two
> modifications of i2c-designware-platdrv (posted previously but slightly
> changed since then) and a PCIe port driver change.
> 
> At this point patches [1-3/7] are pretty much on the way in and the driver
> material depends on review comments (it is pointless to apply [4/7] without
> [5-6/7], so it depends on them in my view).
> 
> I'm sending this from a system running with all of the series applied, although
> admittedly not using i2c-designware-platdrv.  However, one of my test machines
> has this one and I haven't seen any adverse effects of these changes on it so
> far.
> 
Both i2cdetect and hexdump from touchscreen and touchpad devices were 
working fine. I used linux-next next-20180108 and patches 4-7/7.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH 5/7] PM: i2c-designware-platdrv: Use DPM_FLAG_SMART_PREPARE

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

On Mon, Jan 08, 2018 at 04:31:49PM +0200, Jarkko Nikula wrote:
> On 01/03/2018 02:35 AM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Modify i2c-designware-platdrv to set DPM_FLAG_SMART_PREPARE for its
> > devices and return 0 from the system suspend ->prepare callback
> > if the device has an ACPI companion object in order to tell the PM
> > core and middle layers to avoid skipping system suspend/resume
> > callbacks for the device in that case (which may be problematic,
> > because the device may be accessed during suspend and resume of
> > other devices via I2C operation regions then).
> > 
> > Also the pm_runtime_suspended() check in dw_i2c_plat_prepare()
> > is not necessary any more, because the core does it when setting
> > power.direct_complete for the device, so drop it.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/i2c/busses/i2c-designware-platdrv.c |   10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Acked-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/7] PM: i2c-designware-platdrv: Optimize power management

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2775 bytes --]

On Mon, Jan 08, 2018 at 04:31:58PM +0200, Jarkko Nikula wrote:
> On 01/03/2018 02:37 AM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Optimize the power management in i2c-designware-platdrv by making it
> > set the DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED which
> > allows some code to be dropped from its PM callbacks.
> > 
> > First, setting DPM_FLAG_SMART_SUSPEND causes the intel-lpss driver
> > to avoid resuming i2c-designware-platdrv devices in its ->prepare
> > callback, so they can stay in runtime suspend after that point even
> > if the direct-complete feature is not used for them.
> > 
> > It also causes the ACPI PM domain and the PM core to avoid invoking
> > "late" and "noirq" suspend callbacks for these devices if they are
> > in runtime suspend at the beginning of the "late" phase of device
> > suspend during system suspend.  That guarantees dw_i2c_plat_suspend()
> > to be called for a device only if it is not in runtime suspend.
> > 
> > Moreover, it causes the device's runtime PM status to be set to
> > "active" after calling dw_i2c_plat_resume() for it, so the
> > driver doesn't need internal flags to avoid invoking either
> > dw_i2c_plat_suspend() or dw_i2c_plat_resume() twice in a row.
> > 
> > Second, setting DPM_FLAG_LEAVE_SUSPENDED enables the optimization
> > allowing the device to stay suspended after system resume under
> > suitable conditions, so again the driver doesn't need to take
> > care of that by itself.
> > 
> > Accordingly, the internal "suspended" and "skip_resume" flags
> > used by the driver are not necessary any more, so drop them and
> > simplify the driver's PM callbacks.
> > 
> > Additionally, notice that dw_i2c_plat_complete() only needs to
> > schedule runtime PM resume for the device if platform firmware
> > has been involved in resuming the system, so make it call
> > pm_resume_via_firmware() to check that.  Also make it check the
> > runtime PM status of the device instead of its direct_complete
> > flag which also works if the device remained suspended due to
> > the DPM_FLAG_LEAVE_SUSPENDED driver flag.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/i2c/busses/i2c-designware-core.h    |    2 -
> >   drivers/i2c/busses/i2c-designware-platdrv.c |   31 ++++++++++------------------
> >   2 files changed, 12 insertions(+), 21 deletions(-)
> > 
> This doesn't apply to linux-next due 0326f9f801b2 ("i2c: designware: rename
> i2c_dw_plat_prepare_clk to i2c_dw_prepare_clk"). It was trivial to fix which
> I did locally for testing.
> 
> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Acked-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/7] PM / core: Direct handling of DPM_FLAG_SMART_SUSPEND and DPM_FLAG_LEAVE_SUSPENDED

[Rafael J. Wysocki]

On Monday, January 8, 2018 3:33:07 PM CET Jarkko Nikula wrote:
> On 01/03/2018 02:29 AM, Rafael J. Wysocki wrote:
> > Resending the original series (except for the first patch that has been
> > applied already) along with some driver code changes based on them as
> > requested by Ulf.
> > 
> > The driver patches are an intel-lpss change (already reviewed), two
> > modifications of i2c-designware-platdrv (posted previously but slightly
> > changed since then) and a PCIe port driver change.
> > 
> > At this point patches [1-3/7] are pretty much on the way in and the driver
> > material depends on review comments (it is pointless to apply [4/7] without
> > [5-6/7], so it depends on them in my view).
> > 
> > I'm sending this from a system running with all of the series applied, although
> > admittedly not using i2c-designware-platdrv.  However, one of my test machines
> > has this one and I haven't seen any adverse effects of these changes on it so
> > far.
> > 
> Both i2cdetect and hexdump from touchscreen and touchpad devices were 
> working fine. I used linux-next next-20180108 and patches 4-7/7.
> 
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Thank you!