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

[Matthew Garrett <mjg59@srcf.ucam.org>]

On Tue, 2004-12-07 at 10:44 +0100, Pavel Machek wrote:

> > Ok, how does this one look? (applies on top of the __init patch from
> > last time)
> 
> It looks way better than last time :-).

Excellent.

> > -
> > +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

swsusp_resume_device? Ok.

> > @@ -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. 

Yeah, I wondered about that, but didn't want to change behaviour.

> > +        dev_t (res);
> 
> Why the ()s?

I have absolutely no idea. Copy and paste error, I think.

> > +        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?

That's just cut and pasted from name_to_dev_t - I assumed there was some
subtlety going on there.

> So... if userspace echos "0:0" into resume file, you attempt to do the
> resume, and oops the kernel? 

Whoops, good catch.

> 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.

Ok, sounds fine.

-- 
Matthew Garrett | mjg59@srcf.ucam.org