| Message ID | 1391546631-7715-3-git-send-email-sebastian.capella@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, February 04, 2014 12:43:50 PM Sebastian Capella wrote: > Checkpatch reports several warnings in hibernate.c > printk use removed, long lines wrapped, whitespace cleanup, > extend short msleeps, while loops on two lines. Well, this isn't a trivial patch. > Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Len Brown <len.brown@intel.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > --- > kernel/power/hibernate.c | 62 ++++++++++++++++++++++++---------------------- > 1 file changed, 32 insertions(+), 30 deletions(-) > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c > index 0121dab..cd1e30c 100644 > --- a/kernel/power/hibernate.c > +++ b/kernel/power/hibernate.c > @@ -94,7 +94,7 @@ EXPORT_SYMBOL(system_entering_hibernation); > #ifdef CONFIG_PM_DEBUG > static void hibernation_debug_sleep(void) > { > - printk(KERN_INFO "hibernation debug: Waiting for 5 seconds.\n"); > + pr_info("hibernation debug: Waiting for 5 seconds.\n"); > mdelay(5000); > } > > @@ -239,7 +239,7 @@ void swsusp_show_speed(struct timeval *start, struct timeval *stop, > centisecs = 1; /* avoid div-by-zero */ > k = nr_pages * (PAGE_SIZE / 1024); > kps = (k * 100) / centisecs; > - printk(KERN_INFO "PM: %s %d kbytes in %d.%02d seconds (%d.%02d MB/s)\n", > + pr_info("PM: %s %d kbytes in %d.%02d seconds (%d.%02d MB/s)\n", > msg, k, > centisecs / 100, centisecs % 100, > kps / 1000, (kps % 1000) / 10); > @@ -260,8 +260,7 @@ static int create_image(int platform_mode) > > error = dpm_suspend_end(PMSG_FREEZE); > if (error) { > - printk(KERN_ERR "PM: Some devices failed to power down, " > - "aborting hibernation\n"); > + pr_err("PM: Some devices failed to power down, aborting hibernation\n"); > return error; > } > > @@ -277,8 +276,7 @@ static int create_image(int platform_mode) > > error = syscore_suspend(); > if (error) { > - printk(KERN_ERR "PM: Some system devices failed to power down, " > - "aborting hibernation\n"); > + pr_err("PM: Some system devices failed to power down, aborting hibernation\n"); > goto Enable_irqs; > } > > @@ -289,8 +287,7 @@ static int create_image(int platform_mode) > save_processor_state(); > error = swsusp_arch_suspend(); > if (error) > - printk(KERN_ERR "PM: Error %d creating hibernation image\n", > - error); > + pr_err("PM: Error %d creating hibernation image\n", error); > /* Restore control flow magically appears here */ > restore_processor_state(); > if (!in_suspend) { > @@ -413,8 +410,7 @@ static int resume_target_kernel(bool platform_mode) > > error = dpm_suspend_end(PMSG_QUIESCE); > if (error) { > - printk(KERN_ERR "PM: Some devices failed to power down, " > - "aborting resume\n"); > + pr_err("PM: Some devices failed to power down, aborting resume\n"); > return error; > } > > @@ -550,7 +546,8 @@ int hibernation_platform_enter(void) > > hibernation_ops->enter(); > /* We should never get here */ > - while (1); > + while (1) > + ; Please remove this change from the patch. I don't care about checkpatch complaining here. > > Power_up: > syscore_resume(); > @@ -611,8 +608,7 @@ static void power_down(void) > */ > error = swsusp_unmark(); > if (error) > - printk(KERN_ERR "PM: Swap will be unusable! " > - "Try swapon -a.\n"); > + pr_err("PM: Swap will be unusable! Try swapon -a.\n"); > return; > #endif > } > @@ -621,8 +617,9 @@ static void power_down(void) > * Valid image is on the disk, if we continue we risk serious data > * corruption after resume. > */ > - printk(KERN_CRIT "PM: Please power down manually\n"); > - while(1); > + pr_crit("PM: Please power down manually\n"); > + while (1) > + ; Same here. > } > > /** > @@ -644,9 +641,9 @@ int hibernate(void) > if (error) > goto Exit; > > - printk(KERN_INFO "PM: Syncing filesystems ... "); > + pr_info("PM: Syncing filesystems ... "); > sys_sync(); > - printk("done.\n"); > + pr_cont("done.\n"); > > error = freeze_processes(); > if (error) > @@ -670,7 +667,7 @@ int hibernate(void) > if (nocompress) > flags |= SF_NOCOMPRESS_MODE; > else > - flags |= SF_CRC32_MODE; > + flags |= SF_CRC32_MODE; > > pr_debug("PM: writing image.\n"); > error = swsusp_write(flags); > @@ -750,7 +747,7 @@ static int software_resume(void) > pr_debug("PM: Checking hibernation image partition %s\n", resume_file); > > if (resume_delay) { > - printk(KERN_INFO "Waiting %dsec before reading resume device...\n", > + pr_info("Waiting %dsec before reading resume device...\n", > resume_delay); > ssleep(resume_delay); > } > @@ -765,7 +762,7 @@ static int software_resume(void) > if (isdigit(resume_file[0]) && resume_wait) { > int partno; > while (!get_gendisk(swsusp_resume_device, &partno)) > - msleep(10); > + msleep(20); That's the reason why it is not trivial. First, the change being made doesn't belong in this patch. Second, what's the problem with the original value? > } > > if (!swsusp_resume_device) { > @@ -776,8 +773,9 @@ static int software_resume(void) > wait_for_device_probe(); > > if (resume_wait) { > - while ((swsusp_resume_device = name_to_dev_t(resume_file)) == 0) > - msleep(10); > + while ((swsusp_resume_device = > + name_to_dev_t(resume_file)) == 0) > + msleep(20); And here? > async_synchronize_full(); > } > > @@ -826,7 +824,7 @@ static int software_resume(void) > if (!error) > hibernation_restore(flags & SF_PLATFORM_MODE); > > - printk(KERN_ERR "PM: Failed to load hibernation image, recovering.\n"); > + pr_err("PM: Failed to load hibernation image, recovering.\n"); > swsusp_free(); > free_basic_memory_bitmaps(); > Thaw: > @@ -965,7 +963,7 @@ power_attr(disk); > static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute *attr, > char *buf) > { > - return sprintf(buf,"%d:%d\n", MAJOR(swsusp_resume_device), > + return sprintf(buf, "%d:%d\n", MAJOR(swsusp_resume_device), > MINOR(swsusp_resume_device)); > } > > @@ -986,7 +984,7 @@ static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr, > lock_system_sleep(); > swsusp_resume_device = res; > unlock_system_sleep(); > - printk(KERN_INFO "PM: Starting manual resume from disk\n"); > + pr_info("PM: Starting manual resume from disk\n"); > noresume = 0; > software_resume(); > ret = n; > @@ -996,13 +994,15 @@ static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr, > > power_attr(resume); > > -static ssize_t image_size_show(struct kobject *kobj, struct kobj_attribute *attr, > +static ssize_t image_size_show(struct kobject *kobj, > + struct kobj_attribute *attr, > char *buf) Why can't you leave the code as is here? > { > return sprintf(buf, "%lu\n", image_size); > } > > -static ssize_t image_size_store(struct kobject *kobj, struct kobj_attribute *attr, > +static ssize_t image_size_store(struct kobject *kobj, > + struct kobj_attribute *attr, > const char *buf, size_t n) And here? > { > unsigned long size; > @@ -1039,7 +1039,7 @@ static ssize_t reserved_size_store(struct kobject *kobj, > > power_attr(reserved_size); > > -static struct attribute * g[] = { > +static struct attribute *g[] = { > &disk_attr.attr, > &resume_attr.attr, > &image_size_attr.attr, > @@ -1066,7 +1066,7 @@ static int __init resume_setup(char *str) > if (noresume) > return 1; > > - strncpy( resume_file, str, 255 ); > + strncpy(resume_file, str, 255); > return 1; > } > > @@ -1106,7 +1106,9 @@ static int __init resumewait_setup(char *str) > > static int __init resumedelay_setup(char *str) > { > - resume_delay = simple_strtoul(str, NULL, 0); > + int ret = kstrtoint(str, 0, &resume_delay); > + /* mask must_check warn; on failure, leaves resume_delay unchanged */ > + (void)ret; And that's not a trivial change surely? And why didn't you do (void)kstrtoint(str, 0, &resume_delay); instead? > return 1; > } > >
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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!
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; }
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(-)