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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJZ5v0hhHv7P8UXEqtRMwC66aSqs115e8gN8rzn_QzZgnVULzA@mail.gmail.com>
Date: Thu, 6 Mar 2025 15:34:26 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>, linux-pm@...r.kernel.org, 
	Len Brown <len.brown@...el.com>, Pavel Machek <pavel@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] PM: s2idle: Drop redundant locks when entering s2idle

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