Re: [PATCH 1/2] PM: s2idle: Drop redundant locks when entering s2idle

[Rafael J. Wysocki]

On Thu, Mar 6, 2025 at 12:36 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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
>