[PATCH v2] PM/Hibernation: Fix the early termination of test modes

[PATCH v2] PM/Hibernation: Fix the early termination of test modes

[Srivatsa S. Bhat]

Commit 2aede851ddf08666f68ffc17be446420e9d2a056
(PM / Hibernate: Freeze kernel threads after preallocating memory)
postponed the freezing of kernel threads to after preallocating memory
for hibernation. But while doing that, the hibernation test TEST_FREEZER
and the test mode HIBERNATION_TESTPROC were not moved accordingly.

As a result, when using these test modes, it only goes upto the freezing of
userspace and exits, when in fact it should go till the complete end of task
freezing stage, namely the freezing of kernel threads as well.

So, move these points of exit to appropriate places so that freezing of
kernel threads is also tested while using these test harnesses. And while at
it, add some documentation about these test modes.

v2: Changed 'freezer_test_done' flag from int to bool.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 Documentation/power/basic-pm-debugging.txt |   16 ++++++++++++----
 kernel/power/hibernate.c                   |   28 ++++++++++++++++++++++------
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/Documentation/power/basic-pm-debugging.txt b/Documentation/power/basic-pm-debugging.txt
index 40a4c65..ba6c879 100644
--- a/Documentation/power/basic-pm-debugging.txt
+++ b/Documentation/power/basic-pm-debugging.txt
@@ -38,10 +38,18 @@ identify what goes wrong.
 
 a) Test modes of hibernation
 
-To find out why hibernation fails on your system, you can use a special testing
-facility available if the kernel is compiled with CONFIG_PM_DEBUG set.  Then,
-there is the file /sys/power/pm_test that can be used to make the hibernation
-core run in a test mode.  There are 5 test modes available:
+To find out why hibernation fails on your system, you can use some special
+testing facilities available if the kernel is compiled with CONFIG_PM_DEBUG set.
+Hibernation will then have 2 more modes that can be used, namely "testproc" and
+"test". When the "testproc" mode is used, it executes everything upto the task
+freezing phase and reverts back. When the "test" mode is used, it goes much
+deeper down, stopping just short of actually hibernating the machine, and then
+reverts back to normal state.
+
+However, there is an even more fine-grained debugging tool, also enabled by
+setting CONFIG_PM_DEBUG: there appears a file /sys/power/pm_test that can be
+used to make the hibernation core run in a test mode.
+There are 5 test modes available:
 
 freezer
 - test the freezing of processes
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index b4511b6..257e8e7 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -55,6 +55,8 @@ enum {
 
 static int hibernation_mode = HIBERNATION_SHUTDOWN;
 
+static bool freezer_test_done;
+
 static const struct platform_hibernation_ops *hibernation_ops;
 
 /**
@@ -347,6 +349,17 @@ int hibernation_snapshot(int platform_mode)
 	if (error)
 		goto Close;
 
+	if (hibernation_test(TEST_FREEZER) ||
+		hibernation_testmode(HIBERNATION_TESTPROC)) {
+
+		/*
+		 * Indicate to the caller that we are returning due to a
+		 * successful freezer test.
+		 */
+		freezer_test_done = true;
+		goto Close;
+	}
+
 	error = dpm_prepare(PMSG_FREEZE);
 	if (error)
 		goto Complete_devices;
@@ -641,15 +654,13 @@ int hibernate(void)
 	if (error)
 		goto Finish;
 
-	if (hibernation_test(TEST_FREEZER))
-		goto Thaw;
-
-	if (hibernation_testmode(HIBERNATION_TESTPROC))
-		goto Thaw;
-
 	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
 	if (error)
 		goto Thaw;
+	if (freezer_test_done) {
+		freezer_test_done = false;
+		goto Thaw;
+	}
 
 	if (in_suspend) {
 		unsigned int flags = 0;
@@ -864,6 +875,11 @@ static const char * const hibernation_modes[] = {
  *
  * If a platform hibernation driver is in use, 'platform' will be supported
  * and will be used by default.  Otherwise, 'shutdown' will be used by default.
+ * The 2 test modes are useful only when CONFIG_PM_DEBUG is set. In that case,
+ * using the 'testproc' mode will do everything upto the freezing of tasks
+ * and exits after undoing all that it did. The 'test' mode is deeper, in that,
+ * it does everything except actually hibernating the machine and exits after
+ * restoring the machine back to its original state.
  * The selected option (i.e. the one corresponding to the current value of
  * hibernation_mode) is enclosed by a square bracket.
  *

Re: [PATCH v2] PM/Hibernation: Fix the early termination of test modes

[Rafael J. Wysocki]

On Friday, November 18, 2011, Srivatsa S. Bhat wrote:
> Commit 2aede851ddf08666f68ffc17be446420e9d2a056
> (PM / Hibernate: Freeze kernel threads after preallocating memory)
> postponed the freezing of kernel threads to after preallocating memory
> for hibernation. But while doing that, the hibernation test TEST_FREEZER
> and the test mode HIBERNATION_TESTPROC were not moved accordingly.
> 
> As a result, when using these test modes, it only goes upto the freezing of
> userspace and exits, when in fact it should go till the complete end of task
> freezing stage, namely the freezing of kernel threads as well.
> 
> So, move these points of exit to appropriate places so that freezing of
> kernel threads is also tested while using these test harnesses. And while at
> it, add some documentation about these test modes.
> 
> v2: Changed 'freezer_test_done' flag from int to bool.
> 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

On a second thought I'm not sure about the documentation changes.
In fact, the "test" and "testproc" modes are deprecated and I was going to
remove them some time this year.  That's why they weren't documented
(i.e. intentionally) and it's better to keep it that way IMO.

> ---
> 
>  Documentation/power/basic-pm-debugging.txt |   16 ++++++++++++----
>  kernel/power/hibernate.c                   |   28 ++++++++++++++++++++++------
>  2 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/power/basic-pm-debugging.txt b/Documentation/power/basic-pm-debugging.txt
> index 40a4c65..ba6c879 100644
> --- a/Documentation/power/basic-pm-debugging.txt
> +++ b/Documentation/power/basic-pm-debugging.txt
> @@ -38,10 +38,18 @@ identify what goes wrong.
>  
>  a) Test modes of hibernation
>  
> -To find out why hibernation fails on your system, you can use a special testing
> -facility available if the kernel is compiled with CONFIG_PM_DEBUG set.  Then,
> -there is the file /sys/power/pm_test that can be used to make the hibernation
> -core run in a test mode.  There are 5 test modes available:
> +To find out why hibernation fails on your system, you can use some special
> +testing facilities available if the kernel is compiled with CONFIG_PM_DEBUG set.
> +Hibernation will then have 2 more modes that can be used, namely "testproc" and
> +"test". When the "testproc" mode is used, it executes everything upto the task
> +freezing phase and reverts back. When the "test" mode is used, it goes much
> +deeper down, stopping just short of actually hibernating the machine, and then
> +reverts back to normal state.
> +
> +However, there is an even more fine-grained debugging tool, also enabled by
> +setting CONFIG_PM_DEBUG: there appears a file /sys/power/pm_test that can be
> +used to make the hibernation core run in a test mode.
> +There are 5 test modes available:
>  
>  freezer
>  - test the freezing of processes
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index b4511b6..257e8e7 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -55,6 +55,8 @@ enum {
>  
>  static int hibernation_mode = HIBERNATION_SHUTDOWN;
>  
> +static bool freezer_test_done;
> +
>  static const struct platform_hibernation_ops *hibernation_ops;
>  
>  /**
> @@ -347,6 +349,17 @@ int hibernation_snapshot(int platform_mode)
>  	if (error)
>  		goto Close;
>  
> +	if (hibernation_test(TEST_FREEZER) ||
> +		hibernation_testmode(HIBERNATION_TESTPROC)) {
> +
> +		/*
> +		 * Indicate to the caller that we are returning due to a
> +		 * successful freezer test.
> +		 */
> +		freezer_test_done = true;
> +		goto Close;
> +	}
> +
>  	error = dpm_prepare(PMSG_FREEZE);
>  	if (error)
>  		goto Complete_devices;
> @@ -641,15 +654,13 @@ int hibernate(void)
>  	if (error)
>  		goto Finish;
>  
> -	if (hibernation_test(TEST_FREEZER))
> -		goto Thaw;
> -
> -	if (hibernation_testmode(HIBERNATION_TESTPROC))
> -		goto Thaw;
> -
>  	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
>  	if (error)
>  		goto Thaw;
> +	if (freezer_test_done) {
> +		freezer_test_done = false;
> +		goto Thaw;
> +	}

What happens if hibernation_snapshot() and freezer_test_done is 'true'?
Won't that leek freezer_test_done to the next hibernation cycle?

>  
>  	if (in_suspend) {
>  		unsigned int flags = 0;
> @@ -864,6 +875,11 @@ static const char * const hibernation_modes[] = {
>   *
>   * If a platform hibernation driver is in use, 'platform' will be supported
>   * and will be used by default.  Otherwise, 'shutdown' will be used by default.
> + * The 2 test modes are useful only when CONFIG_PM_DEBUG is set. In that case,
> + * using the 'testproc' mode will do everything upto the freezing of tasks
> + * and exits after undoing all that it did. The 'test' mode is deeper, in that,
> + * it does everything except actually hibernating the machine and exits after
> + * restoring the machine back to its original state.
>   * The selected option (i.e. the one corresponding to the current value of
>   * hibernation_mode) is enclosed by a square bracket.
>   *

Thanks,
Rafael

Re: [PATCH v2] PM/Hibernation: Fix the early termination of test modes

[Srivatsa S. Bhat]

On 11/18/2011 02:50 PM, Rafael J. Wysocki wrote:
> On Friday, November 18, 2011, Srivatsa S. Bhat wrote:
>> Commit 2aede851ddf08666f68ffc17be446420e9d2a056
>> (PM / Hibernate: Freeze kernel threads after preallocating memory)
>> postponed the freezing of kernel threads to after preallocating memory
>> for hibernation. But while doing that, the hibernation test TEST_FREEZER
>> and the test mode HIBERNATION_TESTPROC were not moved accordingly.
>>
>> As a result, when using these test modes, it only goes upto the freezing of
>> userspace and exits, when in fact it should go till the complete end of task
>> freezing stage, namely the freezing of kernel threads as well.
>>
>> So, move these points of exit to appropriate places so that freezing of
>> kernel threads is also tested while using these test harnesses. And while at
>> it, add some documentation about these test modes.
>>
>> v2: Changed 'freezer_test_done' flag from int to bool.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> 
> On a second thought I'm not sure about the documentation changes.
> In fact, the "test" and "testproc" modes are deprecated and I was going to
> remove them some time this year.  That's why they weren't documented
> (i.e. intentionally) and it's better to keep it that way IMO.
> 

I was actually wondering about the same thing (why are test and testproc
both still there, when pm_test is also there)..

So I'll drop the documentation changes in this patch, and maybe send a
separate patch that will remove the 2? Will that be good?

>> ---
>>
>>  Documentation/power/basic-pm-debugging.txt |   16 ++++++++++++----
>>  kernel/power/hibernate.c                   |   28 ++++++++++++++++++++++------
>>  2 files changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/power/basic-pm-debugging.txt b/Documentation/power/basic-pm-debugging.txt
>> index 40a4c65..ba6c879 100644
>> --- a/Documentation/power/basic-pm-debugging.txt
>> +++ b/Documentation/power/basic-pm-debugging.txt
>> @@ -38,10 +38,18 @@ identify what goes wrong.
>>  
>>  a) Test modes of hibernation
>>  
>> -To find out why hibernation fails on your system, you can use a special testing
>> -facility available if the kernel is compiled with CONFIG_PM_DEBUG set.  Then,
>> -there is the file /sys/power/pm_test that can be used to make the hibernation
>> -core run in a test mode.  There are 5 test modes available:
>> +To find out why hibernation fails on your system, you can use some special
>> +testing facilities available if the kernel is compiled with CONFIG_PM_DEBUG set.
>> +Hibernation will then have 2 more modes that can be used, namely "testproc" and
>> +"test". When the "testproc" mode is used, it executes everything upto the task
>> +freezing phase and reverts back. When the "test" mode is used, it goes much
>> +deeper down, stopping just short of actually hibernating the machine, and then
>> +reverts back to normal state.
>> +
>> +However, there is an even more fine-grained debugging tool, also enabled by
>> +setting CONFIG_PM_DEBUG: there appears a file /sys/power/pm_test that can be
>> +used to make the hibernation core run in a test mode.
>> +There are 5 test modes available:
>>  
>>  freezer
>>  - test the freezing of processes
>> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>> index b4511b6..257e8e7 100644
>> --- a/kernel/power/hibernate.c
>> +++ b/kernel/power/hibernate.c
>> @@ -55,6 +55,8 @@ enum {
>>  
>>  static int hibernation_mode = HIBERNATION_SHUTDOWN;
>>  
>> +static bool freezer_test_done;
>> +
>>  static const struct platform_hibernation_ops *hibernation_ops;
>>  
>>  /**
>> @@ -347,6 +349,17 @@ int hibernation_snapshot(int platform_mode)
>>  	if (error)
>>  		goto Close;
>>  
>> +	if (hibernation_test(TEST_FREEZER) ||
>> +		hibernation_testmode(HIBERNATION_TESTPROC)) {
>> +
>> +		/*
>> +		 * Indicate to the caller that we are returning due to a
>> +		 * successful freezer test.
>> +		 */
>> +		freezer_test_done = true;
>> +		goto Close;
>> +	}
>> +
>>  	error = dpm_prepare(PMSG_FREEZE);
>>  	if (error)
>>  		goto Complete_devices;
>> @@ -641,15 +654,13 @@ int hibernate(void)
>>  	if (error)
>>  		goto Finish;
>>  
>> -	if (hibernation_test(TEST_FREEZER))
>> -		goto Thaw;
>> -
>> -	if (hibernation_testmode(HIBERNATION_TESTPROC))
>> -		goto Thaw;
>> -
>>  	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
>>  	if (error)
>>  		goto Thaw;
>> +	if (freezer_test_done) {
>> +		freezer_test_done = false;
>> +		goto Thaw;
>> +	}
> 
> What happens if hibernation_snapshot() and freezer_test_done is 'true'?
> Won't that leek freezer_test_done to the next hibernation cycle?
> 

Oh yes, thanks for catching that! I'll fix that and send an updated patch.

>>  
>>  	if (in_suspend) {
>>  		unsigned int flags = 0;
>> @@ -864,6 +875,11 @@ static const char * const hibernation_modes[] = {
>>   *
>>   * If a platform hibernation driver is in use, 'platform' will be supported
>>   * and will be used by default.  Otherwise, 'shutdown' will be used by default.
>> + * The 2 test modes are useful only when CONFIG_PM_DEBUG is set. In that case,
>> + * using the 'testproc' mode will do everything upto the freezing of tasks
>> + * and exits after undoing all that it did. The 'test' mode is deeper, in that,
>> + * it does everything except actually hibernating the machine and exits after
>> + * restoring the machine back to its original state.
>>   * The selected option (i.e. the one corresponding to the current value of
>>   * hibernation_mode) is enclosed by a square bracket.
>>   *
> 

Thanks,
Srivatsa S. Bhat

Re: [PATCH v2] PM/Hibernation: Fix the early termination of test modes

[Rafael J. Wysocki]

On Friday, November 18, 2011, Srivatsa S. Bhat wrote:
> On 11/18/2011 02:50 PM, Rafael J. Wysocki wrote:
> > On Friday, November 18, 2011, Srivatsa S. Bhat wrote:
> >> Commit 2aede851ddf08666f68ffc17be446420e9d2a056
> >> (PM / Hibernate: Freeze kernel threads after preallocating memory)
> >> postponed the freezing of kernel threads to after preallocating memory
> >> for hibernation. But while doing that, the hibernation test TEST_FREEZER
> >> and the test mode HIBERNATION_TESTPROC were not moved accordingly.
> >>
> >> As a result, when using these test modes, it only goes upto the freezing of
> >> userspace and exits, when in fact it should go till the complete end of task
> >> freezing stage, namely the freezing of kernel threads as well.
> >>
> >> So, move these points of exit to appropriate places so that freezing of
> >> kernel threads is also tested while using these test harnesses. And while at
> >> it, add some documentation about these test modes.
> >>
> >> v2: Changed 'freezer_test_done' flag from int to bool.
> >>
> >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> > 
> > On a second thought I'm not sure about the documentation changes.
> > In fact, the "test" and "testproc" modes are deprecated and I was going to
> > remove them some time this year.  That's why they weren't documented
> > (i.e. intentionally) and it's better to keep it that way IMO.
> > 
> 
> I was actually wondering about the same thing (why are test and testproc
> both still there, when pm_test is also there)..
> 
> So I'll drop the documentation changes in this patch, and maybe send a
> separate patch that will remove the 2? Will that be good?

Yes, please.

> >> ---
> >>
> >>  Documentation/power/basic-pm-debugging.txt |   16 ++++++++++++----
> >>  kernel/power/hibernate.c                   |   28 ++++++++++++++++++++++------
> >>  2 files changed, 34 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/Documentation/power/basic-pm-debugging.txt b/Documentation/power/basic-pm-debugging.txt
> >> index 40a4c65..ba6c879 100644
> >> --- a/Documentation/power/basic-pm-debugging.txt
> >> +++ b/Documentation/power/basic-pm-debugging.txt
> >> @@ -38,10 +38,18 @@ identify what goes wrong.
> >>  
> >>  a) Test modes of hibernation
> >>  
> >> -To find out why hibernation fails on your system, you can use a special testing
> >> -facility available if the kernel is compiled with CONFIG_PM_DEBUG set.  Then,
> >> -there is the file /sys/power/pm_test that can be used to make the hibernation
> >> -core run in a test mode.  There are 5 test modes available:
> >> +To find out why hibernation fails on your system, you can use some special
> >> +testing facilities available if the kernel is compiled with CONFIG_PM_DEBUG set.
> >> +Hibernation will then have 2 more modes that can be used, namely "testproc" and
> >> +"test". When the "testproc" mode is used, it executes everything upto the task
> >> +freezing phase and reverts back. When the "test" mode is used, it goes much
> >> +deeper down, stopping just short of actually hibernating the machine, and then
> >> +reverts back to normal state.
> >> +
> >> +However, there is an even more fine-grained debugging tool, also enabled by
> >> +setting CONFIG_PM_DEBUG: there appears a file /sys/power/pm_test that can be
> >> +used to make the hibernation core run in a test mode.
> >> +There are 5 test modes available:
> >>  
> >>  freezer
> >>  - test the freezing of processes
> >> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> >> index b4511b6..257e8e7 100644
> >> --- a/kernel/power/hibernate.c
> >> +++ b/kernel/power/hibernate.c
> >> @@ -55,6 +55,8 @@ enum {
> >>  
> >>  static int hibernation_mode = HIBERNATION_SHUTDOWN;
> >>  
> >> +static bool freezer_test_done;
> >> +
> >>  static const struct platform_hibernation_ops *hibernation_ops;
> >>  
> >>  /**
> >> @@ -347,6 +349,17 @@ int hibernation_snapshot(int platform_mode)
> >>  	if (error)
> >>  		goto Close;
> >>  
> >> +	if (hibernation_test(TEST_FREEZER) ||
> >> +		hibernation_testmode(HIBERNATION_TESTPROC)) {
> >> +
> >> +		/*
> >> +		 * Indicate to the caller that we are returning due to a
> >> +		 * successful freezer test.
> >> +		 */
> >> +		freezer_test_done = true;
> >> +		goto Close;
> >> +	}
> >> +
> >>  	error = dpm_prepare(PMSG_FREEZE);
> >>  	if (error)
> >>  		goto Complete_devices;
> >> @@ -641,15 +654,13 @@ int hibernate(void)
> >>  	if (error)
> >>  		goto Finish;
> >>  
> >> -	if (hibernation_test(TEST_FREEZER))
> >> -		goto Thaw;
> >> -
> >> -	if (hibernation_testmode(HIBERNATION_TESTPROC))
> >> -		goto Thaw;
> >> -
> >>  	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
> >>  	if (error)
> >>  		goto Thaw;
> >> +	if (freezer_test_done) {
> >> +		freezer_test_done = false;
> >> +		goto Thaw;
> >> +	}
> > 
> > What happens if hibernation_snapshot() and freezer_test_done is 'true'?
> > Won't that leek freezer_test_done to the next hibernation cycle?
> > 
> 
> Oh yes, thanks for catching that! I'll fix that and send an updated patch.

OK

Thanks,
Rafael

Re: [PATCH v2] PM/Hibernation: Fix the early termination of test modes

[Srivatsa S. Bhat]

On 11/18/2011 02:57 PM, Srivatsa S. Bhat wrote:
> On 11/18/2011 02:50 PM, Rafael J. Wysocki wrote:
>>> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>>> index b4511b6..257e8e7 100644
>>> --- a/kernel/power/hibernate.c
>>> +++ b/kernel/power/hibernate.c
>>> @@ -55,6 +55,8 @@ enum {
>>>  
>>>  static int hibernation_mode = HIBERNATION_SHUTDOWN;
>>>  
>>> +static bool freezer_test_done;
>>> +
>>>  static const struct platform_hibernation_ops *hibernation_ops;
>>>  
>>>  /**
>>> @@ -347,6 +349,17 @@ int hibernation_snapshot(int platform_mode)
>>>  	if (error)
>>>  		goto Close;
>>>  
>>> +	if (hibernation_test(TEST_FREEZER) ||
>>> +		hibernation_testmode(HIBERNATION_TESTPROC)) {
>>> +
>>> +		/*
>>> +		 * Indicate to the caller that we are returning due to a
>>> +		 * successful freezer test.
>>> +		 */
>>> +		freezer_test_done = true;
>>> +		goto Close;
>>> +	}
>>> +
>>>  	error = dpm_prepare(PMSG_FREEZE);
>>>  	if (error)
>>>  		goto Complete_devices;
>>> @@ -641,15 +654,13 @@ int hibernate(void)
>>>  	if (error)
>>>  		goto Finish;
>>>  
>>> -	if (hibernation_test(TEST_FREEZER))
>>> -		goto Thaw;
>>> -
>>> -	if (hibernation_testmode(HIBERNATION_TESTPROC))
>>> -		goto Thaw;
>>> -
>>>  	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
>>>  	if (error)
>>>  		goto Thaw;
>>> +	if (freezer_test_done) {
>>> +		freezer_test_done = false;
>>> +		goto Thaw;
>>> +	}
>>
>> What happens if hibernation_snapshot() and freezer_test_done is 'true'?
>> Won't that leek freezer_test_done to the next hibernation cycle?
>>
> 
> Oh yes, thanks for catching that! I'll fix that and send an updated patch.
>

Sorry, I think I hit reply too soon.
To restate your question, you are asking if freezer_test_done was set in
hibernation_snapshot() and then there was a genuine error in
hibernation_snapshot() which it returned, then won't freezer_test_done
leak to the next cycle.., right?

If there is a genuine error before hibernation_test(TEST_FREEZER)... is
done, then we will never set freezer_test_done flag to true.

And whenever freezer_test_done is set (inside hibernation_snapshot) there
is no way it will continue further executing normal code. It will jump to
the Close label and come out, so that here in hibernate(), the first
check for error will fail (because it returned 0) and the subsequent
check for freezer_test_done will succeed. So I don't see how it can leak.

Or, am I missing something?

Thanks,
Srivatsa S. Bhat

Re: [PATCH v2] PM/Hibernation: Fix the early termination of test modes

[Rafael J. Wysocki]

On Friday, November 18, 2011, Srivatsa S. Bhat wrote:
> On 11/18/2011 02:57 PM, Srivatsa S. Bhat wrote:
> > On 11/18/2011 02:50 PM, Rafael J. Wysocki wrote:
> >>> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> >>> index b4511b6..257e8e7 100644
> >>> --- a/kernel/power/hibernate.c
> >>> +++ b/kernel/power/hibernate.c
> >>> @@ -55,6 +55,8 @@ enum {
> >>>  
> >>>  static int hibernation_mode = HIBERNATION_SHUTDOWN;
> >>>  
> >>> +static bool freezer_test_done;
> >>> +
> >>>  static const struct platform_hibernation_ops *hibernation_ops;
> >>>  
> >>>  /**
> >>> @@ -347,6 +349,17 @@ int hibernation_snapshot(int platform_mode)
> >>>  	if (error)
> >>>  		goto Close;
> >>>  
> >>> +	if (hibernation_test(TEST_FREEZER) ||
> >>> +		hibernation_testmode(HIBERNATION_TESTPROC)) {
> >>> +
> >>> +		/*
> >>> +		 * Indicate to the caller that we are returning due to a
> >>> +		 * successful freezer test.
> >>> +		 */
> >>> +		freezer_test_done = true;
> >>> +		goto Close;
> >>> +	}
> >>> +
> >>>  	error = dpm_prepare(PMSG_FREEZE);
> >>>  	if (error)
> >>>  		goto Complete_devices;
> >>> @@ -641,15 +654,13 @@ int hibernate(void)
> >>>  	if (error)
> >>>  		goto Finish;
> >>>  
> >>> -	if (hibernation_test(TEST_FREEZER))
> >>> -		goto Thaw;
> >>> -
> >>> -	if (hibernation_testmode(HIBERNATION_TESTPROC))
> >>> -		goto Thaw;
> >>> -
> >>>  	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
> >>>  	if (error)
> >>>  		goto Thaw;
> >>> +	if (freezer_test_done) {
> >>> +		freezer_test_done = false;
> >>> +		goto Thaw;
> >>> +	}
> >>
> >> What happens if hibernation_snapshot() and freezer_test_done is 'true'?
> >> Won't that leek freezer_test_done to the next hibernation cycle?
> >>
> > 
> > Oh yes, thanks for catching that! I'll fix that and send an updated patch.
> >
> 
> Sorry, I think I hit reply too soon.
> To restate your question, you are asking if freezer_test_done was set in
> hibernation_snapshot() and then there was a genuine error in
> hibernation_snapshot() which it returned, then won't freezer_test_done
> leak to the next cycle.., right?
> 
> If there is a genuine error before hibernation_test(TEST_FREEZER)... is
> done, then we will never set freezer_test_done flag to true.
> 
> And whenever freezer_test_done is set (inside hibernation_snapshot) there
> is no way it will continue further executing normal code. It will jump to
> the Close label and come out, so that here in hibernate(), the first
> check for error will fail (because it returned 0) and the subsequent
> check for freezer_test_done will succeed. So I don't see how it can leak.
> 
> Or, am I missing something?

No, that's fine, I just wasn't sure. :-)

I'm going to apply your patch without the documentation changes.

Thanks,
Rafael

Re: [PATCH v2] PM/Hibernation: Fix the early termination of test modes

[Srivatsa S. Bhat]

On 11/19/2011 03:30 AM, Rafael J. Wysocki wrote:
> On Friday, November 18, 2011, Srivatsa S. Bhat wrote:
>> On 11/18/2011 02:57 PM, Srivatsa S. Bhat wrote:
>>> On 11/18/2011 02:50 PM, Rafael J. Wysocki wrote:
>>>>> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>>>>> index b4511b6..257e8e7 100644
>>>>> --- a/kernel/power/hibernate.c
>>>>> +++ b/kernel/power/hibernate.c
>>>>> @@ -55,6 +55,8 @@ enum {
>>>>>  
>>>>>  static int hibernation_mode = HIBERNATION_SHUTDOWN;
>>>>>  
>>>>> +static bool freezer_test_done;
>>>>> +
>>>>>  static const struct platform_hibernation_ops *hibernation_ops;
>>>>>  
>>>>>  /**
>>>>> @@ -347,6 +349,17 @@ int hibernation_snapshot(int platform_mode)
>>>>>  	if (error)
>>>>>  		goto Close;
>>>>>  
>>>>> +	if (hibernation_test(TEST_FREEZER) ||
>>>>> +		hibernation_testmode(HIBERNATION_TESTPROC)) {
>>>>> +
>>>>> +		/*
>>>>> +		 * Indicate to the caller that we are returning due to a
>>>>> +		 * successful freezer test.
>>>>> +		 */
>>>>> +		freezer_test_done = true;
>>>>> +		goto Close;
>>>>> +	}
>>>>> +
>>>>>  	error = dpm_prepare(PMSG_FREEZE);
>>>>>  	if (error)
>>>>>  		goto Complete_devices;
>>>>> @@ -641,15 +654,13 @@ int hibernate(void)
>>>>>  	if (error)
>>>>>  		goto Finish;
>>>>>  
>>>>> -	if (hibernation_test(TEST_FREEZER))
>>>>> -		goto Thaw;
>>>>> -
>>>>> -	if (hibernation_testmode(HIBERNATION_TESTPROC))
>>>>> -		goto Thaw;
>>>>> -
>>>>>  	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
>>>>>  	if (error)
>>>>>  		goto Thaw;
>>>>> +	if (freezer_test_done) {
>>>>> +		freezer_test_done = false;
>>>>> +		goto Thaw;
>>>>> +	}
>>>>
>>>> What happens if hibernation_snapshot() and freezer_test_done is 'true'?
>>>> Won't that leek freezer_test_done to the next hibernation cycle?
>>>>
>>>
>>> Oh yes, thanks for catching that! I'll fix that and send an updated patch.
>>>
>>
>> Sorry, I think I hit reply too soon.
>> To restate your question, you are asking if freezer_test_done was set in
>> hibernation_snapshot() and then there was a genuine error in
>> hibernation_snapshot() which it returned, then won't freezer_test_done
>> leak to the next cycle.., right?
>>
>> If there is a genuine error before hibernation_test(TEST_FREEZER)... is
>> done, then we will never set freezer_test_done flag to true.
>>
>> And whenever freezer_test_done is set (inside hibernation_snapshot) there
>> is no way it will continue further executing normal code. It will jump to
>> the Close label and come out, so that here in hibernate(), the first
>> check for error will fail (because it returned 0) and the subsequent
>> check for freezer_test_done will succeed. So I don't see how it can leak.
>>
>> Or, am I missing something?
> 
> No, that's fine, I just wasn't sure. :-)
> 
> I'm going to apply your patch without the documentation changes.
> 

Great! Thanks a lot!

Thanks,
Srivatsa S. Bhat