On Tue, Feb 18, 2025 at 09:16:48PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx/ [1]
> Fixes: 7585946243d6 ("PM: sleep: core: Restrict power.set_active propagation")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> # drivers/pci
> ---
>
> v1 -> v2:
> * Add helper function for reading power.smart_suspend() (Ulf), add the R-by.
> * Rearrange conditionals in device_prepare_smart_suspend() so that the checks
> involving locking are done last.
>
> ---
> drivers/acpi/device_pm.c | 4 +-
> drivers/base/power/main.c | 63 +++++++++++++++++++++++++++++++++++-----------
> drivers/mfd/intel-lpss.c | 2 -
> drivers/pci/pci-driver.c | 6 +---
> include/linux/device.h | 5 +++
> include/linux/pm.h | 2 -
> 6 files changed, 60 insertions(+), 22 deletions(-)
>
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1161,7 +1161,7 @@
> */
> 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 @@
> */
> 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);
>
> --- 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_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 @@
> 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_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 @@
> 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_pm_smart_suspend(dev) && 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_pm_smart_suspend(dev))
> 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_pm_smart_suspend(dev) || 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_pm_smart_suspend(dev) || pci_dev_need_resume(pci_dev)) {
> pm_runtime_resume(dev);
> pci_dev->state_saved = false;
> } else {
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1025,6 +1025,11 @@
> return !!(dev->power.driver_flags & flags);
> }
>
> +static inline bool dev_pm_smart_suspend(struct device *dev)
> +{
> + return dev->power.smart_suspend;
> +}
> +
> static inline void device_lock(struct device *dev)
> {
> mutex_lock(&dev->mutex);
> --- 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;
>
>
>
