Re: [PATCH 2/3] PM / Sleep: Make enter_state() in kernel/power/suspend.c static

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday, February 13, 2012, Srivatsa S. Bhat wrote:
> On 02/13/2012 08:42 PM, Rafael J. Wysocki wrote:
> 
> > On Monday, February 13, 2012, Srivatsa S. Bhat wrote:
> >> On 02/13/2012 02:54 AM, Rafael J. Wysocki wrote:
> >>
> >>> On Sunday, February 12, 2012, Srivatsa S. Bhat wrote:
> >>>> On 02/12/2012 04:34 AM, Rafael J. Wysocki wrote:
> >>>>
> >>>>> From: Rafael J. Wysocki <rjw@sisk.pl>
> >>>>>
> >>>>> The enter_state() function in kernel/power/suspend.c should be
> >>>>> static and state_store() in kernel/power/suspend.c should call
> >>>>> pm_suspend() instead of it, so make that happen (which also reduces
> >>>>> code duplication related to suspend statistics).
> >>>>>
> >>>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >>>>> ---
> >>>>>  kernel/power/main.c    |    6 +-----
> >>>>>  kernel/power/power.h   |    2 --
> >>>>>  kernel/power/suspend.c |    2 +-
> >>>>>  3 files changed, 2 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> Index: linux/kernel/power/main.c
> >>>>> ===================================================================
> >>>>> --- linux.orig/kernel/power/main.c
> >>>>> +++ linux/kernel/power/main.c
> >>>>> @@ -292,11 +292,7 @@ static ssize_t state_store(struct kobjec
> >>>>>  #ifdef CONFIG_SUSPEND
> >>>>>  	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
> >>>>>  		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
> >>>>> -			break;
> >>>>> -	}
> >>>>> -	if (state < PM_SUSPEND_MAX && *s) {
> >>>>> -		error = enter_state(state);
> >>>>> -		suspend_stats_update(error);
> >>>>> +			error = pm_suspend(state);
> >>>>
> >>>>
> >>>> This code will not stop after calling pm_suspend(). It will try more iterations
> >>>> in the for loop right?
> >>>
> >>> Well, only one string in pm_states[] can be matched at a time, but I agree that
> >>> it doesn't make sense to continue the loop after we've found a match.
> >>>
> >>>> We can probably keep the 'for' loop as it is, and just replace the 'if' block
> >>>> following the 'for' loop by: error = pm_suspend(state);
> >>>
> >>> I think the patch below is correct.
> >>>
> >>> Thanks,
> >>> Rafael
> >>>
> >>> ---
> >>> From: Rafael J. Wysocki <rjw@sisk.pl>
> >>> Subject: PM / Sleep: Make enter_state() in kernel/power/suspend.c static
> >>>
> >>> The enter_state() function in kernel/power/suspend.c should be
> >>> static and state_store() in kernel/power/suspend.c should call
> >>> pm_suspend() instead of it, so make that happen (which also reduces
> >>> code duplication related to suspend statistics).
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >>> ---
> >>
> >>
> >> Yeah, this version of the patch will work fine.
> >>
> >> Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> > 
> > Thanks!
> > 
> > I wonder if that should be Reviewed-by, tough?  You evidently have reviewed
> > the patch (actually, all three of them).
> > 
> 
> 
> Anything is fine :-) It is not very clear to me when to use Reviewed-by and
> when to use Acked-by.. so I randomly chose one of them.. :-)
> But please enlighten me as to when to use which one, so that in the future, I
> can use the right one :-)

"Acked-by" means, roughly, "I have no objection" or "looks good to me",
depending on who's saying that, but it doesn't imply that you've had more than
a cursory look at the patch in question.  "Reviewied-by", in contrast, means
something like "I have reviewed the patch in detail and haven't found anything
wrong in it" (which obviously means that you have no objection too, because
otherwise you'd have found _something_ wrong in the patch).

So, "Acked-by" from anyone other than the relevant maintainer is just an
"I'm for" declaration, while "Reviewied-by" from _anyone_ carries some actual
weight. 

Thanks,
Rafael