| 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
| ||
|
Date: Tue, 18 Jun 2024 19:27:18 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Kaiyen Chang <kaiyen.chang@...el.com>
Cc: rafael@...nel.org, pavel@....cz, len.brown@...el.com,
gregkh@...uxfoundation.org, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] PM: Start asynchronous suspend threads upfront
On Tue, Jun 18, 2024 at 2:13 PM Kaiyen Chang <kaiyen.chang@...el.com> wrote:
>
> Currently, when performing a suspend operation, all devices on the
> dpm_list must wait for the "synchronous" devices that are listed
> after them to complete before the main suspend thread can start
> their suspend routines, even if they are "asynchronous". If the
> suspend routine of a synchronous device must enter a waiting state
> for some reason, it will cause the main suspend thread to go to
> sleep, thereby delaying the processing of all subsequent devices,
> including asynchronous ones, ultimately extending the overall
> suspend time.
>
> By starting the asynchronous suspend threads upfront, we can allow
> the system to handle the suspend routines of these asynchronous
> devices in parallel, without waiting for the suspend routines of
> the synchronous devices listed after them to complete, and without
> breaking their order with respect to their parents and children.
> This way, even if the main suspend thread is blocked, these
> asynchronous suspend threads can continue to run without being
> affected.
>
> Signed-off-by: Kaiyen Chang <kaiyen.chang@...el.com>
How exactly has this been tested?
In the past, changes going in this direction caused system suspend to
hang on at least some platforms (including the ones in my office).
> ---
> Change from v1: Fix some unclear parts in the commit messages.
> ---
> drivers/base/power/main.c | 90 +++++++++++++++++++++++++--------------
> 1 file changed, 57 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 4a67e83300e1..6ddd6ef36625 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1283,6 +1283,7 @@ static void async_suspend_noirq(void *data, async_cookie_t cookie)
>
> static int dpm_noirq_suspend_devices(pm_message_t state)
> {
> + struct device *dev;
> ktime_t starttime = ktime_get();
> int error = 0;
>
> @@ -1293,26 +1294,33 @@ static int dpm_noirq_suspend_devices(pm_message_t state)
>
> mutex_lock(&dpm_list_mtx);
>
> + /*
> + * Trigger the suspend of "async" devices upfront so they don't have to
> + * wait for the "non-async" ones that don't depend on them.
> + */
> +
> + list_for_each_entry_reverse(dev, &dpm_late_early_list, power.entry)
> + dpm_async_fn(dev, async_suspend_noirq);
> +
> while (!list_empty(&dpm_late_early_list)) {
> - struct device *dev = to_device(dpm_late_early_list.prev);
> + dev = to_device(dpm_late_early_list.prev);
>
> list_move(&dev->power.entry, &dpm_noirq_list);
>
> - if (dpm_async_fn(dev, async_suspend_noirq))
> - continue;
> -
> - get_device(dev);
> + if (!dev->power.async_in_progress) {
> + get_device(dev);
>
> - mutex_unlock(&dpm_list_mtx);
> + mutex_unlock(&dpm_list_mtx);
>
> - error = device_suspend_noirq(dev, state, false);
> + error = device_suspend_noirq(dev, state, false);
>
> - put_device(dev);
> + put_device(dev);
>
> - mutex_lock(&dpm_list_mtx);
> + mutex_lock(&dpm_list_mtx);
>
> - if (error || async_error)
> - break;
> + if (error || async_error)
> + break;
> + }
> }
>
> mutex_unlock(&dpm_list_mtx);
> @@ -1454,6 +1462,7 @@ static void async_suspend_late(void *data, async_cookie_t cookie)
> */
> int dpm_suspend_late(pm_message_t state)
> {
> + struct device *dev;
> ktime_t starttime = ktime_get();
> int error = 0;
>
> @@ -1466,26 +1475,33 @@ int dpm_suspend_late(pm_message_t state)
>
> mutex_lock(&dpm_list_mtx);
>
> + /*
> + * Trigger the suspend of "async" devices upfront so they don't have to
> + * wait for the "non-async" ones that don't depend on them.
> + */
> +
> + list_for_each_entry_reverse(dev, &dpm_suspended_list, power.entry)
> + dpm_async_fn(dev, async_suspend_late);
> +
> while (!list_empty(&dpm_suspended_list)) {
> - struct device *dev = to_device(dpm_suspended_list.prev);
> + dev = to_device(dpm_suspended_list.prev);
>
> list_move(&dev->power.entry, &dpm_late_early_list);
>
> - if (dpm_async_fn(dev, async_suspend_late))
> - continue;
> -
> - get_device(dev);
> + if (!dev->power.async_in_progress) {
> + get_device(dev);
>
> - mutex_unlock(&dpm_list_mtx);
> + mutex_unlock(&dpm_list_mtx);
>
> - error = device_suspend_late(dev, state, false);
> + error = device_suspend_late(dev, state, false);
>
> - put_device(dev);
> + put_device(dev);
>
> - mutex_lock(&dpm_list_mtx);
> + mutex_lock(&dpm_list_mtx);
>
> - if (error || async_error)
> - break;
> + if (error || async_error)
> + break;
> + }
> }
>
> mutex_unlock(&dpm_list_mtx);
> @@ -1719,6 +1735,7 @@ static void async_suspend(void *data, async_cookie_t cookie)
> */
> int dpm_suspend(pm_message_t state)
> {
> + struct device *dev;
> ktime_t starttime = ktime_get();
> int error = 0;
>
> @@ -1733,26 +1750,33 @@ int dpm_suspend(pm_message_t state)
>
> mutex_lock(&dpm_list_mtx);
>
> + /*
> + * Trigger the suspend of "async" devices upfront so they don't have to
> + * wait for the "non-async" ones that don't depend on them.
> + */
> +
> + list_for_each_entry_reverse(dev, &dpm_prepared_list, power.entry)
> + dpm_async_fn(dev, async_suspend);
> +
> while (!list_empty(&dpm_prepared_list)) {
> - struct device *dev = to_device(dpm_prepared_list.prev);
> + dev = to_device(dpm_prepared_list.prev);
>
> list_move(&dev->power.entry, &dpm_suspended_list);
>
> - if (dpm_async_fn(dev, async_suspend))
> - continue;
> -
> - get_device(dev);
> + if (!dev->power.async_in_progress) {
> + get_device(dev);
>
> - mutex_unlock(&dpm_list_mtx);
> + mutex_unlock(&dpm_list_mtx);
>
> - error = device_suspend(dev, state, false);
> + error = device_suspend(dev, state, false);
>
> - put_device(dev);
> + put_device(dev);
>
> - mutex_lock(&dpm_list_mtx);
> + mutex_lock(&dpm_list_mtx);
>
> - if (error || async_error)
> - break;
> + if (error || async_error)
> + break;
> + }
> }
>
> mutex_unlock(&dpm_list_mtx);
> --
> 2.34.1
>
Powered by blists - more mailing lists