PM: sleep: Use DPM_FLAG_SMART_SUSPEND conditionally

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/
Fixes: 7585946243d6 ("PM: sleep: core: Restrict power.set_active propagation")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Bjorn Helgaas <bhelgaas@google.com> # drivers/pci
Link: https://patch.msgid.link/1914558.tdWV9SEqCh@rjwysocki.net

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 3b4d048c49417303bf5c4451e94e6d9f3dd77e56..dbd4446025ecdb9c2e193cf76e983b649aa5e20d 100644 (file)
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1161,7 +1161,7 @@ EXPORT_SYMBOL_GPL(acpi_subsys_complete);
  */
 int acpi_subsys_suspend(struct device *dev)
 {
-       if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
+       if (!dev_pm_smart_suspend(dev) ||
            acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
                pm_runtime_resume(dev);
 
@@ -1320,7 +1320,7 @@ EXPORT_SYMBOL_GPL(acpi_subsys_restore_early);
  */
 int acpi_subsys_poweroff(struct device *dev)
 {
-       if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
+       if (!dev_pm_smart_suspend(dev) ||
            acpi_dev_needs_resume(dev, ACPI_COMPANION(dev)))
                pm_runtime_resume(dev);
 

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index acabd9f3e60f37bf06f957323ac29bde40e503d6..cc9903065900d108bbc6860fb48dbc20bd2b78dc 100644 (file)
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -656,15 +656,13 @@ static void device_resume_noirq(struct device *dev, pm_message_t state, bool asy
         * 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_pm_smart_suspend(dev))
                pm_runtime_set_active(dev);
-               dev->power.set_active = false;
-       }
 
        if (dev->pm_domain) {
                info = "noirq power domain ";
@@ -1282,14 +1280,8 @@ Skip:
              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 @@ int dpm_suspend(pm_message_t state)
        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_pm_smart_suspend(dev))
+               return;
+
+       if (dev->parent && !dev_pm_smart_suspend(dev->parent) &&
+           !dev->parent->power.ignore_children && !pm_runtime_blocked(dev->parent)) {
+               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 (!dev_pm_smart_suspend(link->supplier) &&
+                   !pm_runtime_blocked(link->supplier)) {
+                       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 @@ unlock:
                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 @@ void device_pm_check_callbacks(struct device *dev)
 
 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_pm_smart_suspend(dev) && pm_runtime_status_suspended(dev);
 }

diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
index 3ba05ebb90359f38414219f42383d565c7c5d4f0..63d6694f71457b2e09d238af0a9bfb897a169a64 100644 (file)
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c
@@ -480,7 +480,7 @@ EXPORT_SYMBOL_NS_GPL(intel_lpss_remove, "INTEL_LPSS");
 
 static int resume_lpss_device(struct device *dev, void *data)
 {
-       if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND))
+       if (!dev_pm_smart_suspend(dev))
                pm_runtime_resume(dev);
 
        return 0;

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index f57ea36d125d6eecfb60a6dc84d189cf9ed9b423..02726f36beb5fc6cd6be3f986f7ae429661e18f9 100644 (file)
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -812,8 +812,7 @@ static int pci_pm_suspend(struct device *dev)
         * 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_pm_smart_suspend(dev) || pci_dev_need_resume(pci_dev)) {
                pm_runtime_resume(dev);
                pci_dev->state_saved = false;
        } else {
@@ -1151,8 +1150,7 @@ static int pci_pm_poweroff(struct device *dev)
        }
 
        /* 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_pm_smart_suspend(dev) || pci_dev_need_resume(pci_dev)) {
                pm_runtime_resume(dev);
                pci_dev->state_saved = false;
        } else {

diff --git a/include/linux/device.h b/include/linux/device.h
index 80a5b32689866c39128b4a564f8521cec663de01..615282365052b3c0217cc9fb2cb8753e3a395d0a 100644 (file)
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1025,6 +1025,15 @@ static inline bool dev_pm_test_driver_flags(struct device *dev, u32 flags)
        return !!(dev->power.driver_flags & flags);
 }
 
+static inline bool dev_pm_smart_suspend(struct device *dev)
+{
+#ifdef CONFIG_PM_SLEEP
+       return dev->power.smart_suspend;
+#else
+       return false;
+#endif
+}
+
 static inline void device_lock(struct device *dev)
 {
        mutex_lock(&dev->mutex);

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 6ca6f34c58c36aa06fef296c738268fc32123e2c..24647108f0ad9b1727b114fc571157d07d3bd995 100644 (file)
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -680,8 +680,8 @@ struct dev_pm_info {
        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;