| 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: <2025081538-grappling-crewmate-8cf5@gregkh>
Date: Fri, 15 Aug 2025 08:20:19 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Samuel Wu <wusamuel@...gle.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
Pavel Machek <pavel@...nel.org>, Danilo Krummrich <dakr@...nel.org>,
Saravana Kannan <saravanak@...gle.com>, kernel-team@...roid.com,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] PM: Support aborting suspend during filesystem sync
On Thu, Aug 14, 2025 at 05:46:34PM -0700, Samuel Wu wrote:
> At the start of suspend, filesystems will sync to save the current state
> of the device. However, the long tail of the filesystem sync can take
> upwards of 25 seconds. If during this filesystem sync there is some
> wakeup or abort signal, it will not be processed until the sync is
> complete; from a user's perspective, this looks like the device is
> unresponsive to any form of input.
>
> This patch adds functionality to handle a suspend abort signal when in
> the filesystem sync phase of suspend. This topic was first discussed by
> Saravana Kannan at LPC 2024 [1], where the general consensus was to
> allow filesystem sync on a parallel thread.
>
> [1]: https://lpc.events/event/18/contributions/1845/
>
> Suggested-by: Saravana Kannan <saravanak@...gle.com>
> Signed-off-by: Samuel Wu <wusamuel@...gle.com>
> ---
> drivers/base/power/wakeup.c | 8 ++++
> include/linux/suspend.h | 3 ++
> kernel/power/process.c | 1 -
> kernel/power/suspend.c | 85 ++++++++++++++++++++++++++++++++++++-
> 4 files changed, 95 insertions(+), 2 deletions(-)
>
> v1 -> v2:
> - Added documentation for suspend_abort_fs_sync()
> - Made suspend_fs_sync_lock and suspend_fs_sync_complete declaration static
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index d1283ff1080b..304368c3a55f 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -570,6 +570,13 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>
> /* Increment the counter of events in progress. */
> cec = atomic_inc_return(&combined_event_count);
> + /*
> + * To maintain the same behavior as pm_wakeup_pending(),
> + * aborting suspend will only happen if events_check_enabled. Similarly,
> + * the abort during fs_sync needs the same check.
> + */
> + if (events_check_enabled)
> + suspend_abort_fs_sync();
>
> trace_wakeup_source_activate(ws->name, cec);
> }
> @@ -899,6 +906,7 @@ EXPORT_SYMBOL_GPL(pm_wakeup_pending);
> void pm_system_wakeup(void)
> {
> atomic_inc(&pm_abort_suspend);
> + suspend_abort_fs_sync();
> s2idle_wake();
> }
> EXPORT_SYMBOL_GPL(pm_system_wakeup);
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 317ae31e89b3..21b1ea275c79 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -276,6 +276,8 @@ extern void arch_suspend_enable_irqs(void);
>
> extern int pm_suspend(suspend_state_t state);
> extern bool sync_on_suspend_enabled;
> +
> +extern void suspend_abort_fs_sync(void);
> #else /* !CONFIG_SUSPEND */
> #define suspend_valid_only_mem NULL
>
> @@ -296,6 +298,7 @@ static inline bool idle_should_enter_s2idle(void) { return false; }
> static inline void __init pm_states_init(void) {}
> static inline void s2idle_set_ops(const struct platform_s2idle_ops *ops) {}
> static inline void s2idle_wake(void) {}
> +static inline void suspend_abort_fs_sync(void) {}
> #endif /* !CONFIG_SUSPEND */
>
> static inline bool pm_suspend_in_progress(void)
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index dc0dfc349f22..8ff68ebaa1e0 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -132,7 +132,6 @@ int freeze_processes(void)
> if (!pm_freezing)
> static_branch_inc(&freezer_active);
>
> - pm_wakeup_clear(0);
> pm_freezing = true;
> error = try_to_freeze_tasks(true);
> if (!error)
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index b4ca17c2fecf..dc37ab942bcb 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -31,6 +31,7 @@
> #include <linux/compiler.h>
> #include <linux/moduleparam.h>
> #include <linux/fs.h>
> +#include <linux/workqueue.h>
>
> #include "power.h"
>
> @@ -74,6 +75,21 @@ bool pm_suspend_default_s2idle(void)
> }
> EXPORT_SYMBOL_GPL(pm_suspend_default_s2idle);
>
> +static bool suspend_fs_sync_queued;
> +static DEFINE_SPINLOCK(suspend_fs_sync_lock);
> +static DECLARE_COMPLETION(suspend_fs_sync_complete);
> +
> +/**
> + * Triggers the completion that aborts suspend. This completion will only have
> + * an effect if called during filesystems sync step of suspend.
> + */
> +void suspend_abort_fs_sync(void)
This is not kerneldoc format, I think the parser will fail on it, right?
Have you tried building the kernel documentation with this patch
applied?
> +{
> + spin_lock(&suspend_fs_sync_lock);
> + complete(&suspend_fs_sync_complete);
> + spin_unlock(&suspend_fs_sync_lock);
> +}
> +
> void s2idle_set_ops(const struct platform_s2idle_ops *ops)
> {
> unsigned int sleep_flags;
> @@ -403,6 +419,71 @@ void __weak arch_suspend_enable_irqs(void)
> local_irq_enable();
> }
>
> +static void sync_filesystems_fn(struct work_struct *work)
> +{
> + ksys_sync_helper();
> +
> + spin_lock(&suspend_fs_sync_lock);
> + suspend_fs_sync_queued = false;
> + complete(&suspend_fs_sync_complete);
> + spin_unlock(&suspend_fs_sync_lock);
> +}
> +static DECLARE_WORK(sync_filesystems, sync_filesystems_fn);
> +
> +/**
> + * suspend_fs_sync_with_abort- Start filesystem sync and handle potential aborts
> + *
> + * Starts filesystem sync in a workqueue, while the main thread uses a
> + * completion to wait for either the filesystem sync to finish or for a wakeup
> + * event. In the case of filesystem sync finishing and triggering the
> + * completion, the suspend path continues as normal. If the complete is due to a
> + * wakeup or abort signal, the code jumps to the suspend abort path while the
> + * filesystem sync finishes in the background.
> + *
> + * An aborted suspend that is followed by another suspend is a potential
> + * scenario that complicates the sequence. This patch handles this by
> + * serializing any filesystem sync; a subsequent suspend's filesystem sync
> + * operation will only start when the previous suspend's filesystem sync has
> + * finished. Even while waiting for the previous suspend's filesystem sync to
> + * finish, the subsequent suspend will still break early if a wakeup completion
> + * is triggered, solving the original issue of filesystem sync blocking abort.
> + */
Shouldn't this documentation go up in the public one?
thanks,
greg k-h
Powered by blists - more mailing lists