Re: [PATCH v1] PM: sleep: core: Synchronize runtime PM status of parents and children

[Rafael J. Wysocki]

On Fri, Feb 7, 2025 at 5:26 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
>
> On Fri, Feb 07, 2025 at 05:06:32PM +0100, Johan Hovold wrote:
> > On Fri, Feb 07, 2025 at 04:41:18PM +0100, Rafael J. Wysocki wrote:
> > > On Fri, Feb 7, 2025 at 3:45 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> > > > On Fri, Feb 07, 2025 at 02:50:29PM +0100, Johan Hovold wrote:
> >
> > > > Ok, so the driver data is never set and runtime PM is never enabled for
> > > > this simple bus device, which uses pm_runtime_force_suspend() for system
> > > > sleep.
> > >
> > > This is kind of confusing.  Why use pm_runtime_force_suspend() if
> > > runtime PM is never enabled and cannot really work?
> >
> > It's only done for some buses that this driver handles. The driver is
> > buggy; I'm preparing a fix for it regardless of the correctness of the
> > commit that now triggered this.
>
> Hmm. The driver implementation is highly odd, but actually works as long
> as the runtime PM status is left at 'suspended' (as
> pm_runtime_force_resume() won't enable runtime PM unless it was enabled
> before suspend).
>
> So we'd strictly only need something like the below if we are going to
> keep the set_active propagation.

I think you are right.

> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> index 5dea31769f9a..d8e029e7e53f 100644
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -109,9 +109,29 @@ static int simple_pm_bus_runtime_resume(struct device *dev)
>         return 0;
>  }
>
> +static int simple_pm_bus_suspend(struct device *dev)
> +{
> +       struct simple_pm_bus *bus = dev_get_drvdata(dev);
> +
> +       if (!bus)
> +               return 0;
> +
> +       return pm_runtime_force_suspend(dev);
> +}
> +
> +static int simple_pm_bus_resume(struct device *dev)
> +{
> +       struct simple_pm_bus *bus = dev_get_drvdata(dev);
> +
> +       if (!bus)
> +               return 0;
> +
> +       return pm_runtime_force_resume(dev);
> +}
> +
>  static const struct dev_pm_ops simple_pm_bus_pm_ops = {
>         RUNTIME_PM_OPS(simple_pm_bus_runtime_suspend, simple_pm_bus_runtime_resume, NULL)
> -       NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +       NOIRQ_SYSTEM_SLEEP_PM_OPS(simple_pm_bus_suspend, simple_pm_bus_resume)
>  };
>
>  #define ONLY_BUS       ((void *) 1) /* Match if the device is only a bus. */

In the meantime, I've cut the attached (untested) patch that should
take care of the pm_runtime_force_suspend() issue altogether.

It changes the code to only set the device's runtime PM status to
"active" if runtime PM is going to be enabled for it by the first
pm_runtime_enable() call, which never happens for devices where
runtime PM has never been enabled (because it is disabled for them
once again in device_suspend_late()) and for devices subject to
pm_runtime_force_suspend() during system suspend (because it disables
runtime PM for them once again).
---
 drivers/base/power/main.c    |   20 ++++++++++++++++----
 drivers/base/power/runtime.c |   41 ++++++++++++++++++++++++++++++++++++-----
 include/linux/pm_runtime.h   |    5 +++++
 3 files changed, 57 insertions(+), 9 deletions(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -655,14 +655,25 @@
 	 * this device later, it needs to appear as "suspended" to PM-runtime,
 	 * so change its status accordingly.
 	 *
-	 * Otherwise, the device is going to be resumed, so set its PM-runtime
-	 * status to "active" unless its power.set_active flag is clear, in
-	 * which case it is not necessary to update its PM-runtime status.
+	 * Otherwise, the device is going to be resumed, so try to update its
+	 * PM-runtime status unless its power.set_active flag is clear, in which
+	 * case it is not necessary to do that.
 	 */
 	if (skip_resume) {
 		pm_runtime_set_suspended(dev);
 	} else if (dev->power.set_active) {
-		pm_runtime_set_active(dev);
+		/*
+		 * If PM-runtime is not going to be actually enabled for the
+		 * device by a subsequent pm_runtime_enabled() call, it may
+		 * have never been enabled or pm_runtime_force_suspend() may
+		 * have been used.  Don't update the PM-runtime statue in
+		 * that case and set power.needs_force_resume in case
+		 * pm_runtime_force_resume() will be called for the device
+		 * subsequently.
+		 */
+		if (pm_runtime_cond_set_active(dev) > 0)
+			dev->power.needs_force_resume = true;
+
 		dev->power.set_active = false;
 	}
 
@@ -988,6 +999,7 @@
  End:
 	error = dpm_run_callback(callback, dev, state, info);
 	dev->power.is_suspended = false;
+	dev->power.needs_force_resume = false;
 
  Unlock:
 	device_unlock(dev);
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1253,9 +1253,10 @@
 EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
 
 /**
- * __pm_runtime_set_status - Set runtime PM status of a device.
+ * pm_runtime_set_status_internal - Set runtime PM status of a device.
  * @dev: Device to handle.
  * @status: New runtime PM status of the device.
+ * @cond: Change the status if runtime PM will be enabled by the next attempt.
  *
  * If runtime PM of the device is disabled or its power.runtime_error field is
  * different from zero, the status may be changed either to RPM_ACTIVE, or to
@@ -1275,8 +1276,12 @@
  * of the @status value) and the suppliers will be deacticated on exit.  The
  * error returned by the failing supplier activation will be returned in that
  * case.
+ *
+ * If @cond is set, only change the status if power.disable_depth is equal to 1,
+ * or do nothing and return (power.disable_depth - 1) otherwise.
  */
-int __pm_runtime_set_status(struct device *dev, unsigned int status)
+static int pm_runtime_set_status_internal(struct device *dev,
+					  unsigned int status, bool cond)
 {
 	struct device *parent = dev->parent;
 	bool notify_parent = false;
@@ -1292,10 +1297,26 @@
 	 * Prevent PM-runtime from being enabled for the device or return an
 	 * error if it is enabled already and working.
 	 */
-	if (dev->power.runtime_error || dev->power.disable_depth)
-		dev->power.disable_depth++;
-	else
+	if (dev->power.runtime_error) {
+		if (cond)
+			error = -EINVAL;
+		else
+			dev->power.disable_depth++;
+	} else if (dev->power.disable_depth) {
+		/*
+		 * If cond is set, only attempt to change the status if the
+		 * next invocation of pm_runtime_enable() for the device is
+		 * going to actually enable runtime PM for it.
+		 *
+		 * This is used in a corner case during system-wide resume.
+		 */
+		if (cond && dev->power.disable_depth > 1)
+			error = dev->power.disable_depth - 1;
+		else
+			dev->power.disable_depth++;
+	} else {
 		error = -EAGAIN;
+	}
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 
@@ -1376,6 +1397,16 @@
 
 	return error;
 }
+
+int pm_runtime_cond_set_active(struct device *dev)
+{
+	return pm_runtime_set_status_internal(dev, RPM_ACTIVE, true);
+}
+
+int __pm_runtime_set_status(struct device *dev, unsigned int status)
+{
+	return pm_runtime_set_status_internal(dev, status, false);
+}
 EXPORT_SYMBOL_GPL(__pm_runtime_set_status);
 
 /**
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -75,6 +75,7 @@
 extern int pm_runtime_get_if_active(struct device *dev);
 extern int pm_runtime_get_if_in_use(struct device *dev);
 extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
+extern int pm_runtime_cond_set_active(struct device *dev);
 extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
 extern void pm_runtime_enable(struct device *dev);
@@ -268,6 +269,10 @@
 {
 	return -EINVAL;
 }
+static inline int pm_runtime_cond_set_active(struct device *dev)
+{
+	return 1;
+}
 static inline int __pm_runtime_set_status(struct device *dev,
 					    unsigned int status) { return 0; }
 static inline int pm_runtime_barrier(struct device *dev) { return 0; }