[PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Rafael J. Wysocki]

Hi Everyone,

This series is a result of the discussion on a recently reported issue
with device runtime PM status propagation during system resume and
the resulting patches:

https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/

Overall, due to restrictions related to pm_runtime_force_suspend() and
pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
setting propagation to the parent of the first device in a dependency
chain that turned out to have to be resumed during system resume even
though it was runtime-suspended before system suspend.

Those restrictions are that (1) pm_runtime_force_suspend() attempts to
suspend devices that have never had runtime PM enabled if their runtime
PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
will skip device whose runtime PM status is currently RPM_ACTIVE.

The purpose of this series is to eliminate the above restrictions and
get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
more with what the core does.

First off, it turns out that detecting devices that have never had
runtime PM enabled is not really hard - it is sufficient to check
their power.last_status data when runtime PM is disabled.  If
power.last_status is RPM_INVALID at that point, runtime PM has never
been enabled for the given device, so patch [01/10] adds a helper
function for checking that.

Patch [02/10] makes the PM core use the new function to avoid setting
power.set_active for devices with no runtime PM support which really
is a fixup on top of

https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/

Patch [03/10] modifies pm_runtime_force_suspend() to skip devices
with no runtime PM support with the help of the new function.

Next, patch [04/10] uses the observation that the runtime PM status
check in pm_runtime_force_resume() is redundant and drops that check.

Patch [05/10] removes the wakeirq enabling from the pm_runtime_force_resume()
error path because it is not really a good idea to enable wakeirqs during
system resume.

Patch [06/10] makes the PM core somewhat more consistent with
pm_runtime_force_suspend() and patch [07/10] prepares it for the subsequent
changes.

Patch [08/10] changes pm_runtime_force_resume() to handle the case in
which the runtime PM status of the device has been updated by the core to
RPM_ACTIVE after pm_runtime_force_suspend() left it in RPM_SUSPENDED.

Patch [09/10] restores the RPM_ACTIVE setting propagation to parents
and suppliers, but it takes exceptions into account (for example, devices
with no runtime PM support).

Finally, patch [10/10] adds a mechanism to discover cases in which runtime PM
is disabled for a device permanently even though it has been enabled for that
device at one point.

Please have a look and let me know if you see any problems.

Thanks!

[PATCH v1 01/10] PM: runtime: Introduce pm_runtime_no_support()

[Rafael J. Wysocki]

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

Introduce a new helper function called pm_runtime_no_support() for
checking the power.last_status value indicating whether or not runtime
PM has ever been enabled for the given device.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/linux/pm_runtime.h |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -182,6 +182,19 @@
 }
 
 /**
+ * pm_runtime_no_support - Check runtime PM support.
+ * @dev: Target device.
+ *
+ * Return %true if runtime PM is currently disabled for @dev and its last
+ * runtime PM status value is %RPM_INVALID, which means that runtime PM has
+ * never been enabled for it, or %false otherwise.
+ */
+static inline bool pm_runtime_no_support(struct device *dev)
+{
+	return dev->power.disable_depth && dev->power.last_status == RPM_INVALID;
+}
+
+/**
  * pm_runtime_enabled - Check if runtime PM is enabled.
  * @dev: Target device.
  *
@@ -284,6 +297,7 @@
 static inline bool pm_runtime_suspended(struct device *dev) { return false; }
 static inline bool pm_runtime_active(struct device *dev) { return true; }
 static inline bool pm_runtime_status_suspended(struct device *dev) { return false; }
+static inline bool pm_runtime_no_support(struct device *dev) { return true; }
 static inline bool pm_runtime_enabled(struct device *dev) { return false; }
 
 static inline void pm_runtime_no_callbacks(struct device *dev) {}

[PATCH v1 02/10] PM: sleep: core: Use pm_runtime_no_support() during set_active updates

[Rafael J. Wysocki]

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

It is pointless to set power.set_active for devices that have never had
runtime PM enabled, so don't do that.

Fixes: 7585946243d6 ("PM: sleep: core: Restrict power.set_active propagation")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1281,9 +1281,11 @@
 		dev->power.must_resume = true;
 
 	if (dev->power.must_resume) {
-		if (dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND)) {
+		if (dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) &&
+		    !pm_runtime_no_support(dev)) {
 			dev->power.set_active = true;
-			if (dev->parent && !dev->parent->power.ignore_children)
+			if (dev->parent && !dev->parent->power.ignore_children &&
+			    !pm_runtime_no_support(dev->parent))
 				dev->parent->power.set_active = true;
 		}
 		dpm_superior_set_must_resume(dev);

[PATCH v1 03/10] PM: runtime: Use pm_runtime_no_support() in pm_runtime_force_suspend()

[Rafael J. Wysocki]

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

Currently, pm_runtime_force_suspend() does not distinguish devices
without runtime PM enabled and it may be called for such devices
incidentally.  In that case, it works inconsistently depending on
the device runtime PM status value which generally is not expected
to be meaningful for devices with runtime PM permanently disabled.

Namely, if the runtime PM status of the device is RPM_SUSPENDED, it
will be ignored as though it had been runtime-suspended, but if its
runtime PM status is RPM_ACTIVE, pm_runtime_force_suspend() will
attempt to suspend it which may not work.

Make pm_runtime_force_suspend() more consistent by adding a
pm_runtime_no_support() check to it that will cause it to skip
devices with no runtime PM support.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/runtime.c |    6 +++++-
 include/linux/pm_runtime.h   |   14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1908,7 +1908,11 @@
 	int ret;
 
 	pm_runtime_disable(dev);
-	if (pm_runtime_status_suspended(dev))
+	/*
+	 * Do not attempt to suspend devices that have been suspended already or
+	 * that have never had runtime PM enabled.
+	 */
+	if (pm_runtime_status_suspended(dev) || pm_runtime_no_support(dev))
 		return 0;
 
 	callback = RPM_GET_CALLBACK(dev, runtime_suspend);

[PATCH v1 04/10] PM: runtime: Drop status check from pm_runtime_force_resume()

[Rafael J. Wysocki]

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

Since pm_runtime_force_resume() requires pm_runtime_force_suspend() to
be called before it on the same device, the runtime PM status of that
device is RPM_SUSPENDED when it is called unless the device's runtime
PM status is changed somewhere else in the meantime.

However, even if that happens, the power.needs_force_resume
check is still required to pass and that flag is only set by
pm_runtime_force_suspend() and it is cleared at the end of
pm_runtime_force_resume(), so it cannot be taken into account
twice in a row.

Accordingly, the pm_runtime_status_suspended(dev) check in
pm_runtime_force_resume() is redundant, so drop it.

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

--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1963,7 +1963,7 @@
 	int (*callback)(struct device *);
 	int ret = 0;
 
-	if (!pm_runtime_status_suspended(dev) || !dev->power.needs_force_resume)
+	if (!dev->power.needs_force_resume)
 		goto out;
 
 	/*

[PATCH v1 05/10] PM: runtime: Do not enable wakeup IRQs during system resume

[Rafael J. Wysocki]

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

Enabling system wakeup IRQs during system resume transitions is
not particularly useful, even on errors, so don't do it in the
pm_runtime_force_resume() error path.

Fixes: c46a0d5ae4f9 ("PM: runtime: Extend support for wakeirq for force_suspend|resume")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/runtime.c |    1 -
 1 file changed, 1 deletion(-)

--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1978,7 +1978,6 @@
 	ret = callback ? callback(dev) : 0;
 	if (ret) {
 		pm_runtime_set_suspended(dev);
-		dev_pm_enable_wake_irq_check(dev, false);
 		goto out;
 	}
 

[PATCH v1 06/10] PM: sleep: Adjust check before setting power.must_resume

[Rafael J. Wysocki]

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

Use pm_runtime_need_not_resume() in the check deciding whether or not
the device's power.must_resume flag needs to be set, so it covers the
device's active children counter in addition to its runtime PM usage
counter, rearrange that check and adjust the comment next to it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c    |   12 +++++-------
 drivers/base/power/runtime.c |    2 +-
 include/linux/pm_runtime.h   |    2 ++
 3 files changed, 8 insertions(+), 8 deletions(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1268,14 +1268,12 @@
 	dev->power.is_noirq_suspended = true;
 
 	/*
-	 * Skipping the resume of devices that were in use right before the
-	 * system suspend (as indicated by their PM-runtime usage counters)
-	 * would be suboptimal.  Also resume them if doing that is not allowed
-	 * to be skipped.
+	 * If it is not allowed to skip the resume of the device or it was in
+	 * use before the system suspend has started (in which case it needs to
+	 * be resumed for consistency), set the "must resume" flag for it.
 	 */
-	if (atomic_read(&dev->power.usage_count) > 1 ||
-	    !(dev_pm_test_driver_flags(dev, DPM_FLAG_MAY_SKIP_RESUME) &&
-	      dev->power.may_skip_resume))
+	if (!(dev_pm_test_driver_flags(dev, DPM_FLAG_MAY_SKIP_RESUME) &&
+	      dev->power.may_skip_resume) || !pm_runtime_need_not_resume(dev))
 		dev->power.must_resume = true;
 
 	if (dev->power.must_resume) {
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1874,7 +1874,7 @@
 	pm_request_idle(link->supplier);
 }
 
-static bool pm_runtime_need_not_resume(struct device *dev)
+bool pm_runtime_need_not_resume(struct device *dev)
 {
 	return atomic_read(&dev->power.usage_count) <= 1 &&
 		(atomic_read(&dev->power.child_count) == 0 ||
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -66,6 +66,7 @@
 
 extern int pm_generic_runtime_suspend(struct device *dev);
 extern int pm_generic_runtime_resume(struct device *dev);
+extern bool pm_runtime_need_not_resume(struct device *dev);
 extern int pm_runtime_force_suspend(struct device *dev);
 extern int pm_runtime_force_resume(struct device *dev);
 
@@ -254,6 +255,7 @@
 
 static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
 static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
+static inline bool pm_runtime_need_not_resume(struct device *dev) {return true; }
 static inline int pm_runtime_force_suspend(struct device *dev) { return 0; }
 static inline int pm_runtime_force_resume(struct device *dev) { return 0; }
 

[PATCH v1 07/10] PM: sleep: Clear the power.set_active upfront

[Rafael J. Wysocki]

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

Instead of clearing power.set_active right after it has been used,
clear it when the other related flags (power.may_skip_resume and
power.must_resume) are initialized, so it remains set throughout
system-wide resume transitions and can be checked at any time while
they are still in progress.

This is done in preparation for subsequent changes and it should not
alter the kernel behavior.

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

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -659,12 +659,10 @@
 	 * status to "active" unless its power.set_active flag is clear, in
 	 * which case it is not necessary to update its PM-runtime status.
 	 */
-	if (skip_resume) {
+	if (skip_resume)
 		pm_runtime_set_suspended(dev);
-	} else if (dev->power.set_active) {
+	else if (dev->power.set_active)
 		pm_runtime_set_active(dev);
-		dev->power.set_active = false;
-	}
 
 	if (dev->pm_domain) {
 		info = "noirq power domain ";
@@ -1662,6 +1660,7 @@
 
 	dev->power.may_skip_resume = true;
 	dev->power.must_resume = !dev_pm_test_driver_flags(dev, DPM_FLAG_MAY_SKIP_RESUME);
+	dev->power.set_active = false;
 
 	dpm_watchdog_set(&wd, dev);
 	device_lock(dev);

[PATCH v1 08/10] PM: sleep: Make pm_runtime_force_resume() look at power.set_active

[Rafael J. Wysocki]

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

In theory (and also in practice after a change to come),
pm_runtime_force_resume() can be called on a device with power.set_active
set, in which case the core has already called pm_runtime_set_active()
on it, and power.needs_force_resume may be clear.  This happens when the
core decides to resume a device because new information on it has become
available during the "noirq" phase of system-wide suspend.

In order to handle that case properly, make pm_runtime_force_resume()
look at power.set_active in addition to power.needs_force_resume, so it
does not skip the device when the former is set.  Namely, make it invoke
the callback for the device then, but without disabling the wake IRQ if
pm_runtime_force_resume() has not enabled it.  Also clear power.set_active
in pm_runtime_force_resume() to prevent it from being taken into account
twice in a row.

Additionally, adjust the pm_runtime_force_resume() kerneldoc comment
and the code comments inside it.  Moreover, remove a remark regarding
DPM_FLAG_SMART_SUSPEND from the pm_runtime_force_suspend() kerneldoc
comment because it is not valid any more after this change.

This change is not expected to alter the behavior in the case when
power.needs_force_resume is set.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Unfortunately, I have not found a way to do this without adding
a new device PM flag and now there are 2 flags specifically for
pm_runtime_force_suspend/resume() which is a bit sad.

Questions for Ulf:

(1) How is the enabling of wakeirqs handled for devices that are runtime-
    suspended before system suspend, so pm_runtime_force_suspend() skips
    them?

(2) What is supposed to happen to wakeirqs during system resume after
    pm_runtime_force_suspend() has enabled them, but hasn't set
    power.needs_force_resume at the same time?

---
 drivers/base/power/runtime.c |   41 ++++++++++++++++++++++++++---------------
 include/linux/pm.h           |    1 +
 2 files changed, 27 insertions(+), 15 deletions(-)

--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1897,10 +1897,6 @@
  * sure the device is put into low power state and it should only be used during
  * system-wide PM transitions to sleep states.  It assumes that the analogous
  * pm_runtime_force_resume() will be used to resume the device.
- *
- * Do not use with DPM_FLAG_SMART_SUSPEND as this can lead to an inconsistent
- * state where this function has called the ->runtime_suspend callback but the
- * PM core marks the driver as runtime active.
  */
 int pm_runtime_force_suspend(struct device *dev)
 {
@@ -1923,6 +1919,7 @@
 		goto err;
 
 	dev_pm_enable_wake_irq_complete(dev);
+	dev->power.wake_irq_enabled = true;
 
 	/*
 	 * If the device can stay in suspend after the system-wide transition
@@ -1950,31 +1947,39 @@
  * pm_runtime_force_resume - Force a device into resume state if needed.
  * @dev: Device to resume.
  *
- * Prior invoking this function we expect the user to have brought the device
- * into low power state by a call to pm_runtime_force_suspend(). Here we reverse
- * those actions and bring the device into full power, if it is expected to be
- * used on system resume.  In the other case, we defer the resume to be managed
- * via runtime PM.
+ * The primary role of this function is to reverse the actions carried out by
+ * pm_runtime_force_suspend() for @dev, so it must always be balanced with a
+ * matching invocation of the latter.  Accordingly, it is only valid to call
+ * this function during system-wide resume transitions.
+ *
+ * Typically, it is used as a system resume callback of a device driver.
  *
- * Typically this function may be invoked from a system resume callback.
+ * However, if @dev had been runtime-suspended before pm_runtime_force_suspend()
+ * was called for it and that function did nothing, but power.set_active has
+ * been set for it by the core, it still needs to be resumed.  That special case
+ * is also handled by this function.
  */
 int pm_runtime_force_resume(struct device *dev)
 {
 	int (*callback)(struct device *);
 	int ret = 0;
 
-	if (!dev->power.needs_force_resume)
+	if (!dev->power.needs_force_resume && !dev->power.set_active)
 		goto out;
 
 	/*
-	 * The value of the parent's children counter is correct already, so
-	 * just update the status of the device.
+	 * The parent's active children counter an the suppliers' usage counters
+	 * are correct already, so just update the status (even though it is
+	 * already RPM_ACTIVE if power.set_active is set).
 	 */
 	__update_runtime_status(dev, RPM_ACTIVE);
 
-	callback = RPM_GET_CALLBACK(dev, runtime_resume);
+	if (dev->power.wake_irq_enabled) {
+		dev_pm_disable_wake_irq_check(dev, false);
+		dev->power.wake_irq_enabled = false;
+	}
 
-	dev_pm_disable_wake_irq_check(dev, false);
+	callback = RPM_GET_CALLBACK(dev, runtime_resume);
 	ret = callback ? callback(dev) : 0;
 	if (ret) {
 		pm_runtime_set_suspended(dev);
@@ -1982,6 +1987,12 @@
 	}
 
 	pm_runtime_mark_last_busy(dev);
+	/*
+	 * Clear power.set_active in case this function runs for the same
+	 * device again.
+	 */
+	dev->power.set_active = false;
+
 out:
 	dev->power.needs_force_resume = 0;
 	pm_runtime_enable(dev);
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -698,6 +698,7 @@
 	bool			request_pending:1;
 	bool			deferred_resume:1;
 	bool			needs_force_resume:1;
+	bool			wake_irq_enabled:1;
 	bool			runtime_auto:1;
 	bool			ignore_children:1;
 	bool			no_callbacks:1;

[PATCH v1 09/10] PM: sleep: Propagate power.set_active in dependency chains

[Rafael J. Wysocki]

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

After preparing pm_runtime_force_resume() for dealing with devices
having power.set_active set, make the core propagate that flag in
dependency chains, so that subordinate device are resumed along with
the ones that they depend on, but take exceptions into account.

Namely, do not set power.set_active for devices that have never had
runtime PM enabled, for parents that have power.ignore_children set and
for suppliers coming from device links with DL_FLAG_PM_RUNTIME unset.

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

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1189,18 +1189,31 @@
 	return PMSG_ON;
 }
 
+static void dpm_cond_set_active(struct device *dev, bool cond)
+{
+	if (cond && !pm_runtime_no_support(dev))
+		dev->power.set_active = true;
+}
+
 static void dpm_superior_set_must_resume(struct device *dev)
 {
+	bool set_active = dev->power.set_active;
 	struct device_link *link;
 	int idx;
 
-	if (dev->parent)
+	if (dev->parent) {
 		dev->parent->power.must_resume = true;
+		dpm_cond_set_active(dev->parent, set_active &&
+				    !dev->parent->power.ignore_children);
+	}
 
 	idx = device_links_read_lock();
 
-	list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node)
+	list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) {
 		link->supplier->power.must_resume = true;
+		dpm_cond_set_active(link->supplier, set_active &&
+				    (link->flags & DL_FLAG_PM_RUNTIME));
+	}
 
 	device_links_read_unlock(idx);
 }
@@ -1277,13 +1290,7 @@
 		dev->power.must_resume = true;
 
 	if (dev->power.must_resume) {
-		if (dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) &&
-		    !pm_runtime_no_support(dev)) {
-			dev->power.set_active = true;
-			if (dev->parent && !dev->parent->power.ignore_children &&
-			    !pm_runtime_no_support(dev->parent))
-				dev->parent->power.set_active = true;
-		}
+		dpm_cond_set_active(dev, dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND));
 		dpm_superior_set_must_resume(dev);
 	}
 

[PATCH v1 10/10] PM: runtime: Discover the lack of runtime PM support

[Rafael J. Wysocki]

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

Previous changes have updated the PM core to special-case devices that
have never had runtime PM enabled in some places, but what if a device
had had runtime PM enabled at one point, but then it was permanently
disabled?  Arguably, there is not much of a difference between such
devices and the devices that have never had runtime PM enabled as far
as system-wide suspend and resume is concerned, so they should be
handled in the same way.

For this reason, add a mechanism for discovering "lost" runtime PM
support in devices with the help of the power.last_status field used
for saving the last runtime PM status of the device known at the time
when runtime PM was disabled for it.

That field is set to RPM_INVALID initially and whenever runtime PM is
enabled for a device (that is, when its power.disable_depth counter
drops down to zero) and it is set to the current runtime PM status of
the device when runtime PM is disabled (that is, the power.disable_depth
counter becomes nonzero).  Therefore, if power.last_status is equal to
RPM_INVALID for a device with runtime PM disabled, it means that
runtime PM has never been enabled for that device.

The PM core will now change the power.last_status value to RPM_UNKNOWN
for devices having runtime PM disabled and power.last_status different
from RPM_INVALID during the "prepare" phase of system suspend.  Then,
__pm_runtime_disable() called subsequently on the device will set
power.last_status to RPM_INVALID unless it changes from RPM_UNKNOWN
to some other value in the meantime which requires enabling runtime PM
for the device.  When power.last_status becomes RPM_INVALID and runtime
PM is still disabled, the device will be handled as a "no runtime PM
support" one from that point on until runtime PM is enabled for it
again.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c    |    6 ++++++
 drivers/base/power/runtime.c |   25 +++++++++++++++++++++++++
 include/linux/pm.h           |    1 +
 include/linux/pm_runtime.h   |    2 ++
 4 files changed, 34 insertions(+)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1817,6 +1817,12 @@
 	 * it again during the complete phase.
 	 */
 	pm_runtime_get_noresume(dev);
+	/*
+	 * Devices that have had runtime PM disabled recently may need to be
+	 * handled as though they have never supported it, so arrange for
+	 * detecting that situation.
+	 */
+	pm_runtime_kick_last_status(dev);
 
 	if (dev->power.syscore)
 		return 0;
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1480,6 +1480,9 @@
 
 	if (dev->power.disable_depth > 0) {
 		dev->power.disable_depth++;
+		if (dev->power.last_status == RPM_UNKNOWN)
+			dev->power.last_status = RPM_INVALID;
+
 		goto out;
 	}
 
@@ -1568,6 +1571,28 @@
 EXPORT_SYMBOL_GPL(devm_pm_runtime_enable);
 
 /**
+ * pm_runtime_kick_last_status - Start runtime PM support verification.
+ * @dev: Target device.
+ *
+ * If runtime PM is currently disabled for @dev, but it has been enabled at one
+ * point, change power.last_status for it to RPM_UNKNOWN, and if it is still
+ * RPM_UNKNOWN when __pm_runtime_disabled() is called for @dev next time, it
+ * will be changed to RPM_INVALID indicating no runtime PM support going
+ * forward until pm_runtime_enable() is called for @dev.
+ *
+ * This function is used by the PM core.
+ */
+void pm_runtime_kick_last_status(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+
+	if (dev->power.disable_depth && dev->power.last_status != RPM_INVALID)
+		dev->power.last_status = RPM_UNKNOWN;
+
+	spin_unlock_irq(&dev->power.lock);
+}
+
+/**
  * pm_runtime_forbid - Block runtime PM of a device.
  * @dev: Device to handle.
  *
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -597,6 +597,7 @@
 	RPM_RESUMING,
 	RPM_SUSPENDED,
 	RPM_SUSPENDING,
+	RPM_UNKNOWN,
 };
 
 /*
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -80,6 +80,7 @@
 extern int pm_runtime_barrier(struct device *dev);
 extern void pm_runtime_enable(struct device *dev);
 extern void __pm_runtime_disable(struct device *dev, bool check_resume);
+extern void pm_runtime_kick_last_status(struct device *dev);
 extern void pm_runtime_allow(struct device *dev);
 extern void pm_runtime_forbid(struct device *dev);
 extern void pm_runtime_no_callbacks(struct device *dev);
@@ -288,6 +289,7 @@
 static inline int pm_runtime_barrier(struct device *dev) { return 0; }
 static inline void pm_runtime_enable(struct device *dev) {}
 static inline void __pm_runtime_disable(struct device *dev, bool c) {}
+static inline void pm_runtime_kick_last_status(struct device *dev) {}
 static inline void pm_runtime_allow(struct device *dev) {}
 static inline void pm_runtime_forbid(struct device *dev) {}
 

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Ulf Hansson]

On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi Everyone,
>
> This series is a result of the discussion on a recently reported issue
> with device runtime PM status propagation during system resume and
> the resulting patches:
>
> https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
>
> Overall, due to restrictions related to pm_runtime_force_suspend() and
> pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> setting propagation to the parent of the first device in a dependency
> chain that turned out to have to be resumed during system resume even
> though it was runtime-suspended before system suspend.
>
> Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> suspend devices that have never had runtime PM enabled if their runtime
> PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> will skip device whose runtime PM status is currently RPM_ACTIVE.
>
> The purpose of this series is to eliminate the above restrictions and
> get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> more with what the core does.

For my understanding, would you mind elaborating a bit more around the
end-goal with this?

Are you trying to make adaptations for
pm_runtime_force_suspend|resume() and the PM core, such that drivers
that uses pm_runtime_force_suspend|resume() should be able to cope
with other drivers for child-devices that make use of
DPM_FLAG_SMART_SUSPEND?

If we can make this work, it would enable the propagation of
RPM_ACTIVE in the PM core for more devices, but still not for all,
right?

The point is, the other bigger issue that I pointed out in our earlier
discussions; all those devices where their drivers/buses don't cope
with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
the PM core from *unconditionally* propagating the RPM_ACTIVE to
parents. I guess this is the best we can do then?

>
> First off, it turns out that detecting devices that have never had
> runtime PM enabled is not really hard - it is sufficient to check
> their power.last_status data when runtime PM is disabled.  If
> power.last_status is RPM_INVALID at that point, runtime PM has never
> been enabled for the given device, so patch [01/10] adds a helper
> function for checking that.
>
> Patch [02/10] makes the PM core use the new function to avoid setting
> power.set_active for devices with no runtime PM support which really
> is a fixup on top of
>
> https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
>
> Patch [03/10] modifies pm_runtime_force_suspend() to skip devices
> with no runtime PM support with the help of the new function.
>
> Next, patch [04/10] uses the observation that the runtime PM status
> check in pm_runtime_force_resume() is redundant and drops that check.
>
> Patch [05/10] removes the wakeirq enabling from the pm_runtime_force_resume()
> error path because it is not really a good idea to enable wakeirqs during
> system resume.
>
> Patch [06/10] makes the PM core somewhat more consistent with
> pm_runtime_force_suspend() and patch [07/10] prepares it for the subsequent
> changes.
>
> Patch [08/10] changes pm_runtime_force_resume() to handle the case in
> which the runtime PM status of the device has been updated by the core to
> RPM_ACTIVE after pm_runtime_force_suspend() left it in RPM_SUSPENDED.
>
> Patch [09/10] restores the RPM_ACTIVE setting propagation to parents
> and suppliers, but it takes exceptions into account (for example, devices
> with no runtime PM support).
>
> Finally, patch [10/10] adds a mechanism to discover cases in which runtime PM
> is disabled for a device permanently even though it has been enabled for that
> device at one point.
>
> Please have a look and let me know if you see any problems.
>
> Thanks!

Kind regards
Uffe

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Rafael J. Wysocki]

On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > Hi Everyone,
> >
> > This series is a result of the discussion on a recently reported issue
> > with device runtime PM status propagation during system resume and
> > the resulting patches:
> >
> > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> >
> > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > setting propagation to the parent of the first device in a dependency
> > chain that turned out to have to be resumed during system resume even
> > though it was runtime-suspended before system suspend.
> >
> > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > suspend devices that have never had runtime PM enabled if their runtime
> > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > will skip device whose runtime PM status is currently RPM_ACTIVE.
> >
> > The purpose of this series is to eliminate the above restrictions and
> > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > more with what the core does.
>
> For my understanding, would you mind elaborating a bit more around the
> end-goal with this?

The end goal is, of course, full integration of runtime PM with system
sleep for all devices.  Eventually.

And it is necessary to make the core and
pm_runtime_force_suspend|resume() work together better for this
purpose.

> Are you trying to make adaptations for
> pm_runtime_force_suspend|resume() and the PM core, such that drivers
> that uses pm_runtime_force_suspend|resume() should be able to cope
> with other drivers for child-devices that make use of
> DPM_FLAG_SMART_SUSPEND?

Yes.

This is a more general case, though, when a device that was
runtime-suspended before system suspend and is left in suspend during
it, needs to be resumed during the system resume that follows.

Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
cannot happen otherwise, but I think that it is a generally valid
case.

> If we can make this work, it would enable the propagation of
> RPM_ACTIVE in the PM core for more devices, but still not for all,
> right?

It is all until there is a known case in which it isn't.  So either
you know a specific case in which it doesn't work, or I don't see a
reason for avoiding it.

ATM the only specific case in which it doesn't work is when
pm_runtime_force_suspend|resume() are used.

> The point is, the other bigger issue that I pointed out in our earlier
> discussions; all those devices where their drivers/buses don't cope
> with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> the PM core from *unconditionally* propagating the RPM_ACTIVE to
> parents. I guess this is the best we can do then?

OK, what are they?

You keep saying that they exist without giving any examples.

Re: [PATCH v1 10/10] PM: runtime: Discover the lack of runtime PM support

[Rafael J. Wysocki]

On Tue, Feb 11, 2025 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Previous changes have updated the PM core to special-case devices that
> have never had runtime PM enabled in some places, but what if a device
> had had runtime PM enabled at one point, but then it was permanently
> disabled?  Arguably, there is not much of a difference between such
> devices and the devices that have never had runtime PM enabled as far
> as system-wide suspend and resume is concerned, so they should be
> handled in the same way.
>
> For this reason, add a mechanism for discovering "lost" runtime PM
> support in devices with the help of the power.last_status field used
> for saving the last runtime PM status of the device known at the time
> when runtime PM was disabled for it.
>
> That field is set to RPM_INVALID initially and whenever runtime PM is
> enabled for a device (that is, when its power.disable_depth counter
> drops down to zero) and it is set to the current runtime PM status of
> the device when runtime PM is disabled (that is, the power.disable_depth
> counter becomes nonzero).  Therefore, if power.last_status is equal to
> RPM_INVALID for a device with runtime PM disabled, it means that
> runtime PM has never been enabled for that device.
>
> The PM core will now change the power.last_status value to RPM_UNKNOWN
> for devices having runtime PM disabled and power.last_status different
> from RPM_INVALID during the "prepare" phase of system suspend.  Then,
> __pm_runtime_disable() called subsequently on the device will set
> power.last_status to RPM_INVALID unless it changes from RPM_UNKNOWN
> to some other value in the meantime which requires enabling runtime PM
> for the device.  When power.last_status becomes RPM_INVALID and runtime
> PM is still disabled, the device will be handled as a "no runtime PM
> support" one from that point on until runtime PM is enabled for it
> again.

So the interim RPM_UNKNOWN value of last_status isn't really
necessary, it may as well be changed directly to RPM_INVALID in
device_prepare().

Scratch this one and I'll replace it with a different patch.

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/main.c    |    6 ++++++
>  drivers/base/power/runtime.c |   25 +++++++++++++++++++++++++
>  include/linux/pm.h           |    1 +
>  include/linux/pm_runtime.h   |    2 ++
>  4 files changed, 34 insertions(+)
>
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1817,6 +1817,12 @@
>          * it again during the complete phase.
>          */
>         pm_runtime_get_noresume(dev);
> +       /*
> +        * Devices that have had runtime PM disabled recently may need to be
> +        * handled as though they have never supported it, so arrange for
> +        * detecting that situation.
> +        */
> +       pm_runtime_kick_last_status(dev);
>
>         if (dev->power.syscore)
>                 return 0;
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1480,6 +1480,9 @@
>
>         if (dev->power.disable_depth > 0) {
>                 dev->power.disable_depth++;
> +               if (dev->power.last_status == RPM_UNKNOWN)
> +                       dev->power.last_status = RPM_INVALID;
> +
>                 goto out;
>         }
>
> @@ -1568,6 +1571,28 @@
>  EXPORT_SYMBOL_GPL(devm_pm_runtime_enable);
>
>  /**
> + * pm_runtime_kick_last_status - Start runtime PM support verification.
> + * @dev: Target device.
> + *
> + * If runtime PM is currently disabled for @dev, but it has been enabled at one
> + * point, change power.last_status for it to RPM_UNKNOWN, and if it is still
> + * RPM_UNKNOWN when __pm_runtime_disabled() is called for @dev next time, it
> + * will be changed to RPM_INVALID indicating no runtime PM support going
> + * forward until pm_runtime_enable() is called for @dev.
> + *
> + * This function is used by the PM core.
> + */
> +void pm_runtime_kick_last_status(struct device *dev)
> +{
> +       spin_lock_irq(&dev->power.lock);
> +
> +       if (dev->power.disable_depth && dev->power.last_status != RPM_INVALID)
> +               dev->power.last_status = RPM_UNKNOWN;
> +
> +       spin_unlock_irq(&dev->power.lock);
> +}
> +
> +/**
>   * pm_runtime_forbid - Block runtime PM of a device.
>   * @dev: Device to handle.
>   *
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -597,6 +597,7 @@
>         RPM_RESUMING,
>         RPM_SUSPENDED,
>         RPM_SUSPENDING,
> +       RPM_UNKNOWN,
>  };
>
>  /*
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -80,6 +80,7 @@
>  extern int pm_runtime_barrier(struct device *dev);
>  extern void pm_runtime_enable(struct device *dev);
>  extern void __pm_runtime_disable(struct device *dev, bool check_resume);
> +extern void pm_runtime_kick_last_status(struct device *dev);
>  extern void pm_runtime_allow(struct device *dev);
>  extern void pm_runtime_forbid(struct device *dev);
>  extern void pm_runtime_no_callbacks(struct device *dev);
> @@ -288,6 +289,7 @@
>  static inline int pm_runtime_barrier(struct device *dev) { return 0; }
>  static inline void pm_runtime_enable(struct device *dev) {}
>  static inline void __pm_runtime_disable(struct device *dev, bool c) {}
> +static inline void pm_runtime_kick_last_status(struct device *dev) {}
>  static inline void pm_runtime_allow(struct device *dev) {}
>  static inline void pm_runtime_forbid(struct device *dev) {}
>
>
>
>
>

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Rafael J. Wysocki]

On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > Hi Everyone,
> > >
> > > This series is a result of the discussion on a recently reported issue
> > > with device runtime PM status propagation during system resume and
> > > the resulting patches:
> > >
> > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > >
> > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > setting propagation to the parent of the first device in a dependency
> > > chain that turned out to have to be resumed during system resume even
> > > though it was runtime-suspended before system suspend.
> > >
> > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > suspend devices that have never had runtime PM enabled if their runtime
> > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > >
> > > The purpose of this series is to eliminate the above restrictions and
> > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > more with what the core does.
> >
> > For my understanding, would you mind elaborating a bit more around the
> > end-goal with this?
>
> The end goal is, of course, full integration of runtime PM with system
> sleep for all devices.  Eventually.
>
> And it is necessary to make the core and
> pm_runtime_force_suspend|resume() work together better for this
> purpose.
>
> > Are you trying to make adaptations for
> > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > that uses pm_runtime_force_suspend|resume() should be able to cope
> > with other drivers for child-devices that make use of
> > DPM_FLAG_SMART_SUSPEND?
>
> Yes.
>
> This is a more general case, though, when a device that was
> runtime-suspended before system suspend and is left in suspend during
> it, needs to be resumed during the system resume that follows.
>
> Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> cannot happen otherwise, but I think that it is a generally valid
> case.
>
> > If we can make this work, it would enable the propagation of
> > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > right?
>
> It is all until there is a known case in which it isn't.  So either
> you know a specific case in which it doesn't work, or I don't see a
> reason for avoiding it.
>
> ATM the only specific case in which it doesn't work is when
> pm_runtime_force_suspend|resume() are used.
>
> > The point is, the other bigger issue that I pointed out in our earlier
> > discussions; all those devices where their drivers/buses don't cope
> > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > parents. I guess this is the best we can do then?
>
> OK, what are they?
>
> You keep saying that they exist without giving any examples.

To put it more bluntly, I'm not aware of any place other than
pm_runtime_force_suspend|resume() that can be confused by changing the
runtime PM status of a device with runtime PM disabled (either
permanently, or transiently during a system suspend-resume cycle) to
RPM_ACTIVE, so if there are any such places, I would appreciate
letting me know what they are.

Note that rpm_active() could start producing confusing results if the
runtime PM status of a device with runtime PM disabled is changed from
RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
-EACCES instead of 1, but changing the status to RPM_ACTIVE will not
confuse it the same way.

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Rafael J. Wysocki]

On Wed, Feb 12, 2025 at 12:33 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > Hi Everyone,
> > > >
> > > > This series is a result of the discussion on a recently reported issue
> > > > with device runtime PM status propagation during system resume and
> > > > the resulting patches:
> > > >
> > > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > > >
> > > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > > setting propagation to the parent of the first device in a dependency
> > > > chain that turned out to have to be resumed during system resume even
> > > > though it was runtime-suspended before system suspend.
> > > >
> > > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > > suspend devices that have never had runtime PM enabled if their runtime
> > > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > > >
> > > > The purpose of this series is to eliminate the above restrictions and
> > > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > > more with what the core does.
> > >
> > > For my understanding, would you mind elaborating a bit more around the
> > > end-goal with this?
> >
> > The end goal is, of course, full integration of runtime PM with system
> > sleep for all devices.  Eventually.
> >
> > And it is necessary to make the core and
> > pm_runtime_force_suspend|resume() work together better for this
> > purpose.
> >
> > > Are you trying to make adaptations for
> > > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > > that uses pm_runtime_force_suspend|resume() should be able to cope
> > > with other drivers for child-devices that make use of
> > > DPM_FLAG_SMART_SUSPEND?
> >
> > Yes.
> >
> > This is a more general case, though, when a device that was
> > runtime-suspended before system suspend and is left in suspend during
> > it, needs to be resumed during the system resume that follows.
> >
> > Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> > cannot happen otherwise, but I think that it is a generally valid
> > case.
> >
> > > If we can make this work, it would enable the propagation of
> > > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > > right?
> >
> > It is all until there is a known case in which it isn't.  So either
> > you know a specific case in which it doesn't work, or I don't see a
> > reason for avoiding it.
> >
> > ATM the only specific case in which it doesn't work is when
> > pm_runtime_force_suspend|resume() are used.
> >
> > > The point is, the other bigger issue that I pointed out in our earlier
> > > discussions; all those devices where their drivers/buses don't cope
> > > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > > parents. I guess this is the best we can do then?
> >
> > OK, what are they?
> >
> > You keep saying that they exist without giving any examples.
>
> To put it more bluntly, I'm not aware of any place other than
> pm_runtime_force_suspend|resume() that can be confused by changing the
> runtime PM status of a device with runtime PM disabled (either
> permanently, or transiently during a system suspend-resume cycle) to
> RPM_ACTIVE, so if there are any such places, I would appreciate
> letting me know what they are.
>
> Note that rpm_active() could start producing confusing results if the

rpm_resume() rather, sorry.

> runtime PM status of a device with runtime PM disabled is changed from
> RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
> -EACCES instead of 1, but changing the status to RPM_ACTIVE will not
> confuse it the same way.

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Ulf Hansson]

On Wed, 12 Feb 2025 at 12:33, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > Hi Everyone,
> > > >
> > > > This series is a result of the discussion on a recently reported issue
> > > > with device runtime PM status propagation during system resume and
> > > > the resulting patches:
> > > >
> > > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > > >
> > > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > > setting propagation to the parent of the first device in a dependency
> > > > chain that turned out to have to be resumed during system resume even
> > > > though it was runtime-suspended before system suspend.
> > > >
> > > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > > suspend devices that have never had runtime PM enabled if their runtime
> > > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > > >
> > > > The purpose of this series is to eliminate the above restrictions and
> > > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > > more with what the core does.
> > >
> > > For my understanding, would you mind elaborating a bit more around the
> > > end-goal with this?
> >
> > The end goal is, of course, full integration of runtime PM with system
> > sleep for all devices.  Eventually.
> >
> > And it is necessary to make the core and
> > pm_runtime_force_suspend|resume() work together better for this
> > purpose.
> >
> > > Are you trying to make adaptations for
> > > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > > that uses pm_runtime_force_suspend|resume() should be able to cope
> > > with other drivers for child-devices that make use of
> > > DPM_FLAG_SMART_SUSPEND?
> >
> > Yes.
> >
> > This is a more general case, though, when a device that was
> > runtime-suspended before system suspend and is left in suspend during
> > it, needs to be resumed during the system resume that follows.
> >
> > Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> > cannot happen otherwise, but I think that it is a generally valid
> > case.
> >
> > > If we can make this work, it would enable the propagation of
> > > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > > right?
> >
> > It is all until there is a known case in which it isn't.  So either
> > you know a specific case in which it doesn't work, or I don't see a
> > reason for avoiding it.
> >
> > ATM the only specific case in which it doesn't work is when
> > pm_runtime_force_suspend|resume() are used.
> >
> > > The point is, the other bigger issue that I pointed out in our earlier
> > > discussions; all those devices where their drivers/buses don't cope
> > > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > > parents. I guess this is the best we can do then?
> >
> > OK, what are they?
> >
> > You keep saying that they exist without giving any examples.
>
> To put it more bluntly, I'm not aware of any place other than
> pm_runtime_force_suspend|resume() that can be confused by changing the
> runtime PM status of a device with runtime PM disabled (either
> permanently, or transiently during a system suspend-resume cycle) to
> RPM_ACTIVE, so if there are any such places, I would appreciate
> letting me know what they are.

Well, sorry I thought you were aware. Anyway, I believe you need to do
your own investigation as it's simply too time consuming for me to
find them all. The problem is that it's not just a simple pattern to
search for, so we would need some clever scripting to move forward to
find them.

To start with, we should look for drivers that enable runtime PM, by
calling pm_runtime_enable().

Additionally, in their system suspend callback they should typically
*not* use pm_runtime_suspended(), pm_runtime_status_suspended() or
pm_runtime_active() as that is usually (but no always) indicating that
they got it right. Then there are those that don't have system
suspend/resume callbacks assigned at all (or uses some other subsystem
specific callback for this), but only uses runtime PM.

On top of that, drivers that makes use of
pm_runtime_force_suspend|resume() should be disregarded, which has
reached beyond 300 as this point.

Anyway, here is just one example that I found from a quick search.
drivers/i2c/busses/i2c-qcom-geni.c

>
> Note that rpm_active() could start producing confusing results if the
> runtime PM status of a device with runtime PM disabled is changed from
> RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
> -EACCES instead of 1, but changing the status to RPM_ACTIVE will not
> confuse it the same way.

Trust me, it will cause problems.

Drivers may call pm_runtime_get_sync() to turn on the resources for
the device after the system has resumed, when runtime PM has been
re-enabled for the device by the PM core.

Those calls to pm_runtime_get_sync() will then not end up invoking any
if ->runtime_resume() callbacks for the device since its state is
already RPM_ACTIVE. Hence, the device will remain in a low power state
even if the driver believes it has been powered on. In many cases,
accessing the device (like reading/writing a register) will often just
just hang in these cases, but in worst cases we could end up getting
even more difficult bugs to debug.

Kind regards
Uffe

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Rafael J. Wysocki]

On Wed, Feb 12, 2025 at 4:15 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 12 Feb 2025 at 12:33, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > >
> > > > > Hi Everyone,
> > > > >
> > > > > This series is a result of the discussion on a recently reported issue
> > > > > with device runtime PM status propagation during system resume and
> > > > > the resulting patches:
> > > > >
> > > > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > > > >
> > > > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > > > setting propagation to the parent of the first device in a dependency
> > > > > chain that turned out to have to be resumed during system resume even
> > > > > though it was runtime-suspended before system suspend.
> > > > >
> > > > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > > > suspend devices that have never had runtime PM enabled if their runtime
> > > > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > > > >
> > > > > The purpose of this series is to eliminate the above restrictions and
> > > > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > > > more with what the core does.
> > > >
> > > > For my understanding, would you mind elaborating a bit more around the
> > > > end-goal with this?
> > >
> > > The end goal is, of course, full integration of runtime PM with system
> > > sleep for all devices.  Eventually.
> > >
> > > And it is necessary to make the core and
> > > pm_runtime_force_suspend|resume() work together better for this
> > > purpose.
> > >
> > > > Are you trying to make adaptations for
> > > > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > > > that uses pm_runtime_force_suspend|resume() should be able to cope
> > > > with other drivers for child-devices that make use of
> > > > DPM_FLAG_SMART_SUSPEND?
> > >
> > > Yes.
> > >
> > > This is a more general case, though, when a device that was
> > > runtime-suspended before system suspend and is left in suspend during
> > > it, needs to be resumed during the system resume that follows.
> > >
> > > Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> > > cannot happen otherwise, but I think that it is a generally valid
> > > case.
> > >
> > > > If we can make this work, it would enable the propagation of
> > > > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > > > right?
> > >
> > > It is all until there is a known case in which it isn't.  So either
> > > you know a specific case in which it doesn't work, or I don't see a
> > > reason for avoiding it.
> > >
> > > ATM the only specific case in which it doesn't work is when
> > > pm_runtime_force_suspend|resume() are used.
> > >
> > > > The point is, the other bigger issue that I pointed out in our earlier
> > > > discussions; all those devices where their drivers/buses don't cope
> > > > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > > > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > > > parents. I guess this is the best we can do then?
> > >
> > > OK, what are they?
> > >
> > > You keep saying that they exist without giving any examples.
> >
> > To put it more bluntly, I'm not aware of any place other than
> > pm_runtime_force_suspend|resume() that can be confused by changing the
> > runtime PM status of a device with runtime PM disabled (either
> > permanently, or transiently during a system suspend-resume cycle) to
> > RPM_ACTIVE, so if there are any such places, I would appreciate
> > letting me know what they are.
>
> Well, sorry I thought you were aware. Anyway, I believe you need to do
> your own investigation as it's simply too time consuming for me to
> find them all. The problem is that it's not just a simple pattern to
> search for, so we would need some clever scripting to move forward to
> find them.
>
> To start with, we should look for drivers that enable runtime PM, by
> calling pm_runtime_enable().
>
> Additionally, in their system suspend callback they should typically
> *not* use pm_runtime_suspended(), pm_runtime_status_suspended() or
> pm_runtime_active() as that is usually (but no always) indicating that
> they got it right. Then there are those that don't have system
> suspend/resume callbacks assigned at all (or uses some other subsystem
> specific callback for this), but only uses runtime PM.
>
> On top of that, drivers that makes use of
> pm_runtime_force_suspend|resume() should be disregarded, which has
> reached beyond 300 as this point.
>
> Anyway, here is just one example that I found from a quick search.
> drivers/i2c/busses/i2c-qcom-geni.c

OK, I see.

It sets the status to RPM_SUSPENDED in geni_i2c_suspend_noirq(), if
not suspended already, and assumes it to stay this way across
geni_i2c_resume_noirq() until someone resumes it via runtime PM.

Fair enough, but somebody should tell them that they don't need to use
pm_runtime_disable/enable() in _noirq.

> >
> > Note that rpm_active() could start producing confusing results if the
> > runtime PM status of a device with runtime PM disabled is changed from
> > RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
> > -EACCES instead of 1, but changing the status to RPM_ACTIVE will not
> > confuse it the same way.
>
> Trust me, it will cause problems.
>
> Drivers may call pm_runtime_get_sync() to turn on the resources for
> the device after the system has resumed, when runtime PM has been
> re-enabled for the device by the PM core.
>
> Those calls to pm_runtime_get_sync() will then not end up invoking any
> if ->runtime_resume() callbacks for the device since its state is
> already RPM_ACTIVE. Hence, the device will remain in a low power state
> even if the driver believes it has been powered on. In many cases,
> accessing the device (like reading/writing a register) will often just
> just hang in these cases, but in worst cases we could end up getting
> even more difficult bugs to debug.

Sure, that would be a problem.

I think I need to find a different way to address the problem I'm
seeing, that is to resume devices that were runtime-suspended before
system suspend along with their superiors.

One way to do it would be to just defer their resume to the point when
the core has enabled runtime PM for them, which means that it has also
enabled runtime PM for all of their superiors, and then call
pm_runtime_resume().

This should work unless one of the superiors has runtime PM disabled
at that point, of course.

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Rafael J. Wysocki]

On Wed, Feb 12, 2025 at 6:05 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 4:15 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 12 Feb 2025 at 12:33, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > >
> > > > > > Hi Everyone,
> > > > > >
> > > > > > This series is a result of the discussion on a recently reported issue
> > > > > > with device runtime PM status propagation during system resume and
> > > > > > the resulting patches:
> > > > > >
> > > > > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > > > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > > > > >
> > > > > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > > > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > > > > setting propagation to the parent of the first device in a dependency
> > > > > > chain that turned out to have to be resumed during system resume even
> > > > > > though it was runtime-suspended before system suspend.
> > > > > >
> > > > > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > > > > suspend devices that have never had runtime PM enabled if their runtime
> > > > > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > > > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > > > > >
> > > > > > The purpose of this series is to eliminate the above restrictions and
> > > > > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > > > > more with what the core does.
> > > > >
> > > > > For my understanding, would you mind elaborating a bit more around the
> > > > > end-goal with this?
> > > >
> > > > The end goal is, of course, full integration of runtime PM with system
> > > > sleep for all devices.  Eventually.
> > > >
> > > > And it is necessary to make the core and
> > > > pm_runtime_force_suspend|resume() work together better for this
> > > > purpose.
> > > >
> > > > > Are you trying to make adaptations for
> > > > > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > > > > that uses pm_runtime_force_suspend|resume() should be able to cope
> > > > > with other drivers for child-devices that make use of
> > > > > DPM_FLAG_SMART_SUSPEND?
> > > >
> > > > Yes.
> > > >
> > > > This is a more general case, though, when a device that was
> > > > runtime-suspended before system suspend and is left in suspend during
> > > > it, needs to be resumed during the system resume that follows.
> > > >
> > > > Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> > > > cannot happen otherwise, but I think that it is a generally valid
> > > > case.
> > > >
> > > > > If we can make this work, it would enable the propagation of
> > > > > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > > > > right?
> > > >
> > > > It is all until there is a known case in which it isn't.  So either
> > > > you know a specific case in which it doesn't work, or I don't see a
> > > > reason for avoiding it.
> > > >
> > > > ATM the only specific case in which it doesn't work is when
> > > > pm_runtime_force_suspend|resume() are used.
> > > >
> > > > > The point is, the other bigger issue that I pointed out in our earlier
> > > > > discussions; all those devices where their drivers/buses don't cope
> > > > > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > > > > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > > > > parents. I guess this is the best we can do then?
> > > >
> > > > OK, what are they?
> > > >
> > > > You keep saying that they exist without giving any examples.
> > >
> > > To put it more bluntly, I'm not aware of any place other than
> > > pm_runtime_force_suspend|resume() that can be confused by changing the
> > > runtime PM status of a device with runtime PM disabled (either
> > > permanently, or transiently during a system suspend-resume cycle) to
> > > RPM_ACTIVE, so if there are any such places, I would appreciate
> > > letting me know what they are.
> >
> > Well, sorry I thought you were aware. Anyway, I believe you need to do
> > your own investigation as it's simply too time consuming for me to
> > find them all. The problem is that it's not just a simple pattern to
> > search for, so we would need some clever scripting to move forward to
> > find them.
> >
> > To start with, we should look for drivers that enable runtime PM, by
> > calling pm_runtime_enable().
> >
> > Additionally, in their system suspend callback they should typically
> > *not* use pm_runtime_suspended(), pm_runtime_status_suspended() or
> > pm_runtime_active() as that is usually (but no always) indicating that
> > they got it right. Then there are those that don't have system
> > suspend/resume callbacks assigned at all (or uses some other subsystem
> > specific callback for this), but only uses runtime PM.
> >
> > On top of that, drivers that makes use of
> > pm_runtime_force_suspend|resume() should be disregarded, which has
> > reached beyond 300 as this point.
> >
> > Anyway, here is just one example that I found from a quick search.
> > drivers/i2c/busses/i2c-qcom-geni.c
>
> OK, I see.
>
> It sets the status to RPM_SUSPENDED in geni_i2c_suspend_noirq(), if
> not suspended already, and assumes it to stay this way across
> geni_i2c_resume_noirq() until someone resumes it via runtime PM.
>
> Fair enough, but somebody should tell them that they don't need to use
> pm_runtime_disable/enable() in _noirq.
>
> > >
> > > Note that rpm_active() could start producing confusing results if the
> > > runtime PM status of a device with runtime PM disabled is changed from
> > > RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
> > > -EACCES instead of 1, but changing the status to RPM_ACTIVE will not
> > > confuse it the same way.
> >
> > Trust me, it will cause problems.
> >
> > Drivers may call pm_runtime_get_sync() to turn on the resources for
> > the device after the system has resumed, when runtime PM has been
> > re-enabled for the device by the PM core.
> >
> > Those calls to pm_runtime_get_sync() will then not end up invoking any
> > if ->runtime_resume() callbacks for the device since its state is
> > already RPM_ACTIVE. Hence, the device will remain in a low power state
> > even if the driver believes it has been powered on. In many cases,
> > accessing the device (like reading/writing a register) will often just
> > just hang in these cases, but in worst cases we could end up getting
> > even more difficult bugs to debug.
>
> Sure, that would be a problem.

We may be making a logical mistake here by assuming that any of these
devices will end up in dependency chains starting at the devices that
I'm concerned about.

> I think I need to find a different way to address the problem I'm
> seeing, that is to resume devices that were runtime-suspended before
> system suspend along with their superiors.

Well, maybe not.

So the "smart suspend" thing can be done only if all of the devices
above the given one in the dependency graph (including the parent,
suppliers etc) either opt in for "smart suspend" or are devices
without PM callbacks and the dependency graphs can be cut at devices
that don't support runtime PM.  This would require another flag in
struct dev_pm_info for the tracking of dependencies, but it should be
entirely doable because all of them are known at the device_prepare()
time.

That should avoid involving anyone who can be surprised.

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Ulf Hansson]

On Wed, 12 Feb 2025 at 18:05, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 4:15 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 12 Feb 2025 at 12:33, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > >
> > > > > > Hi Everyone,
> > > > > >
> > > > > > This series is a result of the discussion on a recently reported issue
> > > > > > with device runtime PM status propagation during system resume and
> > > > > > the resulting patches:
> > > > > >
> > > > > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > > > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > > > > >
> > > > > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > > > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > > > > setting propagation to the parent of the first device in a dependency
> > > > > > chain that turned out to have to be resumed during system resume even
> > > > > > though it was runtime-suspended before system suspend.
> > > > > >
> > > > > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > > > > suspend devices that have never had runtime PM enabled if their runtime
> > > > > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > > > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > > > > >
> > > > > > The purpose of this series is to eliminate the above restrictions and
> > > > > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > > > > more with what the core does.
> > > > >
> > > > > For my understanding, would you mind elaborating a bit more around the
> > > > > end-goal with this?
> > > >
> > > > The end goal is, of course, full integration of runtime PM with system
> > > > sleep for all devices.  Eventually.
> > > >
> > > > And it is necessary to make the core and
> > > > pm_runtime_force_suspend|resume() work together better for this
> > > > purpose.
> > > >
> > > > > Are you trying to make adaptations for
> > > > > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > > > > that uses pm_runtime_force_suspend|resume() should be able to cope
> > > > > with other drivers for child-devices that make use of
> > > > > DPM_FLAG_SMART_SUSPEND?
> > > >
> > > > Yes.
> > > >
> > > > This is a more general case, though, when a device that was
> > > > runtime-suspended before system suspend and is left in suspend during
> > > > it, needs to be resumed during the system resume that follows.
> > > >
> > > > Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> > > > cannot happen otherwise, but I think that it is a generally valid
> > > > case.
> > > >
> > > > > If we can make this work, it would enable the propagation of
> > > > > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > > > > right?
> > > >
> > > > It is all until there is a known case in which it isn't.  So either
> > > > you know a specific case in which it doesn't work, or I don't see a
> > > > reason for avoiding it.
> > > >
> > > > ATM the only specific case in which it doesn't work is when
> > > > pm_runtime_force_suspend|resume() are used.
> > > >
> > > > > The point is, the other bigger issue that I pointed out in our earlier
> > > > > discussions; all those devices where their drivers/buses don't cope
> > > > > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > > > > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > > > > parents. I guess this is the best we can do then?
> > > >
> > > > OK, what are they?
> > > >
> > > > You keep saying that they exist without giving any examples.
> > >
> > > To put it more bluntly, I'm not aware of any place other than
> > > pm_runtime_force_suspend|resume() that can be confused by changing the
> > > runtime PM status of a device with runtime PM disabled (either
> > > permanently, or transiently during a system suspend-resume cycle) to
> > > RPM_ACTIVE, so if there are any such places, I would appreciate
> > > letting me know what they are.
> >
> > Well, sorry I thought you were aware. Anyway, I believe you need to do
> > your own investigation as it's simply too time consuming for me to
> > find them all. The problem is that it's not just a simple pattern to
> > search for, so we would need some clever scripting to move forward to
> > find them.
> >
> > To start with, we should look for drivers that enable runtime PM, by
> > calling pm_runtime_enable().
> >
> > Additionally, in their system suspend callback they should typically
> > *not* use pm_runtime_suspended(), pm_runtime_status_suspended() or
> > pm_runtime_active() as that is usually (but no always) indicating that
> > they got it right. Then there are those that don't have system
> > suspend/resume callbacks assigned at all (or uses some other subsystem
> > specific callback for this), but only uses runtime PM.
> >
> > On top of that, drivers that makes use of
> > pm_runtime_force_suspend|resume() should be disregarded, which has
> > reached beyond 300 as this point.
> >
> > Anyway, here is just one example that I found from a quick search.
> > drivers/i2c/busses/i2c-qcom-geni.c
>
> OK, I see.
>
> It sets the status to RPM_SUSPENDED in geni_i2c_suspend_noirq(), if
> not suspended already, and assumes it to stay this way across
> geni_i2c_resume_noirq() until someone resumes it via runtime PM.
>
> Fair enough, but somebody should tell them that they don't need to use
> pm_runtime_disable/enable() in _noirq.
>
> > >
> > > Note that rpm_active() could start producing confusing results if the
> > > runtime PM status of a device with runtime PM disabled is changed from
> > > RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
> > > -EACCES instead of 1, but changing the status to RPM_ACTIVE will not
> > > confuse it the same way.
> >
> > Trust me, it will cause problems.
> >
> > Drivers may call pm_runtime_get_sync() to turn on the resources for
> > the device after the system has resumed, when runtime PM has been
> > re-enabled for the device by the PM core.
> >
> > Those calls to pm_runtime_get_sync() will then not end up invoking any
> > if ->runtime_resume() callbacks for the device since its state is
> > already RPM_ACTIVE. Hence, the device will remain in a low power state
> > even if the driver believes it has been powered on. In many cases,
> > accessing the device (like reading/writing a register) will often just
> > just hang in these cases, but in worst cases we could end up getting
> > even more difficult bugs to debug.
>
> Sure, that would be a problem.
>
> I think I need to find a different way to address the problem I'm
> seeing, that is to resume devices that were runtime-suspended before
> system suspend along with their superiors.
>
> One way to do it would be to just defer their resume to the point when
> the core has enabled runtime PM for them, which means that it has also
> enabled runtime PM for all of their superiors, and then call
> pm_runtime_resume().
>
> This should work unless one of the superiors has runtime PM disabled
> at that point, of course.

Right, so typically users of pm_runtime_force_suspend|resume() from
the regular ->suspend|resume() callbacks would not work, because they
disable/enable runtime PM. Although, we could probably fix this quite
easily by making some adaptations in
pm_runtime_force_suspend|resume().

I am not sure if this approach would have any other issue though, but
it seems like it could make sense to explore this approach. In general
drivers should cope with their devices being runtime resumed if
runtime PM is enabled, right!?

If this works, it seems like a generic and a good solution to me.

Kind regards
Uffe

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Ulf Hansson]

On Wed, 12 Feb 2025 at 20:04, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 6:05 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Feb 12, 2025 at 4:15 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 12 Feb 2025 at 12:33, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > >
> > > > > > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > > >
> > > > > > > Hi Everyone,
> > > > > > >
> > > > > > > This series is a result of the discussion on a recently reported issue
> > > > > > > with device runtime PM status propagation during system resume and
> > > > > > > the resulting patches:
> > > > > > >
> > > > > > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > > > > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > > > > > >
> > > > > > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > > > > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > > > > > setting propagation to the parent of the first device in a dependency
> > > > > > > chain that turned out to have to be resumed during system resume even
> > > > > > > though it was runtime-suspended before system suspend.
> > > > > > >
> > > > > > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > > > > > suspend devices that have never had runtime PM enabled if their runtime
> > > > > > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > > > > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > > > > > >
> > > > > > > The purpose of this series is to eliminate the above restrictions and
> > > > > > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > > > > > more with what the core does.
> > > > > >
> > > > > > For my understanding, would you mind elaborating a bit more around the
> > > > > > end-goal with this?
> > > > >
> > > > > The end goal is, of course, full integration of runtime PM with system
> > > > > sleep for all devices.  Eventually.
> > > > >
> > > > > And it is necessary to make the core and
> > > > > pm_runtime_force_suspend|resume() work together better for this
> > > > > purpose.
> > > > >
> > > > > > Are you trying to make adaptations for
> > > > > > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > > > > > that uses pm_runtime_force_suspend|resume() should be able to cope
> > > > > > with other drivers for child-devices that make use of
> > > > > > DPM_FLAG_SMART_SUSPEND?
> > > > >
> > > > > Yes.
> > > > >
> > > > > This is a more general case, though, when a device that was
> > > > > runtime-suspended before system suspend and is left in suspend during
> > > > > it, needs to be resumed during the system resume that follows.
> > > > >
> > > > > Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> > > > > cannot happen otherwise, but I think that it is a generally valid
> > > > > case.
> > > > >
> > > > > > If we can make this work, it would enable the propagation of
> > > > > > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > > > > > right?
> > > > >
> > > > > It is all until there is a known case in which it isn't.  So either
> > > > > you know a specific case in which it doesn't work, or I don't see a
> > > > > reason for avoiding it.
> > > > >
> > > > > ATM the only specific case in which it doesn't work is when
> > > > > pm_runtime_force_suspend|resume() are used.
> > > > >
> > > > > > The point is, the other bigger issue that I pointed out in our earlier
> > > > > > discussions; all those devices where their drivers/buses don't cope
> > > > > > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > > > > > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > > > > > parents. I guess this is the best we can do then?
> > > > >
> > > > > OK, what are they?
> > > > >
> > > > > You keep saying that they exist without giving any examples.
> > > >
> > > > To put it more bluntly, I'm not aware of any place other than
> > > > pm_runtime_force_suspend|resume() that can be confused by changing the
> > > > runtime PM status of a device with runtime PM disabled (either
> > > > permanently, or transiently during a system suspend-resume cycle) to
> > > > RPM_ACTIVE, so if there are any such places, I would appreciate
> > > > letting me know what they are.
> > >
> > > Well, sorry I thought you were aware. Anyway, I believe you need to do
> > > your own investigation as it's simply too time consuming for me to
> > > find them all. The problem is that it's not just a simple pattern to
> > > search for, so we would need some clever scripting to move forward to
> > > find them.
> > >
> > > To start with, we should look for drivers that enable runtime PM, by
> > > calling pm_runtime_enable().
> > >
> > > Additionally, in their system suspend callback they should typically
> > > *not* use pm_runtime_suspended(), pm_runtime_status_suspended() or
> > > pm_runtime_active() as that is usually (but no always) indicating that
> > > they got it right. Then there are those that don't have system
> > > suspend/resume callbacks assigned at all (or uses some other subsystem
> > > specific callback for this), but only uses runtime PM.
> > >
> > > On top of that, drivers that makes use of
> > > pm_runtime_force_suspend|resume() should be disregarded, which has
> > > reached beyond 300 as this point.
> > >
> > > Anyway, here is just one example that I found from a quick search.
> > > drivers/i2c/busses/i2c-qcom-geni.c
> >
> > OK, I see.
> >
> > It sets the status to RPM_SUSPENDED in geni_i2c_suspend_noirq(), if
> > not suspended already, and assumes it to stay this way across
> > geni_i2c_resume_noirq() until someone resumes it via runtime PM.
> >
> > Fair enough, but somebody should tell them that they don't need to use
> > pm_runtime_disable/enable() in _noirq.
> >
> > > >
> > > > Note that rpm_active() could start producing confusing results if the
> > > > runtime PM status of a device with runtime PM disabled is changed from
> > > > RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
> > > > -EACCES instead of 1, but changing the status to RPM_ACTIVE will not
> > > > confuse it the same way.
> > >
> > > Trust me, it will cause problems.
> > >
> > > Drivers may call pm_runtime_get_sync() to turn on the resources for
> > > the device after the system has resumed, when runtime PM has been
> > > re-enabled for the device by the PM core.
> > >
> > > Those calls to pm_runtime_get_sync() will then not end up invoking any
> > > if ->runtime_resume() callbacks for the device since its state is
> > > already RPM_ACTIVE. Hence, the device will remain in a low power state
> > > even if the driver believes it has been powered on. In many cases,
> > > accessing the device (like reading/writing a register) will often just
> > > just hang in these cases, but in worst cases we could end up getting
> > > even more difficult bugs to debug.
> >
> > Sure, that would be a problem.
>
> We may be making a logical mistake here by assuming that any of these
> devices will end up in dependency chains starting at the devices that
> I'm concerned about.
>
> > I think I need to find a different way to address the problem I'm
> > seeing, that is to resume devices that were runtime-suspended before
> > system suspend along with their superiors.
>
> Well, maybe not.
>
> So the "smart suspend" thing can be done only if all of the devices
> above the given one in the dependency graph (including the parent,
> suppliers etc) either opt in for "smart suspend" or are devices
> without PM callbacks and the dependency graphs can be cut at devices
> that don't support runtime PM.  This would require another flag in
> struct dev_pm_info for the tracking of dependencies, but it should be
> entirely doable because all of them are known at the device_prepare()
> time.
>
> That should avoid involving anyone who can be surprised.

This would be an improvement, while it is questionable for how
valuable this would be in the end. As soon as there is a
parent/supplier that has PM callbacks, we would need to prevent the
propagation. I am also a bit worried about adding more corner cases to
the PM core, as it's not entirely easy to understand them, I think.

That said, maybe it's the best we can do.

Kind regards
Uffe

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Rafael J. Wysocki]

On Thu, Feb 13, 2025 at 2:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 12 Feb 2025 at 18:05, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Feb 12, 2025 at 4:15 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 12 Feb 2025 at 12:33, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > >
> > > > > > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > > >
> > > > > > > Hi Everyone,
> > > > > > >
> > > > > > > This series is a result of the discussion on a recently reported issue
> > > > > > > with device runtime PM status propagation during system resume and
> > > > > > > the resulting patches:
> > > > > > >
> > > > > > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > > > > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > > > > > >
> > > > > > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > > > > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > > > > > setting propagation to the parent of the first device in a dependency
> > > > > > > chain that turned out to have to be resumed during system resume even
> > > > > > > though it was runtime-suspended before system suspend.
> > > > > > >
> > > > > > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > > > > > suspend devices that have never had runtime PM enabled if their runtime
> > > > > > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > > > > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > > > > > >
> > > > > > > The purpose of this series is to eliminate the above restrictions and
> > > > > > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > > > > > more with what the core does.
> > > > > >
> > > > > > For my understanding, would you mind elaborating a bit more around the
> > > > > > end-goal with this?
> > > > >
> > > > > The end goal is, of course, full integration of runtime PM with system
> > > > > sleep for all devices.  Eventually.
> > > > >
> > > > > And it is necessary to make the core and
> > > > > pm_runtime_force_suspend|resume() work together better for this
> > > > > purpose.
> > > > >
> > > > > > Are you trying to make adaptations for
> > > > > > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > > > > > that uses pm_runtime_force_suspend|resume() should be able to cope
> > > > > > with other drivers for child-devices that make use of
> > > > > > DPM_FLAG_SMART_SUSPEND?
> > > > >
> > > > > Yes.
> > > > >
> > > > > This is a more general case, though, when a device that was
> > > > > runtime-suspended before system suspend and is left in suspend during
> > > > > it, needs to be resumed during the system resume that follows.
> > > > >
> > > > > Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> > > > > cannot happen otherwise, but I think that it is a generally valid
> > > > > case.
> > > > >
> > > > > > If we can make this work, it would enable the propagation of
> > > > > > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > > > > > right?
> > > > >
> > > > > It is all until there is a known case in which it isn't.  So either
> > > > > you know a specific case in which it doesn't work, or I don't see a
> > > > > reason for avoiding it.
> > > > >
> > > > > ATM the only specific case in which it doesn't work is when
> > > > > pm_runtime_force_suspend|resume() are used.
> > > > >
> > > > > > The point is, the other bigger issue that I pointed out in our earlier
> > > > > > discussions; all those devices where their drivers/buses don't cope
> > > > > > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > > > > > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > > > > > parents. I guess this is the best we can do then?
> > > > >
> > > > > OK, what are they?
> > > > >
> > > > > You keep saying that they exist without giving any examples.
> > > >
> > > > To put it more bluntly, I'm not aware of any place other than
> > > > pm_runtime_force_suspend|resume() that can be confused by changing the
> > > > runtime PM status of a device with runtime PM disabled (either
> > > > permanently, or transiently during a system suspend-resume cycle) to
> > > > RPM_ACTIVE, so if there are any such places, I would appreciate
> > > > letting me know what they are.
> > >
> > > Well, sorry I thought you were aware. Anyway, I believe you need to do
> > > your own investigation as it's simply too time consuming for me to
> > > find them all. The problem is that it's not just a simple pattern to
> > > search for, so we would need some clever scripting to move forward to
> > > find them.
> > >
> > > To start with, we should look for drivers that enable runtime PM, by
> > > calling pm_runtime_enable().
> > >
> > > Additionally, in their system suspend callback they should typically
> > > *not* use pm_runtime_suspended(), pm_runtime_status_suspended() or
> > > pm_runtime_active() as that is usually (but no always) indicating that
> > > they got it right. Then there are those that don't have system
> > > suspend/resume callbacks assigned at all (or uses some other subsystem
> > > specific callback for this), but only uses runtime PM.
> > >
> > > On top of that, drivers that makes use of
> > > pm_runtime_force_suspend|resume() should be disregarded, which has
> > > reached beyond 300 as this point.
> > >
> > > Anyway, here is just one example that I found from a quick search.
> > > drivers/i2c/busses/i2c-qcom-geni.c
> >
> > OK, I see.
> >
> > It sets the status to RPM_SUSPENDED in geni_i2c_suspend_noirq(), if
> > not suspended already, and assumes it to stay this way across
> > geni_i2c_resume_noirq() until someone resumes it via runtime PM.
> >
> > Fair enough, but somebody should tell them that they don't need to use
> > pm_runtime_disable/enable() in _noirq.
> >
> > > >
> > > > Note that rpm_active() could start producing confusing results if the
> > > > runtime PM status of a device with runtime PM disabled is changed from
> > > > RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
> > > > -EACCES instead of 1, but changing the status to RPM_ACTIVE will not
> > > > confuse it the same way.
> > >
> > > Trust me, it will cause problems.
> > >
> > > Drivers may call pm_runtime_get_sync() to turn on the resources for
> > > the device after the system has resumed, when runtime PM has been
> > > re-enabled for the device by the PM core.
> > >
> > > Those calls to pm_runtime_get_sync() will then not end up invoking any
> > > if ->runtime_resume() callbacks for the device since its state is
> > > already RPM_ACTIVE. Hence, the device will remain in a low power state
> > > even if the driver believes it has been powered on. In many cases,
> > > accessing the device (like reading/writing a register) will often just
> > > just hang in these cases, but in worst cases we could end up getting
> > > even more difficult bugs to debug.
> >
> > Sure, that would be a problem.
> >
> > I think I need to find a different way to address the problem I'm
> > seeing, that is to resume devices that were runtime-suspended before
> > system suspend along with their superiors.
> >
> > One way to do it would be to just defer their resume to the point when
> > the core has enabled runtime PM for them, which means that it has also
> > enabled runtime PM for all of their superiors, and then call
> > pm_runtime_resume().
> >
> > This should work unless one of the superiors has runtime PM disabled
> > at that point, of course.
>
> Right, so typically users of pm_runtime_force_suspend|resume() from
> the regular ->suspend|resume() callbacks would not work, because they
> disable/enable runtime PM. Although, we could probably fix this quite
> easily by making some adaptations in
> pm_runtime_force_suspend|resume().
>
> I am not sure if this approach would have any other issue though, but
> it seems like it could make sense to explore this approach. In general
> drivers should cope with their devices being runtime resumed if
> runtime PM is enabled, right!?
>
> If this works, it seems like a generic and a good solution to me.

For PCI ports, though, it would require some tuning of
->runtime_resume(), so it is not as simple as it would seem to be in
the end.

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Rafael J. Wysocki]

On Thu, Feb 13, 2025 at 2:38 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 12 Feb 2025 at 20:04, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Feb 12, 2025 at 6:05 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Feb 12, 2025 at 4:15 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Wed, 12 Feb 2025 at 12:33, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > >
> > > > > > On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > >
> > > > > > > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > > > >
> > > > > > > > Hi Everyone,
> > > > > > > >
> > > > > > > > This series is a result of the discussion on a recently reported issue
> > > > > > > > with device runtime PM status propagation during system resume and
> > > > > > > > the resulting patches:
> > > > > > > >
> > > > > > > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > > > > > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > > > > > > >
> > > > > > > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > > > > > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > > > > > > setting propagation to the parent of the first device in a dependency
> > > > > > > > chain that turned out to have to be resumed during system resume even
> > > > > > > > though it was runtime-suspended before system suspend.
> > > > > > > >
> > > > > > > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > > > > > > suspend devices that have never had runtime PM enabled if their runtime
> > > > > > > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > > > > > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > > > > > > >
> > > > > > > > The purpose of this series is to eliminate the above restrictions and
> > > > > > > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > > > > > > more with what the core does.
> > > > > > >
> > > > > > > For my understanding, would you mind elaborating a bit more around the
> > > > > > > end-goal with this?
> > > > > >
> > > > > > The end goal is, of course, full integration of runtime PM with system
> > > > > > sleep for all devices.  Eventually.
> > > > > >
> > > > > > And it is necessary to make the core and
> > > > > > pm_runtime_force_suspend|resume() work together better for this
> > > > > > purpose.
> > > > > >
> > > > > > > Are you trying to make adaptations for
> > > > > > > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > > > > > > that uses pm_runtime_force_suspend|resume() should be able to cope
> > > > > > > with other drivers for child-devices that make use of
> > > > > > > DPM_FLAG_SMART_SUSPEND?
> > > > > >
> > > > > > Yes.
> > > > > >
> > > > > > This is a more general case, though, when a device that was
> > > > > > runtime-suspended before system suspend and is left in suspend during
> > > > > > it, needs to be resumed during the system resume that follows.
> > > > > >
> > > > > > Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> > > > > > cannot happen otherwise, but I think that it is a generally valid
> > > > > > case.
> > > > > >
> > > > > > > If we can make this work, it would enable the propagation of
> > > > > > > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > > > > > > right?
> > > > > >
> > > > > > It is all until there is a known case in which it isn't.  So either
> > > > > > you know a specific case in which it doesn't work, or I don't see a
> > > > > > reason for avoiding it.
> > > > > >
> > > > > > ATM the only specific case in which it doesn't work is when
> > > > > > pm_runtime_force_suspend|resume() are used.
> > > > > >
> > > > > > > The point is, the other bigger issue that I pointed out in our earlier
> > > > > > > discussions; all those devices where their drivers/buses don't cope
> > > > > > > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > > > > > > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > > > > > > parents. I guess this is the best we can do then?
> > > > > >
> > > > > > OK, what are they?
> > > > > >
> > > > > > You keep saying that they exist without giving any examples.
> > > > >
> > > > > To put it more bluntly, I'm not aware of any place other than
> > > > > pm_runtime_force_suspend|resume() that can be confused by changing the
> > > > > runtime PM status of a device with runtime PM disabled (either
> > > > > permanently, or transiently during a system suspend-resume cycle) to
> > > > > RPM_ACTIVE, so if there are any such places, I would appreciate
> > > > > letting me know what they are.
> > > >
> > > > Well, sorry I thought you were aware. Anyway, I believe you need to do
> > > > your own investigation as it's simply too time consuming for me to
> > > > find them all. The problem is that it's not just a simple pattern to
> > > > search for, so we would need some clever scripting to move forward to
> > > > find them.
> > > >
> > > > To start with, we should look for drivers that enable runtime PM, by
> > > > calling pm_runtime_enable().
> > > >
> > > > Additionally, in their system suspend callback they should typically
> > > > *not* use pm_runtime_suspended(), pm_runtime_status_suspended() or
> > > > pm_runtime_active() as that is usually (but no always) indicating that
> > > > they got it right. Then there are those that don't have system
> > > > suspend/resume callbacks assigned at all (or uses some other subsystem
> > > > specific callback for this), but only uses runtime PM.
> > > >
> > > > On top of that, drivers that makes use of
> > > > pm_runtime_force_suspend|resume() should be disregarded, which has
> > > > reached beyond 300 as this point.
> > > >
> > > > Anyway, here is just one example that I found from a quick search.
> > > > drivers/i2c/busses/i2c-qcom-geni.c
> > >
> > > OK, I see.
> > >
> > > It sets the status to RPM_SUSPENDED in geni_i2c_suspend_noirq(), if
> > > not suspended already, and assumes it to stay this way across
> > > geni_i2c_resume_noirq() until someone resumes it via runtime PM.
> > >
> > > Fair enough, but somebody should tell them that they don't need to use
> > > pm_runtime_disable/enable() in _noirq.

BTW, I'm wondering how this driver handles the case in which someone
runs pm_runtime_get*() on its device and doesn't release the reference
throughout a system suspend-resume cycle, which is a valid thing to do
albeit unusual.

> > > > >
> > > > > Note that rpm_active() could start producing confusing results if the
> > > > > runtime PM status of a device with runtime PM disabled is changed from
> > > > > RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
> > > > > -EACCES instead of 1, but changing the status to RPM_ACTIVE will not
> > > > > confuse it the same way.
> > > >
> > > > Trust me, it will cause problems.
> > > >
> > > > Drivers may call pm_runtime_get_sync() to turn on the resources for
> > > > the device after the system has resumed, when runtime PM has been
> > > > re-enabled for the device by the PM core.
> > > >
> > > > Those calls to pm_runtime_get_sync() will then not end up invoking any
> > > > if ->runtime_resume() callbacks for the device since its state is
> > > > already RPM_ACTIVE. Hence, the device will remain in a low power state
> > > > even if the driver believes it has been powered on. In many cases,
> > > > accessing the device (like reading/writing a register) will often just
> > > > just hang in these cases, but in worst cases we could end up getting
> > > > even more difficult bugs to debug.
> > >
> > > Sure, that would be a problem.
> >
> > We may be making a logical mistake here by assuming that any of these
> > devices will end up in dependency chains starting at the devices that
> > I'm concerned about.
> >
> > > I think I need to find a different way to address the problem I'm
> > > seeing, that is to resume devices that were runtime-suspended before
> > > system suspend along with their superiors.
> >
> > Well, maybe not.
> >
> > So the "smart suspend" thing can be done only if all of the devices
> > above the given one in the dependency graph (including the parent,
> > suppliers etc) either opt in for "smart suspend" or are devices
> > without PM callbacks and the dependency graphs can be cut at devices
> > that don't support runtime PM.  This would require another flag in
> > struct dev_pm_info for the tracking of dependencies, but it should be
> > entirely doable because all of them are known at the device_prepare()
> > time.
> >
> > That should avoid involving anyone who can be surprised.
>
> This would be an improvement, while it is questionable for how
> valuable this would be in the end. As soon as there is a
> parent/supplier that has PM callbacks, we would need to prevent the
> propagation.

One without "smart suspend" that is.  Yes.

For the current users of DPM_FLAG_SMART_SUSPEND this shouldn't
actually matter because it is the case already AFAICS, so I'm not
worried about this.

> I am also a bit worried about adding more corner cases to
> the PM core, as it's not entirely easy to understand them, I think.

That is always a valid concern, but sometimes there is no other way to
make progress.

> That said, maybe it's the best we can do.

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Ulf Hansson]

On Thu, 13 Feb 2025 at 21:17, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Feb 13, 2025 at 2:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 12 Feb 2025 at 18:05, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Feb 12, 2025 at 4:15 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Wed, 12 Feb 2025 at 12:33, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > >
> > > > > > On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > >
> > > > > > > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > > > >
> > > > > > > > Hi Everyone,
> > > > > > > >
> > > > > > > > This series is a result of the discussion on a recently reported issue
> > > > > > > > with device runtime PM status propagation during system resume and
> > > > > > > > the resulting patches:
> > > > > > > >
> > > > > > > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > > > > > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > > > > > > >
> > > > > > > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > > > > > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > > > > > > setting propagation to the parent of the first device in a dependency
> > > > > > > > chain that turned out to have to be resumed during system resume even
> > > > > > > > though it was runtime-suspended before system suspend.
> > > > > > > >
> > > > > > > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > > > > > > suspend devices that have never had runtime PM enabled if their runtime
> > > > > > > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > > > > > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > > > > > > >
> > > > > > > > The purpose of this series is to eliminate the above restrictions and
> > > > > > > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > > > > > > more with what the core does.
> > > > > > >
> > > > > > > For my understanding, would you mind elaborating a bit more around the
> > > > > > > end-goal with this?
> > > > > >
> > > > > > The end goal is, of course, full integration of runtime PM with system
> > > > > > sleep for all devices.  Eventually.
> > > > > >
> > > > > > And it is necessary to make the core and
> > > > > > pm_runtime_force_suspend|resume() work together better for this
> > > > > > purpose.
> > > > > >
> > > > > > > Are you trying to make adaptations for
> > > > > > > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > > > > > > that uses pm_runtime_force_suspend|resume() should be able to cope
> > > > > > > with other drivers for child-devices that make use of
> > > > > > > DPM_FLAG_SMART_SUSPEND?
> > > > > >
> > > > > > Yes.
> > > > > >
> > > > > > This is a more general case, though, when a device that was
> > > > > > runtime-suspended before system suspend and is left in suspend during
> > > > > > it, needs to be resumed during the system resume that follows.
> > > > > >
> > > > > > Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> > > > > > cannot happen otherwise, but I think that it is a generally valid
> > > > > > case.
> > > > > >
> > > > > > > If we can make this work, it would enable the propagation of
> > > > > > > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > > > > > > right?
> > > > > >
> > > > > > It is all until there is a known case in which it isn't.  So either
> > > > > > you know a specific case in which it doesn't work, or I don't see a
> > > > > > reason for avoiding it.
> > > > > >
> > > > > > ATM the only specific case in which it doesn't work is when
> > > > > > pm_runtime_force_suspend|resume() are used.
> > > > > >
> > > > > > > The point is, the other bigger issue that I pointed out in our earlier
> > > > > > > discussions; all those devices where their drivers/buses don't cope
> > > > > > > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > > > > > > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > > > > > > parents. I guess this is the best we can do then?
> > > > > >
> > > > > > OK, what are they?
> > > > > >
> > > > > > You keep saying that they exist without giving any examples.
> > > > >
> > > > > To put it more bluntly, I'm not aware of any place other than
> > > > > pm_runtime_force_suspend|resume() that can be confused by changing the
> > > > > runtime PM status of a device with runtime PM disabled (either
> > > > > permanently, or transiently during a system suspend-resume cycle) to
> > > > > RPM_ACTIVE, so if there are any such places, I would appreciate
> > > > > letting me know what they are.
> > > >
> > > > Well, sorry I thought you were aware. Anyway, I believe you need to do
> > > > your own investigation as it's simply too time consuming for me to
> > > > find them all. The problem is that it's not just a simple pattern to
> > > > search for, so we would need some clever scripting to move forward to
> > > > find them.
> > > >
> > > > To start with, we should look for drivers that enable runtime PM, by
> > > > calling pm_runtime_enable().
> > > >
> > > > Additionally, in their system suspend callback they should typically
> > > > *not* use pm_runtime_suspended(), pm_runtime_status_suspended() or
> > > > pm_runtime_active() as that is usually (but no always) indicating that
> > > > they got it right. Then there are those that don't have system
> > > > suspend/resume callbacks assigned at all (or uses some other subsystem
> > > > specific callback for this), but only uses runtime PM.
> > > >
> > > > On top of that, drivers that makes use of
> > > > pm_runtime_force_suspend|resume() should be disregarded, which has
> > > > reached beyond 300 as this point.
> > > >
> > > > Anyway, here is just one example that I found from a quick search.
> > > > drivers/i2c/busses/i2c-qcom-geni.c
> > >
> > > OK, I see.
> > >
> > > It sets the status to RPM_SUSPENDED in geni_i2c_suspend_noirq(), if
> > > not suspended already, and assumes it to stay this way across
> > > geni_i2c_resume_noirq() until someone resumes it via runtime PM.
> > >
> > > Fair enough, but somebody should tell them that they don't need to use
> > > pm_runtime_disable/enable() in _noirq.
> > >
> > > > >
> > > > > Note that rpm_active() could start producing confusing results if the
> > > > > runtime PM status of a device with runtime PM disabled is changed from
> > > > > RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
> > > > > -EACCES instead of 1, but changing the status to RPM_ACTIVE will not
> > > > > confuse it the same way.
> > > >
> > > > Trust me, it will cause problems.
> > > >
> > > > Drivers may call pm_runtime_get_sync() to turn on the resources for
> > > > the device after the system has resumed, when runtime PM has been
> > > > re-enabled for the device by the PM core.
> > > >
> > > > Those calls to pm_runtime_get_sync() will then not end up invoking any
> > > > if ->runtime_resume() callbacks for the device since its state is
> > > > already RPM_ACTIVE. Hence, the device will remain in a low power state
> > > > even if the driver believes it has been powered on. In many cases,
> > > > accessing the device (like reading/writing a register) will often just
> > > > just hang in these cases, but in worst cases we could end up getting
> > > > even more difficult bugs to debug.
> > >
> > > Sure, that would be a problem.
> > >
> > > I think I need to find a different way to address the problem I'm
> > > seeing, that is to resume devices that were runtime-suspended before
> > > system suspend along with their superiors.
> > >
> > > One way to do it would be to just defer their resume to the point when
> > > the core has enabled runtime PM for them, which means that it has also
> > > enabled runtime PM for all of their superiors, and then call
> > > pm_runtime_resume().
> > >
> > > This should work unless one of the superiors has runtime PM disabled
> > > at that point, of course.
> >
> > Right, so typically users of pm_runtime_force_suspend|resume() from
> > the regular ->suspend|resume() callbacks would not work, because they
> > disable/enable runtime PM. Although, we could probably fix this quite
> > easily by making some adaptations in
> > pm_runtime_force_suspend|resume().
> >
> > I am not sure if this approach would have any other issue though, but
> > it seems like it could make sense to explore this approach. In general
> > drivers should cope with their devices being runtime resumed if
> > runtime PM is enabled, right!?
> >
> > If this works, it seems like a generic and a good solution to me.
>
> For PCI ports, though, it would require some tuning of
> ->runtime_resume(), so it is not as simple as it would seem to be in
> the end.

Okay. Perhaps it would be worth it to try this out anyway, as it would
allow us to keep the PM core as simple as possible?

Kind regards
Uffe

Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume() agree more

[Rafael J. Wysocki]

On Fri, Feb 14, 2025 at 10:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 13 Feb 2025 at 21:17, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Feb 13, 2025 at 2:11 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 12 Feb 2025 at 18:05, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Wed, Feb 12, 2025 at 4:15 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Wed, 12 Feb 2025 at 12:33, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > >
> > > > > > On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > > >
> > > > > > > On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > > >
> > > > > > > > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > > > > >
> > > > > > > > > Hi Everyone,
> > > > > > > > >
> > > > > > > > > This series is a result of the discussion on a recently reported issue
> > > > > > > > > with device runtime PM status propagation during system resume and
> > > > > > > > > the resulting patches:
> > > > > > > > >
> > > > > > > > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > > > > > > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > > > > > > > >
> > > > > > > > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > > > > > > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > > > > > > > setting propagation to the parent of the first device in a dependency
> > > > > > > > > chain that turned out to have to be resumed during system resume even
> > > > > > > > > though it was runtime-suspended before system suspend.
> > > > > > > > >
> > > > > > > > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > > > > > > > suspend devices that have never had runtime PM enabled if their runtime
> > > > > > > > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > > > > > > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > > > > > > > >
> > > > > > > > > The purpose of this series is to eliminate the above restrictions and
> > > > > > > > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > > > > > > > more with what the core does.
> > > > > > > >
> > > > > > > > For my understanding, would you mind elaborating a bit more around the
> > > > > > > > end-goal with this?
> > > > > > >
> > > > > > > The end goal is, of course, full integration of runtime PM with system
> > > > > > > sleep for all devices.  Eventually.
> > > > > > >
> > > > > > > And it is necessary to make the core and
> > > > > > > pm_runtime_force_suspend|resume() work together better for this
> > > > > > > purpose.
> > > > > > >
> > > > > > > > Are you trying to make adaptations for
> > > > > > > > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > > > > > > > that uses pm_runtime_force_suspend|resume() should be able to cope
> > > > > > > > with other drivers for child-devices that make use of
> > > > > > > > DPM_FLAG_SMART_SUSPEND?
> > > > > > >
> > > > > > > Yes.
> > > > > > >
> > > > > > > This is a more general case, though, when a device that was
> > > > > > > runtime-suspended before system suspend and is left in suspend during
> > > > > > > it, needs to be resumed during the system resume that follows.
> > > > > > >
> > > > > > > Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> > > > > > > cannot happen otherwise, but I think that it is a generally valid
> > > > > > > case.
> > > > > > >
> > > > > > > > If we can make this work, it would enable the propagation of
> > > > > > > > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > > > > > > > right?
> > > > > > >
> > > > > > > It is all until there is a known case in which it isn't.  So either
> > > > > > > you know a specific case in which it doesn't work, or I don't see a
> > > > > > > reason for avoiding it.
> > > > > > >
> > > > > > > ATM the only specific case in which it doesn't work is when
> > > > > > > pm_runtime_force_suspend|resume() are used.
> > > > > > >
> > > > > > > > The point is, the other bigger issue that I pointed out in our earlier
> > > > > > > > discussions; all those devices where their drivers/buses don't cope
> > > > > > > > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > > > > > > > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > > > > > > > parents. I guess this is the best we can do then?
> > > > > > >
> > > > > > > OK, what are they?
> > > > > > >
> > > > > > > You keep saying that they exist without giving any examples.
> > > > > >
> > > > > > To put it more bluntly, I'm not aware of any place other than
> > > > > > pm_runtime_force_suspend|resume() that can be confused by changing the
> > > > > > runtime PM status of a device with runtime PM disabled (either
> > > > > > permanently, or transiently during a system suspend-resume cycle) to
> > > > > > RPM_ACTIVE, so if there are any such places, I would appreciate
> > > > > > letting me know what they are.
> > > > >
> > > > > Well, sorry I thought you were aware. Anyway, I believe you need to do
> > > > > your own investigation as it's simply too time consuming for me to
> > > > > find them all. The problem is that it's not just a simple pattern to
> > > > > search for, so we would need some clever scripting to move forward to
> > > > > find them.
> > > > >
> > > > > To start with, we should look for drivers that enable runtime PM, by
> > > > > calling pm_runtime_enable().
> > > > >
> > > > > Additionally, in their system suspend callback they should typically
> > > > > *not* use pm_runtime_suspended(), pm_runtime_status_suspended() or
> > > > > pm_runtime_active() as that is usually (but no always) indicating that
> > > > > they got it right. Then there are those that don't have system
> > > > > suspend/resume callbacks assigned at all (or uses some other subsystem
> > > > > specific callback for this), but only uses runtime PM.
> > > > >
> > > > > On top of that, drivers that makes use of
> > > > > pm_runtime_force_suspend|resume() should be disregarded, which has
> > > > > reached beyond 300 as this point.
> > > > >
> > > > > Anyway, here is just one example that I found from a quick search.
> > > > > drivers/i2c/busses/i2c-qcom-geni.c
> > > >
> > > > OK, I see.
> > > >
> > > > It sets the status to RPM_SUSPENDED in geni_i2c_suspend_noirq(), if
> > > > not suspended already, and assumes it to stay this way across
> > > > geni_i2c_resume_noirq() until someone resumes it via runtime PM.
> > > >
> > > > Fair enough, but somebody should tell them that they don't need to use
> > > > pm_runtime_disable/enable() in _noirq.
> > > >
> > > > > >
> > > > > > Note that rpm_active() could start producing confusing results if the
> > > > > > runtime PM status of a device with runtime PM disabled is changed from
> > > > > > RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
> > > > > > -EACCES instead of 1, but changing the status to RPM_ACTIVE will not
> > > > > > confuse it the same way.
> > > > >
> > > > > Trust me, it will cause problems.
> > > > >
> > > > > Drivers may call pm_runtime_get_sync() to turn on the resources for
> > > > > the device after the system has resumed, when runtime PM has been
> > > > > re-enabled for the device by the PM core.
> > > > >
> > > > > Those calls to pm_runtime_get_sync() will then not end up invoking any
> > > > > if ->runtime_resume() callbacks for the device since its state is
> > > > > already RPM_ACTIVE. Hence, the device will remain in a low power state
> > > > > even if the driver believes it has been powered on. In many cases,
> > > > > accessing the device (like reading/writing a register) will often just
> > > > > just hang in these cases, but in worst cases we could end up getting
> > > > > even more difficult bugs to debug.
> > > >
> > > > Sure, that would be a problem.
> > > >
> > > > I think I need to find a different way to address the problem I'm
> > > > seeing, that is to resume devices that were runtime-suspended before
> > > > system suspend along with their superiors.
> > > >
> > > > One way to do it would be to just defer their resume to the point when
> > > > the core has enabled runtime PM for them, which means that it has also
> > > > enabled runtime PM for all of their superiors, and then call
> > > > pm_runtime_resume().
> > > >
> > > > This should work unless one of the superiors has runtime PM disabled
> > > > at that point, of course.
> > >
> > > Right, so typically users of pm_runtime_force_suspend|resume() from
> > > the regular ->suspend|resume() callbacks would not work, because they
> > > disable/enable runtime PM. Although, we could probably fix this quite
> > > easily by making some adaptations in
> > > pm_runtime_force_suspend|resume().
> > >
> > > I am not sure if this approach would have any other issue though, but
> > > it seems like it could make sense to explore this approach. In general
> > > drivers should cope with their devices being runtime resumed if
> > > runtime PM is enabled, right!?

In theory.

In practice, though, to start with, this can only be done for devices
whose drivers opt-in (or don't care) and starting with "leaf" devices
(no children or consumers).

> > > If this works, it seems like a generic and a good solution to me.
> >
> > For PCI ports, though, it would require some tuning of
> > ->runtime_resume(), so it is not as simple as it would seem to be in
> > the end.
>
> Okay. Perhaps it would be worth it to try this out anyway, as it would
> allow us to keep the PM core as simple as possible?

Except that initially it will need the same opt-in from everybody
involved as the set active propagation, at least initially, so IMV it
is better to start enforcing the opt-in requirement first and then
convert stuff to the new approach.