diff mbox series

[2/4] arm64: hibernate: remove WARN_ON in save_processor_state

Message ID 20230525025555.24104-3-songshuaishuai@tinylab.org
State New
Headers
Series Remove WARN_ON in save_processor_state |

Commit Message

Song Shuai May 25, 2023, 2:55 a.m. UTC 
  During hibernation or restoration, freeze_secondary_cpus
checks num_online_cpus via BUG_ON, and the subsequent
save_processor_state also does the checking with WARN_ON.

So remove the unnecessary checking in save_processor_state.

Signed-off-by: Song Shuai <songshuaishuai@tinylab.org>
---
 arch/arm64/kernel/hibernate.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Will Deacon June 5, 2023, 2:28 p.m. UTC | #1 
On Thu, May 25, 2023 at 10:55:53AM +0800, Song Shuai wrote:
> During hibernation or restoration, freeze_secondary_cpus
> checks num_online_cpus via BUG_ON, and the subsequent
> save_processor_state also does the checking with WARN_ON.
> 
> So remove the unnecessary checking in save_processor_state.

This is a very terse summary of why this is safe.

Looking at the code, freeze_secondary_cpus() does indeed check
num_online_cpus(), or it returns an error which then causes the hibernation
to fail. However, this is all in the CONFIG_PM_SLEEP_SMP=y case and it's
far less clear whether your assertion is true if that option is disabled.

Please can you describe your reasoning in more detail, and cover the case
where CONFIG_PM_SLEEP_SMP=n as well, please?

Will
Song Shuai June 7, 2023, 3 a.m. UTC | #2 
在 2023/6/5 22:28, Will Deacon 写道:
> On Thu, May 25, 2023 at 10:55:53AM +0800, Song Shuai wrote:
>> During hibernation or restoration, freeze_secondary_cpus
>> checks num_online_cpus via BUG_ON, and the subsequent
>> save_processor_state also does the checking with WARN_ON.
>>
>> So remove the unnecessary checking in save_processor_state.
> 
> This is a very terse summary of why this is safe.
> 
> Looking at the code, freeze_secondary_cpus() does indeed check
> num_online_cpus(), or it returns an error which then causes the hibernation
> to fail. However, this is all in the CONFIG_PM_SLEEP_SMP=y case and it's
> far less clear whether your assertion is true if that option is disabled.
> 
> Please can you describe your reasoning in more detail, and cover the case
> where CONFIG_PM_SLEEP_SMP=n as well, please?

With HIBERNATION enabled, the sole possible condition to disable 
CONFIG_PM_SLEEP_SMP
is !SMP where num_online_cpus is always 1. We also don't have to check 
it in save_processor_state.

> 
> Will
>
Will Deacon June 8, 2023, 5:26 p.m. UTC | #3 
On Wed, Jun 07, 2023 at 11:00:08AM +0800, Song Shuai wrote:
> 
> 
> 在 2023/6/5 22:28, Will Deacon 写道:
> > On Thu, May 25, 2023 at 10:55:53AM +0800, Song Shuai wrote:
> > > During hibernation or restoration, freeze_secondary_cpus
> > > checks num_online_cpus via BUG_ON, and the subsequent
> > > save_processor_state also does the checking with WARN_ON.
> > > 
> > > So remove the unnecessary checking in save_processor_state.
> > 
> > This is a very terse summary of why this is safe.
> > 
> > Looking at the code, freeze_secondary_cpus() does indeed check
> > num_online_cpus(), or it returns an error which then causes the hibernation
> > to fail. However, this is all in the CONFIG_PM_SLEEP_SMP=y case and it's
> > far less clear whether your assertion is true if that option is disabled.
> > 
> > Please can you describe your reasoning in more detail, and cover the case
> > where CONFIG_PM_SLEEP_SMP=n as well, please?
> 
> With HIBERNATION enabled, the sole possible condition to disable
> CONFIG_PM_SLEEP_SMP
> is !SMP where num_online_cpus is always 1. We also don't have to check it in
> save_processor_state.

Thanks. Please add that to the commit message.

Will
diff mbox series

Patch

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index 788597a6b6a2..02870beb271e 100644
--- a/arch/arm64/kernel/hibernate.c
+++ b/arch/arm64/kernel/hibernate.c
@@ -99,7 +99,6 @@  int pfn_is_nosave(unsigned long pfn)
 
 void notrace save_processor_state(void)
 {
-	WARN_ON(num_online_cpus() != 1);
 }
 
 void notrace restore_processor_state(void)