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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
