Patchset related to hibernation resume: one enhancement to make the use of an existing file more general and one documentation update.
Both patches are based on the 3.11-rc6 tag. This was tested on a Pandaboard with partial hibernation support, and compiled on x86.
Further testing is needed on other platforms. Please let me know if you're able to verify this on any other systems.
[PATCH RFC 1/2] PM / Hibernate: use name_to_dev_t to parse resume Use name_to_dev_t to parse the /sys/power/resume file making the syntax more flexible. It supports the previous use syntax and additionally can support other formats such as /dev/devicenode and UUID= formats.
By changing /sys/debug/resume to accept the same syntax as the resume=device parameter, we can parse the resume=device in the initrd init script and use the resume device directly from the kernel command line.
kernel/power/hibernate.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
[PATCH RFC 2/2] PM / Hibernate: add section for resume options This adds a small section to the swsusp.txt file to address the options for resuming. This comments on the manual resume option which is used when resorting to an initrd or initramfs for resuming. Resuming from late init is discussed later in the document, but it seemed appropriate to list them together.
Documentation/power/swsusp.txt | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
Thanks,
Sebastian
Use the name_to_dev_t call to parse the device name echo'd to to /sys/power/resume. This imitates the method used in hibernate.c in software_resume, and allows the resume partition to be specified using other equivalent device formats as well. By allowing /sys/debug/resume to accept the same syntax as the resume=device parameter, we can parse the resume=device in the init script and use the resume device directly from the kernel command line.
Signed-off-by: Sebastian Capella sebastian.capella@linaro.org --- kernel/power/hibernate.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index b26f5f1..51d4c29 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -971,15 +971,19 @@ static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute *attr, static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t n) { - unsigned int maj, min; dev_t res; int ret = -EINVAL; + int len = n; + char *devcpy;
- if (sscanf(buf, "%u:%u", &maj, &min) != 2) - goto out; + if (buf[len-1] == '\n') + len--; + + devcpy = kstrndup(buf, len, GFP_KERNEL); + res = name_to_dev_t(devcpy); + kfree(devcpy);
- res = MKDEV(maj,min); - if (maj != MAJOR(res) || min != MINOR(res)) + if (res == 0) goto out;
lock_system_sleep();
Hi!
Use the name_to_dev_t call to parse the device name echo'd to to /sys/power/resume. This imitates the method used in hibernate.c in software_resume, and allows the resume partition to be specified using other equivalent device formats as well. By allowing /sys/debug/resume to accept the same syntax as the resume=device parameter, we can parse the resume=device in the init script and use the resume device directly from the kernel command line.
Signed-off-by: Sebastian Capella sebastian.capella@linaro.org
kernel/power/hibernate.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index b26f5f1..51d4c29 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -971,15 +971,19 @@ static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute *attr, static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t n) {
- unsigned int maj, min; dev_t res; int ret = -EINVAL;
- int len = n;
- char *devcpy;
- if (sscanf(buf, "%u:%u", &maj, &min) != 2)
goto out;
- if (buf[len-1] == '\n')
len--;- devcpy = kstrndup(buf, len, GFP_KERNEL);
- res = name_to_dev_t(devcpy);
- kfree(devcpy);
Is the allocation actually neccessary? At the very least this should test for NULL... Pavel
Thanks Pavel! I'll add the check for NULL.
name_to_dev_t expects a non-const name, but the buffer passed in is const. I also am removing the '\n' if found at the end of the string which would violate the const.
Thanks!
Sebastian
On 25 August 2013 08:38, Pavel Machek pavel@ucw.cz wrote:
Hi!
Use the name_to_dev_t call to parse the device name echo'd to to /sys/power/resume. This imitates the method used in hibernate.c in software_resume, and allows the resume partition to be specified using other equivalent device formats as well. By allowing /sys/debug/resume to accept the same syntax as the resume=device parameter, we can parse the resume=device in the init script and use the resume device directly from the kernel command line.
Signed-off-by: Sebastian Capella sebastian.capella@linaro.org
kernel/power/hibernate.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index b26f5f1..51d4c29 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -971,15 +971,19 @@ static ssize_t resume_show(struct kobject *kobj,
struct kobj_attribute *attr,
static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute
*attr,
const char *buf, size_t n){
unsigned int maj, min; dev_t res; int ret = -EINVAL;
int len = n;char *devcpy;
if (sscanf(buf, "%u:%u", &maj, &min) != 2)goto out;
if (buf[len-1] == '\n')len--;devcpy = kstrndup(buf, len, GFP_KERNEL);res = name_to_dev_t(devcpy);kfree(devcpy);Is the allocation actually neccessary? At the very least this should test for NULL...
Pavel
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Apologies for my previous top post reply...
Quoting Pavel Machek (2013-08-25 08:38:11)
Is the allocation actually neccessary? At the very least this should test for NULL...
Thanks Pavel! I'll add the check for NULL.
name_to_dev_t expects a non-const name, but the buffer passed in is const. I also am removing the '\n' if found at the end of the string which would violate the const.
Thanks!
Sebastian
On Mon 2013-08-26 10:40:50, Sebastian Capella wrote:
Apologies for my previous top post reply...
Quoting Pavel Machek (2013-08-25 08:38:11)
Is the allocation actually neccessary? At the very least this should test for NULL...
Thanks Pavel! I'll add the check for NULL.
name_to_dev_t expects a non-const name, but the buffer passed in is const. I also am removing the '\n' if found at the end of the string which would violate the const.
Fix name_to_dev_t, then. No need to do memory allocation just to work around const.
[You can also take a look why it is const in the first place. I don't think it needs to be.]
Pavel
Quoting Pavel Machek (2013-08-30 04:35:33)
On Mon 2013-08-26 10:40:50, Sebastian Capella wrote:
Quoting Pavel Machek (2013-08-25 08:38:11)
Is the allocation actually neccessary? At the very least this should test for NULL...
name_to_dev_t expects a non-const name, but the buffer passed in is const. I also am removing the '\n' if found at the end of the string which would violate the const.
Fix name_to_dev_t, then. No need to do memory allocation just to work around const.
Hi Pavel,
The issue is really Removing the \n from the user space input. The flow is: const input buf -> copy to work buffer, remove newline -> name_to_dev_t
ssize_t resume_store(..., const char *buf, size_t n) // copy buf, strip off trailing newline, pass to name_to_dev_t dev_t name_to_dev_t(char *name)
The const in the restore_store buffer comes from the function type of the store member of the kobj_attribute. I don't believe this should be changed.
Currently, name_to_dev_t will fail in some cases if a trailing \n is present. Is it more appropriate to handle stripping the newline in the store function rather than modifying name_to_dev_t to clean it up?
It seems logical for name_to_dev_t to take a const name parameter as there should be no reason to modify the name buffer passed to it. I'll be happy to make a patch to do this, but without hardening name_to_dev_t against trailing newlines, it would not be neccesary for this problem.
Thanks for your time and comments!
Sebastian
Quoting Sebastian Capella (2013-08-30 11:42:30)
Quoting Pavel Machek (2013-08-30 04:35:33)
On Mon 2013-08-26 10:40:50, Sebastian Capella wrote:
Quoting Pavel Machek (2013-08-25 08:38:11)
Is the allocation actually neccessary? At the very least this should test for NULL...
name_to_dev_t expects a non-const name, but the buffer passed in is const. I also am removing the '\n' if found at the end of the string which would violate the const.
Fix name_to_dev_t, then. No need to do memory allocation just to work around const.
Hi Pavel,
The issue is really Removing the \n from the user space input. The flow is: const input buf -> copy to work buffer, remove newline -> name_to_dev_t
ssize_t resume_store(..., const char *buf, size_t n) // copy buf, strip off trailing newline, pass to name_to_dev_t dev_t name_to_dev_t(char *name)
The const in the restore_store buffer comes from the function type of the store member of the kobj_attribute. I don't believe this should be changed.
Currently, name_to_dev_t will fail in some cases if a trailing \n is present. Is it more appropriate to handle stripping the newline in the store function rather than modifying name_to_dev_t to clean it up?
It seems logical for name_to_dev_t to take a const name parameter as there should be no reason to modify the name buffer passed to it. I'll be happy to make a patch to do this, but without hardening name_to_dev_t against trailing newlines, it would not be neccesary for this problem.
Thanks for your time and comments!
Hi Pavel,
Do you have any more feedback regarding leaving the strndup?
Thanks!
Sebastian
On Tue 2013-09-17 13:50:21, Sebastian Capella wrote:
Quoting Sebastian Capella (2013-08-30 11:42:30)
Quoting Pavel Machek (2013-08-30 04:35:33)
On Mon 2013-08-26 10:40:50, Sebastian Capella wrote:
Quoting Pavel Machek (2013-08-25 08:38:11)
Is the allocation actually neccessary? At the very least this should test for NULL...
name_to_dev_t expects a non-const name, but the buffer passed in is const. I also am removing the '\n' if found at the end of the string which would violate the const.
Fix name_to_dev_t, then. No need to do memory allocation just to work around const.
Hi Pavel,
The issue is really Removing the \n from the user space input. The flow is: const input buf -> copy to work buffer, remove newline -> name_to_dev_t
ssize_t resume_store(..., const char *buf, size_t n) // copy buf, strip off trailing newline, pass to name_to_dev_t dev_t name_to_dev_t(char *name)
The const in the restore_store buffer comes from the function type of the store member of the kobj_attribute. I don't believe this should be changed.
Currently, name_to_dev_t will fail in some cases if a trailing \n is present. Is it more appropriate to handle stripping the newline in the store function rather than modifying name_to_dev_t to clean it up?
It seems logical for name_to_dev_t to take a const name parameter as there should be no reason to modify the name buffer passed to it. I'll be happy to make a patch to do this, but without hardening name_to_dev_t against trailing newlines, it would not be neccesary for this problem.
Thanks for your time and comments!
Hi Pavel,
Do you have any more feedback regarding leaving the strndup?
I think you should modify name_to_dev_t, then. Doing memory allocation just to work around \n limitation is ugly. Pavel
Expand the existing documentation to explicitly list the options for resuming a hibernation image, including the manual resume option which can be used from the initrd or initramfs and the kernel init resume.
Signed-off-by: Sebastian Capella sebastian.capella@linaro.org --- Documentation/power/swsusp.txt | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/Documentation/power/swsusp.txt b/Documentation/power/swsusp.txt index 0b4b63e..079160e 100644 --- a/Documentation/power/swsusp.txt +++ b/Documentation/power/swsusp.txt @@ -50,6 +50,19 @@ echo N > /sys/power/image_size
before suspend (it is limited to 500 MB by default).
+. The resume process checks for the presence of the resume device, +if found, it then checks the contents for the hibernation image signature. +If both are found, it resumes the hibernation image. + +. The resume process may be triggered in two ways: + 1) During lateinit: If resume=/dev/your_swap_partition is specified on + the kernel command line, lateinit runs the resume process. If the + resume device has not been probed yet, the resume process fails and + bootup continues. + 2) Manually from an initrd or initramfs: May be run from + the init script by using the /sys/power/resume file. It is vital + that this be done prior to remounting any filesystems (even as + read-only) otherwise data may be corrupted.
Article about goals and implementation of Software Suspend for Linux ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -326,7 +339,7 @@ Q: How can distributions ship a swsusp-supporting kernel with modular disk drivers (especially SATA)?
A: Well, it can be done, load the drivers, then do echo into -/sys/power/disk/resume file from initrd. Be sure not to mount +/sys/power/resume file from initrd. Be sure not to mount anything, not even read-only mount, or you are going to lose your data.
On Wed 2013-08-21 12:46:53, Sebastian Capella wrote:
Expand the existing documentation to explicitly list the options for resuming a hibernation image, including the manual resume option which can be used from the initrd or initramfs and the kernel init resume.
Signed-off-by: Sebastian Capella sebastian.capella@linaro.org
Acked-by: Pavel Machek pavel@ucw.cz
-
Pavel Machek -
Sebastian Capella