Re: [PATCH v2 3/4] PM: sleep: Use DPM_FLAG_SMART_SUSPEND conditionally

[Bjorn Helgaas <helgaas@kernel.org>]

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;
> 
> 
>