| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPDyKFoB0fQCabahYpx=A_Ns7vJgWYdK=rxuHk+XHVv35cFvWQ@mail.gmail.com>
Date: Tue, 18 Feb 2025 13:48:54 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: Linux PM <linux-pm@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>,
Alan Stern <stern@...land.harvard.edu>, Bjorn Helgaas <helgaas@...nel.org>,
Linux PCI <linux-pci@...r.kernel.org>, Johan Hovold <johan@...nel.org>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, Jon Hunter <jonathanh@...dia.com>,
Linux ACPI <linux-acpi@...r.kernel.org>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Saravana Kannan <saravanak@...gle.com>
Subject: Re: [PATCH v1 3/3] PM: sleep: Use DPM_FLAG_SMART_SUSPEND conditionally
+ Saravana
On Mon, 17 Feb 2025 at 21:19, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...el.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);
>From an execution overhead point of view, did you check if the above
code had some measurable impact on the latency for dpm_prepare()?
> +}
> +
> /**
> * 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;
>
>
>
Kind regards
Uffe
Powered by blists - more mailing lists