[PATCH RFC 0/2] PM / Hibernate: sysfs resume

[PATCH RFC 0/2] PM / Hibernate: sysfs resume

[Sebastian Capella]

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

[PATCH RFC 1/2] PM / Hibernate: use name_to_dev_t to parse resume

[Sebastian Capella]

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();
-- 
1.7.9.5

[PATCH RFC 1/2] PM / Hibernate: use name_to_dev_t to parse resume

[Pavel Machek]

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

[PATCH RFC 1/2] PM / Hibernate: use name_to_dev_t to parse resume

[Sebastian Capella]

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

[PATCH RFC 1/2] PM / Hibernate: use name_to_dev_t to parse resume

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH RFC 1/2] PM / Hibernate: use name_to_dev_t to parse resume

[Sebastian Capella]

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

[PATCH RFC 1/2] PM / Hibernate: use name_to_dev_t to parse resume

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH RFC 2/2] PM / Hibernate: add section for resume options

[Sebastian Capella]

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

[PATCH RFC 2/2] PM / Hibernate: add section for resume options

[Pavel Machek]

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>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html