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

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

[Matthew Garrett]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

Hi,

These two patches do two things:

1) The first removes __init declarations from swsusp code
2) The second allows for the resume device to be set from userspace (ie,
without having to use name_to_dev_t) and also allows for resumes to be
triggered from userspace.

A /sys/power/resume file is added. Doing

echo -n "set 03:02" >/sys/power/resume

will set /dev/hda2 as the resume device. 

echo -n "resume 03:02" >/sys/power/resume

will attempt a resume from /dev/hda2. Patches are against 2.6.10-rc3.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

[-- Attachment #2: __init.diff --]
[-- Type: text/x-patch, Size: 2113 bytes --]

diff -ur ../../../kernel/power/swsusp.c kernel/power/swsusp.c
--- ../../../kernel/power/swsusp.c	2004-12-05 16:11:19.000000000 +0000
+++ kernel/power/swsusp.c	2004-12-05 18:31:40.000000000 +0000
@@ -907,7 +907,7 @@
 /*
  * Returns true if given address/order collides with any orig_address 
  */
-static int __init does_collide_order(suspend_pagedir_t *pagedir, unsigned long addr,
+static int does_collide_order(suspend_pagedir_t *pagedir, unsigned long addr,
 		int order)
 {
 	int i;
@@ -925,7 +925,7 @@
  * We check here that pagedir & pages it points to won't collide with pages
  * where we're going to restore from the loaded pages later
  */
-static int __init check_pagedir(void)
+static int check_pagedir(void)
 {
 	int i;
 
@@ -943,7 +943,7 @@
 	return 0;
 }
 
-static int __init swsusp_pagedir_relocate(void)
+static int swsusp_pagedir_relocate(void)
 {
 	/*
 	 * We have to avoid recursion (not to overflow kernel stack),
@@ -1075,7 +1075,7 @@
  * I really don't think that it's foolproof but more than nothing..
  */
 
-static const char * __init sanity_check(void)
+static const char * sanity_check(void)
 {
 	dump_info();
 	if(swsusp_info.version_code != LINUX_VERSION_CODE)
@@ -1096,7 +1096,7 @@
 }
 
 
-static int __init check_header(void)
+static int check_header(void)
 {
 	const char * reason = NULL;
 	int error;
@@ -1113,7 +1113,7 @@
 	return error;
 }
 
-static int __init check_sig(void)
+static int check_sig(void)
 {
 	int error;
 
@@ -1143,7 +1143,7 @@
  *	already did that.
  */
 
-static int __init data_read(void)
+static int data_read(void)
 {
 	struct pbe * p;
 	int error;
@@ -1170,7 +1170,7 @@
 
 extern dev_t __init name_to_dev_t(const char *line);
 
-static int __init read_pagedir(void)
+static int read_pagedir(void)
 {
 	unsigned long addr;
 	int i, n = swsusp_info.pagedir_pages;
@@ -1197,7 +1197,7 @@
 	return error;
 }
 
-static int __init read_suspend_image(void)
+static int read_suspend_image(void)
 {
 	int error = 0;
 
@@ -1216,7 +1216,7 @@
  *	pmdisk_read - Read saved image from swap.
  */
 
-int __init swsusp_read(void)
+int swsusp_read(void)
 {
 	int error;
 

[-- Attachment #3: swsusp_initrd.diff --]
[-- Type: text/x-patch, Size: 4583 bytes --]

diff -ur kernel.bak/power/disk.c kernel/power/disk.c
--- kernel.bak/power/disk.c	2004-12-05 16:11:19.000000000 +0000
+++ kernel/power/disk.c	2004-12-05 20:28:32.000000000 +0000
@@ -26,9 +26,10 @@
 extern int swsusp_suspend(void);
 extern int swsusp_write(void);
 extern int swsusp_read(void);
+extern int swsusp_read_from(dev_t resume_device);
 extern int swsusp_resume(void);
 extern int swsusp_free(void);
-
+extern void swsusp_set_resume_device(dev_t resume_device);
 
 static int noresume = 0;
 char resume_file[256] = CONFIG_PM_STD_PARTITION;
@@ -198,18 +199,15 @@
 
 
 /**
- *	software_resume - Resume from a saved image.
+ *	software_resume_from - Do the actual grunt work of resume
  *
- *	Called as a late_initcall (so all devices are discovered and
- *	initialized), we call pmdisk to see if we have a saved image or not.
+ *      We call pmdisk to see if we have a saved image or not.
  *	If so, we quiesce devices, the restore the saved image. We will
  *	return above (in pm_suspend_disk() ) if everything goes well.
- *	Otherwise, we fail gracefully and return to the normally
- *	scheduled program.
  *
  */
 
-static int software_resume(void)
+static int software_resume_from(dev_t resume_device)
 {
 	int error;
 
@@ -223,9 +221,13 @@
 
 	pr_debug("PM: Reading pmdisk image.\n");
 
-	if ((error = swsusp_read()))
-		goto Done;
-
+	if (resume_device) {
+		if ((error = swsusp_read_from(resume_device)))
+			goto Done;
+	} else {
+		if ((error = swsusp_read()))
+			goto Done;
+	}
 	pr_debug("PM: Preparing system for restore.\n");
 
 	if ((error = prepare()))
@@ -245,6 +247,23 @@
 	return 0;
 }
 
+/**
+ *	software_resume - Resume from a saved image.
+ *
+ *	Called as a late_initcall (so all devices are discovered and
+ *	initialized), we simply trigger software_resume_from without
+ *      giving it an explicit resume device. This will then allow
+ *      swsusp to parse the resume argument passed to the kernel. With
+ *      luck, we end up in pm_suspend_disk(). Otherwise, we fail gracefully 
+ *      and return to the normally scheduled program.
+ *
+ */
+
+static int software_resume(void)
+{
+	return software_resume_from(0);
+}
+
 late_initcall(software_resume);
 
 
@@ -327,17 +346,54 @@
 
 power_attr(disk);
 
+static ssize_t resume_show(struct subsystem * subsys, char * buf) {
+	return sprintf(buf,"set resume\n");
+}
+
+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);
+
+	p = memchr(buf, '\n', n);
+	len = p ? p - buf : n;
+
+	if (sscanf(buf, "resume %u:%u", &maj, &min) == 2) {          
+		res = MKDEV(maj, min);
+		if (maj == MAJOR(res) && min == MINOR(res)) {
+			error = software_resume_from(res);
+		} else {
+			error = EINVAL;
+		}
+	} else if (sscanf(buf, "set %u:%u", &maj, &min) == 2) {
+		res = MKDEV(maj, min);
+		if (maj == MAJOR(res) && min == MINOR(res)) {
+			swsusp_set_resume_device(res);
+		} else {
+			error = EINVAL;
+		}
+	} else {
+		error = EINVAL;
+	}
+	
+	return error ? error : n;
+}
+
+power_attr(resume);
+
 static struct attribute * g[] = {
 	&disk_attr.attr,
+	&resume_attr.attr,
 	NULL,
 };
 
-
 static struct attribute_group attr_group = {
-	.attrs = g,
+        .attrs = g,
 };
 
-
 static int __init pm_disk_init(void)
 {
 	return sysfs_create_group(&power_subsys.kset.kobj,&attr_group);
diff -ur kernel.bak/power/swsusp.c kernel/power/swsusp.c
--- kernel.bak/power/swsusp.c	2004-12-05 18:31:40.000000000 +0000
+++ kernel/power/swsusp.c	2004-12-05 20:30:02.000000000 +0000
@@ -174,6 +174,11 @@
 		resume_device == MKDEV(imajor(inode), iminor(inode));
 }
 
+void swsusp_set_resume_device(dev_t device)
+{
+	resume_device = device;
+}
+
 int swsusp_swap_check(void) /* This is called before saving image */
 {
 	int i, len;
@@ -1216,16 +1221,10 @@
  *	pmdisk_read - Read saved image from swap.
  */
 
-int swsusp_read(void)
+int swsusp_read_from(dev_t resume_device)
 {
 	int error;
 
-	if (!strlen(resume_file))
-		return -ENOENT;
-
-	resume_device = name_to_dev_t(resume_file);
-	pr_debug("swsusp: Resume From Partition: %s\n", resume_file);
-
 	resume_bdev = open_by_devnum(resume_device, FMODE_READ);
 	if (!IS_ERR(resume_bdev)) {
 		set_blocksize(resume_bdev, PAGE_SIZE);
@@ -1240,3 +1239,14 @@
 		pr_debug("pmdisk: Error %d resuming\n", error);
 	return error;
 }
+
+int swsusp_read(void)
+{
+	if (!strlen(resume_file))
+		return -ENOENT;
+
+	resume_device = name_to_dev_t(resume_file);
+	pr_debug("swsusp: Resume From Partition: %s\n", resume_file);
+	return swsusp_read_from(resume_device);
+}
+

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

[Pavel Machek]

Hi!

> These two patches do two things:
> 
> 1) The first removes __init declarations from swsusp code
> 2) The second allows for the resume device to be set from userspace (ie,
> without having to use name_to_dev_t) and also allows for resumes to be
> triggered from userspace.
> 
> A /sys/power/resume file is added. Doing
> 
> echo -n "set 03:02" >/sys/power/resume

I'd prefer not to have this one. Is it actually usefull? Then resume
could be triggered by echo -n "03:02" > /sys/power/resume...

> will set /dev/hda2 as the resume device. 
> 
> echo -n "resume 03:02" >/sys/power/resume
> 
> will attempt a resume from /dev/hda2. Patches are against
> 2.6.10-rc3.

Nice way to loose your data :-). [Note note note -- I'm not going to
merge it before my bigdiff goes to mainline, so expect to rediff it
when 2.6.10 is out. I can generate the big diff if you want to test it
now.]

You really need to make sure that userland processes are stopped
before swsusp-resume is started. You should do freeze_process(). Then
resume process depends on having enough memory available, so you
probably want to free_some_memory() and warn in documentation about
the fact.

Ugh, and you really should document "list of bad ideas with resume
from userspace". It is extremely easy to shoot yourself into the foot
with this one.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

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

[Pavel Machek]

Hi!

> These two patches do two things:

(ahha, you attached instead of inlining, that's why it is hard to
quote it...)

> 1) The first removes __init declarations from swsusp code

That one looks okay.

> 2) The second allows for the resume device to be set from userspace (ie,
> without having to use name_to_dev_t) and also allows for resumes to be
> triggered from userspace.

Ouch, as for "loose your data". Properly stopping drivers before final
data are copied has just become way more critical. You may want to
check that the code does the right thing...

diff -ur kernel.bak/power/disk.c kernel/power/disk.c
--- kernel.bak/power/disk.c	2004-12-05 16:11:19.000000000 +0000
+++ kernel/power/disk.c	2004-12-05 20:28:32.000000000 +0000
@@ -26,9 +26,10 @@
 extern int swsusp_suspend(void);
 extern int swsusp_write(void);
 extern int swsusp_read(void);
+extern int swsusp_read_from(dev_t resume_device);
 extern int swsusp_resume(void);
 extern int swsusp_free(void);
-
+extern void swsusp_set_resume_device(dev_t resume_device);
 
 static int noresume = 0;
 char resume_file[256] = CONFIG_PM_STD_PARTITION;

Can't you just store resume_device into the global variable and be
done with that?


@@ -198,18 +199,15 @@
 
 
 /**
- *	software_resume - Resume from a saved image.
+ *	software_resume_from - Do the actual grunt work of resume
  *
- *	Called as a late_initcall (so all devices are discovered and
- *	initialized), we call pmdisk to see if we have a saved image or not.
+ *      We call pmdisk to see if we have a saved image or not.
  *	If so, we quiesce devices, the restore the saved image. We will
  *	return above (in pm_suspend_disk() ) if everything goes well.
- *	Otherwise, we fail gracefully and return to the normally
- *	scheduled program.
  *
  */
 
-static int software_resume(void)
+static int software_resume_from(dev_t resume_device)
 {
 	int error;
 
@@ -223,9 +221,13 @@
 
 	pr_debug("PM: Reading pmdisk image.\n");
 
-	if ((error = swsusp_read()))
-		goto Done;
-
+	if (resume_device) {
+		if ((error = swsusp_read_from(resume_device)))
+			goto Done;
+	} else {
+		if ((error = swsusp_read()))
+			goto Done;
+	}
 	pr_debug("PM: Preparing system for restore.\n");
 
 	if ((error = prepare()))
@@ -245,6 +247,23 @@
 	return 0;
 }
 
+/**
+ *	software_resume - Resume from a saved image.
+ *
+ *	Called as a late_initcall (so all devices are discovered and
+ *	initialized), we simply trigger software_resume_from without
+ *      giving it an explicit resume device. This will then allow
+ *      swsusp to parse the resume argument passed to the kernel. With
+ *      luck, we end up in pm_suspend_disk(). Otherwise, we fail gracefully 
+ *      and return to the normally scheduled program.
+ *
+ */
+
+static int software_resume(void)
+{
+	return software_resume_from(0);
+}
+
 late_initcall(software_resume);
 

I'd do software_resume_from(resume_device), meaning less ugly ifs, but
just writing is probably even easier...


@@ -327,17 +346,54 @@
 
 power_attr(disk);
 
+static ssize_t resume_show(struct subsystem * subsys, char * buf) {
+	return sprintf(buf,"set resume\n");
+}
+
+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);
+
+	p = memchr(buf, '\n', n);
+	len = p ? p - buf : n;
+
+	if (sscanf(buf, "resume %u:%u", &maj, &min) == 2) {          
+		res = MKDEV(maj, min);
+		if (maj == MAJOR(res) && min == MINOR(res)) {
+			error = software_resume_from(res);
+		} else {
+			error = EINVAL;
+		}
+	} else if (sscanf(buf, "set %u:%u", &maj, &min) == 2) {
+		res = MKDEV(maj, min);
+		if (maj == MAJOR(res) && min == MINOR(res)) {
+			swsusp_set_resume_device(res);
+		} else {
+			error = EINVAL;
+		}
+	} else {
+		error = EINVAL;
+	}
+	
+	return error ? error : n;
+}

Error should be -EINVAL.

diff -ur kernel.bak/power/swsusp.c kernel/power/swsusp.c
--- kernel.bak/power/swsusp.c	2004-12-05 18:31:40.000000000 +0000
+++ kernel/power/swsusp.c	2004-12-05 20:30:02.000000000 +0000
@@ -174,6 +174,11 @@
 		resume_device == MKDEV(imajor(inode), iminor(inode));
 }
 
+void swsusp_set_resume_device(dev_t device)
+{
+	resume_device = device;
+}
+
 int swsusp_swap_check(void) /* This is called before saving image */
 {
 	int i, len;

Please just export it.

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

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

[Matthew Garrett]

On Sun, 2004-12-05 at 22:12 +0100, Pavel Machek wrote:

> > echo -n "set 03:02" >/sys/power/resume
> 
> I'd prefer not to have this one. Is it actually usefull? Then resume
> could be triggered by echo -n "03:02" > /sys/power/resume...

resume_device is set by swsusp_read, which requires name_to_dev_t to be
working. At the point where that's called, the device driver hasn't been
loaded and we don't have the information to get the dev_t. Once the
driver has been loaded, name_to_dev_t has already been discarded (it's
marked __init). So we need to set resume_device somehow.

> > will set /dev/hda2 as the resume device. 
> > 
> > echo -n "resume 03:02" >/sys/power/resume
> > 
> > will attempt a resume from /dev/hda2. Patches are against
> > 2.6.10-rc3.
> 
> Nice way to loose your data :-). [Note note note -- I'm not going to
> merge it before my bigdiff goes to mainline, so expect to rediff it
> when 2.6.10 is out. I can generate the big diff if you want to test it
> now.]

Heh. Yes, that's no problem. A new bigdiff for -rc3 would be helpful.

> You really need to make sure that userland processes are stopped
> before swsusp-resume is started. You should do freeze_process(). Then
> resume process depends on having enough memory available, so you
> probably want to free_some_memory() and warn in documentation about
> the fact.

Ok. I'll look into that. The main reason I want code like this is that
Debian use modular IDE drivers that are stored in the initrd. The disks
won't be touched until the root file system is mounted, and we'll
trigger the resume before then, so there shouldn't be any risk of data
loss. At this point, there shouldn't be any userspace running other than
a single shell script - do you think it's still a problem?

> Ugh, and you really should document "list of bad ideas with resume
> from userspace". It is extremely easy to shoot yourself into the foot
> with this one.

Will do.

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

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

[Pavel Machek]

Hi!

> > > echo -n "set 03:02" >/sys/power/resume
> > 
> > I'd prefer not to have this one. Is it actually usefull? Then resume
> > could be triggered by echo -n "03:02" > /sys/power/resume...
> 
> resume_device is set by swsusp_read, which requires name_to_dev_t to be
> working. At the point where that's called, the device driver hasn't been
> loaded and we don't have the information to get the dev_t. Once the
> driver has been loaded, name_to_dev_t has already been discarded (it's
> marked __init). So we need to set resume_device somehow.

What about move of resume_device setup somewhere sooner?

> Heh. Yes, that's no problem. A new bigdiff for -rc3 would be
> helpful.

Hmm, I'm still on 2.6.9, but this code did not change much. I'll
generate it.

> > You really need to make sure that userland processes are stopped
> > before swsusp-resume is started. You should do freeze_process(). Then
> > resume process depends on having enough memory available, so you
> > probably want to free_some_memory() and warn in documentation about
> > the fact.
> 
> Ok. I'll look into that. The main reason I want code like this is that
> Debian use modular IDE drivers that are stored in the initrd. The disks
> won't be touched until the root file system is mounted, and we'll
> trigger the resume before then, so there shouldn't be any risk of data
> loss. At this point, there shouldn't be any userspace running other than
> a single shell script - do you think it's still a problem?

Single shell script would probably do no harm, but then, you want this
to go into mainline, not into Debian kernel, right? ;-).

Actually freezing processes is good thing to do even for normal
resume. We pretty much know there are no harmfull processes running
there, but better safe than sorry.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

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

[Matthew Garrett]

On Sun, 2004-12-05 at 22:29 +0100, Pavel Machek wrote:

> > resume_device is set by swsusp_read, which requires name_to_dev_t to be
> > working. At the point where that's called, the device driver hasn't been
> > loaded and we don't have the information to get the dev_t. Once the
> > driver has been loaded, name_to_dev_t has already been discarded (it's
> > marked __init). So we need to set resume_device somehow.
> 
> What about move of resume_device setup somewhere sooner?

Ah - we could always set resume_device, even if there's nothing to
resume. That way, it'd be set correctly for userspace later on. Ok, I
think I can make that work.

> > Heh. Yes, that's no problem. A new bigdiff for -rc3 would be
> > helpful.
> 
> Hmm, I'm still on 2.6.9, but this code did not change much. I'll
> generate it.

Thanks!

> > Ok. I'll look into that. The main reason I want code like this is that
> > Debian use modular IDE drivers that are stored in the initrd. The disks
> > won't be touched until the root file system is mounted, and we'll
> > trigger the resume before then, so there shouldn't be any risk of data
> > loss. At this point, there shouldn't be any userspace running other than
> > a single shell script - do you think it's still a problem?
> 
> Single shell script would probably do no harm, but then, you want this
> to go into mainline, not into Debian kernel, right? ;-).

Heh.

> Actually freezing processes is good thing to do even for normal
> resume. We pretty much know there are no harmfull processes running
> there, but better safe than sorry.

Ok, I'll deal with that once I've got the post 2.6.10 code to work with.

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

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

[Pavel Machek]

Hi!

> > > resume_device is set by swsusp_read, which requires name_to_dev_t to be
> > > working. At the point where that's called, the device driver hasn't been
> > > loaded and we don't have the information to get the dev_t. Once the
> > > driver has been loaded, name_to_dev_t has already been discarded (it's
> > > marked __init). So we need to set resume_device somehow.
> > 
> > What about move of resume_device setup somewhere sooner?
> 
> Ah - we could always set resume_device, even if there's nothing to
> resume. That way, it'd be set correctly for userspace later on. Ok, I
> think I can make that work.

Yes.

> > > Heh. Yes, that's no problem. A new bigdiff for -rc3 would be
> > > helpful.
> > 
> > Hmm, I'm still on 2.6.9, but this code did not change much. I'll
> > generate it.
> 
> Thanks!

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

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

[Stefan Seyfried]

Pavel Machek wrote:

>>echo -n "set 03:02" >/sys/power/resume
> 
> I'd prefer not to have this one. Is it actually usefull? Then resume
> could be triggered by echo -n "03:02" > /sys/power/resume...

This could be made into a useful interface:

echo "set 03:02" > /sys/power/resume
sets the resume device _and_ verifies its signature etc, the basic
sanity check done before resume. If this fails, we can go into some
interactive setup etc.
OTOH, the "resume 03:02" could also just fail if the image is invalid...

       Stefan

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

[Matthew Garrett]

Ok, how does this one look? (applies on top of the __init patch from
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.


diff -ur kernel.bak/power/disk.c kernel/power/disk.c
--- kernel.bak/power/disk.c	2004-12-05 16:11:19.000000000 +0000
+++ kernel/power/disk.c	2004-12-06 22:29:34.000000000 +0000
@@ -1,3 +1,4 @@
+
 /*
  * kernel/power/disk.c - Suspend-to-disk support.
  *
@@ -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;
@@ -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;
 
@@ -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);
+
+        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)) {
+                        swsusp_resume_device = res;
+                        error = software_resume();
+                } else {
+                        error = -EINVAL;
+                }
+        } else {
+                error = -EINVAL;
+        }
+
+        return error ? error : n;
+}
+
+power_attr(resume);
+
 static struct attribute * g[] = {
 	&disk_attr.attr,
+	&resume_attr.attr,
 	NULL,
 };
 
diff -ur kernel.bak/power/swsusp.c kernel/power/swsusp.c
--- kernel.bak/power/swsusp.c	2004-12-05 18:31:40.000000000 +0000
+++ kernel/power/swsusp.c	2004-12-06 23:02:21.000000000 +0000
@@ -81,7 +81,7 @@
 int nr_copy_pages_check;
 
 extern char resume_file[];
-static dev_t resume_device;
+dev_t swsusp_resume_device = 0;
 /* Local variables that should not be affected by save */
 unsigned int nr_copy_pages __nosavedata = 0;
 
@@ -171,7 +171,7 @@
 	struct inode *inode = file->f_dentry->d_inode;
 
 	return S_ISBLK(inode->i_mode) &&
-		resume_device == MKDEV(imajor(inode), iminor(inode));
+		swsusp_resume_device == MKDEV(imajor(inode), iminor(inode));
 }
 
 int swsusp_swap_check(void) /* This is called before saving image */
@@ -740,7 +740,7 @@
  *	space avaiable.
  *
  *	FIXME: si_swapinfo(&i) returns all swap devices information.
- *	We should only consider resume_device. 
+ *	We should only consider swsusp_resume_device. 
  */
 
 static int enough_swap(void)
@@ -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);
+	}
+	
+	resume_bdev = open_by_devnum(swsusp_resume_device, FMODE_READ);
 
-	resume_bdev = open_by_devnum(resume_device, FMODE_READ);
 	if (!IS_ERR(resume_bdev)) {
 		set_blocksize(resume_bdev, PAGE_SIZE);
 		error = read_suspend_image();

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

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

[Pavel Machek]

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!

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

[Matthew Garrett]

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

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

[Pavel Machek]

Hi!

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

Yes.

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

Ok, maybe there's some subtlety ;-). If someone knows, please
comment...
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!