Skip to contentSkip to conversation

Fix sysdev time support

PATCH In review Nigel Cunningham 2 participants 9 messages Nov 11, 2004 – Nov 12, 2004 SuperH

lore
Share & tools
lore

Nigel Cunningham

author
#0 lore raw

cover Fix sysdev time support

Hi Andrew et al.

Since Suspend2 (and swsusp) have begun to use the sysdev_suspend and _resume methods, I've discovered some issues with the time support.

time_suspend and _resume call get_cmos_time twice, resulting (!CONFIG_EFI) in an extra second delay for each call (see below). For suspend2, and probably for swsusp (I haven't seen Pavel's handling yet), a call to the _suspend method is almost immediately followed by a call to the _resume method (with the atomic save/restore of memory in between), so that the user sees at least a 3 second delay (ave 1.5 for suspend + 1.5 for resume) simply because of these calls.

get_cmos_time may call efi_get_time, which is marked __init and (according to comments in its header) designed to run in physical mode; I assume, not understanding PAE yet, that this will need further consideration.

get_cmos_time may instead call mach_get_cmos_time (or equiv for other archs), which waits (!x86_64) until the end of a cmos time update before returning the value. For suspending, we don't need to wait for the 'edge' of a second when suspending; we just need a consistent value.

Finally, the calculation of the clock skew due to suspending uses a mixture of signed and unsigned longs. This can result (based on empirical results) in the clock being out by 1 hr, 11 minutes and 31s post resume.

I proposed three patches:
1) Optimise time_suspend and time_resume so that get_cmos_time is called exactly once in each case. This drops 1 second from each call.
2) Add a new parameter & appropriate logic to get_cmos_time and to mach_get_cmos_time and equivalents, allowing the caller to specify whether we should wait for the beginning of a new second. When suspending, don't wait; when resuming, do.
3) Switch sleep_start in arch/i386/kernel/time from a long to an unsigned long, thereby addressing the math issue.

I also wonder if the jiffies+= logic in the x86 code should be applied to x86_64 too?

Regards,

Nigel

Signature · 7 lines
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

You see, at just the right time, when we were still powerless, Christ
died for the ungodly.		-- Romans 5:6
author
#1 lore raw

PATCH 1/3 Fix sysdev time support

Make time_suspend and time_resume call get_cmos_time once only, so as to eliminate unnecessary 1 second delays in suspending and resuming.

arch/i386/kernel/time.c +6 -4
diff -ruN 990-old/arch/i386/kernel/time.c 990-new/arch/i386/kernel/time.c
@@ -323,20 +323,22 @@
 
 static int time_suspend(struct sys_device *dev, u32 state)
 {
+	long cmos_time = get_cmos_time();
 	/*
 	 * Estimate time zone so that set_time can update the clock
 	 */
-	clock_cmos_diff = -get_cmos_time();
+	clock_cmos_diff = -cmos_time;
 	clock_cmos_diff += get_seconds();
-	sleep_start = get_cmos_time();
+	sleep_start = cmos_time;
 	return 0;
 }
 
 static int time_resume(struct sys_device *dev)
 {
 	unsigned long flags;
-	unsigned long sec = get_cmos_time() + clock_cmos_diff;
-	unsigned long sleep_length = get_cmos_time() - sleep_start;
+	unsigned long cmos_time = get_cmos_time();
+	unsigned long sec = cmos_time + clock_cmos_diff;
+	unsigned long sleep_length = cmos_time - sleep_start;
 
 	write_seqlock_irqsave(&xtime_lock, flags);
 	xtime.tv_sec = sec;
Signature · 7 lines
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

You see, at just the right time, when we were still powerless, Christ
died for the ungodly.		-- Romans 5:6
#2 lore raw

Hi!

Quoted 2 lines

Make time_suspend and time_resume call get_cmos_time once only, so as to eliminate unnecessary 1 second delays in suspending and resuming.

Looks okay to me.
Pavel

Signature · 1 line
Boycott Kodak -- for their patent abuse against Java.
author
#3 lore raw

PATCH 2/3 Fix sysdev time support

In reply to Nigel Cunningham

Add an extra parameter to get_cmos_time and to arch specific routines, allowing the user to specify whether we should wait for the beginning of a new second, or simply ensure that the time returned is accurate.

arch/cris/kernel/time.c +1 -1
diff -ruN 991-old/arch/cris/kernel/time.c 991-new/arch/cris/kernel/time.c
@@ -173,7 +173,7 @@
 /* grab the time from the RTC chip */
 
 unsigned long
-get_cmos_time(void)
+get_cmos_time(int wait)
 {
 	unsigned int year, mon, day, hour, min, sec;
arch/i386/kernel/apm.c +3 -3
diff -ruN 991-old/arch/i386/kernel/apm.c 991-new/arch/i386/kernel/apm.c
@@ -232,7 +232,7 @@
 #include "io_ports.h"
 
 extern spinlock_t i8253_lock;
-extern unsigned long get_cmos_time(void);
+extern unsigned long get_cmos_time(int wait);
 extern void machine_real_restart(unsigned char *, int);
 
 #if defined(CONFIG_APM_DISPLAY_BLANK) && defined(CONFIG_VT)

@@ -1148,7 +1148,7 @@
 static void set_time(void)
 {
 	if (got_clock_diff) {	/* Must know time zone in order to set clock */
-		xtime.tv_sec = get_cmos_time() + clock_cmos_diff;
+		xtime.tv_sec = get_cmos_time(1) + clock_cmos_diff;
 		xtime.tv_nsec = 0; 
 	} 
 }

@@ -1159,7 +1159,7 @@
 	/*
 	 * Estimate time zone so that set_time can update the clock
 	 */
-	clock_cmos_diff = -get_cmos_time();
+	clock_cmos_diff = -get_cmos_time(0);
 	clock_cmos_diff += get_seconds();
 	got_clock_diff = 1;
 #endif
arch/i386/kernel/i386_ksyms.c +1 -1
diff -ruN 991-old/arch/i386/kernel/i386_ksyms.c 991-new/arch/i386/kernel/i386_ksyms.c
@@ -57,7 +57,7 @@
 #endif
 
 extern unsigned long cpu_khz;
-extern unsigned long get_cmos_time(void);
+extern unsigned long get_cmos_time(int wait);
 
 /* platform dependent support */
 EXPORT_SYMBOL(boot_cpu_data);
arch/i386/kernel/time.c +7 -7
diff -ruN 991-old/arch/i386/kernel/time.c 991-new/arch/i386/kernel/time.c
@@ -303,7 +303,7 @@
 }
 
 /* not static: needed by APM */
-unsigned long get_cmos_time(void)
+unsigned long get_cmos_time(int wait)
 {
 	unsigned long retval;
 

@@ -311,8 +311,8 @@
 
 	if (efi_enabled)
 		retval = efi_get_time();
-	else
-		retval = mach_get_cmos_time();
+	else {
+		retval = mach_get_cmos_time(wait);
 
 	spin_unlock(&rtc_lock);
 

@@ -323,7 +323,7 @@
 
 static int time_suspend(struct sys_device *dev, u32 state)
 {
-	long cmos_time = get_cmos_time();
+	long cmos_time = get_cmos_time(0);
 	/*
 	 * Estimate time zone so that set_time can update the clock
 	 */

@@ -336,7 +336,7 @@
 static int time_resume(struct sys_device *dev)
 {
 	unsigned long flags;
-	unsigned long cmos_time = get_cmos_time();
+	unsigned long cmos_time = get_cmos_time(1);
 	unsigned long sec = cmos_time + clock_cmos_diff;
 	unsigned long sleep_length = cmos_time - sleep_start;
 

@@ -376,7 +376,7 @@
 /* Duplicate of time_init() below, with hpet_enable part added */
 void __init hpet_time_init(void)
 {
-	xtime.tv_sec = get_cmos_time();
+	xtime.tv_sec = get_cmos_time(1);
 	wall_to_monotonic.tv_sec = -xtime.tv_sec;
 	xtime.tv_nsec = (INITIAL_JIFFIES % HZ) * (NSEC_PER_SEC / HZ);
 	wall_to_monotonic.tv_nsec = -xtime.tv_nsec;

@@ -404,7 +404,7 @@
 		return;
 	}
 #endif
-	xtime.tv_sec = get_cmos_time();
+	xtime.tv_sec = get_cmos_time(1);
 	wall_to_monotonic.tv_sec = -xtime.tv_sec;
 	xtime.tv_nsec = (INITIAL_JIFFIES % HZ) * (NSEC_PER_SEC / HZ);
 	wall_to_monotonic.tv_nsec = -xtime.tv_nsec;
arch/sh/boards/mpc1211/rtc.c +9 -7
diff -ruN 991-old/arch/sh/boards/mpc1211/rtc.c 991-new/arch/sh/boards/mpc1211/rtc.c
@@ -20,7 +20,7 @@
 #endif
 
 /* arc/i386/kernel/time.c */
-unsigned long get_cmos_time(void)
+unsigned long get_cmos_time(int wait)
 {
 	unsigned int year, mon, day, hour, min, sec;
 	int i;

@@ -32,12 +32,14 @@
 	 * Let's hope other operating systems interpret the RTC the same way.
 	 */
 	/* read RTC exactly on falling edge of update flag */
-	for (i = 0 ; i < 1000000 ; i++)	/* may take up to 1 second... */
-		if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP)
-			break;
-	for (i = 0 ; i < 1000000 ; i++)	/* must try at least 2.228 ms */
-		if (!(CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP))
-			break;
+	if (wait) {
+		for (i = 0 ; i < 1000000 ; i++)	/* may take up to 1 second... */
+			if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP)
+				break;
+		for (i = 0 ; i < 1000000 ; i++)	/* must try at least 2.228 ms */
+			if (!(CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP))
+				break;
+	}
 	do { /* Isn't this overkill ? UIP above should guarantee consistency */
 		sec = CMOS_READ(RTC_SECONDS);
 		min = CMOS_READ(RTC_MINUTES);
arch/x86_64/kernel/time.c +12 -7
diff -ruN 991-old/arch/x86_64/kernel/time.c 991-new/arch/x86_64/kernel/time.c
@@ -495,7 +495,7 @@
 	return cycles_2_ns(a);
 }
 
-unsigned long get_cmos_time(void)
+unsigned long get_cmos_time(int wait)
 {
 	unsigned int timeout, year, mon, day, hour, min, sec;
 	unsigned char last, this;

@@ -514,23 +514,28 @@
 	timeout = 1000000;
 	last = this = 0;
 
-	while (timeout && last && !this) {
-		last = this;
-		this = CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP;
-		timeout--;
+	if (wait) {
+		while (timeout && last && !this) {
+			last = this;
+			this = CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP;
+			timeout--;
+		}
 	}
 
 /*
  * Here we are safe to assume the registers won't change for a whole second, so
  * we just go ahead and read them.
-	 */
-
+ *
+ * Since we may not wait here, re-added while loop from x86. -Nigel Cunningham
+ */
+	do {
 		sec = CMOS_READ(RTC_SECONDS);
 		min = CMOS_READ(RTC_MINUTES);
 		hour = CMOS_READ(RTC_HOURS);
 		day = CMOS_READ(RTC_DAY_OF_MONTH);
 		mon = CMOS_READ(RTC_MONTH);
 		year = CMOS_READ(RTC_YEAR);
+	} while (sec != CMOS_READ(RTC_SECONDS));
 
 	spin_unlock_irqrestore(&rtc_lock, flags);
arch/x86_64/kernel/x8664_ksyms.c +1 -1
diff -ruN 991-old/arch/x86_64/kernel/x8664_ksyms.c 991-new/arch/x86_64/kernel/x8664_ksyms.c
@@ -45,7 +45,7 @@
 EXPORT_SYMBOL(drive_info);
 #endif
 
-extern unsigned long get_cmos_time(void);
+extern unsigned long get_cmos_time(int wait);
 
 /* platform dependent support */
 EXPORT_SYMBOL(boot_cpu_data);
include/asm-i386/mach-default/mach_time.h +9 -7
diff -ruN 991-old/include/asm-i386/mach-default/mach_time.h 991-new/include/asm-i386/mach-default/mach_time.h
@@ -79,7 +79,7 @@
 	return retval;
 }
 
-static inline unsigned long mach_get_cmos_time(void)
+static inline unsigned long mach_get_cmos_time(int wait)
 {
 	unsigned int year, mon, day, hour, min, sec;
 	int i;

@@ -90,12 +90,14 @@
 	 * Let's hope other operating systems interpret the RTC the same way.
 	 */
 	/* read RTC exactly on falling edge of update flag */
-	for (i = 0 ; i < 1000000 ; i++)	/* may take up to 1 second... */
-		if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP)
-			break;
-	for (i = 0 ; i < 1000000 ; i++)	/* must try at least 2.228 ms */
-		if (!(CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP))
-			break;
+	if (wait) {
+		for (i = 0 ; i < 1000000 ; i++)	/* may take up to 1 second... */
+			if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP)
+				break;
+		for (i = 0 ; i < 1000000 ; i++)	/* must try at least 2.228 ms */
+			if (!(CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP))
+				break;
+	}
 	do { /* Isn't this overkill ? UIP above should guarantee consistency */
 		sec = CMOS_READ(RTC_SECONDS);
 		min = CMOS_READ(RTC_MINUTES);
Signature · 7 lines
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

You see, at just the right time, when we were still powerless, Christ
died for the ungodly.		-- Romans 5:6
#4 lore raw

Hi!

Quoted 3 lines

Add an extra parameter to get_cmos_time and to arch specific routines, allowing the user to specify whether we should wait for the beginning of a new second, or simply ensure that the time returned is accurate.

Well, I'd introduce __get_cmos_time() which is fast and implement get_cmos_time() using fast version if that is possible. That solves "change all callers" problem...
Pavel

Signature · 1 line
Boycott Kodak -- for their patent abuse against Java.
author
#5 lore raw

PATCH 3/3 Fix sysdev time support

In reply to Nigel Cunningham

Fix type of sleep_start, so as to eliminate clock skew due to math errors.

arch/i386/kernel/time.c +2 -1
diff -ruN 992-old/arch/i386/kernel/time.c 992-new/arch/i386/kernel/time.c
@@ -319,7 +319,8 @@
 	return retval;
 }
 
-static long clock_cmos_diff, sleep_start;
+static long clock_cmos_diff;
+static unsigned long sleep_start;
 
 static int time_suspend(struct sys_device *dev, u32 state)
 {
Signature · 7 lines
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

You see, at just the right time, when we were still powerless, Christ
died for the ungodly.		-- Romans 5:6
#6 lore raw

Hi!

Quoted 2 lines

Fix type of sleep_start, so as to eliminate clock skew due to math errors.

Are you sure? I do not think long signed/unsigned problem can skew the clock by 1hour. I could see skewing clock by few years, but not by one hour...
Pavel

Signature · 1 line
Boycott Kodak -- for their patent abuse against Java.
author
#7 lore raw

Hi.

Quoted 8 lines · Pavel Machek

Hi!

Quoted 2 lines

Fix type of sleep_start, so as to eliminate clock skew due to math errors.

Are you sure? I do not think long signed/unsigned problem can skew the clock by 1hour. I could see skewing clock by few years, but not by one hour...

It seemed small to me, too. Perhaps I just didn't notice the shift in the date. I'll look again, if you like.

Regards,

Nigel

Signature · 7 lines
Nigel Cunningham
Pastoral Worker
Christian Reformed Church of Tuggeranong
PO Box 1004, Tuggeranong, ACT 2901

You see, at just the right time, when we were still powerless, Christ
died for the ungodly.		-- Romans 5:6
#8 lore raw

Hi!

Quoted 9 lines
Quoted 6 lines
Quoted 2 lines

Fix type of sleep_start, so as to eliminate clock skew due to math errors.

Are you sure? I do not think long signed/unsigned problem can skew the clock by 1hour. I could see skewing clock by few years, but not by one hour...

It seemed small to me, too. Perhaps I just didn't notice the shift in the date. I'll look again, if you like.

Yes, I'd like to understand this problem.
Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!