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

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; }