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
--
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
Fix type of sleep_start, so as to eliminate clock skew due to math errors.
diff -ruN 992-old/arch/i386/kernel/time.c 992-new/arch/i386/kernel/time.c
--- 992-old/arch/i386/kernel/time.c 2004-11-12 09:19:22.219857040 +1100
+++ 992-new/arch/i386/kernel/time.c 2004-11-12 09:13:12.000000000 +1100
@@ -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)
{
--
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
Make time_suspend and time_resume call get_cmos_time once only, so as to
eliminate unnecessary 1 second delays in suspending and resuming.
diff -ruN 990-old/arch/i386/kernel/time.c 990-new/arch/i386/kernel/time.c
--- 990-old/arch/i386/kernel/time.c 2004-11-12 08:12:58.275103280 +1100
+++ 990-new/arch/i386/kernel/time.c 2004-11-12 07:16:41.000000000 +1100
@@ -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;
--
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
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.
diff -ruN 991-old/arch/cris/kernel/time.c 991-new/arch/cris/kernel/time.c
--- 991-old/arch/cris/kernel/time.c 2004-11-03 21:53:48.000000000 +1100
+++ 991-new/arch/cris/kernel/time.c 2004-11-12 09:20:09.000000000 +1100
@@ -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;
diff -ruN 991-old/arch/i386/kernel/apm.c 991-new/arch/i386/kernel/apm.c
--- 991-old/arch/i386/kernel/apm.c 2004-11-03 21:51:14.000000000 +1100
+++ 991-new/arch/i386/kernel/apm.c 2004-11-12 09:20:09.000000000 +1100
@@ -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
diff -ruN 991-old/arch/i386/kernel/i386_ksyms.c 991-new/arch/i386/kernel/i386_ksyms.c
--- 991-old/arch/i386/kernel/i386_ksyms.c 2004-11-03 21:55:02.000000000 +1100
+++ 991-new/arch/i386/kernel/i386_ksyms.c 2004-11-12 09:20:09.000000000 +1100
@@ -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);
diff -ruN 991-old/arch/i386/kernel/time.c 991-new/arch/i386/kernel/time.c
--- 991-old/arch/i386/kernel/time.c 2004-11-12 09:27:07.129404328 +1100
+++ 991-new/arch/i386/kernel/time.c 2004-11-12 09:27:13.982362520 +1100
@@ -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;
diff -ruN 991-old/arch/sh/boards/mpc1211/rtc.c 991-new/arch/sh/boards/mpc1211/rtc.c
--- 991-old/arch/sh/boards/mpc1211/rtc.c 2004-11-03 21:53:48.000000000 +1100
+++ 991-new/arch/sh/boards/mpc1211/rtc.c 2004-11-12 09:20:09.000000000 +1100
@@ -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);
diff -ruN 991-old/arch/x86_64/kernel/time.c 991-new/arch/x86_64/kernel/time.c
--- 991-old/arch/x86_64/kernel/time.c 2004-11-03 21:54:16.000000000 +1100
+++ 991-new/arch/x86_64/kernel/time.c 2004-11-12 09:20:09.000000000 +1100
@@ -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);
diff -ruN 991-old/arch/x86_64/kernel/x8664_ksyms.c 991-new/arch/x86_64/kernel/x8664_ksyms.c
--- 991-old/arch/x86_64/kernel/x8664_ksyms.c 2004-11-03 21:51:36.000000000 +1100
+++ 991-new/arch/x86_64/kernel/x8664_ksyms.c 2004-11-12 09:20:09.000000000 +1100
@@ -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);
diff -ruN 991-old/include/asm-i386/mach-default/mach_time.h 991-new/include/asm-i386/mach-default/mach_time.h
--- 991-old/include/asm-i386/mach-default/mach_time.h 2004-11-03 21:53:12.000000000 +1100
+++ 991-new/include/asm-i386/mach-default/mach_time.h 2004-11-12 09:20:09.000000000 +1100
@@ -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);
--
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
Hi!
> 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
--
Boycott Kodak -- for their patent abuse against Java.
Hi!
> 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
--
Boycott Kodak -- for their patent abuse against Java.
Hi!
> 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
--
Boycott Kodak -- for their patent abuse against Java.
Hi.
On Fri, 2004-11-12 at 19:00, Pavel Machek wrote:
> Hi!
>
> > 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
--
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
Hi!
> > > 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
--
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!