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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Rik van Riel <[email protected]> (commit_signer:5/10=50%)
Cc: Michel Lespinasse <[email protected]>
Cc: Shaohua Li <[email protected]>
Cc: Jerome Marchand <[email protected]>
Cc: Mikulas Patocka <[email protected]>
Cc: Joonsoo Kim <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Alexey Dobriyan <[email protected]>
---
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
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 <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Len Brown <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
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
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 <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Len Brown <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
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
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.
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 <[email protected]>
> Cc: Pavel Machek <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> 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.
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 <[email protected]>
> Cc: Pavel Machek <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> 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.
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.
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 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.
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.
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.
On Tue, 4 Feb 2014 12:43:49 -0800 Sebastian Capella <[email protected]> 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.
On Wed, 05 Feb 2014 14:55:52 -0800 Sebastian Capella <[email protected]> wrote:
> Quoting Andrew Morton (2014-02-05 13:50:52)
> > On Tue, 4 Feb 2014 12:43:49 -0800 Sebastian Capella <[email protected]> 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?