[GIT PULL][PATCH 0/4] Timekeeping items for 4.10

[GIT PULL][PATCH 0/4] Timekeeping items for 4.10

[John Stultz]

Hey Thomas, Ingo,
  Just a few small patches I have queued for 4.10.

Please let me know if you have any objections.

You can grab the patches via git pull as specified below.

thanks
-john

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>


The following changes since commit a909d3e636995ba7c349e2ca5dbb528154d4ac30:

  Linux 4.9-rc3 (2016-10-29 13:52:02 -0700)

are available in the git repository at:

  https://git.linaro.org/people/john.stultz/linux.git fortglx/4.10/time

for you to fetch changes up to 61aa18b038bf53be5b67ece7db86ad78be0716a4:

  timekeeping: clocksource_cyc2ns: Document intended range limitation (2016-11-18 20:39:49 -0800)

----------------------------------------------------------------
Baolin Wang (1):
  time: alarmtimer: Add the trcepoints for alarmtimer

Chen Yu (1):
  timekeeping: Ignore the bogus sleep time if pm_trace is enabled

Chris Metcalf (1):
  timekeeping: clocksource_cyc2ns: Document intended range limitation

Colin Ian King (1):
  selftests/timers: Fix spelling mistake "Asyncrhonous" ->
    "Asynchronous"

 arch/x86/kernel/rtc.c                             |  9 +++
 drivers/base/power/trace.c                        | 27 +++++++
 drivers/rtc/rtc-cmos.c                            |  7 ++
 include/linux/clocksource.h                       |  5 +-
 include/linux/mc146818rtc.h                       |  1 +
 include/linux/pm-trace.h                          |  9 ++-
 include/trace/events/alarmtimer.h                 | 92 +++++++++++++++++++++++
 kernel/time/alarmtimer.c                          | 16 +++-
 tools/testing/selftests/timers/skew_consistency.c |  2 +-
 9 files changed, 163 insertions(+), 5 deletions(-)
 create mode 100644 include/trace/events/alarmtimer.h

-- 
2.7.4

[PATCH 1/4] time: alarmtimer: Add the trcepoints for alarmtimer

[John Stultz]

From: Baolin Wang <baolin.wang@linaro.org>

For system debugging, we sometimes want to know who sets one
alarm timer, the time of the timer, when the timer started and
fired and so on. Thus adding tracepoints can help us trace the
alarmtimer information.

For example, when we debug the system supend/resume, if the
system is always resumed by RTC alarm, we can find out which
process set the alarm timer to resume system by below trace log:

......

Binder:3292_2-3304  [000] d..2   149.981123: alarmtimer_cancel:
alarmtimer:ffffffc1319a7800 type:REALTIME
expires:1325463120000000000 now:1325376810370370245

Binder:3292_2-3304  [000] d..2   149.981136: alarmtimer_start:
alarmtimer:ffffffc1319a7800 type:REALTIME
expires:1325376840000000000 now:1325376810370384591

Binder:3292_9-3953  [000] d..2   150.212991: alarmtimer_cancel:
alarmtimer:ffffffc1319a5a00 type:BOOTTIME
expires:179552000000 now:150154008122

Binder:3292_9-3953  [000] d..2   150.213006: alarmtimer_start:
alarmtimer:ffffffc1319a5a00 type:BOOTTIME
expires:179551000000 now:150154025622

......

system_server-3000  [002] ...1  162.701940: alarmtimer_suspend:
alarmtimer type:REALTIME expires:1325376839802714584

......

>From the trace log, we can find out the 'Binder:3292_2' process
set one alarm timer which resumes the system.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/trace/events/alarmtimer.h | 92 +++++++++++++++++++++++++++++++++++++++
 kernel/time/alarmtimer.c          | 16 ++++++-
 2 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100644 include/trace/events/alarmtimer.h

diff --git a/include/trace/events/alarmtimer.h b/include/trace/events/alarmtimer.h
new file mode 100644
index 0000000..61ea556
--- /dev/null
+++ b/include/trace/events/alarmtimer.h
@@ -0,0 +1,92 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM alarmtimer
+
+#if !defined(_TRACE_ALARMTIMER_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_ALARMTIMER_H
+
+#include <linux/alarmtimer.h>
+#include <linux/rtc.h>
+#include <linux/tracepoint.h>
+
+TRACE_DEFINE_ENUM(ALARM_REALTIME);
+TRACE_DEFINE_ENUM(ALARM_BOOTTIME);
+
+#define show_alarm_type(type)	__print_flags(type, " | ",	\
+	{ 1 << ALARM_REALTIME, "REALTIME" },			\
+	{ 1 << ALARM_BOOTTIME, "BOOTTIME" })
+
+TRACE_EVENT(alarmtimer_suspend,
+
+	TP_PROTO(ktime_t expires, int flag),
+
+	TP_ARGS(expires, flag),
+
+	TP_STRUCT__entry(
+		__field(s64, expires)
+		__field(unsigned char, alarm_type)
+	),
+
+	TP_fast_assign(
+		__entry->expires = expires.tv64;
+		__entry->alarm_type = flag;
+	),
+
+	TP_printk("alarmtimer type:%s expires:%llu",
+		  show_alarm_type((1 << __entry->alarm_type)),
+		  __entry->expires
+	)
+);
+
+DECLARE_EVENT_CLASS(alarm_class,
+
+	TP_PROTO(struct alarm *alarm, ktime_t now),
+
+	TP_ARGS(alarm, now),
+
+	TP_STRUCT__entry(
+		__field(void *,	alarm)
+		__field(unsigned char, alarm_type)
+		__field(s64, expires)
+		__field(s64, now)
+	),
+
+	TP_fast_assign(
+		__entry->alarm = alarm;
+		__entry->alarm_type = alarm->type;
+		__entry->expires = alarm->node.expires.tv64;
+		__entry->now = now.tv64;
+	),
+
+	TP_printk("alarmtimer:%p type:%s expires:%llu now:%llu",
+		  __entry->alarm,
+		  show_alarm_type((1 << __entry->alarm_type)),
+		  __entry->expires,
+		  __entry->now
+	)
+);
+
+DEFINE_EVENT(alarm_class, alarmtimer_fired,
+
+	TP_PROTO(struct alarm *alarm, ktime_t now),
+
+	TP_ARGS(alarm, now)
+);
+
+DEFINE_EVENT(alarm_class, alarmtimer_start,
+
+	TP_PROTO(struct alarm *alarm, ktime_t now),
+
+	TP_ARGS(alarm, now)
+);
+
+DEFINE_EVENT(alarm_class, alarmtimer_cancel,
+
+	TP_PROTO(struct alarm *alarm, ktime_t now),
+
+	TP_ARGS(alarm, now)
+);
+
+#endif /* _TRACE_ALARMTIMER_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 12dd190..8084e0c 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -26,6 +26,9 @@
 #include <linux/workqueue.h>
 #include <linux/freezer.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/alarmtimer.h>
+
 /**
  * struct alarm_base - Alarm timer bases
  * @lock:		Lock for syncrhonized access to the base
@@ -194,6 +197,7 @@ static enum hrtimer_restart alarmtimer_fired(struct hrtimer *timer)
 	}
 	spin_unlock_irqrestore(&base->lock, flags);
 
+	trace_alarmtimer_fired(alarm, base->gettime());
 	return ret;
 
 }
@@ -222,7 +226,7 @@ static int alarmtimer_suspend(struct device *dev)
 	ktime_t min, now;
 	unsigned long flags;
 	struct rtc_device *rtc;
-	int i;
+	int i, type = 0;
 	int ret;
 
 	spin_lock_irqsave(&freezer_delta_lock, flags);
@@ -247,8 +251,10 @@ static int alarmtimer_suspend(struct device *dev)
 		if (!next)
 			continue;
 		delta = ktime_sub(next->expires, base->gettime());
-		if (!min.tv64 || (delta.tv64 < min.tv64))
+		if (!min.tv64 || (delta.tv64 < min.tv64)) {
 			min = delta;
+			type = i;
+		}
 	}
 	if (min.tv64 == 0)
 		return 0;
@@ -264,6 +270,8 @@ static int alarmtimer_suspend(struct device *dev)
 	now = rtc_tm_to_ktime(tm);
 	now = ktime_add(now, min);
 
+	trace_alarmtimer_suspend(now, type);
+
 	/* Set alarm, if in the past reject suspend briefly to handle */
 	ret = rtc_timer_start(rtc, &rtctimer, now, ktime_set(0, 0));
 	if (ret < 0)
@@ -342,6 +350,8 @@ void alarm_start(struct alarm *alarm, ktime_t start)
 	alarmtimer_enqueue(base, alarm);
 	hrtimer_start(&alarm->timer, alarm->node.expires, HRTIMER_MODE_ABS);
 	spin_unlock_irqrestore(&base->lock, flags);
+
+	trace_alarmtimer_start(alarm, base->gettime());
 }
 EXPORT_SYMBOL_GPL(alarm_start);
 
@@ -390,6 +400,8 @@ int alarm_try_to_cancel(struct alarm *alarm)
 	if (ret >= 0)
 		alarmtimer_dequeue(base, alarm);
 	spin_unlock_irqrestore(&base->lock, flags);
+
+	trace_alarmtimer_cancel(alarm, base->gettime());
 	return ret;
 }
 EXPORT_SYMBOL_GPL(alarm_try_to_cancel);
-- 
2.7.4

Re: [PATCH 1/4] time: alarmtimer: Add the trcepoints for alarmtimer

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> @@ -222,7 +226,7 @@ static int alarmtimer_suspend(struct device *dev)
>  	ktime_t min, now;
>  	unsigned long flags;
>  	struct rtc_device *rtc;
> -	int i;
> +	int i, type = 0;
>  	int ret;
>  
>  	spin_lock_irqsave(&freezer_delta_lock, flags);
> @@ -247,8 +251,10 @@ static int alarmtimer_suspend(struct device *dev)
>  		if (!next)
>  			continue;
>  		delta = ktime_sub(next->expires, base->gettime());
> -		if (!min.tv64 || (delta.tv64 < min.tv64))
> +		if (!min.tv64 || (delta.tv64 < min.tv64)) {
>  			min = delta;
> +			type = i;
> +		}
>  	}
>  	if (min.tv64 == 0)
>  		return 0;
> @@ -264,6 +270,8 @@ static int alarmtimer_suspend(struct device *dev)
>  	now = rtc_tm_to_ktime(tm);
>  	now = ktime_add(now, min);
>  
> +	trace_alarmtimer_suspend(now, type);
> +
>  	/* Set alarm, if in the past reject suspend briefly to handle */
>  	ret = rtc_timer_start(rtc, &rtctimer, now, ktime_set(0, 0));
>  	if (ret < 0)

Hm, there's a weirdness here: if freezer_delta != 0 when alarmtimer_suspend() is 
called then type might be '0', although it's not alarm_bases[0] this value is 
picked up from.

Wouldn't it be better to initialize 'type' to -1 instead? (And rename it to 
min_type or so.)

That would disambiguate the freezer_delta special case in the trace.

(Unless I missed something.)

Thanks,

	Ingo

Re: [PATCH 1/4] time: alarmtimer: Add the trcepoints for alarmtimer

[Baolin Wang]

Hi Ingo,

On 21 November 2016 at 16:13, Ingo Molnar <mingo@kernel.org> wrote:
>
> * John Stultz <john.stultz@linaro.org> wrote:
>
>> @@ -222,7 +226,7 @@ static int alarmtimer_suspend(struct device *dev)
>>       ktime_t min, now;
>>       unsigned long flags;
>>       struct rtc_device *rtc;
>> -     int i;
>> +     int i, type = 0;
>>       int ret;
>>
>>       spin_lock_irqsave(&freezer_delta_lock, flags);
>> @@ -247,8 +251,10 @@ static int alarmtimer_suspend(struct device *dev)
>>               if (!next)
>>                       continue;
>>               delta = ktime_sub(next->expires, base->gettime());
>> -             if (!min.tv64 || (delta.tv64 < min.tv64))
>> +             if (!min.tv64 || (delta.tv64 < min.tv64)) {
>>                       min = delta;
>> +                     type = i;
>> +             }
>>       }
>>       if (min.tv64 == 0)
>>               return 0;
>> @@ -264,6 +270,8 @@ static int alarmtimer_suspend(struct device *dev)
>>       now = rtc_tm_to_ktime(tm);
>>       now = ktime_add(now, min);
>>
>> +     trace_alarmtimer_suspend(now, type);
>> +
>>       /* Set alarm, if in the past reject suspend briefly to handle */
>>       ret = rtc_timer_start(rtc, &rtctimer, now, ktime_set(0, 0));
>>       if (ret < 0)
>
> Hm, there's a weirdness here: if freezer_delta != 0 when alarmtimer_suspend() is
> called then type might be '0', although it's not alarm_bases[0] this value is
> picked up from.
>
> Wouldn't it be better to initialize 'type' to -1 instead? (And rename it to
> min_type or so.)

Make sense. I will send out new patch to fix this. Thanks for your comments.

>
> That would disambiguate the freezer_delta special case in the trace.
>
> (Unless I missed something.)
>
> Thanks,
>
>         Ingo



-- 
Baolin.wang
Best Regards

Re: [PATCH 1/4] time: alarmtimer: Add the trcepoints for alarmtimer

[Thomas Gleixner]

On Mon, 21 Nov 2016, Baolin Wang wrote:
> On 21 November 2016 at 16:13, Ingo Molnar <mingo@kernel.org> wrote:
> > Hm, there's a weirdness here: if freezer_delta != 0 when alarmtimer_suspend() is
> > called then type might be '0', although it's not alarm_bases[0] this value is
> > picked up from.
> >
> > Wouldn't it be better to initialize 'type' to -1 instead? (And rename it to
> > min_type or so.)
> 
> Make sense. I will send out new patch to fix this. Thanks for your comments.

Can you please fix the subject line while at it? 'trcepoints' does not look right

Thanks,

	tglx

Re: [PATCH 1/4] time: alarmtimer: Add the trcepoints for alarmtimer

[Baolin Wang]

On 21 November 2016 at 16:56, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 21 Nov 2016, Baolin Wang wrote:
>> On 21 November 2016 at 16:13, Ingo Molnar <mingo@kernel.org> wrote:
>> > Hm, there's a weirdness here: if freezer_delta != 0 when alarmtimer_suspend() is
>> > called then type might be '0', although it's not alarm_bases[0] this value is
>> > picked up from.
>> >
>> > Wouldn't it be better to initialize 'type' to -1 instead? (And rename it to
>> > min_type or so.)
>>
>> Make sense. I will send out new patch to fix this. Thanks for your comments.
>
> Can you please fix the subject line while at it? 'trcepoints' does not look right

Sorry for mistake, I will fix that. Thanks.

-- 
Baolin.wang
Best Regards

[PATCH 2/4] selftests/timers: Fix spelling mistake "Asyncrhonous" -> "Asynchronous"

[John Stultz]

From: Colin Ian King <colin.king@canonical.com>

Trivial fix to spelling mistake

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 tools/testing/selftests/timers/skew_consistency.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/timers/skew_consistency.c b/tools/testing/selftests/timers/skew_consistency.c
index 5562f84..2a996e0 100644
--- a/tools/testing/selftests/timers/skew_consistency.c
+++ b/tools/testing/selftests/timers/skew_consistency.c
@@ -57,7 +57,7 @@ int main(int argv, char **argc)
 	pid_t pid;
 
 
-	printf("Running Asyncrhonous Frequency Changing Tests...\n");
+	printf("Running Asynchronous Frequency Changing Tests...\n");
 
 	pid = fork();
 	if (!pid)
-- 
2.7.4

[PATCH 3/4] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[John Stultz]

From: Chen Yu <yu.c.chen@intel.com>

Previously we encountered some memory overflow issues due to
the bogus sleep time brought by inconsistent rtc, which is
triggered when pm_trace is enabled, and we have fixed it
in recent kernel. However it's improper in the first place
to call __timekeeping_inject_sleeptime() in case that pm_trace
is enabled simply because that "hash" time value will wreckage
the timekeeping subsystem.

This patch is originally written by Thomas, which would bypass
the bogus rtc interval when pm_trace is enabled.
Meanwhile, if system succeed to resume back with pm_trace set, the
users are warned to adjust the bogus rtc either by ntp-date or rdate,
by resetting pm_trace_rtc_abused to false, otherwise above tools might
not work as expected.

Originally-from: Thomas Gleixner <tglx@linutronix.de>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Xunlei Pang <xlpang@redhat.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Len Brown <lenb@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 arch/x86/kernel/rtc.c       |  9 +++++++++
 drivers/base/power/trace.c  | 27 +++++++++++++++++++++++++++
 drivers/rtc/rtc-cmos.c      |  7 +++++++
 include/linux/mc146818rtc.h |  1 +
 include/linux/pm-trace.h    |  9 ++++++++-
 5 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 79c6311c..898383c 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -64,6 +64,15 @@ void mach_get_cmos_time(struct timespec *now)
 	unsigned int status, year, mon, day, hour, min, sec, century = 0;
 	unsigned long flags;
 
+	/*
+	 * If pm trace abused the RTC as storage set the timespec to 0
+	 * which tells the caller that this RTC value is bogus.
+	 */
+	if (!pm_trace_rtc_valid()) {
+		now->tv_sec = now->tv_nsec = 0;
+		return;
+	}
+
 	spin_lock_irqsave(&rtc_lock, flags);
 
 	/*
diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
index efec10b..209e214 100644
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c
@@ -10,6 +10,7 @@
 #include <linux/pm-trace.h>
 #include <linux/export.h>
 #include <linux/rtc.h>
+#include <linux/suspend.h>
 
 #include <linux/mc146818rtc.h>
 
@@ -74,6 +75,9 @@
 
 #define DEVSEED (7919)
 
+bool pm_trace_rtc_abused __read_mostly;
+EXPORT_SYMBOL(pm_trace_rtc_abused);
+
 static unsigned int dev_hash_value;
 
 static int set_magic_time(unsigned int user, unsigned int file, unsigned int device)
@@ -104,6 +108,7 @@ static int set_magic_time(unsigned int user, unsigned int file, unsigned int dev
 	time.tm_min = (n % 20) * 3;
 	n /= 20;
 	mc146818_set_time(&time);
+	pm_trace_rtc_abused = true;
 	return n ? -1 : 0;
 }
 
@@ -238,10 +243,32 @@ int show_trace_dev_match(char *buf, size_t size)
 	device_pm_unlock();
 	return ret;
 }
+static int pm_trace_notify(struct notifier_block *nb,
+				unsigned long mode, void *_unused)
+{
+	switch (mode) {
+	case PM_POST_HIBERNATION:
+	case PM_POST_SUSPEND:
+		if (pm_trace_rtc_abused) {
+			pm_trace_rtc_abused = false;
+			pr_warn("Possible incorrect RTC due to pm_trace,"
+				"please use ntp-date or rdate to reset.\n");
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static struct notifier_block pm_trace_nb = {
+	.notifier_call = pm_trace_notify,
+};
 
 static int early_resume_init(void)
 {
 	hash_value_early_read = read_magic_time();
+	register_pm_notifier(&pm_trace_nb);
 	return 0;
 }
 
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index dd3d598..3d9aedc 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -191,6 +191,13 @@ static inline void cmos_write_bank2(unsigned char val, unsigned char addr)
 
 static int cmos_read_time(struct device *dev, struct rtc_time *t)
 {
+	/*
+	 * If pmtrace abused the RTC for storage tell the caller that it is
+	 * unusable.
+	 */
+	if (!pm_trace_rtc_valid())
+		return -EIO;
+
 	/* REVISIT:  if the clock has a "century" register, use
 	 * that instead of the heuristic in mc146818_get_time().
 	 * That'll make Y3K compatility (year > 2070) easy!
diff --git a/include/linux/mc146818rtc.h b/include/linux/mc146818rtc.h
index a585b4b..0661af1 100644
--- a/include/linux/mc146818rtc.h
+++ b/include/linux/mc146818rtc.h
@@ -16,6 +16,7 @@
 #include <asm/mc146818rtc.h>		/* register access macros */
 #include <linux/bcd.h>
 #include <linux/delay.h>
+#include <linux/pm-trace.h>
 
 #ifdef __KERNEL__
 #include <linux/spinlock.h>		/* spinlock_t */
diff --git a/include/linux/pm-trace.h b/include/linux/pm-trace.h
index ecbde7a..7b78793 100644
--- a/include/linux/pm-trace.h
+++ b/include/linux/pm-trace.h
@@ -1,11 +1,17 @@
 #ifndef PM_TRACE_H
 #define PM_TRACE_H
 
+#include <linux/types.h>
 #ifdef CONFIG_PM_TRACE
 #include <asm/pm-trace.h>
-#include <linux/types.h>
 
 extern int pm_trace_enabled;
+extern bool pm_trace_rtc_abused;
+
+static inline bool pm_trace_rtc_valid(void)
+{
+	return !pm_trace_rtc_abused;
+}
 
 static inline int pm_trace_is_enabled(void)
 {
@@ -24,6 +30,7 @@ extern int show_trace_dev_match(char *buf, size_t size);
 
 #else
 
+static inline bool pm_trace_rtc_valid(void) { return true; }
 static inline int pm_trace_is_enabled(void) { return 0; }
 
 #define TRACE_DEVICE(dev) do { } while (0)
-- 
2.7.4

Re: [PATCH 3/4] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> +static int pm_trace_notify(struct notifier_block *nb,
> +				unsigned long mode, void *_unused)
> +{
> +	switch (mode) {
> +	case PM_POST_HIBERNATION:
> +	case PM_POST_SUSPEND:
> +		if (pm_trace_rtc_abused) {
> +			pm_trace_rtc_abused = false;
> +			pr_warn("Possible incorrect RTC due to pm_trace,"
> +				"please use ntp-date or rdate to reset.\n");

Please don't break user-visible strings just to pacify checkpatch!

The bogus linebreak above hides a type in the user string:

  Possible incorrect RTC due to pm_trace,please use ntp-date or rdate to reset.

(There's a missing space after the comma.)

Best practice is to preserve the continuous nature of the user string in the code. 

In addition to that, please quote suggested command names, i.e. something like:

  Possible incorrect RTC due to pm_trace, please use 'ntp-date' or 'rdate' to reset it.

> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -191,6 +191,13 @@ static inline void cmos_write_bank2(unsigned char val, unsigned char addr)
>  
>  static int cmos_read_time(struct device *dev, struct rtc_time *t)
>  {
> +	/*
> +	 * If pmtrace abused the RTC for storage tell the caller that it is
> +	 * unusable.
> +	 */
> +	if (!pm_trace_rtc_valid())
> +		return -EIO;

Please standardize the spelling of 'pm_trace', as there's 3 variants present in 
this patch alone:

  'pm_trace'
  'pm trace'
  'pmtrace'

(Not to mention pm-trace.h - but that's a pre-existing inconsistency unrelated to 
your patch.)

Thanks,

	Ingo

RE: [PATCH 3/4] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Chen, Yu C]

Hi,

> -----Original Message-----
> From: Ingo Molnar [mailto:mingo.kernel.org@gmail.com] On Behalf Of Ingo
> Molnar
> Sent: Monday, November 21, 2016 4:18 PM
> To: John Stultz
> Cc: lkml; Chen, Yu C; Rafael J. Wysocki; Xunlei Pang; Ingo Molnar; Len Brown; H.
> Peter Anvin; Pavel Machek; Thomas Gleixner; Prarit Bhargava; Richard Cochran
> Subject: Re: [PATCH 3/4] timekeeping: Ignore the bogus sleep time if pm_trace
> is enabled
> 
> 
> * John Stultz <john.stultz@linaro.org> wrote:
> 
> > +static int pm_trace_notify(struct notifier_block *nb,
> > +				unsigned long mode, void *_unused) {
> > +	switch (mode) {
> > +	case PM_POST_HIBERNATION:
> > +	case PM_POST_SUSPEND:
> > +		if (pm_trace_rtc_abused) {
> > +			pm_trace_rtc_abused = false;
> > +			pr_warn("Possible incorrect RTC due to pm_trace,"
> > +				"please use ntp-date or rdate to reset.\n");
> 
> Please don't break user-visible strings just to pacify checkpatch!
> 
> The bogus linebreak above hides a type in the user string:
> 
>   Possible incorrect RTC due to pm_trace,please use ntp-date or rdate to reset.
> 
> (There's a missing space after the comma.)
> 
> Best practice is to preserve the continuous nature of the user string in the code.
> 
> In addition to that, please quote suggested command names, i.e. something like:
> 
>   Possible incorrect RTC due to pm_trace, please use 'ntp-date' or 'rdate' to
> reset it.
OK, will do.
> 
> > --- a/drivers/rtc/rtc-cmos.c
> > +++ b/drivers/rtc/rtc-cmos.c
> > @@ -191,6 +191,13 @@ static inline void cmos_write_bank2(unsigned char
> > val, unsigned char addr)
> >
> >  static int cmos_read_time(struct device *dev, struct rtc_time *t)  {
> > +	/*
> > +	 * If pmtrace abused the RTC for storage tell the caller that it is
> > +	 * unusable.
> > +	 */
> > +	if (!pm_trace_rtc_valid())
> > +		return -EIO;
> 
> Please standardize the spelling of 'pm_trace', as there's 3 variants present in
> this patch alone:
> 
>   'pm_trace'
>   'pm trace'
>   'pmtrace'
OK, will do.
> 
> (Not to mention pm-trace.h - but that's a pre-existing inconsistency unrelated
> to your patch.)
> 
> Thanks,
> 
> 	Ingo

Thanks!

Re: [PATCH 3/4] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[John Stultz]

On Thu, Nov 24, 2016 at 1:54 AM, Chen, Yu C <yu.c.chen@intel.com> wrote:
> Hi,
>
>> -----Original Message-----
>> From: Ingo Molnar [mailto:mingo.kernel.org@gmail.com] On Behalf Of Ingo
>> Molnar
>> Sent: Monday, November 21, 2016 4:18 PM
>> To: John Stultz
>> Cc: lkml; Chen, Yu C; Rafael J. Wysocki; Xunlei Pang; Ingo Molnar; Len Brown; H.
>> Peter Anvin; Pavel Machek; Thomas Gleixner; Prarit Bhargava; Richard Cochran
>> Subject: Re: [PATCH 3/4] timekeeping: Ignore the bogus sleep time if pm_trace
>> is enabled
>>
>>
>> * John Stultz <john.stultz@linaro.org> wrote:
>>
>> > +static int pm_trace_notify(struct notifier_block *nb,
>> > +                           unsigned long mode, void *_unused) {
>> > +   switch (mode) {
>> > +   case PM_POST_HIBERNATION:
>> > +   case PM_POST_SUSPEND:
>> > +           if (pm_trace_rtc_abused) {
>> > +                   pm_trace_rtc_abused = false;
>> > +                   pr_warn("Possible incorrect RTC due to pm_trace,"
>> > +                           "please use ntp-date or rdate to reset.\n");
>>
>> Please don't break user-visible strings just to pacify checkpatch!
>>
>> The bogus linebreak above hides a type in the user string:
>>
>>   Possible incorrect RTC due to pm_trace,please use ntp-date or rdate to reset.
>>
>> (There's a missing space after the comma.)
>>
>> Best practice is to preserve the continuous nature of the user string in the code.
>>
>> In addition to that, please quote suggested command names, i.e. something like:
>>
>>   Possible incorrect RTC due to pm_trace, please use 'ntp-date' or 'rdate' to
>> reset it.
> OK, will do.

Just a heads up. I've already made these changes in my tree.

thanks
-john

RE: [PATCH 3/4] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

[Chen, Yu C]

> -----Original Message-----
> From: John Stultz [mailto:john.stultz@linaro.org]
> Sent: Tuesday, November 29, 2016 2:39 AM
> To: Chen, Yu C
> Cc: Ingo Molnar; lkml; Rafael J. Wysocki; Xunlei Pang; Ingo Molnar; Len Brown;
> H. Peter Anvin; Pavel Machek; Thomas Gleixner; Prarit Bhargava; Richard
> Cochran
> Subject: Re: [PATCH 3/4] timekeeping: Ignore the bogus sleep time if pm_trace
> is enabled
> 
> On Thu, Nov 24, 2016 at 1:54 AM, Chen, Yu C <yu.c.chen@intel.com> wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Ingo Molnar [mailto:mingo.kernel.org@gmail.com] On Behalf Of
> >> Ingo Molnar
> >> Sent: Monday, November 21, 2016 4:18 PM
> >> To: John Stultz
> >> Cc: lkml; Chen, Yu C; Rafael J. Wysocki; Xunlei Pang; Ingo Molnar; Len Brown;
> H.
> >> Peter Anvin; Pavel Machek; Thomas Gleixner; Prarit Bhargava; Richard
> >> Cochran
> >> Subject: Re: [PATCH 3/4] timekeeping: Ignore the bogus sleep time if
> >> pm_trace is enabled
> >>
> >>
> >> * John Stultz <john.stultz@linaro.org> wrote:
> >>
> >> > +static int pm_trace_notify(struct notifier_block *nb,
> >> > +                           unsigned long mode, void *_unused) {
> >> > +   switch (mode) {
> >> > +   case PM_POST_HIBERNATION:
> >> > +   case PM_POST_SUSPEND:
> >> > +           if (pm_trace_rtc_abused) {
> >> > +                   pm_trace_rtc_abused = false;
> >> > +                   pr_warn("Possible incorrect RTC due to pm_trace,"
> >> > +                           "please use ntp-date or rdate to
> >> > +reset.\n");
> >>
> >> Please don't break user-visible strings just to pacify checkpatch!
> >>
> >> The bogus linebreak above hides a type in the user string:
> >>
> >>   Possible incorrect RTC due to pm_trace,please use ntp-date or rdate to
> reset.
> >>
> >> (There's a missing space after the comma.)
> >>
> >> Best practice is to preserve the continuous nature of the user string in the
> code.
> >>
> >> In addition to that, please quote suggested command names, i.e. something
> like:
> >>
> >>   Possible incorrect RTC due to pm_trace, please use 'ntp-date' or
> >> 'rdate' to reset it.
> > OK, will do.
> 
> Just a heads up. I've already made these changes in my tree.
> 
> thanks
> -john
OK, thanks very much!  john. 

Yu

[PATCH 4/4] timekeeping: clocksource_cyc2ns: Document intended range limitation

[John Stultz]

From: Chris Metcalf <cmetcalf@mellanox.com>

The "cycles" argument should not be an absolute clocksource cycle
value, as the implementation's arithmetic will overflow relatively
easily with wide (64 bit) clocksource counters.

For performance, the implementation is simple and fast, since the
function is intended for only relatively small delta values of
clocksource cycles.

Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
[jstultz: Fixed up to merge against HEAD & commit message tweaks]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/linux/clocksource.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 0839818..0881bca 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -169,7 +169,10 @@ static inline u32 clocksource_hz2mult(u32 hz, u32 shift_constant)
  * @mult:	cycle to nanosecond multiplier
  * @shift:	cycle to nanosecond divisor (power of two)
  *
- * Converts cycles to nanoseconds, using the given mult and shift.
+ * Converts clocksource cycles to nanoseconds, using the given mult and shift.
+ * The code is optimized for performance and not intended to work
+ * with absolute clocksource cycles, as it will easily overflow,
+ * but just intended for relative (delta) clocksource cycles.
  *
  * XXX - This could use some mult_lxl_ll() asm optimization
  */
-- 
2.7.4

Re: [PATCH 4/4] timekeeping: clocksource_cyc2ns: Document intended range limitation

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> From: Chris Metcalf <cmetcalf@mellanox.com>
> 
> The "cycles" argument should not be an absolute clocksource cycle
> value, as the implementation's arithmetic will overflow relatively
> easily with wide (64 bit) clocksource counters.
> 
> For performance, the implementation is simple and fast, since the
> function is intended for only relatively small delta values of
> clocksource cycles.
> 
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Prarit Bhargava <prarit@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
> [jstultz: Fixed up to merge against HEAD & commit message tweaks]
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  include/linux/clocksource.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 0839818..0881bca 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -169,7 +169,10 @@ static inline u32 clocksource_hz2mult(u32 hz, u32 shift_constant)
>   * @mult:	cycle to nanosecond multiplier
>   * @shift:	cycle to nanosecond divisor (power of two)
>   *
> - * Converts cycles to nanoseconds, using the given mult and shift.
> + * Converts clocksource cycles to nanoseconds, using the given mult and shift.
> + * The code is optimized for performance and not intended to work
> + * with absolute clocksource cycles, as it will easily overflow,
> + * but just intended for relative (delta) clocksource cycles.

Had to read this explanation twice, how about:

    * Converts clocksource cycles to nanoseconds, using the given @mult and @shift.
    * The code is optimized for performance and is not intended to work
    * with absolute clocksource cycles (as those will easily overflow),
    * but is only intended to be used with relative (delta) clocksource cycles.

Did I get it right?

Thanks,

	Ingo

Re: [PATCH 4/4] timekeeping: clocksource_cyc2ns: Document intended range limitation

[Chris Metcalf]

On 11/21/2016 3:54 AM, Ingo Molnar wrote:
> * John Stultz <john.stultz@linaro.org> wrote:
>
>> From: Chris Metcalf <cmetcalf@mellanox.com>
>>
>> The "cycles" argument should not be an absolute clocksource cycle
>> value, as the implementation's arithmetic will overflow relatively
>> easily with wide (64 bit) clocksource counters.
>>
>> For performance, the implementation is simple and fast, since the
>> function is intended for only relatively small delta values of
>> clocksource cycles.
>>
>> Cc: Richard Cochran <richardcochran@gmail.com>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Prarit Bhargava <prarit@redhat.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
>> [jstultz: Fixed up to merge against HEAD & commit message tweaks]
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>>   include/linux/clocksource.h | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
>> index 0839818..0881bca 100644
>> --- a/include/linux/clocksource.h
>> +++ b/include/linux/clocksource.h
>> @@ -169,7 +169,10 @@ static inline u32 clocksource_hz2mult(u32 hz, u32 shift_constant)
>>    * @mult:	cycle to nanosecond multiplier
>>    * @shift:	cycle to nanosecond divisor (power of two)
>>    *
>> - * Converts cycles to nanoseconds, using the given mult and shift.
>> + * Converts clocksource cycles to nanoseconds, using the given mult and shift.
>> + * The code is optimized for performance and not intended to work
>> + * with absolute clocksource cycles, as it will easily overflow,
>> + * but just intended for relative (delta) clocksource cycles.
> Had to read this explanation twice, how about:
>
>      * Converts clocksource cycles to nanoseconds, using the given @mult and @shift.
>      * The code is optimized for performance and is not intended to work
>      * with absolute clocksource cycles (as those will easily overflow),
>      * but is only intended to be used with relative (delta) clocksource cycles.
>
> Did I get it right?

Yes, I think that's an improvement.  Thanks!

John, I assume you can just fix this up?

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com

Re: [PATCH 4/4] timekeeping: clocksource_cyc2ns: Document intended range limitation

[John Stultz]

On Mon, Nov 21, 2016 at 7:28 AM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> On 11/21/2016 3:54 AM, Ingo Molnar wrote:
>>
>> * John Stultz <john.stultz@linaro.org> wrote:
>>
>>> From: Chris Metcalf <cmetcalf@mellanox.com>
>>>
>>> The "cycles" argument should not be an absolute clocksource cycle
>>> value, as the implementation's arithmetic will overflow relatively
>>> easily with wide (64 bit) clocksource counters.
>>>
>>> For performance, the implementation is simple and fast, since the
>>> function is intended for only relatively small delta values of
>>> clocksource cycles.
>>>
>>> Cc: Richard Cochran <richardcochran@gmail.com>
>>> Cc: Ingo Molnar <mingo@kernel.org>
>>> Cc: Prarit Bhargava <prarit@redhat.com>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
>>> [jstultz: Fixed up to merge against HEAD & commit message tweaks]
>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>> ---
>>>   include/linux/clocksource.h | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
>>> index 0839818..0881bca 100644
>>> --- a/include/linux/clocksource.h
>>> +++ b/include/linux/clocksource.h
>>> @@ -169,7 +169,10 @@ static inline u32 clocksource_hz2mult(u32 hz, u32
>>> shift_constant)
>>>    * @mult:     cycle to nanosecond multiplier
>>>    * @shift:    cycle to nanosecond divisor (power of two)
>>>    *
>>> - * Converts cycles to nanoseconds, using the given mult and shift.
>>> + * Converts clocksource cycles to nanoseconds, using the given mult and
>>> shift.
>>> + * The code is optimized for performance and not intended to work
>>> + * with absolute clocksource cycles, as it will easily overflow,
>>> + * but just intended for relative (delta) clocksource cycles.
>>
>> Had to read this explanation twice, how about:
>>
>>      * Converts clocksource cycles to nanoseconds, using the given @mult
>> and @shift.
>>      * The code is optimized for performance and is not intended to work
>>      * with absolute clocksource cycles (as those will easily overflow),
>>      * but is only intended to be used with relative (delta) clocksource
>> cycles.
>>
>> Did I get it right?
>
>
> Yes, I think that's an improvement.  Thanks!
>
> John, I assume you can just fix this up?

Sure. Reworded to take Ingo's suggestions.

thanks
-john