[PATCH v1 0/3] PM: Use DPM_FLAG_SMART_SUSPEND conditionally

[PATCH v1 0/3] PM: Use DPM_FLAG_SMART_SUSPEND conditionally

[Rafael J. Wysocki]

Hi Everyone,

This is a follow-up for the discussion on the patchset at

https://lore.kernel.org/linux-pm/2314745.iZASKD2KPV@rjwysocki.net/

that suggested an adjustment of the approach.

Accordingly, this series modifies the PM core and the users of
DPM_FLAG_SMART_SUSPEND to take it into account only if it is
consistently used in dependency graphs, as described in the
changelog of patch [3/3].

Patches [1-2/3] are preparatory and they arrange for the handling
of devices with no runtime PM support in a meaningful way.

Thanks!

[PATCH v1 1/3] PM: Block enabling of runtime PM during system suspend

[Rafael J. Wysocki]

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

If device_prepare() runs on a device that has never had runtime
PM enabled so far, it may reasonably assume that runtime PM will
not be enabled for that device during the system suspend-resume
cycle currently in progress, but this has never been guaranteed.

To verify this assumption, make device_prepare() arrange for
triggering a device warning accompanied by a call trace dump if
runtime PM is enabled for such a device after it has returned.

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

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1109,6 +1109,8 @@
 	device_unlock(dev);
 
 out:
+	/* If enabling runtime PM for the device is blocked, unblock it. */
+	pm_runtime_unblock(dev);
 	pm_runtime_put(dev);
 }
 
@@ -1815,6 +1817,13 @@
 	 * it again during the complete phase.
 	 */
 	pm_runtime_get_noresume(dev);
+	/*
+	 * If runtime PM is disabled for the device at this point and it has
+	 * never been enabled so far, it should not be enabled until this system
+	 * suspend-resume cycle is complete, so prepare to trigger a warning on
+	 * subsequent attempts to enable it.
+	 */
+	pm_runtime_block_if_disabled(dev);
 
 	if (dev->power.syscore)
 		return 0;
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1460,6 +1460,26 @@
 }
 EXPORT_SYMBOL_GPL(pm_runtime_barrier);
 
+void pm_runtime_block_if_disabled(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_BLOCKED;
+
+	spin_unlock_irq(&dev->power.lock);
+}
+
+void pm_runtime_unblock(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+
+	if (dev->power.last_status == RPM_BLOCKED)
+		dev->power.last_status = RPM_INVALID;
+
+	spin_unlock_irq(&dev->power.lock);
+}
+
 void __pm_runtime_disable(struct device *dev, bool check_resume)
 {
 	spin_lock_irq(&dev->power.lock);
@@ -1518,6 +1538,10 @@
 	if (--dev->power.disable_depth > 0)
 		goto out;
 
+	if (dev->power.last_status == RPM_BLOCKED) {
+		dev_warn(dev, "Attempt to enabled runtime PM when it is blocked\n");
+		dump_stack();
+	}
 	dev->power.last_status = RPM_INVALID;
 	dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
 
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -597,6 +597,7 @@
 	RPM_RESUMING,
 	RPM_SUSPENDED,
 	RPM_SUSPENDING,
+	RPM_BLOCKED,
 };
 
 /*
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -77,8 +77,10 @@
 extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
 extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
+extern void pm_runtime_block_if_disabled(struct device *dev);
+extern void pm_runtime_unblock(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_disable(struct device *dev, bool regular);
 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);
@@ -271,6 +273,8 @@
 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; }
+static inline void pm_runtime_block_if_disabled(struct device *dev) {}
+static inline void pm_runtime_unblock(struct device *dev) {}
 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_allow(struct device *dev) {}

[PATCH v1 2/3] PM: runtime: Introduce pm_runtime_blocked()

[Rafael J. Wysocki]

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

Introduce a new helper function called pm_runtime_blocked() for
checking the power.last_status value indicating whether or not the
enabling of runtime PM for the given device has been blocked (which
happens in the "prepare" phase of system-wide suspend if runtime
PM is disabled for the given device at that point and has never
been enabled so far).

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

--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1555,6 +1555,23 @@
 }
 EXPORT_SYMBOL_GPL(pm_runtime_enable);
 
+bool pm_runtime_blocked(struct device *dev)
+{
+	bool ret;
+
+	/*
+	 * dev->power.last_status is a bit field, so in case it is updated via
+	 * RMW, read it under the spin lock.
+	 */
+	spin_lock_irq(&dev->power.lock);
+
+	ret = dev->power.last_status == RPM_BLOCKED;
+
+	spin_unlock_irq(&dev->power.lock);
+
+	return ret;
+}
+
 static void pm_runtime_disable_action(void *data)
 {
 	pm_runtime_dont_use_autosuspend(data);
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -81,6 +81,7 @@
 extern void pm_runtime_unblock(struct device *dev);
 extern void pm_runtime_enable(struct device *dev);
 extern void __pm_runtime_disable(struct device *dev, bool regular);
+extern bool pm_runtime_blocked(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);
@@ -277,6 +278,7 @@
 static inline void pm_runtime_unblock(struct device *dev) {}
 static inline void pm_runtime_enable(struct device *dev) {}
 static inline void __pm_runtime_disable(struct device *dev, bool c) {}
+static inline bool pm_runtime_blocked(struct device *dev) { return true; }
 static inline void pm_runtime_allow(struct device *dev) {}
 static inline void pm_runtime_forbid(struct device *dev) {}
 

[PATCH v1 3/3] PM: sleep: Use DPM_FLAG_SMART_SUSPEND conditionally

[Rafael J. Wysocki]

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

A recent discussion has revealed that using DPM_FLAG_SMART_SUSPEND
unconditionally is generally problematic because it may lead to
situations in which the device's runtime PM information is internally
inconsistent or does not reflect its real state [1].

For this reason, change the handling of DPM_FLAG_SMART_SUSPEND so that
it is only taken into account if it is consistently set by the drivers
of all devices having any PM callbacks throughout dependency graphs in
accordance with the following rules:

 - The "smart suspend" feature is only enabled for devices whose drivers
   ask for it (that is, set DPM_FLAG_SMART_SUSPEND) and for devices
   without PM callbacks unless they have never had runtime PM enabled.

 - The "smart suspend" feature is not enabled for a device if it has not
   been enabled for the device's parent unless the parent does not take
   children into account or it has never had runtime PM enabled.

 - The "smart suspend" feature is not enabled for a device if it has not
   been enabled for one of the device's suppliers taking runtime PM into
   account unless that supplier has never had runtime PM enabled.

Namely, introduce a new device PM flag called smart_suspend that is only
set if the above conditions are met and update all DPM_FLAG_SMART_SUSPEND
users to check power.smart_suspend instead of directly checking the
latter.

At the same time, drop the power.set_active flage introduced recently
in commit 3775fc538f53 ("PM: sleep: core: Synchronize runtime PM status
of parents and children") because it is now sufficient to check
power.smart_suspend along with the dev_pm_skip_resume() return value
to decide whether or not pm_runtime_set_active() needs to be called
for the device.

Link: https://lore.kernel.org/linux-pm/CAPDyKFroyU3YDSfw_Y6k3giVfajg3NQGwNWeteJWqpW29BojhQ@mail.gmail.com/  [1]
Fixes: 7585946243d6 ("PM: sleep: core: Restrict power.set_active propagation")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c  |    6 +---
 drivers/base/power/main.c |   63 +++++++++++++++++++++++++++++++++++-----------
 drivers/mfd/intel-lpss.c  |    2 -
 drivers/pci/pci-driver.c  |    6 +---
 include/linux/pm.h        |    2 -
 5 files changed, 55 insertions(+), 24 deletions(-)

--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1161,8 +1161,7 @@
  */
 int acpi_subsys_suspend(struct device *dev)
 {
-	if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
-	    acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
+	if (!dev->power.smart_suspend || acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
 		pm_runtime_resume(dev);
 
 	return pm_generic_suspend(dev);
@@ -1320,8 +1319,7 @@
  */
 int acpi_subsys_poweroff(struct device *dev)
 {
-	if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
-	    acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
+	if (!dev->power.smart_suspend || acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
 		pm_runtime_resume(dev);
 
 	return pm_generic_poweroff(dev);
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -656,15 +656,13 @@
 	 * 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
+	 * status to "active" unless its power.smart_suspend 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.smart_suspend)
 		pm_runtime_set_active(dev);
-		dev->power.set_active = false;
-	}
 
 	if (dev->pm_domain) {
 		info = "noirq power domain ";
@@ -1282,14 +1280,8 @@
 	      dev->power.may_skip_resume))
 		dev->power.must_resume = true;
 
-	if (dev->power.must_resume) {
-		if (dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND)) {
-			dev->power.set_active = true;
-			if (dev->parent && !dev->parent->power.ignore_children)
-				dev->parent->power.set_active = true;
-		}
+	if (dev->power.must_resume)
 		dpm_superior_set_must_resume(dev);
-	}
 
 Complete:
 	complete_all(&dev->power.completion);
@@ -1797,6 +1789,49 @@
 	return error;
 }
 
+static void device_prepare_smart_suspend(struct device *dev)
+{
+	struct device_link *link;
+	int idx;
+
+	/*
+	 * The "smart suspend" feature is enabled for devices whose drivers ask
+	 * for it and for devices without PM callbacks unless runtime PM is
+	 * disabled and enabling it is blocked for them.
+	 *
+	 * However, if "smart suspend" is not enabled for the device's parent
+	 * or any of its suppliers that take runtime PM into account, it cannot
+	 * be enabled for the device either.
+	 */
+	dev->power.smart_suspend = (dev->power.no_pm_callbacks ||
+		dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND)) &&
+		!pm_runtime_blocked(dev);
+
+	if (!dev->power.smart_suspend)
+		return;
+
+	if (dev->parent && !pm_runtime_blocked(dev->parent) &&
+	    !dev->parent->power.ignore_children && !dev->parent->power.smart_suspend) {
+		dev->power.smart_suspend = false;
+		return;
+	}
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) {
+		if (!(link->flags | DL_FLAG_PM_RUNTIME))
+			continue;
+
+		if (!pm_runtime_blocked(link->supplier) &&
+		    !link->supplier->power.smart_suspend) {
+			dev->power.smart_suspend = false;
+			break;
+		}
+	}
+
+	device_links_read_unlock(idx);
+}
+
 /**
  * device_prepare - Prepare a device for system power transition.
  * @dev: Device to handle.
@@ -1858,6 +1893,7 @@
 		pm_runtime_put(dev);
 		return ret;
 	}
+	device_prepare_smart_suspend(dev);
 	/*
 	 * A positive return value from ->prepare() means "this device appears
 	 * to be runtime-suspended and its state is fine, so if it really is
@@ -2033,6 +2069,5 @@
 
 bool dev_pm_skip_suspend(struct device *dev)
 {
-	return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) &&
-		pm_runtime_status_suspended(dev);
+	return dev->power.smart_suspend && pm_runtime_status_suspended(dev);
 }
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c
@@ -480,7 +480,7 @@
 
 static int resume_lpss_device(struct device *dev, void *data)
 {
-	if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND))
+	if (!dev->power.smart_suspend)
 		pm_runtime_resume(dev);
 
 	return 0;
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -812,8 +812,7 @@
 	 * suspend callbacks can cope with runtime-suspended devices, it is
 	 * better to resume the device from runtime suspend here.
 	 */
-	if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
-	    pci_dev_need_resume(pci_dev)) {
+	if (!dev->power.smart_suspend || pci_dev_need_resume(pci_dev)) {
 		pm_runtime_resume(dev);
 		pci_dev->state_saved = false;
 	} else {
@@ -1151,8 +1150,7 @@
 	}
 
 	/* The reason to do that is the same as in pci_pm_suspend(). */
-	if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
-	    pci_dev_need_resume(pci_dev)) {
+	if (!dev->power.smart_suspend || pci_dev_need_resume(pci_dev)) {
 		pm_runtime_resume(dev);
 		pci_dev->state_saved = false;
 	} else {
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -680,8 +680,8 @@
 	bool			syscore:1;
 	bool			no_pm_callbacks:1;	/* Owned by the PM core */
 	bool			async_in_progress:1;	/* Owned by the PM core */
+	bool			smart_suspend:1;	/* Owned by the PM core */
 	bool			must_resume:1;		/* Owned by the PM core */
-	bool			set_active:1;		/* Owned by the PM core */
 	bool			may_skip_resume:1;	/* Set by subsystems */
 #else
 	bool			should_wakeup:1;

Re: [PATCH v1 3/3] PM: sleep: Use DPM_FLAG_SMART_SUSPEND conditionally

[Ulf Hansson]

+ Saravana

On Mon, 17 Feb 2025 at 21:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> A recent discussion has revealed that using DPM_FLAG_SMART_SUSPEND
> unconditionally is generally problematic because it may lead to
> situations in which the device's runtime PM information is internally
> inconsistent or does not reflect its real state [1].
>
> For this reason, change the handling of DPM_FLAG_SMART_SUSPEND so that
> it is only taken into account if it is consistently set by the drivers
> of all devices having any PM callbacks throughout dependency graphs in
> accordance with the following rules:
>
>  - The "smart suspend" feature is only enabled for devices whose drivers
>    ask for it (that is, set DPM_FLAG_SMART_SUSPEND) and for devices
>    without PM callbacks unless they have never had runtime PM enabled.
>
>  - The "smart suspend" feature is not enabled for a device if it has not
>    been enabled for the device's parent unless the parent does not take
>    children into account or it has never had runtime PM enabled.
>
>  - The "smart suspend" feature is not enabled for a device if it has not
>    been enabled for one of the device's suppliers taking runtime PM into
>    account unless that supplier has never had runtime PM enabled.
>
> Namely, introduce a new device PM flag called smart_suspend that is only
> set if the above conditions are met and update all DPM_FLAG_SMART_SUSPEND
> users to check power.smart_suspend instead of directly checking the
> latter.
>
> At the same time, drop the power.set_active flage introduced recently
> in commit 3775fc538f53 ("PM: sleep: core: Synchronize runtime PM status
> of parents and children") because it is now sufficient to check
> power.smart_suspend along with the dev_pm_skip_resume() return value
> to decide whether or not pm_runtime_set_active() needs to be called
> for the device.
>
> Link: https://lore.kernel.org/linux-pm/CAPDyKFroyU3YDSfw_Y6k3giVfajg3NQGwNWeteJWqpW29BojhQ@mail.gmail.com/  [1]
> Fixes: 7585946243d6 ("PM: sleep: core: Restrict power.set_active propagation")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/device_pm.c  |    6 +---
>  drivers/base/power/main.c |   63 +++++++++++++++++++++++++++++++++++-----------
>  drivers/mfd/intel-lpss.c  |    2 -
>  drivers/pci/pci-driver.c  |    6 +---
>  include/linux/pm.h        |    2 -
>  5 files changed, 55 insertions(+), 24 deletions(-)
>
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1161,8 +1161,7 @@
>   */
>  int acpi_subsys_suspend(struct device *dev)
>  {
> -       if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
> -           acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
> +       if (!dev->power.smart_suspend || acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))

Nitpick: Rather than checking the dev->power.smart_suspend flag
directly here, perhaps we should provide a helper function that
returns true when dev->power.smart_suspend is set? In this way, it's
the PM core soley that operates on the flag.

>                 pm_runtime_resume(dev);
>
>         return pm_generic_suspend(dev);
> @@ -1320,8 +1319,7 @@
>   */
>  int acpi_subsys_poweroff(struct device *dev)
>  {
> -       if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
> -           acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
> +       if (!dev->power.smart_suspend || acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
>                 pm_runtime_resume(dev);
>
>         return pm_generic_poweroff(dev);
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -656,15 +656,13 @@
>          * 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
> +        * status to "active" unless its power.smart_suspend 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.smart_suspend)
>                 pm_runtime_set_active(dev);
> -               dev->power.set_active = false;
> -       }
>
>         if (dev->pm_domain) {
>                 info = "noirq power domain ";
> @@ -1282,14 +1280,8 @@
>               dev->power.may_skip_resume))
>                 dev->power.must_resume = true;
>
> -       if (dev->power.must_resume) {
> -               if (dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND)) {
> -                       dev->power.set_active = true;
> -                       if (dev->parent && !dev->parent->power.ignore_children)
> -                               dev->parent->power.set_active = true;
> -               }
> +       if (dev->power.must_resume)
>                 dpm_superior_set_must_resume(dev);
> -       }
>
>  Complete:
>         complete_all(&dev->power.completion);
> @@ -1797,6 +1789,49 @@
>         return error;
>  }
>
> +static void device_prepare_smart_suspend(struct device *dev)
> +{
> +       struct device_link *link;
> +       int idx;
> +
> +       /*
> +        * The "smart suspend" feature is enabled for devices whose drivers ask
> +        * for it and for devices without PM callbacks unless runtime PM is
> +        * disabled and enabling it is blocked for them.
> +        *
> +        * However, if "smart suspend" is not enabled for the device's parent
> +        * or any of its suppliers that take runtime PM into account, it cannot
> +        * be enabled for the device either.
> +        */
> +       dev->power.smart_suspend = (dev->power.no_pm_callbacks ||
> +               dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND)) &&
> +               !pm_runtime_blocked(dev);
> +
> +       if (!dev->power.smart_suspend)
> +               return;
> +
> +       if (dev->parent && !pm_runtime_blocked(dev->parent) &&
> +           !dev->parent->power.ignore_children && !dev->parent->power.smart_suspend) {
> +               dev->power.smart_suspend = false;
> +               return;
> +       }
> +
> +       idx = device_links_read_lock();
> +
> +       list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) {
> +               if (!(link->flags | DL_FLAG_PM_RUNTIME))
> +                       continue;
> +
> +               if (!pm_runtime_blocked(link->supplier) &&
> +                   !link->supplier->power.smart_suspend) {

This requires device_prepare() for all suppliers to be run before its
consumer. Is that always the case?

> +                       dev->power.smart_suspend = false;
> +                       break;
> +               }
> +       }
> +
> +       device_links_read_unlock(idx);

From an execution overhead point of view, did you check if the above
code had some measurable impact on the latency for dpm_prepare()?

> +}
> +
>  /**
>   * device_prepare - Prepare a device for system power transition.
>   * @dev: Device to handle.
> @@ -1858,6 +1893,7 @@
>                 pm_runtime_put(dev);
>                 return ret;
>         }
> +       device_prepare_smart_suspend(dev);
>         /*
>          * A positive return value from ->prepare() means "this device appears
>          * to be runtime-suspended and its state is fine, so if it really is
> @@ -2033,6 +2069,5 @@
>
>  bool dev_pm_skip_suspend(struct device *dev)
>  {
> -       return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) &&
> -               pm_runtime_status_suspended(dev);
> +       return dev->power.smart_suspend && pm_runtime_status_suspended(dev);
>  }
> --- a/drivers/mfd/intel-lpss.c
> +++ b/drivers/mfd/intel-lpss.c
> @@ -480,7 +480,7 @@
>
>  static int resume_lpss_device(struct device *dev, void *data)
>  {
> -       if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND))
> +       if (!dev->power.smart_suspend)
>                 pm_runtime_resume(dev);
>
>         return 0;
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -812,8 +812,7 @@
>          * suspend callbacks can cope with runtime-suspended devices, it is
>          * better to resume the device from runtime suspend here.
>          */
> -       if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
> -           pci_dev_need_resume(pci_dev)) {
> +       if (!dev->power.smart_suspend || pci_dev_need_resume(pci_dev)) {
>                 pm_runtime_resume(dev);
>                 pci_dev->state_saved = false;
>         } else {
> @@ -1151,8 +1150,7 @@
>         }
>
>         /* The reason to do that is the same as in pci_pm_suspend(). */
> -       if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
> -           pci_dev_need_resume(pci_dev)) {
> +       if (!dev->power.smart_suspend || pci_dev_need_resume(pci_dev)) {
>                 pm_runtime_resume(dev);
>                 pci_dev->state_saved = false;
>         } else {
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -680,8 +680,8 @@
>         bool                    syscore:1;
>         bool                    no_pm_callbacks:1;      /* Owned by the PM core */
>         bool                    async_in_progress:1;    /* Owned by the PM core */
> +       bool                    smart_suspend:1;        /* Owned by the PM core */
>         bool                    must_resume:1;          /* Owned by the PM core */
> -       bool                    set_active:1;           /* Owned by the PM core */
>         bool                    may_skip_resume:1;      /* Set by subsystems */
>  #else
>         bool                    should_wakeup:1;
>
>
>

Kind regards
Uffe

Re: [PATCH v1 1/3] PM: Block enabling of runtime PM during system suspend

[Ulf Hansson]

On Mon, 17 Feb 2025 at 21:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> If device_prepare() runs on a device that has never had runtime
> PM enabled so far, it may reasonably assume that runtime PM will
> not be enabled for that device during the system suspend-resume
> cycle currently in progress, but this has never been guaranteed.
>
> To verify this assumption, make device_prepare() arrange for
> triggering a device warning accompanied by a call trace dump if
> runtime PM is enabled for such a device after it has returned.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/main.c    |    9 +++++++++
>  drivers/base/power/runtime.c |   24 ++++++++++++++++++++++++
>  include/linux/pm.h           |    1 +
>  include/linux/pm_runtime.h   |    6 +++++-
>  4 files changed, 39 insertions(+), 1 deletion(-)
>
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1109,6 +1109,8 @@
>         device_unlock(dev);
>
>  out:
> +       /* If enabling runtime PM for the device is blocked, unblock it. */
> +       pm_runtime_unblock(dev);
>         pm_runtime_put(dev);
>  }
>
> @@ -1815,6 +1817,13 @@
>          * it again during the complete phase.
>          */
>         pm_runtime_get_noresume(dev);
> +       /*
> +        * If runtime PM is disabled for the device at this point and it has
> +        * never been enabled so far, it should not be enabled until this system
> +        * suspend-resume cycle is complete, so prepare to trigger a warning on
> +        * subsequent attempts to enable it.
> +        */
> +       pm_runtime_block_if_disabled(dev);
>
>         if (dev->power.syscore)
>                 return 0;
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1460,6 +1460,26 @@
>  }
>  EXPORT_SYMBOL_GPL(pm_runtime_barrier);
>
> +void pm_runtime_block_if_disabled(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_BLOCKED;
> +
> +       spin_unlock_irq(&dev->power.lock);
> +}
> +
> +void pm_runtime_unblock(struct device *dev)
> +{
> +       spin_lock_irq(&dev->power.lock);
> +
> +       if (dev->power.last_status == RPM_BLOCKED)
> +               dev->power.last_status = RPM_INVALID;
> +
> +       spin_unlock_irq(&dev->power.lock);
> +}
> +
>  void __pm_runtime_disable(struct device *dev, bool check_resume)
>  {
>         spin_lock_irq(&dev->power.lock);
> @@ -1518,6 +1538,10 @@
>         if (--dev->power.disable_depth > 0)
>                 goto out;
>
> +       if (dev->power.last_status == RPM_BLOCKED) {
> +               dev_warn(dev, "Attempt to enabled runtime PM when it is blocked\n");

/s/enabled/enable

> +               dump_stack();
> +       }
>         dev->power.last_status = RPM_INVALID;
>         dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
>
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -597,6 +597,7 @@
>         RPM_RESUMING,
>         RPM_SUSPENDED,
>         RPM_SUSPENDING,
> +       RPM_BLOCKED,
>  };
>
>  /*
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -77,8 +77,10 @@
>  extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
>  extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
>  extern int pm_runtime_barrier(struct device *dev);
> +extern void pm_runtime_block_if_disabled(struct device *dev);
> +extern void pm_runtime_unblock(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_disable(struct device *dev, bool regular);

This looks unrelated to the $subject patch.

>  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);
> @@ -271,6 +273,8 @@
>  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; }
> +static inline void pm_runtime_block_if_disabled(struct device *dev) {}
> +static inline void pm_runtime_unblock(struct device *dev) {}
>  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_allow(struct device *dev) {}
>
>
>

With the minor things above fixed, feel free to add:

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

Kind regards
Uffe

Re: [PATCH v1 2/3] PM: runtime: Introduce pm_runtime_blocked()

[Ulf Hansson]

On Mon, 17 Feb 2025 at 21:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Introduce a new helper function called pm_runtime_blocked() for
> checking the power.last_status value indicating whether or not the
> enabling of runtime PM for the given device has been blocked (which
> happens in the "prepare" phase of system-wide suspend if runtime
> PM is disabled for the given device at that point and has never
> been enabled so far).
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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

Kind regards
Uffe


> ---
>  drivers/base/power/runtime.c |   17 +++++++++++++++++
>  include/linux/pm_runtime.h   |    2 ++
>  2 files changed, 19 insertions(+)
>
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1555,6 +1555,23 @@
>  }
>  EXPORT_SYMBOL_GPL(pm_runtime_enable);
>
> +bool pm_runtime_blocked(struct device *dev)
> +{
> +       bool ret;
> +
> +       /*
> +        * dev->power.last_status is a bit field, so in case it is updated via
> +        * RMW, read it under the spin lock.
> +        */
> +       spin_lock_irq(&dev->power.lock);
> +
> +       ret = dev->power.last_status == RPM_BLOCKED;
> +
> +       spin_unlock_irq(&dev->power.lock);
> +
> +       return ret;
> +}
> +
>  static void pm_runtime_disable_action(void *data)
>  {
>         pm_runtime_dont_use_autosuspend(data);
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -81,6 +81,7 @@
>  extern void pm_runtime_unblock(struct device *dev);
>  extern void pm_runtime_enable(struct device *dev);
>  extern void __pm_runtime_disable(struct device *dev, bool regular);
> +extern bool pm_runtime_blocked(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);
> @@ -277,6 +278,7 @@
>  static inline void pm_runtime_unblock(struct device *dev) {}
>  static inline void pm_runtime_enable(struct device *dev) {}
>  static inline void __pm_runtime_disable(struct device *dev, bool c) {}
> +static inline bool pm_runtime_blocked(struct device *dev) { return true; }
>  static inline void pm_runtime_allow(struct device *dev) {}
>  static inline void pm_runtime_forbid(struct device *dev) {}
>
>
>
>

Re: [PATCH v1 3/3] PM: sleep: Use DPM_FLAG_SMART_SUSPEND conditionally

[Rafael J. Wysocki]

On Tue, Feb 18, 2025 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> + Saravana
>
> On Mon, 17 Feb 2025 at 21:19, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > A recent discussion has revealed that using DPM_FLAG_SMART_SUSPEND
> > unconditionally is generally problematic because it may lead to
> > situations in which the device's runtime PM information is internally
> > inconsistent or does not reflect its real state [1].
> >
> > For this reason, change the handling of DPM_FLAG_SMART_SUSPEND so that
> > it is only taken into account if it is consistently set by the drivers
> > of all devices having any PM callbacks throughout dependency graphs in
> > accordance with the following rules:
> >
> >  - The "smart suspend" feature is only enabled for devices whose drivers
> >    ask for it (that is, set DPM_FLAG_SMART_SUSPEND) and for devices
> >    without PM callbacks unless they have never had runtime PM enabled.
> >
> >  - The "smart suspend" feature is not enabled for a device if it has not
> >    been enabled for the device's parent unless the parent does not take
> >    children into account or it has never had runtime PM enabled.
> >
> >  - The "smart suspend" feature is not enabled for a device if it has not
> >    been enabled for one of the device's suppliers taking runtime PM into
> >    account unless that supplier has never had runtime PM enabled.
> >
> > Namely, introduce a new device PM flag called smart_suspend that is only
> > set if the above conditions are met and update all DPM_FLAG_SMART_SUSPEND
> > users to check power.smart_suspend instead of directly checking the
> > latter.
> >
> > At the same time, drop the power.set_active flage introduced recently
> > in commit 3775fc538f53 ("PM: sleep: core: Synchronize runtime PM status
> > of parents and children") because it is now sufficient to check
> > power.smart_suspend along with the dev_pm_skip_resume() return value
> > to decide whether or not pm_runtime_set_active() needs to be called
> > for the device.
> >
> > Link: https://lore.kernel.org/linux-pm/CAPDyKFroyU3YDSfw_Y6k3giVfajg3NQGwNWeteJWqpW29BojhQ@mail.gmail.com/  [1]
> > Fixes: 7585946243d6 ("PM: sleep: core: Restrict power.set_active propagation")
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/device_pm.c  |    6 +---
> >  drivers/base/power/main.c |   63 +++++++++++++++++++++++++++++++++++-----------
> >  drivers/mfd/intel-lpss.c  |    2 -
> >  drivers/pci/pci-driver.c  |    6 +---
> >  include/linux/pm.h        |    2 -
> >  5 files changed, 55 insertions(+), 24 deletions(-)
> >
> > --- a/drivers/acpi/device_pm.c
> > +++ b/drivers/acpi/device_pm.c
> > @@ -1161,8 +1161,7 @@
> >   */
> >  int acpi_subsys_suspend(struct device *dev)
> >  {
> > -       if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
> > -           acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
> > +       if (!dev->power.smart_suspend || acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
>
> Nitpick: Rather than checking the dev->power.smart_suspend flag
> directly here, perhaps we should provide a helper function that
> returns true when dev->power.smart_suspend is set? In this way, it's
> the PM core soley that operates on the flag.

I can add a wrapper around this, sure.

> >                 pm_runtime_resume(dev);
> >
> >         return pm_generic_suspend(dev);
> > @@ -1320,8 +1319,7 @@
> >   */
> >  int acpi_subsys_poweroff(struct device *dev)
> >  {
> > -       if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
> > -           acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
> > +       if (!dev->power.smart_suspend || acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
> >                 pm_runtime_resume(dev);
> >
> >         return pm_generic_poweroff(dev);
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -656,15 +656,13 @@
> >          * 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
> > +        * status to "active" unless its power.smart_suspend 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.smart_suspend)
> >                 pm_runtime_set_active(dev);
> > -               dev->power.set_active = false;
> > -       }
> >
> >         if (dev->pm_domain) {
> >                 info = "noirq power domain ";
> > @@ -1282,14 +1280,8 @@
> >               dev->power.may_skip_resume))
> >                 dev->power.must_resume = true;
> >
> > -       if (dev->power.must_resume) {
> > -               if (dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND)) {
> > -                       dev->power.set_active = true;
> > -                       if (dev->parent && !dev->parent->power.ignore_children)
> > -                               dev->parent->power.set_active = true;
> > -               }
> > +       if (dev->power.must_resume)
> >                 dpm_superior_set_must_resume(dev);
> > -       }
> >
> >  Complete:
> >         complete_all(&dev->power.completion);
> > @@ -1797,6 +1789,49 @@
> >         return error;
> >  }
> >
> > +static void device_prepare_smart_suspend(struct device *dev)
> > +{
> > +       struct device_link *link;
> > +       int idx;
> > +
> > +       /*
> > +        * The "smart suspend" feature is enabled for devices whose drivers ask
> > +        * for it and for devices without PM callbacks unless runtime PM is
> > +        * disabled and enabling it is blocked for them.
> > +        *
> > +        * However, if "smart suspend" is not enabled for the device's parent
> > +        * or any of its suppliers that take runtime PM into account, it cannot
> > +        * be enabled for the device either.
> > +        */
> > +       dev->power.smart_suspend = (dev->power.no_pm_callbacks ||
> > +               dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND)) &&
> > +               !pm_runtime_blocked(dev);
> > +
> > +       if (!dev->power.smart_suspend)
> > +               return;
> > +
> > +       if (dev->parent && !pm_runtime_blocked(dev->parent) &&
> > +           !dev->parent->power.ignore_children && !dev->parent->power.smart_suspend) {
> > +               dev->power.smart_suspend = false;
> > +               return;
> > +       }
> > +
> > +       idx = device_links_read_lock();
> > +
> > +       list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) {
> > +               if (!(link->flags | DL_FLAG_PM_RUNTIME))
> > +                       continue;
> > +
> > +               if (!pm_runtime_blocked(link->supplier) &&
> > +                   !link->supplier->power.smart_suspend) {
>
> This requires device_prepare() for all suppliers to be run before its
> consumer. Is that always the case?

Yes, it is by design.

> > +                       dev->power.smart_suspend = false;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       device_links_read_unlock(idx);
>
> From an execution overhead point of view, did you check if the above
> code had some measurable impact on the latency for dpm_prepare()?

It didn't on my systems.

For the vast majority of devices the overhead is just checking
power.no_pm_callbacks and DPM_FLAG_SMART_SUSPEND.  For some,
pm_runtime_blocked() needs to be called which requires grabbing a
spinlock and there are only a few with power.smart_suspend set to
start with.

I'm wondering why you didn't have this concern regarding other changes
that involved walking suppliers or consumers, though.

Re: [PATCH v1 1/3] PM: Block enabling of runtime PM during system suspend

[Rafael J. Wysocki]

On Tue, Feb 18, 2025 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 17 Feb 2025 at 21:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If device_prepare() runs on a device that has never had runtime
> > PM enabled so far, it may reasonably assume that runtime PM will
> > not be enabled for that device during the system suspend-resume
> > cycle currently in progress, but this has never been guaranteed.
> >
> > To verify this assumption, make device_prepare() arrange for
> > triggering a device warning accompanied by a call trace dump if
> > runtime PM is enabled for such a device after it has returned.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/main.c    |    9 +++++++++
> >  drivers/base/power/runtime.c |   24 ++++++++++++++++++++++++
> >  include/linux/pm.h           |    1 +
> >  include/linux/pm_runtime.h   |    6 +++++-
> >  4 files changed, 39 insertions(+), 1 deletion(-)
> >
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -1109,6 +1109,8 @@
> >         device_unlock(dev);
> >
> >  out:
> > +       /* If enabling runtime PM for the device is blocked, unblock it. */
> > +       pm_runtime_unblock(dev);
> >         pm_runtime_put(dev);
> >  }
> >
> > @@ -1815,6 +1817,13 @@
> >          * it again during the complete phase.
> >          */
> >         pm_runtime_get_noresume(dev);
> > +       /*
> > +        * If runtime PM is disabled for the device at this point and it has
> > +        * never been enabled so far, it should not be enabled until this system
> > +        * suspend-resume cycle is complete, so prepare to trigger a warning on
> > +        * subsequent attempts to enable it.
> > +        */
> > +       pm_runtime_block_if_disabled(dev);
> >
> >         if (dev->power.syscore)
> >                 return 0;
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1460,6 +1460,26 @@
> >  }
> >  EXPORT_SYMBOL_GPL(pm_runtime_barrier);
> >
> > +void pm_runtime_block_if_disabled(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_BLOCKED;
> > +
> > +       spin_unlock_irq(&dev->power.lock);
> > +}
> > +
> > +void pm_runtime_unblock(struct device *dev)
> > +{
> > +       spin_lock_irq(&dev->power.lock);
> > +
> > +       if (dev->power.last_status == RPM_BLOCKED)
> > +               dev->power.last_status = RPM_INVALID;
> > +
> > +       spin_unlock_irq(&dev->power.lock);
> > +}
> > +
> >  void __pm_runtime_disable(struct device *dev, bool check_resume)
> >  {
> >         spin_lock_irq(&dev->power.lock);
> > @@ -1518,6 +1538,10 @@
> >         if (--dev->power.disable_depth > 0)
> >                 goto out;
> >
> > +       if (dev->power.last_status == RPM_BLOCKED) {
> > +               dev_warn(dev, "Attempt to enabled runtime PM when it is blocked\n");
>
> /s/enabled/enable
>
> > +               dump_stack();
> > +       }
> >         dev->power.last_status = RPM_INVALID;
> >         dev->power.accounting_timestamp = ktime_get_mono_fast_ns();
> >
> > --- a/include/linux/pm.h
> > +++ b/include/linux/pm.h
> > @@ -597,6 +597,7 @@
> >         RPM_RESUMING,
> >         RPM_SUSPENDED,
> >         RPM_SUSPENDING,
> > +       RPM_BLOCKED,
> >  };
> >
> >  /*
> > --- a/include/linux/pm_runtime.h
> > +++ b/include/linux/pm_runtime.h
> > @@ -77,8 +77,10 @@
> >  extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
> >  extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
> >  extern int pm_runtime_barrier(struct device *dev);
> > +extern void pm_runtime_block_if_disabled(struct device *dev);
> > +extern void pm_runtime_unblock(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_disable(struct device *dev, bool regular);
>
> This looks unrelated to the $subject patch.
>
> >  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);
> > @@ -271,6 +273,8 @@
> >  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; }
> > +static inline void pm_runtime_block_if_disabled(struct device *dev) {}
> > +static inline void pm_runtime_unblock(struct device *dev) {}
> >  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_allow(struct device *dev) {}
> >
> >
> >
>
> With the minor things above fixed, feel free to add:

I'll fix these in v2.

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

Thanks!

Re: [PATCH v1 3/3] PM: sleep: Use DPM_FLAG_SMART_SUSPEND conditionally

[Ulf Hansson]

[...]

> > >
> > > +static void device_prepare_smart_suspend(struct device *dev)
> > > +{
> > > +       struct device_link *link;
> > > +       int idx;
> > > +
> > > +       /*
> > > +        * The "smart suspend" feature is enabled for devices whose drivers ask
> > > +        * for it and for devices without PM callbacks unless runtime PM is
> > > +        * disabled and enabling it is blocked for them.
> > > +        *
> > > +        * However, if "smart suspend" is not enabled for the device's parent
> > > +        * or any of its suppliers that take runtime PM into account, it cannot
> > > +        * be enabled for the device either.
> > > +        */
> > > +       dev->power.smart_suspend = (dev->power.no_pm_callbacks ||
> > > +               dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND)) &&
> > > +               !pm_runtime_blocked(dev);
> > > +
> > > +       if (!dev->power.smart_suspend)
> > > +               return;
> > > +
> > > +       if (dev->parent && !pm_runtime_blocked(dev->parent) &&
> > > +           !dev->parent->power.ignore_children && !dev->parent->power.smart_suspend) {
> > > +               dev->power.smart_suspend = false;
> > > +               return;
> > > +       }
> > > +
> > > +       idx = device_links_read_lock();
> > > +
> > > +       list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) {
> > > +               if (!(link->flags | DL_FLAG_PM_RUNTIME))
> > > +                       continue;
> > > +
> > > +               if (!pm_runtime_blocked(link->supplier) &&
> > > +                   !link->supplier->power.smart_suspend) {
> >
> > This requires device_prepare() for all suppliers to be run before its
> > consumer. Is that always the case?
>
> Yes, it is by design.

Okay! I was worried that fw_devlink could mess this up.

>
> > > +                       dev->power.smart_suspend = false;
> > > +                       break;
> > > +               }
> > > +       }
> > > +
> > > +       device_links_read_unlock(idx);
> >
> > From an execution overhead point of view, did you check if the above
> > code had some measurable impact on the latency for dpm_prepare()?
>
> It didn't on my systems.
>
> For the vast majority of devices the overhead is just checking
> power.no_pm_callbacks and DPM_FLAG_SMART_SUSPEND.  For some,
> pm_runtime_blocked() needs to be called which requires grabbing a
> spinlock and there are only a few with power.smart_suspend set to
> start with.
>
> I'm wondering why you didn't have this concern regarding other changes
> that involved walking suppliers or consumers, though.

Well, the concern is mostly generic from my side. When introducing
code that potentially could impact latency during system
suspend/resume, we should at least check it.

That said, I think the approach makes sense, so no objections from my side!

Feel free to add:
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe