| Message ID | 20250306113549.796524-2-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, 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: > > The calls to cpus_read_lock|unlock() protects us from getting CPUS > hotplugged, while entering suspend-to-idle. However, when s2idle_enter() is > called we should be far beyond the point when CPUs may be hotplugged. > Let's therefore simplify the code and drop the use of the lock. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > kernel/power/suspend.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > index 09f8397bae15..e7aca4e40561 100644 > --- a/kernel/power/suspend.c > +++ b/kernel/power/suspend.c > @@ -98,8 +98,6 @@ static void s2idle_enter(void) > s2idle_state = S2IDLE_STATE_ENTER; > raw_spin_unlock_irq(&s2idle_lock); > > - cpus_read_lock(); > - As you said above, this is not expected to be contended, so it mostly serves as an annotation. The correctness of the code "protected" by it in fact depends on the number of CPUs not changing while it runs and this needs to be documented this way or another. I guess a comment to that effect can be used here instead of the locking. > /* Push all the CPUs into the idle loop. */ > wake_up_all_idle_cpus(); > /* Make the current CPU wait so it can enter the idle loop too. */ > @@ -112,8 +110,6 @@ static void s2idle_enter(void) > */ > wake_up_all_idle_cpus(); > > - cpus_read_unlock(); > - > raw_spin_lock_irq(&s2idle_lock); > > out: > -- > 2.43.0 >
On Thu, 6 Mar 2025 at 15:34, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Mar 6, 2025 at 12:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > The calls to cpus_read_lock|unlock() protects us from getting CPUS > > hotplugged, while entering suspend-to-idle. However, when s2idle_enter() is > > called we should be far beyond the point when CPUs may be hotplugged. > > Let's therefore simplify the code and drop the use of the lock. > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > --- > > kernel/power/suspend.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > > index 09f8397bae15..e7aca4e40561 100644 > > --- a/kernel/power/suspend.c > > +++ b/kernel/power/suspend.c > > @@ -98,8 +98,6 @@ static void s2idle_enter(void) > > s2idle_state = S2IDLE_STATE_ENTER; > > raw_spin_unlock_irq(&s2idle_lock); > > > > - cpus_read_lock(); > > - > > As you said above, this is not expected to be contended, so it mostly > serves as an annotation. > > The correctness of the code "protected" by it in fact depends on the > number of CPUs not changing while it runs and this needs to be > documented this way or another. > > I guess a comment to that effect can be used here instead of the locking. Okay. I will update the patch and add a comment. Thanks for reviewing! [...] Kind regards Uffe
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c index 09f8397bae15..e7aca4e40561 100644 --- a/kernel/power/suspend.c +++ b/kernel/power/suspend.c @@ -98,8 +98,6 @@ static void s2idle_enter(void) s2idle_state = S2IDLE_STATE_ENTER; raw_spin_unlock_irq(&s2idle_lock); - cpus_read_lock(); - /* Push all the CPUs into the idle loop. */ wake_up_all_idle_cpus(); /* Make the current CPU wait so it can enter the idle loop too. */ @@ -112,8 +110,6 @@ static void s2idle_enter(void) */ wake_up_all_idle_cpus(); - cpus_read_unlock(); - raw_spin_lock_irq(&s2idle_lock); out:
The calls to cpus_read_lock|unlock() protects us from getting CPUS hotplugged, while entering suspend-to-idle. However, when s2idle_enter() is called we should be far beyond the point when CPUs may be hotplugged. Let's therefore simplify the code and drop the use of the lock. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- kernel/power/suspend.c | 4 ---- 1 file changed, 4 deletions(-)