On Tue, Feb 18, 2025 at 1:49 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> + 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.
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.
