suspend/resume support for PIT (time.c)
Hi!
This adds proper suspend/resume support for PIT. That means that clock are actually correct after suspend/resume. Please apply,
Pavel
arch/i386/kernel/time.c +23 -0
@@ -307,7 +307,30 @@return retval; } +static long clock_cmos_diff; + +static int pit_suspend (struct sys_device *dev,u32 state) +{ +/* +* Estimate time zone so that set_time can update the clock +*/ + clock_cmos_diff = -get_cmos_time (); + clock_cmos_diff +=get_seconds (); +return 0 ; +} + +static int pit_resume (struct sys_device *dev) +{ +write_seqlock_irq (&xtime_lock); + xtime.tv_sec =get_cmos_time () + clock_cmos_diff; + xtime.tv_nsec =0 ; +write_sequnlock_irq (&xtime_lock); +return 0 ; +} +static struct sysdev_class pit_sysclass = { + .resume = pit_resume, + .suspend = pit_suspend,set_kset_name ("pit" ), };
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 9 lines · Pavel Machek
+static int
pit_resume(struct sys_device *dev)
+{
+write_seqlock_irq(&xtime_lock);
+ xtime.tv_sec =get_cmos_time()+clock_cmos_diff;
+ xtime.tv_nsec = 0;
+write_sequnlock_irq(&xtime_lock);
+ return 0;
+}
Have you checked the lock ranking here? get_cmos_time() takes a ton of locks, especially if EFI is enabled. Do all those rank inside xtime_lock?
It _looks_ right, but perhaps it would be saner to move the get_coms_time() call outside the lock?
Hi!
Quoted 9 lines
Quoted 8 lines
+static int
pit_resume(struct sys_device *dev)
+{
+write_seqlock_irq(&xtime_lock);
+ xtime.tv_sec =get_cmos_time()+clock_cmos_diff;
+ xtime.tv_nsec = 0;
+write_sequnlock_irq(&xtime_lock);
+ return 0;
+}
Quoted 5 lines
Have you checked the lock ranking here?
get_cmos_time()takes a ton of locks, especially if EFI is enabled. Do all those rank insidextime_lock?It _looks_ right, but perhaps it would be saner to move the
get_coms_time()call outside the lock?
Yes, good idea, I do not want to check every change in get_cmos_time().
Pavel
arch/i386/kernel/time.c +24 -0
@@ -307,7 +307,31 @@return retval; } +static long clock_cmos_diff; + +static int pit_suspend (struct sys_device *dev,u32 state) +{ +/* +* Estimate time zone so that set_time can update the clock +*/ + clock_cmos_diff = -get_cmos_time (); + clock_cmos_diff +=get_seconds (); +return 0 ; +} + +static int pit_resume (struct sys_device *dev) +{ +unsigned long sec =get_cmos_time () + clock_cmos_diff; +write_seqlock_irq (&xtime_lock); + xtime.tv_sec = sec; + xtime.tv_nsec =0 ; +write_sequnlock_irq (&xtime_lock); +return 0 ; +} +static struct sysdev_class pit_sysclass = { + .resume = pit_resume, + .suspend = pit_suspend,set_kset_name ("pit" ), };
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 1 line · Pavel Machek
+static int
pit_suspend(struct sys_device *dev, u32 state)
[snip]
Quoted 8 lines
arch/i386/kernel/time.c +8
-0+static int pit_resume (struct sys_device *dev) +{ +write_seqlock_irq (&xtime_lock); + xtime.tv_sec =get_cmos_time () + clock_cmos_diff; + xtime.tv_nsec =0 ; +write_sequnlock_irq (&xtime_lock); +return 0 ; +}
[snip]
arch/i386/kernel/time.c line 335
Quoted 5 lines
static struct
sysdev_classpit_sysclass= {arch/i386/kernel/time.c +2
-0+ .resume = pit_resume, + .suspend = pit_suspend, set_kset_name("pit"), };
As none of this really has anything to do w/ the PIT, and to avoid confusion w/ the PIT timesource code, could we rename this to "time_suspend" and "time_resume"?
thanks
-john
Pavel Machek
AppliedHi!
Quoted 12 lines · Pavel Machek
Quoted 1 line
+static int
pit_suspend(struct sys_device *dev, u32 state)[snip]
Quoted 8 lines
arch/i386/kernel/time.c +8
-0+static int pit_resume (struct sys_device *dev) +{ +write_seqlock_irq (&xtime_lock); + xtime.tv_sec =get_cmos_time () + clock_cmos_diff; + xtime.tv_nsec =0 ; +write_sequnlock_irq (&xtime_lock); +return 0 ; +}[snip]
Quoted 9 lines
arch/i386/kernel/time.c line 335
Quoted 5 lines
static struct
sysdev_classpit_sysclass= {arch/i386/kernel/time.c +2
-0+ .resume = pit_resume, + .suspend = pit_suspend, set_kset_name("pit"), };As none of this really has anything to do w/ the PIT, and to avoid confusion w/ the PIT timesource code, could we rename this to "
time_suspend" and "time_resume"?
Applied, altrough I'll not try to push it for a while. (I'm not sure if Andrew applied previous patches and do not want to clash).
Pavel
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]
Quoted 7 lines · Pavel Machek
Quoted 4 lines
As none of this really has anything to do w/ the PIT, and to avoid confusion w/ the PIT timesource code, could we rename this to "
time_suspend" and "time_resume"?Applied, altrough I'll not try to push it for a while. (I'm not sure if Andrew applied previous patches and do not want to clash).
I already converted the patch. Here's what I currently have:
From: Pavel Machek <[email protected]>
This adds proper suspend/resume support for PIT. That means that clock are actually correct after suspend/resume.
---
25-akpm/arch/i386/kernel/time.c | 24 ++++++++++++++++++++++++ 1 files changed, 24 insertions(+)
arch/i386/kernel/time.c renamed from arch/i386/kernel/time.c~suspend-resume-for-PIT +24 -0
@@ -307,7 +307,31 @@ unsigned long get_cmos_time(void)return retval; } +static long clock_cmos_diff; + +static int time_suspend (struct sys_device *dev,u32 state) +{ +/* +* Estimate time zone so that set_time can update the clock +*/ + clock_cmos_diff = -get_cmos_time (); + clock_cmos_diff +=get_seconds (); +return 0 ; +} + +static int time_resume (struct sys_device *dev) +{ +unsigned long sec =get_cmos_time () + clock_cmos_diff; +write_seqlock_irq (&xtime_lock); + xtime.tv_sec = sec; + xtime.tv_nsec =0 ; +write_sequnlock_irq (&xtime_lock); +return 0 ; +} +static struct sysdev_class pit_sysclass = { + .resume = time_resume, + .suspend = time_suspend,set_kset_name ("pit" ), };
_
Hi!
Quoted 10 lines
Quoted 7 lines · Pavel Machek
Quoted 4 lines
As none of this really has anything to do w/ the PIT, and to avoid confusion w/ the PIT timesource code, could we rename this to "
time_suspend" and "time_resume"?Applied, altrough I'll not try to push it for a while. (I'm not sure if Andrew applied previous patches and do not want to clash).
I already converted the patch. Here's what I currently have:
Looks good, thanks a lot.
Quoted 48 lines
From: Pavel Machek <[email protected]>
This adds proper suspend/resume support for PIT. That means that clock are actually correct after suspend/resume.
---
25-akpm/arch/i386/kernel/time.c | 24 ++++++++++++++++++++++++ 1 files changed, 24 insertions(+)diff -puN
arch/i386/kernel/time.c~suspend-resume-for-PITarch/i386/kernel/time.carch/i386/kernel/time.c renamed from arch/i386/kernel/time.c~suspend-resume-for-PIT +24
-0@@ -307,7 +307,31 @@ unsigned long get_cmos_time(void)return retval; } +static long clock_cmos_diff; + +static int time_suspend (struct sys_device *dev,u32 state) +{ +/* +* Estimate time zone so that set_time can update the clock +*/ + clock_cmos_diff = -get_cmos_time (); + clock_cmos_diff +=get_seconds (); +return 0 ; +} + +static int time_resume (struct sys_device *dev) +{ +unsigned long sec =get_cmos_time () + clock_cmos_diff; +write_seqlock_irq (&xtime_lock); + xtime.tv_sec = sec; + xtime.tv_nsec =0 ; +write_sequnlock_irq (&xtime_lock); +return 0 ; +} +static struct sysdev_class pit_sysclass = { + .resume = time_resume, + .suspend = time_suspend,set_kset_name ("pit" ), };_
Signature · 2 lines
When do you have a heart between your knees? [Johanka's followup: and *two* hearts?]