Re: [PATCH] Resume from initramfs

[Pavel Machek <pavel@suse.cz>]

Hi!

> seeing as it is that the current software suspend allows suspending only 
> from built-in devices I've hacked the swsusp code to allow also for 
> manual resume. Thus it is now be capable to suspend to modular devices also.
> This is actually based on a previous patch by mjg59 at scrf.ucam.org, 
> augmented by suggestions from Pavel Machek.
> For a clean implementation I've split up the function swsusp_read() into 
> the distinct functions swsusp_check() and swsusp_read().
> Furthermore the function prepare() has been split into 
> prepare_processes() and prepare_devices().
> With this we now have the functionality to first check whether the 
> device really contains a resume image, then freeze all processes and 
> free some memory, read in the resume image and shut down all devices.
> It actually makes checking for a resume image faster than the current 
> implementation.
> 
> resume can be started by 'echo <major>:<minor> > /sys/power/resume".
> Note that this _only_ works from within initramfs when _no_ devices are 
> mounted. Otherwise resume will not be able to freeze the swapper task 
> and consequently fail.
> And yes, it needs to be properly documented. Will do once the patch is 
> accepted in principle :-).

In priciple it looks okay, but minor details still need to be ironed
out.

> Oh, and the usual applies: works for me, might eat your disk, beware of 
> nasal demons.

:-) Please try to inline patches, it makes it easier to reply to
them.

At one point you did something like

	read_data()
	blk_device_put()

If read_data does blk_device_get(), it should also do the put()
itself. Otherwise some caller will forget it.

									Pavel

> --- linux-2.6.10/init/do_mounts.c.orig	2005-01-28 10:25:35.000000000 +0100
> +++ linux-2.6.10/init/do_mounts.c	2005-01-28 10:30:43.000000000 +0100
> @@ -135,7 +135,7 @@ fail:
>   *	is mounted on rootfs /sys.
>   */
>  
> -dev_t __init name_to_dev_t(char *name)
> +dev_t name_to_dev_t(char *name)
>  {
>  	char s[32];
>  	char *p;

Why do you need this one? /sys/power/resume accepts numeric values, it
should not need to translate...

> @@ -144,7 +144,8 @@ dev_t __init name_to_dev_t(char *name)
>  
>  #ifdef CONFIG_SYSFS
>  	int mkdir_err = sys_mkdir("/sys", 0700);
> -	if (sys_mount("sysfs", "/sys", "sysfs", 0, NULL) < 0)
> +	int mount_err = sys_mount("sysfs", "/sys", "sysfs", 0, NULL);
> +	if (mount_err < 0 && mount_err != -EBUSY)
>  		goto out;
>  #endif
>  

This is probably not acceptable. Why do you need it? It should be
easily doable from initrd.

> --- linux-2.6.10/kernel/power/disk.c.orig	2005-01-28 10:25:28.000000000 +0100
> +++ linux-2.6.10/kernel/power/disk.c	2005-01-31 11:59:04.308199464 +0100
> @@ -121,45 +125,54 @@ static void finish(void)
>  }
>  
>  
> -static int prepare(void)
> +static int prepare_processes(void)
>  {
>  	int error;
>  
>  	pm_prepare_console();
>  
>  	sys_sync();
> -	if (freeze_processes()) {
> +
> +	if (freeze_processes() > 1) {
>  		error = -EBUSY;
> -		goto Thaw;
> +		return error;
>  	}

What does freeze_processes() == 1 mean and why is it suddenly ok?

> -	pr_debug("PM: Preparing system for restore.\n");
> +	pr_debug("PM: Preparing devices for restore.\n");
>  
> -	if ((error = prepare()))
> +	if ((error = prepare_devices())) {
>  		goto Free;
> +	}

I'd not add parenthesis for single command....

> +	if (sscanf(buf, "%u:%u", &maj, &min) == 2) {
> +		res = MKDEV(maj,min);
> +		if (maj == MAJOR(res) && min == MINOR(res)) {
> +			swsusp_resume_device = res;
> +			printk("Attempting manual resume\n");
> +			noresume = 0;
> +			set_current_state(TASK_STOPPED);
> +			software_resume();
> +			set_current_state(TASK_RUNNING);

Ugh, now that is "interesting" hack. You set yourself as stopped to
avoid refrigerator.... Is it really needed?

> --- linux-2.6.10/kernel/power/swsusp.c.orig	2005-01-28 10:25:28.000000000 +0100
> +++ linux-2.6.10/kernel/power/swsusp.c	2005-01-28 16:45:14.000000000 +0100
> +	} else {
> +		pr_debug("swsusp: Resume From Partition %d:%d\n",
> +			 MAJOR(swsusp_resume_device),MINOR(swsusp_resume_device));

Missing space after ",".

> -	resume_bdev = open_by_devnum(resume_device, FMODE_READ);
> +	resume_bdev = open_by_devnum(swsusp_resume_device, FMODE_READ);
>  	if (!IS_ERR(resume_bdev)) {
>  		set_blocksize(resume_bdev, PAGE_SIZE);
> -		error = read_suspend_image();
> -		blkdev_put(resume_bdev);
> +		if((error = check_suspend_image()))
> +		    blkdev_put(resume_bdev);

Please put space after "if" and fix formatting here.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!