It's unnecessary to re-initialize smp_ops.play_dead to play_dead as
it naturally goes back to play_dead in the freshly booted kernel upon
device resume.

Suggested-by: Huacai Chen <chenhuacai@loongson.cn>
Co-developed-by: Wentao Guan <guanwentao@uniontech.com>
Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
Signed-off-by: WangYuli <wangyuli@uniontech.com>
---
 arch/x86/power/cpu.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 63230ff8cf4f..023cf9421467 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -297,9 +297,6 @@ static void __noreturn resume_play_dead(void)
 
 int hibernate_resume_nonboot_cpu_disable(void)
 {
-	void (*play_dead)(void) = smp_ops.play_dead;
-	int ret;
-
 	/*
 	 * Ensure that MONITOR/MWAIT will not be used in the "play dead" loop
 	 * during hibernate image restoration, because it is likely that the
@@ -316,13 +313,11 @@ int hibernate_resume_nonboot_cpu_disable(void)
 	 * resume) sleep afterwards, and the resumed kernel will decide itself
 	 * what to do with them.
 	 */
-	ret = cpuhp_smt_enable();
+	int ret = cpuhp_smt_enable();
 	if (ret)
 		return ret;
 	smp_ops.play_dead = resume_play_dead;
-	ret = freeze_secondary_cpus(0);
-	smp_ops.play_dead = play_dead;
-	return ret;
+	return freeze_secondary_cpus(0);
 }
 #endif
 
-- 
2.47.2

On Mon, Feb 24, 2025 at 8:45 AM WangYuli <wangyuli@uniontech.com> wrote:
>
> It's unnecessary to re-initialize smp_ops.play_dead to play_dead as
> it naturally goes back to play_dead in the freshly booted kernel upon
> device resume.

Is it really?

And what if freeze_secondary_cpus() returns an error, for one example?

> Suggested-by: Huacai Chen <chenhuacai@loongson.cn>
> Co-developed-by: Wentao Guan <guanwentao@uniontech.com>
> Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
> Signed-off-by: WangYuli <wangyuli@uniontech.com>
> ---
>  arch/x86/power/cpu.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
> index 63230ff8cf4f..023cf9421467 100644
> --- a/arch/x86/power/cpu.c
> +++ b/arch/x86/power/cpu.c
> @@ -297,9 +297,6 @@ static void __noreturn resume_play_dead(void)
>
>  int hibernate_resume_nonboot_cpu_disable(void)
>  {
> -       void (*play_dead)(void) = smp_ops.play_dead;
> -       int ret;
> -
>         /*
>          * Ensure that MONITOR/MWAIT will not be used in the "play dead" loop
>          * during hibernate image restoration, because it is likely that the
> @@ -316,13 +313,11 @@ int hibernate_resume_nonboot_cpu_disable(void)
>          * resume) sleep afterwards, and the resumed kernel will decide itself
>          * what to do with them.
>          */
> -       ret = cpuhp_smt_enable();
> +       int ret = cpuhp_smt_enable();
>         if (ret)
>                 return ret;
>         smp_ops.play_dead = resume_play_dead;
> -       ret = freeze_secondary_cpus(0);
> -       smp_ops.play_dead = play_dead;
> -       return ret;
> +       return freeze_secondary_cpus(0);
>  }
>  #endif
>
> --
> 2.47.2
>

Hello,

Thanks for your reply.

In my opinion, the only logic different before the patch is delete smp_ops.play_dead
save and restore, as the comment "the resumed kernel will decide itself" and same 
logic as which in arch/arm64/kernel/hibernate.c, the ok path will work as expect.

When discussing the error path and ret value that we not restore play_dead,
I will try to analyze the difference between native_play_dead and resume_play_dead,
and sev_es_play_dead [the all possiable three value], and I see some mwait and hlt
way difference.[maybe it happens as disable the cpu failed and goes to Enable_cpus
path in func:resume_target_kernel in hibernate.c? ] Is that it desgin to do and we can
move it to a common place in hibernate.c and left some comments ? 

BRs
Wentao Guan

On Wed, Feb 26, 2025 at 7:08 PM Wentao Guan <guanwentao@uniontech.com> wrote:
>
> Hello,
>
> Thanks for your reply.
>
> In my opinion, the only logic different before the patch is delete smp_ops.play_dead
> save and restore, as the comment "the resumed kernel will decide itself" and same
> logic as which in arch/arm64/kernel/hibernate.c, the ok path will work as expect.

Yes, the OK path will work as expected so long as smp_ops.play_dead is
switched over to resume_play_dead before calling
freeze_secondary_cpus().

> When discussing the error path and ret value that we not restore play_dead,
> I will try to analyze the difference between native_play_dead and resume_play_dead,
> and sev_es_play_dead [the all possiable three value], and I see some mwait and hlt
> way difference.[maybe it happens as disable the cpu failed and goes to Enable_cpus
> path in func:resume_target_kernel in hibernate.c? ] Is that it desgin to do and we can
> move it to a common place in hibernate.c and left some comments ?

Why not leave it as is?