| Message ID | 4966939.GXAFRqVoOG@rjwysocki.net |
|---|---|
| Headers | show |
| Series | PM: Use DPM_FLAG_SMART_SUSPEND conditionally | expand |
+ 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. > 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);
On Mon, 17 Feb 2025 at 21:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > If device_prepare() runs on a device that has never had runtime > PM enabled so far, it may reasonably assume that runtime PM will > not be enabled for that device during the system suspend-resume > cycle currently in progress, but this has never been guaranteed. > > To verify this assumption, make device_prepare() arrange for > triggering a device warning accompanied by a call trace dump if > runtime PM is enabled for such a device after it has returned. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/base/power/main.c | 9 +++++++++ > drivers/base/power/runtime.c | 24 ++++++++++++++++++++++++ > include/linux/pm.h | 1 + > include/linux/pm_runtime.h | 6 +++++- > 4 files changed, 39 insertions(+), 1 deletion(-) > > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -1109,6 +1109,8 @@ > device_unlock(dev); > > out: > + /* If enabling runtime PM for the device is blocked, unblock it. */ > + pm_runtime_unblock(dev); > pm_runtime_put(dev); > } > > @@ -1815,6 +1817,13 @@ > * it again during the complete phase. > */ > pm_runtime_get_noresume(dev); > + /* > + * If runtime PM is disabled for the device at this point and it has > + * never been enabled so far, it should not be enabled until this system > + * suspend-resume cycle is complete, so prepare to trigger a warning on > + * subsequent attempts to enable it. > + */ > + pm_runtime_block_if_disabled(dev); > > if (dev->power.syscore) > return 0; > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -1460,6 +1460,26 @@ > } > EXPORT_SYMBOL_GPL(pm_runtime_barrier); > > +void pm_runtime_block_if_disabled(struct device *dev) > +{ > + spin_lock_irq(&dev->power.lock); > + > + if (dev->power.disable_depth && dev->power.last_status == RPM_INVALID) > + dev->power.last_status = RPM_BLOCKED; > + > + spin_unlock_irq(&dev->power.lock); > +} > + > +void pm_runtime_unblock(struct device *dev) > +{ > + spin_lock_irq(&dev->power.lock); > + > + if (dev->power.last_status == RPM_BLOCKED) > + dev->power.last_status = RPM_INVALID; > + > + spin_unlock_irq(&dev->power.lock); > +} > + > void __pm_runtime_disable(struct device *dev, bool check_resume) > { > spin_lock_irq(&dev->power.lock); > @@ -1518,6 +1538,10 @@ > if (--dev->power.disable_depth > 0) > goto out; > > + if (dev->power.last_status == RPM_BLOCKED) { > + dev_warn(dev, "Attempt to enabled runtime PM when it is blocked\n"); /s/enabled/enable > + dump_stack(); > + } > dev->power.last_status = RPM_INVALID; > dev->power.accounting_timestamp = ktime_get_mono_fast_ns(); > > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -597,6 +597,7 @@ > RPM_RESUMING, > RPM_SUSPENDED, > RPM_SUSPENDING, > + RPM_BLOCKED, > }; > > /* > --- a/include/linux/pm_runtime.h > +++ b/include/linux/pm_runtime.h > @@ -77,8 +77,10 @@ > extern int pm_schedule_suspend(struct device *dev, unsigned int delay); > extern int __pm_runtime_set_status(struct device *dev, unsigned int status); > extern int pm_runtime_barrier(struct device *dev); > +extern void pm_runtime_block_if_disabled(struct device *dev); > +extern void pm_runtime_unblock(struct device *dev); > extern void pm_runtime_enable(struct device *dev); > -extern void __pm_runtime_disable(struct device *dev, bool check_resume); > +extern void __pm_runtime_disable(struct device *dev, bool regular); This looks unrelated to the $subject patch. > extern void pm_runtime_allow(struct device *dev); > extern void pm_runtime_forbid(struct device *dev); > extern void pm_runtime_no_callbacks(struct device *dev); > @@ -271,6 +273,8 @@ > static inline int __pm_runtime_set_status(struct device *dev, > unsigned int status) { return 0; } > static inline int pm_runtime_barrier(struct device *dev) { return 0; } > +static inline void pm_runtime_block_if_disabled(struct device *dev) {} > +static inline void pm_runtime_unblock(struct device *dev) {} > static inline void pm_runtime_enable(struct device *dev) {} > static inline void __pm_runtime_disable(struct device *dev, bool c) {} > static inline void pm_runtime_allow(struct device *dev) {} > > > With the minor things above fixed, feel free to add: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe
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.
On Tue, Feb 18, 2025 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Mon, 17 Feb 2025 at 21:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > If device_prepare() runs on a device that has never had runtime > > PM enabled so far, it may reasonably assume that runtime PM will > > not be enabled for that device during the system suspend-resume > > cycle currently in progress, but this has never been guaranteed. > > > > To verify this assumption, make device_prepare() arrange for > > triggering a device warning accompanied by a call trace dump if > > runtime PM is enabled for such a device after it has returned. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/base/power/main.c | 9 +++++++++ > > drivers/base/power/runtime.c | 24 ++++++++++++++++++++++++ > > include/linux/pm.h | 1 + > > include/linux/pm_runtime.h | 6 +++++- > > 4 files changed, 39 insertions(+), 1 deletion(-) > > > > --- a/drivers/base/power/main.c > > +++ b/drivers/base/power/main.c > > @@ -1109,6 +1109,8 @@ > > device_unlock(dev); > > > > out: > > + /* If enabling runtime PM for the device is blocked, unblock it. */ > > + pm_runtime_unblock(dev); > > pm_runtime_put(dev); > > } > > > > @@ -1815,6 +1817,13 @@ > > * it again during the complete phase. > > */ > > pm_runtime_get_noresume(dev); > > + /* > > + * If runtime PM is disabled for the device at this point and it has > > + * never been enabled so far, it should not be enabled until this system > > + * suspend-resume cycle is complete, so prepare to trigger a warning on > > + * subsequent attempts to enable it. > > + */ > > + pm_runtime_block_if_disabled(dev); > > > > if (dev->power.syscore) > > return 0; > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -1460,6 +1460,26 @@ > > } > > EXPORT_SYMBOL_GPL(pm_runtime_barrier); > > > > +void pm_runtime_block_if_disabled(struct device *dev) > > +{ > > + spin_lock_irq(&dev->power.lock); > > + > > + if (dev->power.disable_depth && dev->power.last_status == RPM_INVALID) > > + dev->power.last_status = RPM_BLOCKED; > > + > > + spin_unlock_irq(&dev->power.lock); > > +} > > + > > +void pm_runtime_unblock(struct device *dev) > > +{ > > + spin_lock_irq(&dev->power.lock); > > + > > + if (dev->power.last_status == RPM_BLOCKED) > > + dev->power.last_status = RPM_INVALID; > > + > > + spin_unlock_irq(&dev->power.lock); > > +} > > + > > void __pm_runtime_disable(struct device *dev, bool check_resume) > > { > > spin_lock_irq(&dev->power.lock); > > @@ -1518,6 +1538,10 @@ > > if (--dev->power.disable_depth > 0) > > goto out; > > > > + if (dev->power.last_status == RPM_BLOCKED) { > > + dev_warn(dev, "Attempt to enabled runtime PM when it is blocked\n"); > > /s/enabled/enable > > > + dump_stack(); > > + } > > dev->power.last_status = RPM_INVALID; > > dev->power.accounting_timestamp = ktime_get_mono_fast_ns(); > > > > --- a/include/linux/pm.h > > +++ b/include/linux/pm.h > > @@ -597,6 +597,7 @@ > > RPM_RESUMING, > > RPM_SUSPENDED, > > RPM_SUSPENDING, > > + RPM_BLOCKED, > > }; > > > > /* > > --- a/include/linux/pm_runtime.h > > +++ b/include/linux/pm_runtime.h > > @@ -77,8 +77,10 @@ > > extern int pm_schedule_suspend(struct device *dev, unsigned int delay); > > extern int __pm_runtime_set_status(struct device *dev, unsigned int status); > > extern int pm_runtime_barrier(struct device *dev); > > +extern void pm_runtime_block_if_disabled(struct device *dev); > > +extern void pm_runtime_unblock(struct device *dev); > > extern void pm_runtime_enable(struct device *dev); > > -extern void __pm_runtime_disable(struct device *dev, bool check_resume); > > +extern void __pm_runtime_disable(struct device *dev, bool regular); > > This looks unrelated to the $subject patch. > > > extern void pm_runtime_allow(struct device *dev); > > extern void pm_runtime_forbid(struct device *dev); > > extern void pm_runtime_no_callbacks(struct device *dev); > > @@ -271,6 +273,8 @@ > > static inline int __pm_runtime_set_status(struct device *dev, > > unsigned int status) { return 0; } > > static inline int pm_runtime_barrier(struct device *dev) { return 0; } > > +static inline void pm_runtime_block_if_disabled(struct device *dev) {} > > +static inline void pm_runtime_unblock(struct device *dev) {} > > static inline void pm_runtime_enable(struct device *dev) {} > > static inline void __pm_runtime_disable(struct device *dev, bool c) {} > > static inline void pm_runtime_allow(struct device *dev) {} > > > > > > > > With the minor things above fixed, feel free to add: I'll fix these in v2. > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Thanks!
[...] > > > > > > +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