[PATCH v7 0/3] hibernation related patches

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

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 <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Rik van Riel <redacted> (commit_signer:5/10=50%)
Cc: Michel Lespinasse <redacted>
Cc: Shaohua Li <shli@kernel.org>
Cc: Jerome Marchand <redacted>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: Joonsoo Kim <redacted>
Cc: Joe Perches <joe@perches.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Alexey Dobriyan <redacted>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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 <redacted>
Cc: Pavel Machek <redacted>
Cc: Len Brown <redacted>
Cc: "Rafael J. Wysocki" <redacted>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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 <redacted>
Cc: Pavel Machek <redacted>
Cc: Len Brown <redacted>
Cc: "Rafael J. Wysocki" <redacted>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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.

[]

[]

What good is changing this from 10 to 20?

here too.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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.

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

Same here.

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?

And here?

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

And here?

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Tuesday, February 04, 2014 12:43:51 PM Sebastian Capella wrote:

What about "if (!name)"?

What about "if (res)"?

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Quoting Joe Perches (2014-02-04 13:21:02)

On Tue, 2014-02-04 at 12:43 -0800, Sebastian Capella wrote:

quoted

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

[]

quoted

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c

[]

quoted

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

quoted

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

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.

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

WARNING: msleep < 20ms can sleep for up to 20ms; see
Documentation/timers/timers-howto.txt
+  msleep(10);

From Documentation/timers/timers-howto.txt

SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):                       
  * Use usleep_range                                               

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

Thanks,

Sebastian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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:

quoted

+     while (1)
+             ;

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

quoted

+     while (1)
+             ;

Same here.

Will do, thanks!

quoted

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

quoted

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

quoted

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

quoted

 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?

I'll include this and the msleep as a separate, non-trivial checkpatch
cleanup patch if the changes remain after this discussion.

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

Better thanks!

Sebastian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Quoting Rafael J. Wysocki (2014-02-04 13:39:43)

On Tuesday, February 04, 2014 12:43:51 PM Sebastian Capella wrote:

quoted

+     if (name == NULL)

What about "if (!name)"?

quoted

+     if (res != 0) {

What about "if (res)"?

Changed, thanks!

Sebastian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Quoting Sebastian Capella (2014-02-04 14:37:33)

Quoting Rafael J. Wysocki (2014-02-04 13:36:29)

quoted

quoted

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

Thanks,

Sebastian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Tuesday, February 04, 2014 02:37:33 PM Sebastian Capella wrote:

Quoting Rafael J. Wysocki (2014-02-04 13:36:29)

quoted

Well, this isn't a trivial patch.

I'll remove the trivial, thanks!

Quoting Rafael J. Wysocki (2014-02-04 13:36:29)

quoted

On Tuesday, February 04, 2014 12:43:50 PM Sebastian Capella wrote:

quoted

+     while (1)
+             ;

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

quoted

+     while (1)
+             ;

Same here.

Will do, thanks!

quoted

quoted

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

quoted

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.

quoted

quoted

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

quoted

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

On Tue, 2014-02-04 at 14:05 -0800, Sebastian Capella wrote:

Quoting Joe Perches (2014-02-04 13:21:02)

quoted

On Tue, 2014-02-04 at 12:43 -0800, Sebastian Capella wrote:

quoted

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

[]

quoted

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c

[]

quoted

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

quoted

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Tuesday, February 04, 2014 03:22:22 PM Sebastian Capella wrote:

Quoting Sebastian Capella (2014-02-04 14:37:33)

quoted

Quoting Rafael J. Wysocki (2014-02-04 13:36:29)

quoted

quoted

 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Quoting Rafael J. Wysocki (2014-02-04 16:03:29)

On Tuesday, February 04, 2014 03:22:22 PM Sebastian Capella wrote:

quoted

Quoting Sebastian Capella (2014-02-04 14:37:33)

quoted

Quoting Rafael J. Wysocki (2014-02-04 13:36:29)

quoted

quoted

 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;

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!

Sebastian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Tuesday, February 04, 2014 04:06:42 PM Sebastian Capella wrote:

Quoting Rafael J. Wysocki (2014-02-04 16:03:29)

quoted

On Tuesday, February 04, 2014 03:22:22 PM Sebastian Capella wrote:

quoted

Quoting Sebastian Capella (2014-02-04 14:37:33)

quoted

Quoting Rafael J. Wysocki (2014-02-04 13:36:29)

quoted

quoted

 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Quoting Rafael J. Wysocki (2014-02-04 16:28:13)

On Tuesday, February 04, 2014 04:06:42 PM Sebastian Capella wrote:

quoted

Quoting Rafael J. Wysocki (2014-02-04 16:03:29)

quoted

On Tuesday, February 04, 2014 03:22:22 PM Sebastian Capella wrote:

quoted

Quoting Sebastian Capella (2014-02-04 14:37:33)

quoted

Quoting Rafael J. Wysocki (2014-02-04 13:36:29)

quoted

quoted

 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?

Thanks,

Sebastian


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Tuesday, February 04, 2014 04:24:13 PM Sebastian Capella wrote:

Quoting Rafael J. Wysocki (2014-02-04 16:28:13)

quoted

On Tuesday, February 04, 2014 04:06:42 PM Sebastian Capella wrote:

quoted

Quoting Rafael J. Wysocki (2014-02-04 16:03:29)

quoted

On Tuesday, February 04, 2014 03:22:22 PM Sebastian Capella wrote:

quoted

Quoting Sebastian Capella (2014-02-04 14:37:33)

quoted

Quoting Rafael J. Wysocki (2014-02-04 13:36:29)

quoted

quoted

 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.

On Tue,  4 Feb 2014 12:43:49 -0800 Sebastian Capella [off-list ref] 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.

Quoting Andrew Morton (2014-02-05 13:50:52)

On Tue,  4 Feb 2014 12:43:49 -0800 Sebastian Capella [off-list ref] wrote:

quoted

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.

If we allow for this, then it would be incorrect to strip whitespace
from the input.  Comments also went down the lines that it would be
better for the kernel not to second guess what is being passed in.

I find stripping the trailing newline to be very useful, and there are
many examples of kernel code doing this.  I think it would be a mistake
to remove this now, and would be confusing for users.  A compromise
is to strip the final newline only if it's present before the null.

This allows the common case of echoing a simple string onto
/sys/power/resume, and behaves as expected in that case.

A complex string without a trailing newline is also handled by quoting or
dding a file onto /sys/power/resume.

In the unlikely event a user has trailing newline as part of the input, then
adding an additional newline to the end will cover that case.  This
is not ideal, but it puts the additional burden onto the complex case
rather than the common case.

Thanks,

Sebastian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Wed, 05 Feb 2014 14:55:52 -0800 Sebastian Capella [off-list ref] wrote:

Quoting Andrew Morton (2014-02-05 13:50:52)

quoted

On Tue,  4 Feb 2014 12:43:49 -0800 Sebastian Capella [off-list ref] wrote:

quoted

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?

Quoting Andrew Morton (2014-02-05 15:01:01)

On Wed, 05 Feb 2014 14:55:52 -0800 Sebastian Capella [off-list ref] wrote:

quoted

Quoting Andrew Morton (2014-02-05 13:50:52)

quoted

On Tue,  4 Feb 2014 12:43:49 -0800 Sebastian Capella [off-list ref] wrote:

quoted

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?

Hi Andrew,

I went through all of the store functions I could find and, though I
found a lot of examples handling \n, I found no other examples
specifically parsing filenames.  Most deal with integers.  Those parsing
commands often use sysfs_streq or otherwise are doing some custom
behavior that wouldn't suit a utility function.

For my purposes, it looks like v2 of the patch seems like the best
starting point based on all of the feedback I've received.  So I'm
moving back to a custom solution for parsing this input.  Unless
someone objects or has comments, I'll post something like the
function below.

static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr,
			    const char *buf, size_t n)
{
	dev_t res;
	int len = n;
	char *name;

	if (len && buf[len-1] == '\n')
		len--;
	name = kstrndup(buf, len, GFP_KERNEL);
	if (!name)
		return -ENOMEM;

	res = name_to_dev_t(name);
	kfree(name);
	if (!res)
		return -EINVAL;

	lock_system_sleep();
	swsusp_resume_device = res;
	unlock_system_sleep();
	printk(KERN_INFO "PM: Starting manual resume from disk\n");
	noresume = 0;
	software_resume();
	return n;
}


Thanks,

Sebastian

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>