[PATCH v7 0/3] hibernation related patches

[PATCH v7 0/3] hibernation related patches

[Sebastian Capella]

Patchset related to hibernation resume:
  - enhancement to make the use of an existing resume file more general
  - add kstrdup_trimnl function which duplicates and trims a single
    trailing newline off of a string
  - cleanup checkpatch warnings in hibernate.c file

  All patches are based on the 3.13 tag.  This was tested on a
  Beaglebone black with partial hibernation support, and compiled for
  x86_64.

[PATCH v7 1/3] mm: add kstrdup_trimnl function
  include/linux/string.h |    1 +
  mm/util.c              |   29 +++++++++++++++++++++++++++++
  2 files changed, 30 insertions(+)

  Adds the kstrdup_trimnl function to duplicate and trim
  at most one trailing newline from a string.
  This is useful for working with user input to sysfs.

[PATCH v7 2/3] trivial: PM / Hibernate: clean up checkpatch in
  kernel/power/hibernate.c |   62 ++++++++++++++++++++++++----------------------
  1 file changed, 32 insertions(+), 30 deletions(-)

  Cleanup checkpatch warnings in kernel/power/hibernate.c

[PATCH v7 3/3] PM / Hibernate: use name_to_dev_t to parse resume
  kernel/power/hibernate.c |   33 +++++++++++++++++----------------
  1 file changed, 17 insertions(+), 16 deletions(-)

  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.


Changes in v7:
--------------
* Switch to trim only one trailing newline if present using kstrdup_trimnl
* remove kstrimdup patch
* add kstrdup_trimnl patch
* Add clean up patch for kernel/power/hibernate.c checkpatch warnings

Changes in v6:
--------------
* Revert tricky / confusing while loop indexing

Changes in v5:
--------------
* Change kstrimdup to minimize allocated memory.  Now allocates only
  the memory needed for the string instead of using strim.

Changes in v4:
--------------
* Dropped name_to_dev_t rework in favor of adding kstrimdup
* adjusted resume_store

Changes in v3:
--------------
* Dropped documentation patch as it went in through trivial
* Added patch for name_to_dev_t to support directly parsing userspace
  buffer

Changes in v2:
--------------
* Added check for null return of kstrndup in hibernate.c


Thanks,

Sebastian

[PATCH v7 1/3] mm: add kstrdup_trimnl function

[Sebastian Capella]

kstrdup_trimnl creates a duplicate of the passed in
null-terminated string.  If a trailing newline is found, it
is removed before duplicating.  This is useful for strings
coming from sysfs that often include trailing whitespace due to
user input.

Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Rik van Riel <riel@redhat.com> (commit_signer:5/10=50%)
Cc: Michel Lespinasse <walken@google.com>
Cc: Shaohua Li <shli@kernel.org>
Cc: Jerome Marchand <jmarchan@redhat.com>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Joe Perches <joe@perches.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
---
 include/linux/string.h |    1 +
 mm/util.c              |   29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index ac889c5..e7ec8c0 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -114,6 +114,7 @@ void *memchr_inv(const void *s, int c, size_t n);
 
 extern char *kstrdup(const char *s, gfp_t gfp);
 extern char *kstrndup(const char *s, size_t len, gfp_t gfp);
+extern char *kstrdup_trimnl(const char *s, gfp_t gfp);
 extern void *kmemdup(const void *src, size_t len, gfp_t gfp);
 
 extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
diff --git a/mm/util.c b/mm/util.c
index 808f375..0bab867 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -1,6 +1,7 @@
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/string.h>
+#include <linux/ctype.h>
 #include <linux/export.h>
 #include <linux/err.h>
 #include <linux/sched.h>
@@ -63,6 +64,34 @@ char *kstrndup(const char *s, size_t max, gfp_t gfp)
 EXPORT_SYMBOL(kstrndup);
 
 /**
+ * kstrdup_trimnl - Copy a %NUL terminated string, removing one trailing
+ * newline if present.
+ * @s: the string to duplicate
+ * @gfp: the GFP mask used in the kmalloc() call when allocating memory
+ *
+ * Returns an address, which the caller must kfree, containing
+ * a duplicate of the passed string with a single trailing newline
+ * removed if present.
+ */
+char *kstrdup_trimnl(const char *s, gfp_t gfp)
+{
+	char *buf;
+	size_t len = strlen(s);
+	if (len >= 1 && s[len - 1] == '\n')
+		len--;
+
+	buf = kmalloc_track_caller(len + 1, gfp);
+	if (!buf)
+		return NULL;
+
+	memcpy(buf, s, len);
+	buf[len] = '\0';
+
+	return buf;
+}
+EXPORT_SYMBOL(kstrdup_trimnl);
+
+/**
  * kmemdup - duplicate region of memory
  *
  * @src: memory region to duplicate
-- 
1.7.9.5

[PATCH v7 2/3] trivial: PM / Hibernate: clean up checkpatch in hibernate.c

[Sebastian Capella]

Checkpatch reports several warnings in hibernate.c
printk use removed, long lines wrapped, whitespace cleanup,
extend short msleeps, while loops on two lines.

Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 kernel/power/hibernate.c |   62 ++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 30 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 0121dab..cd1e30c 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -94,7 +94,7 @@ EXPORT_SYMBOL(system_entering_hibernation);
 #ifdef CONFIG_PM_DEBUG
 static void hibernation_debug_sleep(void)
 {
-	printk(KERN_INFO "hibernation debug: Waiting for 5 seconds.\n");
+	pr_info("hibernation debug: Waiting for 5 seconds.\n");
 	mdelay(5000);
 }
 
@@ -239,7 +239,7 @@ void swsusp_show_speed(struct timeval *start, struct timeval *stop,
 		centisecs = 1;	/* avoid div-by-zero */
 	k = nr_pages * (PAGE_SIZE / 1024);
 	kps = (k * 100) / centisecs;
-	printk(KERN_INFO "PM: %s %d kbytes in %d.%02d seconds (%d.%02d MB/s)\n",
+	pr_info("PM: %s %d kbytes in %d.%02d seconds (%d.%02d MB/s)\n",
 			msg, k,
 			centisecs / 100, centisecs % 100,
 			kps / 1000, (kps % 1000) / 10);
@@ -260,8 +260,7 @@ static int create_image(int platform_mode)
 
 	error = dpm_suspend_end(PMSG_FREEZE);
 	if (error) {
-		printk(KERN_ERR "PM: Some devices failed to power down, "
-			"aborting hibernation\n");
+		pr_err("PM: Some devices failed to power down, aborting hibernation\n");
 		return error;
 	}
 
@@ -277,8 +276,7 @@ static int create_image(int platform_mode)
 
 	error = syscore_suspend();
 	if (error) {
-		printk(KERN_ERR "PM: Some system devices failed to power down, "
-			"aborting hibernation\n");
+		pr_err("PM: Some system devices failed to power down, aborting hibernation\n");
 		goto Enable_irqs;
 	}
 
@@ -289,8 +287,7 @@ static int create_image(int platform_mode)
 	save_processor_state();
 	error = swsusp_arch_suspend();
 	if (error)
-		printk(KERN_ERR "PM: Error %d creating hibernation image\n",
-			error);
+		pr_err("PM: Error %d creating hibernation image\n", error);
 	/* Restore control flow magically appears here */
 	restore_processor_state();
 	if (!in_suspend) {
@@ -413,8 +410,7 @@ static int resume_target_kernel(bool platform_mode)
 
 	error = dpm_suspend_end(PMSG_QUIESCE);
 	if (error) {
-		printk(KERN_ERR "PM: Some devices failed to power down, "
-			"aborting resume\n");
+		pr_err("PM: Some devices failed to power down, aborting resume\n");
 		return error;
 	}
 
@@ -550,7 +546,8 @@ int hibernation_platform_enter(void)
 
 	hibernation_ops->enter();
 	/* We should never get here */
-	while (1);
+	while (1)
+		;
 
  Power_up:
 	syscore_resume();
@@ -611,8 +608,7 @@ static void power_down(void)
 		 */
 		error = swsusp_unmark();
 		if (error)
-			printk(KERN_ERR "PM: Swap will be unusable! "
-			                "Try swapon -a.\n");
+			pr_err("PM: Swap will be unusable! Try swapon -a.\n");
 		return;
 #endif
 	}
@@ -621,8 +617,9 @@ static void power_down(void)
 	 * Valid image is on the disk, if we continue we risk serious data
 	 * corruption after resume.
 	 */
-	printk(KERN_CRIT "PM: Please power down manually\n");
-	while(1);
+	pr_crit("PM: Please power down manually\n");
+	while (1)
+		;
 }
 
 /**
@@ -644,9 +641,9 @@ int hibernate(void)
 	if (error)
 		goto Exit;
 
-	printk(KERN_INFO "PM: Syncing filesystems ... ");
+	pr_info("PM: Syncing filesystems ... ");
 	sys_sync();
-	printk("done.\n");
+	pr_cont("done.\n");
 
 	error = freeze_processes();
 	if (error)
@@ -670,7 +667,7 @@ int hibernate(void)
 		if (nocompress)
 			flags |= SF_NOCOMPRESS_MODE;
 		else
-		        flags |= SF_CRC32_MODE;
+			flags |= SF_CRC32_MODE;
 
 		pr_debug("PM: writing image.\n");
 		error = swsusp_write(flags);
@@ -750,7 +747,7 @@ static int software_resume(void)
 	pr_debug("PM: Checking hibernation image partition %s\n", resume_file);
 
 	if (resume_delay) {
-		printk(KERN_INFO "Waiting %dsec before reading resume device...\n",
+		pr_info("Waiting %dsec before reading resume device...\n",
 			resume_delay);
 		ssleep(resume_delay);
 	}
@@ -765,7 +762,7 @@ static int software_resume(void)
 	if (isdigit(resume_file[0]) && resume_wait) {
 		int partno;
 		while (!get_gendisk(swsusp_resume_device, &partno))
-			msleep(10);
+			msleep(20);
 	}
 
 	if (!swsusp_resume_device) {
@@ -776,8 +773,9 @@ static int software_resume(void)
 		wait_for_device_probe();
 
 		if (resume_wait) {
-			while ((swsusp_resume_device = name_to_dev_t(resume_file)) == 0)
-				msleep(10);
+			while ((swsusp_resume_device =
+					name_to_dev_t(resume_file)) == 0)
+				msleep(20);
 			async_synchronize_full();
 		}
 
@@ -826,7 +824,7 @@ static int software_resume(void)
 	if (!error)
 		hibernation_restore(flags & SF_PLATFORM_MODE);
 
-	printk(KERN_ERR "PM: Failed to load hibernation image, recovering.\n");
+	pr_err("PM: Failed to load hibernation image, recovering.\n");
 	swsusp_free();
 	free_basic_memory_bitmaps();
  Thaw:
@@ -965,7 +963,7 @@ power_attr(disk);
 static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute *attr,
 			   char *buf)
 {
-	return sprintf(buf,"%d:%d\n", MAJOR(swsusp_resume_device),
+	return sprintf(buf, "%d:%d\n", MAJOR(swsusp_resume_device),
 		       MINOR(swsusp_resume_device));
 }
 
@@ -986,7 +984,7 @@ static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr,
 	lock_system_sleep();
 	swsusp_resume_device = res;
 	unlock_system_sleep();
-	printk(KERN_INFO "PM: Starting manual resume from disk\n");
+	pr_info("PM: Starting manual resume from disk\n");
 	noresume = 0;
 	software_resume();
 	ret = n;
@@ -996,13 +994,15 @@ static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 power_attr(resume);
 
-static ssize_t image_size_show(struct kobject *kobj, struct kobj_attribute *attr,
+static ssize_t image_size_show(struct kobject *kobj,
+			       struct kobj_attribute *attr,
 			       char *buf)
 {
 	return sprintf(buf, "%lu\n", image_size);
 }
 
-static ssize_t image_size_store(struct kobject *kobj, struct kobj_attribute *attr,
+static ssize_t image_size_store(struct kobject *kobj,
+				struct kobj_attribute *attr,
 				const char *buf, size_t n)
 {
 	unsigned long size;
@@ -1039,7 +1039,7 @@ static ssize_t reserved_size_store(struct kobject *kobj,
 
 power_attr(reserved_size);
 
-static struct attribute * g[] = {
+static struct attribute *g[] = {
 	&disk_attr.attr,
 	&resume_attr.attr,
 	&image_size_attr.attr,
@@ -1066,7 +1066,7 @@ static int __init resume_setup(char *str)
 	if (noresume)
 		return 1;
 
-	strncpy( resume_file, str, 255 );
+	strncpy(resume_file, str, 255);
 	return 1;
 }
 
@@ -1106,7 +1106,9 @@ static int __init resumewait_setup(char *str)
 
 static int __init resumedelay_setup(char *str)
 {
-	resume_delay = simple_strtoul(str, NULL, 0);
+	int ret = kstrtoint(str, 0, &resume_delay);
+	/* mask must_check warn; on failure, leaves resume_delay unchanged */
+	(void)ret;
 	return 1;
 }
 
-- 
1.7.9.5

[PATCH v7 3/3] 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>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
---
 kernel/power/hibernate.c |   33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index cd1e30c..3abd192 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -970,26 +970,27 @@ 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;
+	char *name = kstrdup_trimnl(buf, GFP_KERNEL);
 
-	if (sscanf(buf, "%u:%u", &maj, &min) != 2)
-		goto out;
+	if (name == NULL)
+		return -ENOMEM;
 
-	res = MKDEV(maj,min);
-	if (maj != MAJOR(res) || min != MINOR(res))
-		goto out;
+	res = name_to_dev_t(name);
 
-	lock_system_sleep();
-	swsusp_resume_device = res;
-	unlock_system_sleep();
-	pr_info("PM: Starting manual resume from disk\n");
-	noresume = 0;
-	software_resume();
-	ret = n;
- out:
-	return ret;
+	if (res != 0) {
+		lock_system_sleep();
+		swsusp_resume_device = res;
+		unlock_system_sleep();
+		pr_info("PM: Starting manual resume from disk\n");
+		noresume = 0;
+		software_resume();
+	} else {
+		n = -EINVAL;
+	}
+
+	kfree(name);
+	return n;
 }
 
 power_attr(resume);
-- 
1.7.9.5

Re: [PATCH v7 2/3] trivial: PM / Hibernate: clean up checkpatch in hibernate.c

[Joe Perches]

On Tue, 2014-02-04 at 12:43 -0800, Sebastian Capella wrote:
> Checkpatch reports several warnings in hibernate.c
> printk use removed, long lines wrapped, whitespace cleanup,
> extend short msleeps, while loops on two lines.
[]
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
[]
> @@ -765,7 +762,7 @@ static int software_resume(void)
>  	if (isdigit(resume_file[0]) && resume_wait) {
>  		int partno;
>  		while (!get_gendisk(swsusp_resume_device, &partno))
> -			msleep(10);
> +			msleep(20);

What good is changing this from 10 to 20?

> @@ -776,8 +773,9 @@ static int software_resume(void)
>  		wait_for_device_probe();
>  
>  		if (resume_wait) {
> -			while ((swsusp_resume_device = name_to_dev_t(resume_file)) == 0)
> -				msleep(10);
> +			while ((swsusp_resume_device =
> +					name_to_dev_t(resume_file)) == 0)
> +				msleep(20);

here too.

Re: [PATCH v7 2/3] trivial: PM / Hibernate: clean up checkpatch in hibernate.c

[Rafael J. Wysocki]

On Tuesday, February 04, 2014 12:43:50 PM Sebastian Capella wrote:
> Checkpatch reports several warnings in hibernate.c
> printk use removed, long lines wrapped, whitespace cleanup,
> extend short msleeps, while loops on two lines.

Well, this isn't a trivial patch.

> Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---
>  kernel/power/hibernate.c |   62 ++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 0121dab..cd1e30c 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -94,7 +94,7 @@ EXPORT_SYMBOL(system_entering_hibernation);
>  #ifdef CONFIG_PM_DEBUG
>  static void hibernation_debug_sleep(void)
>  {
> -	printk(KERN_INFO "hibernation debug: Waiting for 5 seconds.\n");
> +	pr_info("hibernation debug: Waiting for 5 seconds.\n");
>  	mdelay(5000);
>  }
>  
> @@ -239,7 +239,7 @@ void swsusp_show_speed(struct timeval *start, struct timeval *stop,
>  		centisecs = 1;	/* avoid div-by-zero */
>  	k = nr_pages * (PAGE_SIZE / 1024);
>  	kps = (k * 100) / centisecs;
> -	printk(KERN_INFO "PM: %s %d kbytes in %d.%02d seconds (%d.%02d MB/s)\n",
> +	pr_info("PM: %s %d kbytes in %d.%02d seconds (%d.%02d MB/s)\n",
>  			msg, k,
>  			centisecs / 100, centisecs % 100,
>  			kps / 1000, (kps % 1000) / 10);
> @@ -260,8 +260,7 @@ static int create_image(int platform_mode)
>  
>  	error = dpm_suspend_end(PMSG_FREEZE);
>  	if (error) {
> -		printk(KERN_ERR "PM: Some devices failed to power down, "
> -			"aborting hibernation\n");
> +		pr_err("PM: Some devices failed to power down, aborting hibernation\n");
>  		return error;
>  	}
>  
> @@ -277,8 +276,7 @@ static int create_image(int platform_mode)
>  
>  	error = syscore_suspend();
>  	if (error) {
> -		printk(KERN_ERR "PM: Some system devices failed to power down, "
> -			"aborting hibernation\n");
> +		pr_err("PM: Some system devices failed to power down, aborting hibernation\n");
>  		goto Enable_irqs;
>  	}
>  
> @@ -289,8 +287,7 @@ static int create_image(int platform_mode)
>  	save_processor_state();
>  	error = swsusp_arch_suspend();
>  	if (error)
> -		printk(KERN_ERR "PM: Error %d creating hibernation image\n",
> -			error);
> +		pr_err("PM: Error %d creating hibernation image\n", error);
>  	/* Restore control flow magically appears here */
>  	restore_processor_state();
>  	if (!in_suspend) {
> @@ -413,8 +410,7 @@ static int resume_target_kernel(bool platform_mode)
>  
>  	error = dpm_suspend_end(PMSG_QUIESCE);
>  	if (error) {
> -		printk(KERN_ERR "PM: Some devices failed to power down, "
> -			"aborting resume\n");
> +		pr_err("PM: Some devices failed to power down, aborting resume\n");
>  		return error;
>  	}
>  
> @@ -550,7 +546,8 @@ int hibernation_platform_enter(void)
>  
>  	hibernation_ops->enter();
>  	/* We should never get here */
> -	while (1);
> +	while (1)
> +		;

Please remove this change from the patch.  I don't care about checkpatch
complaining here.

>  
>   Power_up:
>  	syscore_resume();
> @@ -611,8 +608,7 @@ static void power_down(void)
>  		 */
>  		error = swsusp_unmark();
>  		if (error)
> -			printk(KERN_ERR "PM: Swap will be unusable! "
> -			                "Try swapon -a.\n");
> +			pr_err("PM: Swap will be unusable! Try swapon -a.\n");
>  		return;
>  #endif
>  	}
> @@ -621,8 +617,9 @@ static void power_down(void)
>  	 * Valid image is on the disk, if we continue we risk serious data
>  	 * corruption after resume.
>  	 */
> -	printk(KERN_CRIT "PM: Please power down manually\n");
> -	while(1);
> +	pr_crit("PM: Please power down manually\n");
> +	while (1)
> +		;

Same here.

>  }
>  
>  /**
> @@ -644,9 +641,9 @@ int hibernate(void)
>  	if (error)
>  		goto Exit;
>  
> -	printk(KERN_INFO "PM: Syncing filesystems ... ");
> +	pr_info("PM: Syncing filesystems ... ");
>  	sys_sync();
> -	printk("done.\n");
> +	pr_cont("done.\n");
>  
>  	error = freeze_processes();
>  	if (error)
> @@ -670,7 +667,7 @@ int hibernate(void)
>  		if (nocompress)
>  			flags |= SF_NOCOMPRESS_MODE;
>  		else
> -		        flags |= SF_CRC32_MODE;
> +			flags |= SF_CRC32_MODE;
>  
>  		pr_debug("PM: writing image.\n");
>  		error = swsusp_write(flags);
> @@ -750,7 +747,7 @@ static int software_resume(void)
>  	pr_debug("PM: Checking hibernation image partition %s\n", resume_file);
>  
>  	if (resume_delay) {
> -		printk(KERN_INFO "Waiting %dsec before reading resume device...\n",
> +		pr_info("Waiting %dsec before reading resume device...\n",
>  			resume_delay);
>  		ssleep(resume_delay);
>  	}
> @@ -765,7 +762,7 @@ static int software_resume(void)
>  	if (isdigit(resume_file[0]) && resume_wait) {
>  		int partno;
>  		while (!get_gendisk(swsusp_resume_device, &partno))
> -			msleep(10);
> +			msleep(20);

That's the reason why it is not trivial.

First, the change being made doesn't belong in this patch.

Second, what's the problem with the original value?

>  	}
>  
>  	if (!swsusp_resume_device) {
> @@ -776,8 +773,9 @@ static int software_resume(void)
>  		wait_for_device_probe();
>  
>  		if (resume_wait) {
> -			while ((swsusp_resume_device = name_to_dev_t(resume_file)) == 0)
> -				msleep(10);
> +			while ((swsusp_resume_device =
> +					name_to_dev_t(resume_file)) == 0)
> +				msleep(20);

And here?

>  			async_synchronize_full();
>  		}
>  
> @@ -826,7 +824,7 @@ static int software_resume(void)
>  	if (!error)
>  		hibernation_restore(flags & SF_PLATFORM_MODE);
>  
> -	printk(KERN_ERR "PM: Failed to load hibernation image, recovering.\n");
> +	pr_err("PM: Failed to load hibernation image, recovering.\n");
>  	swsusp_free();
>  	free_basic_memory_bitmaps();
>   Thaw:
> @@ -965,7 +963,7 @@ power_attr(disk);
>  static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute *attr,
>  			   char *buf)
>  {
> -	return sprintf(buf,"%d:%d\n", MAJOR(swsusp_resume_device),
> +	return sprintf(buf, "%d:%d\n", MAJOR(swsusp_resume_device),
>  		       MINOR(swsusp_resume_device));
>  }
>  
> @@ -986,7 +984,7 @@ static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	lock_system_sleep();
>  	swsusp_resume_device = res;
>  	unlock_system_sleep();
> -	printk(KERN_INFO "PM: Starting manual resume from disk\n");
> +	pr_info("PM: Starting manual resume from disk\n");
>  	noresume = 0;
>  	software_resume();
>  	ret = n;
> @@ -996,13 +994,15 @@ static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  power_attr(resume);
>  
> -static ssize_t image_size_show(struct kobject *kobj, struct kobj_attribute *attr,
> +static ssize_t image_size_show(struct kobject *kobj,
> +			       struct kobj_attribute *attr,
>  			       char *buf)

Why can't you leave the code as is here?

>  {
>  	return sprintf(buf, "%lu\n", image_size);
>  }
>  
> -static ssize_t image_size_store(struct kobject *kobj, struct kobj_attribute *attr,
> +static ssize_t image_size_store(struct kobject *kobj,
> +				struct kobj_attribute *attr,
>  				const char *buf, size_t n)

And here?

>  {
>  	unsigned long size;
> @@ -1039,7 +1039,7 @@ static ssize_t reserved_size_store(struct kobject *kobj,
>  
>  power_attr(reserved_size);
>  
> -static struct attribute * g[] = {
> +static struct attribute *g[] = {
>  	&disk_attr.attr,
>  	&resume_attr.attr,
>  	&image_size_attr.attr,
> @@ -1066,7 +1066,7 @@ static int __init resume_setup(char *str)
>  	if (noresume)
>  		return 1;
>  
> -	strncpy( resume_file, str, 255 );
> +	strncpy(resume_file, str, 255);
>  	return 1;
>  }
>  
> @@ -1106,7 +1106,9 @@ static int __init resumewait_setup(char *str)
>  
>  static int __init resumedelay_setup(char *str)
>  {
> -	resume_delay = simple_strtoul(str, NULL, 0);
> +	int ret = kstrtoint(str, 0, &resume_delay);
> +	/* mask must_check warn; on failure, leaves resume_delay unchanged */
> +	(void)ret;

And that's not a trivial change surely?

And why didn't you do (void)kstrtoint(str, 0, &resume_delay); instead?

>  	return 1;
>  }
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v7 3/3] PM / Hibernate: use name_to_dev_t to parse resume

[Rafael J. Wysocki]

On Tuesday, February 04, 2014 12:43:51 PM Sebastian Capella wrote:
> 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>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---
>  kernel/power/hibernate.c |   33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index cd1e30c..3abd192 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -970,26 +970,27 @@ 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;
> +	char *name = kstrdup_trimnl(buf, GFP_KERNEL);
>  
> -	if (sscanf(buf, "%u:%u", &maj, &min) != 2)
> -		goto out;
> +	if (name == NULL)

What about "if (!name)"?

> +		return -ENOMEM;
>  
> -	res = MKDEV(maj,min);
> -	if (maj != MAJOR(res) || min != MINOR(res))
> -		goto out;
> +	res = name_to_dev_t(name);
>  
> -	lock_system_sleep();
> -	swsusp_resume_device = res;
> -	unlock_system_sleep();
> -	pr_info("PM: Starting manual resume from disk\n");
> -	noresume = 0;
> -	software_resume();
> -	ret = n;
> - out:
> -	return ret;
> +	if (res != 0) {

What about "if (res)"?

> +		lock_system_sleep();
> +		swsusp_resume_device = res;
> +		unlock_system_sleep();
> +		pr_info("PM: Starting manual resume from disk\n");
> +		noresume = 0;
> +		software_resume();
> +	} else {
> +		n = -EINVAL;
> +	}
> +
> +	kfree(name);
> +	return n;
>  }
>  
>  power_attr(resume);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v7 2/3] trivial: PM / Hibernate: clean up checkpatch in hibernate.c

[Joe Perches]

On Tue, 2014-02-04 at 14:05 -0800, Sebastian Capella wrote:
> Quoting Joe Perches (2014-02-04 13:21:02)
> > On Tue, 2014-02-04 at 12:43 -0800, Sebastian Capella wrote:
> > > Checkpatch reports several warnings in hibernate.c
> > > printk use removed, long lines wrapped, whitespace cleanup,
> > > extend short msleeps, while loops on two lines.
> > []
> > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > []
> > > @@ -765,7 +762,7 @@ static int software_resume(void)
> > >       if (isdigit(resume_file[0]) && resume_wait) {
> > >               int partno;
> > >               while (!get_gendisk(swsusp_resume_device, &partno))
> > > -                     msleep(10);
> > > +                     msleep(20);
> > 
> > What good is changing this from 10 to 20?
> > 
> > > @@ -776,8 +773,9 @@ static int software_resume(void)
> > >               wait_for_device_probe();
> > >  
> > >               if (resume_wait) {
> > > -                     while ((swsusp_resume_device = name_to_dev_t(resume_file)) == 0)
> > > -                             msleep(10);
> > > +                     while ((swsusp_resume_device =
> > > +                                     name_to_dev_t(resume_file)) == 0)
> > > +                             msleep(20);
> > 
> > here too.
> 
> Thanks Joe!
> 
> I'm happy to make whatever change is best.  I just ran into one
> checkpatch warning around a printk I indented and figured I'd try to get
> them all if I could.

Shutting up checkpatch for the sake of shutting of
checkpatch is sometimes not the right thing to do.

> The delays in question didn't appear timing critical as both are looping
> waiting for device discovery to complete.  They're only enabled when using
> the resumewait command line parameter.

Any time it happens faster doesn't hurt and
can therefore could resume faster no?

> Is this an incorrect checkpatch warning?  The message from checkpatch
> implies using msleep for smaller values can be misleading.

That's true, but it doesn't mean it's required
to change the code.

>   - Why not msleep for (1ms - 20ms)?                               
>     Explained originally here:                               
>       http://lkml.org/lkml/2007/8/3/250                
>     msleep(1~20) may not do what the caller intends, and     
>     will often sleep longer (~20 ms actual sleep for any     
>     value given in the 1~20ms range). In many cases this     
>     is not the desired behavior. 
> 
> When I look at kernel/timers.c in my current kernel, I see msleep is
> using msecs_to_jiffies + 1, and on my current platform this appears to
> be ~20msec as the jiffies are 10ms.

And on platforms where HZ is 1000, it's
still slightly faster.

I'd just leave it alone.

Re: [PATCH v7 2/3] trivial: PM / Hibernate: clean up checkpatch in hibernate.c

[Rafael J. Wysocki]

On Tuesday, February 04, 2014 02:37:33 PM Sebastian Capella wrote:
> Quoting Rafael J. Wysocki (2014-02-04 13:36:29)
> > Well, this isn't a trivial patch.
> 
> I'll remove the trivial, thanks!
> 
> Quoting Rafael J. Wysocki (2014-02-04 13:36:29)
> > On Tuesday, February 04, 2014 12:43:50 PM Sebastian Capella wrote:
> > > +     while (1)
> > > +             ;
> > Please remove this change from the patch.  I don't care about checkpatch
> > complaining here.
> > > +     while (1)
> > > +             ;
> > Same here.
> 
> Will do, thanks!
> 
> > > @@ -765,7 +762,7 @@ static int software_resume(void)
> > >       if (isdigit(resume_file[0]) && resume_wait) {
> > >               int partno;
> > >               while (!get_gendisk(swsusp_resume_device, &partno))
> > > -                     msleep(10);
> > > +                     msleep(20);
> > 
> > That's the reason why it is not trivial.
> > 
> > First, the change being made doesn't belong in this patch.
> 
> Thanks I'll separate it if it remains.
> 
> > Second, what's the problem with the original value?
> 
> The warning from checkpatch implies that it's misleading to
> msleep < 20ms since msleep is using msec_to_jiffies + 1 for
> the duration.  In any case, this is polling for devices discovery to
> complete.  It is used when resumewait is specified on the command
> line telling hibernate to wait for the resume device to appear.

What checkpatch is saying is about *new* code, not the existing one.

You need to have a *reason* to change the way the existing code works
and the above explanation doesn't sound like a good one to me in this
particular case.

> > > -static ssize_t image_size_show(struct kobject *kobj, struct kobj_attribute *attr,
> > > +static ssize_t image_size_show(struct kobject *kobj,
> > > +                            struct kobj_attribute *attr,
> > Why can't you leave the code as is here?
> > > -static ssize_t image_size_store(struct kobject *kobj, struct kobj_attribute *attr,
> > > +static ssize_t image_size_store(struct kobject *kobj,
> > > +                             struct kobj_attribute *attr,
> > And here?
> 
> Purely long line cleanup. (>80 colunms)

Please don't do any >80 columns cleanups in any patches you want me to apply.
Seriously.  This is irritating and unuseful.

And if you don't want checkpatch to complain about that, please send a patch
to modify checkpatch accordingly.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v7 2/3] trivial: PM / Hibernate: clean up checkpatch in hibernate.c

[Rafael J. Wysocki]

On Tuesday, February 04, 2014 03:22:22 PM Sebastian Capella wrote:
> Quoting Sebastian Capella (2014-02-04 14:37:33)
> > Quoting Rafael J. Wysocki (2014-02-04 13:36:29)
> > > >  static int __init resumedelay_setup(char *str)
> > > >  {
> > > > -     resume_delay = simple_strtoul(str, NULL, 0);
> > > > +     int ret = kstrtoint(str, 0, &resume_delay);
> > > > +     /* mask must_check warn; on failure, leaves resume_delay unchanged */
> > > > +     (void)ret;
> 
> One unintended consequence of this change is that it'll now accept a
> negative integer parameter.

Well, what about using kstrtouint(), then?

> I'll rework this to have the same behavior as before.
> 
> BTW, one question, is the __must_check really needed on kstrtoint?
> Wouldn't it be acceptable to rely on kstrtoint to not update resume_delay
> if it's unable to parse an integer out of the string?  Couldn't that be
> a sufficient effect without requiring checking the return?

Well, kstrtoint() is used in some security-sensitive places AFAICS, so it
really is better to check its return value in general.  The __must_check
reminds people about that.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v7 2/3] trivial: PM / Hibernate: clean up checkpatch in hibernate.c

[Rafael J. Wysocki]

On Tuesday, February 04, 2014 04:06:42 PM Sebastian Capella wrote:
> Quoting Rafael J. Wysocki (2014-02-04 16:03:29)
> > On Tuesday, February 04, 2014 03:22:22 PM Sebastian Capella wrote:
> > > Quoting Sebastian Capella (2014-02-04 14:37:33)
> > > > Quoting Rafael J. Wysocki (2014-02-04 13:36:29)
> > > > > >  static int __init resumedelay_setup(char *str)
> > > > > >  {
> > > > > > -     resume_delay = simple_strtoul(str, NULL, 0);
> > > > > > +     int ret = kstrtoint(str, 0, &resume_delay);
> > > > > > +     /* mask must_check warn; on failure, leaves resume_delay unchanged */
> > > > > > +     (void)ret;
> > > 
> > > One unintended consequence of this change is that it'll now accept a
> > > negative integer parameter.
> > 
> > Well, what about using kstrtouint(), then?
> I was thinking of doing something like:
> 
> 	int delay, res;
> 	res = kstrtoint(str, 0, &delay);
> 	if (!res && delay >= 0)
> 		resume_delay = delay;
> 	return 1;

It uses simple_strtoul() for a reason.  You can change the type of resume_delay
to match, but the basic question is:

Why exactly do you want to change that thing?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v7 2/3] trivial: PM / Hibernate: clean up checkpatch in hibernate.c

[Rafael J. Wysocki]

On Tuesday, February 04, 2014 04:24:13 PM Sebastian Capella wrote:
> Quoting Rafael J. Wysocki (2014-02-04 16:28:13)
> > On Tuesday, February 04, 2014 04:06:42 PM Sebastian Capella wrote:
> > > Quoting Rafael J. Wysocki (2014-02-04 16:03:29)
> > > > On Tuesday, February 04, 2014 03:22:22 PM Sebastian Capella wrote:
> > > > > Quoting Sebastian Capella (2014-02-04 14:37:33)
> > > > > > Quoting Rafael J. Wysocki (2014-02-04 13:36:29)
> > > > > > > >  static int __init resumedelay_setup(char *str)
> > > > > > > >  {
> > > > > > > > -     resume_delay = simple_strtoul(str, NULL, 0);
> > > > > > > > +     int ret = kstrtoint(str, 0, &resume_delay);
> > > > > > > > +     /* mask must_check warn; on failure, leaves resume_delay unchanged */
> > > > > > > > +     (void)ret;
> > > > > 
> > > > > One unintended consequence of this change is that it'll now accept a
> > > > > negative integer parameter.
> > > > 
> > > > Well, what about using kstrtouint(), then?
> > > I was thinking of doing something like:
> > > 
> > >       int delay, res;
> > >       res = kstrtoint(str, 0, &delay);
> > >       if (!res && delay >= 0)
> > >               resume_delay = delay;
> > >       return 1;
> > 
> > It uses simple_strtoul() for a reason.  You can change the type of resume_delay
> > to match, but the basic question is:
> > 
> > Why exactly do you want to change that thing?
> 
> This entire patch is a result of a single checkpatch warning from a printk
> that I indented.
> 
> I was hoping to be helpful by removing all of the warnings from this
> file, since I was going to have a separate cleanup patch for the printk.
> 
> I can see this is not a good direction.
> 
> Would it be better also to leave the file's printks as they were and drop
> the cleanup patch completely?

Well, I had considered changing them to pr_something, but decided that it
wasn't worth the effort.  Quite frankly, I'd leave the code as is. :-)

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v7 1/3] mm: add kstrdup_trimnl function

[Andrew Morton]

On Tue,  4 Feb 2014 12:43:49 -0800 Sebastian Capella <sebastian.capella@linaro.org> wrote:

> kstrdup_trimnl creates a duplicate of the passed in
> null-terminated string.  If a trailing newline is found, it
> is removed before duplicating.  This is useful for strings
> coming from sysfs that often include trailing whitespace due to
> user input.

hm, why?  I doubt if any caller of this wants to retain leading and/or
trailing spaces and/or tabs.

Re: [PATCH v7 1/3] mm: add kstrdup_trimnl function

[Andrew Morton]

On Wed, 05 Feb 2014 14:55:52 -0800 Sebastian Capella <sebastian.capella@linaro.org> wrote:

> Quoting Andrew Morton (2014-02-05 13:50:52)
> > On Tue,  4 Feb 2014 12:43:49 -0800 Sebastian Capella <sebastian.capella@linaro.org> wrote:
> > 
> > > kstrdup_trimnl creates a duplicate of the passed in
> > > null-terminated string.  If a trailing newline is found, it
> > > is removed before duplicating.  This is useful for strings
> > > coming from sysfs that often include trailing whitespace due to
> > > user input.
> > 
> > hm, why?  I doubt if any caller of this wants to retain leading and/or
> > trailing spaces and/or tabs.
> 
> Hi Andrew,
> 
> I agree the common case doesn't usually need leading or trailing whitespace.
> 
> Pavel and others pointed out that a valid filename could contain
> newlines/whitespace at any position.

The number of cases in which we provide the kernel with a filename via
sysfs will be very very small, or zero.

If we can go through existing code and find at least a few sites which
can usefully employ kstrdup_trimnl() then fine, we have evidence.  But
I doubt if we can do that?