Message-ID: <3817761.MHq7AAxBmi@rjwysocki.net>
Date: Tue, 11 Feb 2025 22:19:13 +0100
From: "Rafael J. Wysocki" <rjw@...ysocki.net>
To: Linux PM <linux-pm@...r.kernel.org>, Ulf Hansson <ulf.hansson@...aro.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
 Alan Stern <stern@...land.harvard.edu>, Johan Hovold <johan@...nel.org>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
 Jon Hunter <jonathanh@...dia.com>,
 Tony Lindgren <tony.lindgren@...ux.intel.com>
Subject:
 [PATCH v1 08/10] PM: sleep: Make pm_runtime_force_resume() look at
 power.set_active

From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...el.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;

Powered by blists - more mailing lists