Re: [PATCH/RFC] Add support to resume swsusp from initrd

[Pavel Machek <pavel@suse.cz>]

Ahoj!

> Ok, how does this one look? (applies on top of the __init patch from
> last time)

It looks way better than last time :-).

> resume_device has been renamed to swsusp_resume_device to avoid
> confusion with the resume_device function in drivers/power. If a resume
> device has been set, it's assumed that userspace has been started. If
> not, it behaves as before. name_to_dev_t is only called if userspace
> hasn't been started - otherwise, we depend on the values provided by
> userspace being correct. If userspace has been started, we freeze tasks
> and free memory before starting the resume. If this looks ok, I'll merge
> it to your changes after 2.6.10.

Ugh, we probably should always stop tasks and always free memory. To
get code paths tested etc.


> @@ -28,7 +29,7 @@
>  extern int swsusp_read(void);
>  extern int swsusp_resume(void);
>  extern int swsusp_free(void);
> -
> +extern dev_t swsusp_resume_device;
>  
>  static int noresume = 0;
>  char resume_file[256] = CONFIG_PM_STD_PARTITION;

Move it to include/linux/suspend.h

> @@ -223,6 +224,18 @@
>  
>  	pr_debug("PM: Reading pmdisk image.\n");
>  
> +	if (swsusp_resume_device) {
> +		/* We want to be really sure that userspace isn't touching
> +		   anything at this point... */
> +		if (freeze_processes()) {
> +			goto Done;
> +		}
> +		
> +		/* And then make sure that we have enough memory to do the
> +		   resume */
> +		free_some_memory();
> +	}
> +
>  	if ((error = swsusp_read()))
>  		goto Done;
>  

This should not be conditional. 

> @@ -327,8 +340,42 @@
>  
>  power_attr(disk);
>  
> +static ssize_t resume_show(struct subsystem * subsys, char * buf) {
> +        return sprintf(buf,"%d:%d\n", MAJOR(swsusp_resume_device),
> +                       MINOR(swsusp_resume_device));
> +}
> +
> +static ssize_t resume_store(struct subsystem * s, const char * buf, size_t n)
> +{
> +        int error = 0;
> +        int len;
> +        char *p;
> +        unsigned maj, min;
> +        dev_t (res);

Why the ()s?

> +        p = memchr(buf, '\n', n);
> +        len = p ? p - buf : n;
> +
> +        if (sscanf(buf, "%u:%u", &maj, &min) == 2) {
> +                res = MKDEV(maj, min);
> +                if (maj == MAJOR(res) && min == MINOR(res)) {

You mkdev, than test that MKDEV worked? Could you add a comment why
its needed?

I'd write it as "if (sscanf ... !=2) return -EINVAL". If (MKDEV is
broken) return -EINVAL, but Andrew would hate me then.

> @@ -1220,13 +1220,16 @@
>  {
>  	int error;
>  
> -	if (!strlen(resume_file))
> -		return -ENOENT;
> +	if (!swsusp_resume_device) {
> +		if (!strlen(resume_file))
> +			return -ENOENT;
>  
> -	resume_device = name_to_dev_t(resume_file);
> -	pr_debug("swsusp: Resume From Partition: %s\n", resume_file);
> +		swsusp_resume_device = name_to_dev_t(resume_file);
> +		pr_debug("swsusp: Resume From Partition: %s\n", resume_file);
> +	}

So... if userspace echos "0:0" into resume file, you attempt to do the
resume, and oops the kernel? Why not doing name_to_dev_t,
unconditionally, while doing resume_setup? And probably kill
CONFIG_PM_STD_PARTITION; I do not like idea of kernel automagically
trying to resume without anything on command line anyway.

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