Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable

[John Stultz <johnstul@us.ibm.com>]

On 11/18/2012 12:09 PM, Kees Cook wrote:
> On Fri, Nov 16, 2012 at 7:16 PM, John Stultz <johnstul@us.ibm.com> wrote:
>> Yea, I wanted to revisit this, because it is an odd case.
>>
>> We don't want to call getnstimeofday() while the timekeeping code is
>> suspended, since the clocksource cycle_last value may be invalid if the
>> hardware was reset during suspend.  Kees is correct,  the WARN_ONs were
>> there to make sure no one tries to use the timekeeping core before its
>> resumed, so removing them is problematic.
>>
>> Your sugggestion of having the __do_gettimeofday() internal accessor that
>> maybe returns an error if timekeeping has been suspended could work.
>>
>> The other possibility is depending on the needs for accuracy with the
>> timestamp, current_kernel_time() might be a better interface to use, since
>> it will return the time at the last tick, and doesn't require accessing the
>> clocksource hardware.  Might that be a simpler solution? Or is sub-tick
>> granularity necessary?
> I think it's only useful to have this to the same granularity as
> sched_clock(), so things can be correlated to dmesg output. If it's
> the same, I'd be fine to switch to using current_kernel_time().
Oof.  That's another can of worms,   sched_clock() resolution isn't tied 
to getnstimeofday(), since you may have cases where the TSC is invalid 
for timekeeping (so we use something slow like the ACPI PM) but ok for 
scheduling, etc.

Even so, its current_kernel_time() is just tick granularity, so that 
probably won't work for you.

So I'm leaning towards Anton's suggestion of adding a new internal 
accessor that returns an error if we're suspended.

Thomas, what do you think of something like what's below?

thanks
-john


diff --git a/include/linux/time.h b/include/linux/time.h
index 4d358e9..0015aea 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -158,6 +158,7 @@ extern int do_setitimer(int which, struct itimerval *value,
  			struct itimerval *ovalue);
  extern unsigned int alarm_setitimer(unsigned int seconds);
  extern int do_getitimer(int which, struct itimerval *value);
+extern int __getnstimeofday(struct timespec *tv);
  extern void getnstimeofday(struct timespec *tv);
  extern void getrawmonotonic(struct timespec *ts);
  extern void getnstime_raw_and_real(struct timespec *ts_raw,
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index e424970..bb2638c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -220,19 +220,20 @@ static void timekeeping_forward_now(struct timekeeper *tk)
  }
  
  /**
- * getnstimeofday - Returns the time of day in a timespec
+ * __getnstimeofday - Returns the time of day in a timespec
   * @ts:		pointer to the timespec to be set
   *
- * Returns the time of day in a timespec.
+ * Returns -1 if timekeeping is suspended.
+ * Returns 0 on success.
   */
-void getnstimeofday(struct timespec *ts)
+int __getnstimeofday(struct timespec *ts)
  {
  	struct timekeeper *tk = &timekeeper;
  	unsigned long seq;
  	s64 nsecs = 0;
  
-	WARN_ON(timekeeping_suspended);
-
+	if (unlikely(timekeeping_suspended));
+		return -1;
  	do {
  		seq = read_seqbegin(&tk->lock);
  
@@ -243,6 +244,19 @@ void getnstimeofday(struct timespec *ts)
  
  	ts->tv_nsec = 0;
  	timespec_add_ns(ts, nsecs);
+	return 0;
+}
+EXPORT_SYMBOL(__getnstimeofday);
+
+/**
+ * getnstimeofday - Returns the time of day in a timespec
+ * @ts:		pointer to the timespec to be set
+ *
+ * Returns the time of day in a timespec.
+ */
+void getnstimeofday(struct timespec *ts)
+{
+	WARN_ON(__getnstimeofday(ts));
  }
  EXPORT_SYMBOL(getnstimeofday);