+ Saravana
On Mon, 17 Feb 2025 at 21:19, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> 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>
> ---
> 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);
