| Message ID | 1914558.tdWV9SEqCh@rjwysocki.net (mailing list archive) |
|---|---|
| State | Queued |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | PM: Use DPM_FLAG_SMART_SUSPEND conditionally | expand |
On Tue, Feb 18, 2025 at 09:16:48PM +0100, Rafael J. Wysocki 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> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Acked-by: Bjorn Helgaas <bhelgaas@google.com> # 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; > > >
On Tue, Feb 18, 2025 at 11:20 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Feb 18, 2025 at 09:16:48PM +0100, Rafael J. Wysocki 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> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> # drivers/pci Thanks!
--- 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;