Re: [PATCH][RFC v5] PM / hibernate: Introduce test_resume mode for hibernation

[Rafael J. Wysocki]

On Tuesday, July 19, 2016 08:36:44 AM Chen Yu wrote:
> test_resume mode is to verify if the snapshot data
> written to swap device can be successfully restored
> to memory. It is useful to ease the debugging process
> on hibernation, since this mode can not only bypass
> the BIOSes/bootloader, but also the system re-initialization.
> 
> For example:
> echo test_resume > /sys/power/disk
> echo disk > /sys/power/state
> 
> [  187.306470] PM: Image saving progress:  70%
> [  187.395298] PM: Image saving progress:  80%
> [  187.476697] PM: Image saving progress:  90%
> [  187.554641] PM: Image saving done.
> [  187.558896] PM: Wrote 594600 kbytes in 0.90 seconds (660.66 MB/s)
> [  187.566000] PM: S|
> [  187.589742] PM: Basic memory bitmaps freed
> [  187.594694] PM: Checking hibernation image
> [  187.599865] PM: Image signature found, resuming
> [  187.605209] PM: Loading hibernation image.
> [  187.665753] PM: Basic memory bitmaps created
> [  187.691397] PM: Using 3 thread(s) for decompression.
> [  187.691397] PM: Loading and decompressing image data (148650 pages)...
> [  187.889719] PM: Image loading progress:   0%
> [  188.100452] PM: Image loading progress:  10%
> [  188.244781] PM: Image loading progress:  20%
> [  189.057305] PM: Image loading done.
> [  189.068793] PM: Image successfully loaded
> 
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> ---
> v5:
>  - Introduce a new function to be shared with software_resume().
> v4:
>  - Fix some errors and modify the comment for software_resume_unthaw.
> v3:
>  - As Pavel mentioned, there was a potential risk in previous
>    version that might break the filesystem. According to Rafael's suggestion,
>    this version avoids that issue by restoring the pages with user/kernel
>    threads kept in frozen. Also updated the patch on top of linux-next.
> ---
>  kernel/power/hibernate.c | 61 ++++++++++++++++++++++++++++++++----------------
>  kernel/power/swap.c      |  6 +++++
>  2 files changed, 47 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 5f3523e..73ec63c 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -52,6 +52,7 @@ enum {
>  #ifdef CONFIG_SUSPEND
>  	HIBERNATION_SUSPEND,
>  #endif
> +	HIBERNATION_TEST_RESUME,
>  	/* keep last */
>  	__HIBERNATION_AFTER_LAST
>  };
> @@ -647,12 +648,39 @@ static void power_down(void)
>  		cpu_relax();
>  }
>  
> +static int load_image_and_restore(void)
> +{
> +	int error;
> +	unsigned int flags;
> +
> +	pr_debug("PM: Loading hibernation image.\n");
> +
> +	lock_device_hotplug();
> +	error = create_basic_memory_bitmaps();
> +	if (error)
> +		goto Unlock;
> +
> +	error = swsusp_read(&flags);
> +	swsusp_close(FMODE_READ);
> +	if (!error)
> +		hibernation_restore(flags & SF_PLATFORM_MODE);
> +
> +	printk(KERN_ERR "PM: Failed to load hibernation image, recovering.\n");
> +	swsusp_free();
> +	free_basic_memory_bitmaps();
> + Unlock:
> +	unlock_device_hotplug();
> +
> +	return error;
> +}
> +
>  /**
>   * hibernate - Carry out system hibernation, including saving the image.
>   */
>  int hibernate(void)
>  {
>  	int error, nr_calls = 0;
> +	bool snapshot_test = false;
>  
>  	if (!hibernation_available()) {
>  		pr_debug("PM: Hibernation not available.\n");
> @@ -704,7 +732,9 @@ int hibernate(void)
>  		pr_debug("PM: writing image.\n");
>  		error = swsusp_write(flags);
>  		swsusp_free();
> -		if (!error)
> +		if (hibernation_mode == HIBERNATION_TEST_RESUME)
> +			snapshot_test = true;
> +		if (!error && !snapshot_test)

The above change isn't correct IMO.

If swsusp_write() returns an error, snapshot_test shouldn't be set (that
basically means "no image", so nothing to test).

So this code should look like

	if (!error) {
		if (hibernation_mode == HIBERNATION_TEST_RESUME)
			snapshot_test = true;
		else
			power_down();
	}

The rest of the patch looks OK to me.

Thanks,
Rafael