| Message ID | 2000822.PYKUYFuaPT@rjwysocki.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/3] PM: Block enabling of runtime PM during system suspend | expand |
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.
[...] > > > > > > +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
--- 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;