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 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
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.
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.
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
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?
Quoting Andrew Morton (2014-02-05 15:01:01)
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?
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
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; }
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.
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.
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
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.
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");@@ -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");@@ -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);@@ -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");@@ -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");#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);@@ -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; }
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.
-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)
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
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. 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
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!
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;
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
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?
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?
Thanks,
Sebastian
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!
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!
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);
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);
Quoting Rafael J. Wysocki (2014-02-04 13:39:43)
On Tuesday, February 04, 2014 12:43:51 PM Sebastian Capella wrote:
if (name == NULL)What about "if (!name)"?
if (res != 0) {What about "if (res)"?
Changed, thanks!
Sebastian
linaro-kernel@lists.linaro.org
-
Andrew Morton -
Joe Perches -
Rafael J. Wysocki -
Sebastian Capella