[v2,3/3] time: clocksource: Add a comment to CLOCK_SOURCE_SUSPEND_NONSTOP

When doing timekeeping_resume(), if the nonstop clocksource
wraps back, "cycle_delta" will miss the wrap time.

It's hard to determine the right CLOCKSOURCE_MASK(xxx) or
something to add code for inspecting such behavior, and we
don't have many existent nonstop clocksources, so just add
a comment to indicate that if have this flag set, people
are aware that this nonstop clocksource won't wrap back
during system suspend/resume.

Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
 include/linux/clocksource.h | 5 +++++
 1 file changed, 5 insertions(+)

On Thu, 22 Jan 2015, Xunlei Pang wrote:
> When doing timekeeping_resume(), if the nonstop clocksource
> wraps back, "cycle_delta" will miss the wrap time.
> 
> It's hard to determine the right CLOCKSOURCE_MASK(xxx) or
> something to add code for inspecting such behavior, and we
> don't have many existent nonstop clocksources, so just add
> a comment to indicate that if have this flag set, people
> are aware that this nonstop clocksource won't wrap back
> during system suspend/resume.

-ENOPARSE

What has the CLOCKSOURCE_MASK() to do with this and why is the fact
relevant, that we only have a few suspend_nonstop clock sources?

> +
> +/*
> + * When setting this flag, you're also supposed to mean that it doesn't
> + * wrap back during system suspend/resume. See timekeeping_resume().

-ENOPARSE

I guess what you want to say here is:

/*
 * clocksource continues to run during suspend and is guaranteed not to
 * wrap around during long suspend periods.
 */

> + */
>  #define CLOCK_SOURCE_SUSPEND_NONSTOP		0x80

Thanks,

	tglx

Hi Thomas,

On 23 January 2015 at 05:07, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 22 Jan 2015, Xunlei Pang wrote:
>> When doing timekeeping_resume(), if the nonstop clocksource
>> wraps back, "cycle_delta" will miss the wrap time.
>>
>> It's hard to determine the right CLOCKSOURCE_MASK(xxx) or
>> something to add code for inspecting such behavior, and we
>> don't have many existent nonstop clocksources, so just add
>> a comment to indicate that if have this flag set, people
>> are aware that this nonstop clocksource won't wrap back
>> during system suspend/resume.
>
> -ENOPARSE
>
> What has the CLOCKSOURCE_MASK() to do with this and why is the fact
> relevant, that we only have a few suspend_nonstop clock sources?

Before this, I tried to add some code to catch such problem at the
time of registering
the clocksource, like using the CLOCKSOURCE_MASK(), for example 64bit counter
will never wrap for us. But there may be other values like CLOCKSOURCE_MASK(56),
I just can't figure out exactly how to do this judge.

So, I think we can only add a comment to let the developer be aware of
this when registering
nonstop clocksource, that's what I want to express.

>
>> +
>> +/*
>> + * When setting this flag, you're also supposed to mean that it doesn't
>> + * wrap back during system suspend/resume. See timekeeping_resume().
>
> -ENOPARSE
>
> I guess what you want to say here is:
>
> /*
>  * clocksource continues to run during suspend and is guaranteed not to
>  * wrap around during long suspend periods.
>  */
>

Yes, this description is way better :-)

Thanks,
Xunlei

On Sat, 24 Jan 2015, Xunlei Pang wrote:

> Before this, I tried to add some code to catch such problem at the
> time of registering the clocksource, like using the
> CLOCKSOURCE_MASK(), for example 64bit counter will never wrap for
> us. But there may be other values like CLOCKSOURCE_MASK(56), I just
> can't figure out exactly how to do this judge.

I don't think there is a good way to do so. Registration time is the
wrong place anyway because the problem depends on:

 - The width of the counter
 - The frequency of the counter

The frequency of the counter might even change after registration. Now
add the unknown duration of the suspend to the picture and you're
completely lost.

All we can do is provide information about the actual wraparound time,
if the CLOCK_SOURCE_SUSPEND_NONSTOP flag is set and the wraparound
time is less than some reasonable margin.

Thanks,

	tglx

Hi Thomas,

On 25 January 2015 at 01:07, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sat, 24 Jan 2015, Xunlei Pang wrote:
>
>> Before this, I tried to add some code to catch such problem at the
>> time of registering the clocksource, like using the
>> CLOCKSOURCE_MASK(), for example 64bit counter will never wrap for
>> us. But there may be other values like CLOCKSOURCE_MASK(56), I just
>> can't figure out exactly how to do this judge.
>
> I don't think there is a good way to do so. Registration time is the
> wrong place anyway because the problem depends on:
>
>  - The width of the counter
>  - The frequency of the counter
>
> The frequency of the counter might even change after registration. Now
> add the unknown duration of the suspend to the picture and you're
> completely lost.
>
> All we can do is provide information about the actual wraparound time,
> if the CLOCK_SOURCE_SUSPEND_NONSTOP flag is set and the wraparound
> time is less than some reasonable margin.
>

Yes, we can only deal with it approximately. How about this?

1) Add a new member about reference wraparound time(max system suspend
period allowed) to struct  clocksource. In
__clocksource_updatefreq_scale(), we can use  "sec" which already
applys 12.5% margin as its value.

2) Add a new tuneable sysctl threshold with a default time period
value(for example, 365 days)
    We can also printk its value when booting or changing its value to
notice people about this.

3) then,  in timekeeping_resume(), we can compare the reference
wraparound of the nonstop clocksource with the sysctl threshold to
decide if it is dependable to use.

Thanks,
Xunlei

>
>
>
>