| Message ID | 20250306113549.796524-3-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | PM: s2idle: A couple of minor lock-simplifications | expand |
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
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); }
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(-)