Re: [PATCH v2 1/3] power: validate wakeup source before activating it.

[Jin Qian]

I have to break the line since it's over 80 characters : ) I'll follow
CodingStyle and send a new patch.

Thanks,
jin

On Wed, May 6, 2015 at 3:39 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> On Wednesday, May 06, 2015 03:11:48 PM Jin Qian wrote:
>> Hi Rafael,
>>
>> I kept braces since checkpatch.pl didn't report error or warning. The
>> if condition has two lines, a brace make the statement more clear I
>> think.
>
> Well, you don't have to break the "if" line. :-)
>
> As per CodingStyle, the braces should not be there.
>
>
>> I removed the format string since ws->name can be random strings if
>> the wakeup_source is not registered (i.e. using prepare functions).
>> It's not very useful to print name if we have stack dump from
>> WARN_ONCE.
>
> OK
>
>
>> On Wed, May 6, 2015 at 3:00 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>> > On Wednesday, May 06, 2015 11:51:07 AM Jin Qian wrote:
>> >> A rogue wakeup source not registered in wakeup_sources list is not visible
>> >> from wakeup_sources_stats_show. Check if the wakeup source is registered
>> >> properly by looking at the timer struct.
>> >>
>> >> Signed-off-by: Jin Qian <jinqian@xxxxxxxxxxx>
>> >> ---
>> >>  drivers/base/power/wakeup.c | 19 +++++++++++++++++++
>> >>  1 file changed, 19 insertions(+)
>> >>
>> >> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> >> index 7726200..aead738 100644
>> >> --- a/drivers/base/power/wakeup.c
>> >> +++ b/drivers/base/power/wakeup.c
>> >> @@ -351,6 +351,20 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
>> >>  }
>> >>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>> >>
>> >> +/**
>> >> + * wakeup_source_not_registered - validate the given wakeup source.
>> >> + * @ws: Wakeup source to be validated.
>> >> + */
>> >> +static bool wakeup_source_not_registered(struct wakeup_source *ws)
>> >> +{
>> >> +     /*
>> >> +      * Use timer struct to check if the given source is initialized
>> >> +      * by wakeup_source_add.
>> >> +      */
>> >> +     return ws->timer.function != pm_wakeup_timer_fn ||
>> >> +                ws->timer.data != (unsigned long)ws;
>> >> +}
>> >> +
>> >>  /*
>> >>   * The functions below use the observation that each wakeup event starts a
>> >>   * period in which the system should not be suspended.  The moment this period
>> >> @@ -391,6 +405,11 @@ static void wakeup_source_activate(struct wakeup_source *ws)
>> >>  {
>> >>       unsigned int cec;
>> >>
>> >> +     if (WARN_ONCE(wakeup_source_not_registered(ws),
>> >> +                     "unregistered wakeup source\n")) {
>> >> +             return;
>> >> +     }
>> >
>> > The braces are not necessary and you can use format strings with WARN_ONCE()
>> > too IIRC.
>> >
>> >> +
>> >>       /*
>> >>        * active wakeup source should bring the system
>> >>        * out of PM_SUSPEND_FREEZE state
>> >>
>> >
>> > --
>> > 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 majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
> --
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/