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

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(-)

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

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

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

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