There's no reason to hold the s2idle_lock longer than necessary. Let's
instead acquire it when really needed in s2idle_enter().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 kernel/power/suspend.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index e7aca4e40561..ca09f26cbf4e 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -91,10 +91,10 @@ static void s2idle_enter(void)
 {
 	trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, true);
 
-	raw_spin_lock_irq(&s2idle_lock);
 	if (pm_wakeup_pending())
 		goto out;
 
+	raw_spin_lock_irq(&s2idle_lock);
 	s2idle_state = S2IDLE_STATE_ENTER;
 	raw_spin_unlock_irq(&s2idle_lock);
 
@@ -111,11 +111,10 @@ static void s2idle_enter(void)
 	wake_up_all_idle_cpus();
 
 	raw_spin_lock_irq(&s2idle_lock);
-
- out:
 	s2idle_state = S2IDLE_STATE_NONE;
 	raw_spin_unlock_irq(&s2idle_lock);
 
+ out:
 	trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, false);
 }
 
-- 
2.43.0

On Thu, Mar 6, 2025 at 12:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> There's no reason to hold the s2idle_lock longer than necessary. Let's
> instead acquire it when really needed in s2idle_enter().
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  kernel/power/suspend.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index e7aca4e40561..ca09f26cbf4e 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -91,10 +91,10 @@ static void s2idle_enter(void)
>  {
>         trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, true);
>
> -       raw_spin_lock_irq(&s2idle_lock);

This is to prevent missing a wakeup event when pm_system_wakeup() runs
at this point on a different CPU.

If you move the locking, it may run as a whole between the
pm_wakeup_pending() check below and the s2idle_state update, so the
wakeup event will be missed.

With the locking in place, the pm_abort_suspend update in
pm_system_wakeup() may still happen at any time, but the code under
the lock in s2idle_wake() after it can only run before the lock is
acquired above or after it is released.

If s2idle_wake() in pm_system_wakeup() runs before the
raw_spin_lock_irq() above, the pm_wakeup_pending() check below will
notice the pm_abort_suspend set and return true, so the suspend will
be aborted (and the pm_abort_suspend update in pm_system_wakeup()
cannot be reordered entirely after the s2idle_wake() call because of
the locking there).

Now, if s2idle_wake() in pm_system_wakeup() runs after the
raw_spin_unlock_irq() below, it will notice the s2idle_state change
and it will update it to S2IDLE_STATE_WAKE, so the suspend will be
aborted.

I guess it would have helped if there had been a comment describing this ...

>         if (pm_wakeup_pending())
>                 goto out;
>
> +       raw_spin_lock_irq(&s2idle_lock);
>         s2idle_state = S2IDLE_STATE_ENTER;
>         raw_spin_unlock_irq(&s2idle_lock);
>
> @@ -111,11 +111,10 @@ static void s2idle_enter(void)
>         wake_up_all_idle_cpus();
>
>         raw_spin_lock_irq(&s2idle_lock);
> -
> - out:
>         s2idle_state = S2IDLE_STATE_NONE;
>         raw_spin_unlock_irq(&s2idle_lock);
>
> + out:
>         trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, false);
>  }
>
> --

On Thu, 6 Mar 2025 at 14:18, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Mar 6, 2025 at 12:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > There's no reason to hold the s2idle_lock longer than necessary. Let's
> > instead acquire it when really needed in s2idle_enter().
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  kernel/power/suspend.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index e7aca4e40561..ca09f26cbf4e 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -91,10 +91,10 @@ static void s2idle_enter(void)
> >  {
> >         trace_suspend_resume(TPS("machine_suspend"), PM_SUSPEND_TO_IDLE, true);
> >
> > -       raw_spin_lock_irq(&s2idle_lock);
>
> This is to prevent missing a wakeup event when pm_system_wakeup() runs
> at this point on a different CPU.
>
> If you move the locking, it may run as a whole between the
> pm_wakeup_pending() check below and the s2idle_state update, so the
> wakeup event will be missed.

Of course, you are right! Thanks for clarifying!

>
> With the locking in place, the pm_abort_suspend update in
> pm_system_wakeup() may still happen at any time, but the code under
> the lock in s2idle_wake() after it can only run before the lock is
> acquired above or after it is released.
>
> If s2idle_wake() in pm_system_wakeup() runs before the
> raw_spin_lock_irq() above, the pm_wakeup_pending() check below will
> notice the pm_abort_suspend set and return true, so the suspend will
> be aborted (and the pm_abort_suspend update in pm_system_wakeup()
> cannot be reordered entirely after the s2idle_wake() call because of
> the locking there).
>
> Now, if s2idle_wake() in pm_system_wakeup() runs after the
> raw_spin_unlock_irq() below, it will notice the s2idle_state change
> and it will update it to S2IDLE_STATE_WAKE, so the suspend will be
> aborted.
>
> I guess it would have helped if there had been a comment describing this ...

Yes, I can send a patch adding a small comment about it, if you think
it makes sense?

[...]

Kind regards
Uffe