Hi!
This adds proper suspend/resume support for PIT. That means that clock
are actually correct after suspend/resume. Please apply,
Pavel
Index: linux/arch/i386/kernel/time.c
===================================================================
--- linux.orig/arch/i386/kernel/time.c 2004-01-09 20:26:08.000000000 +0100
+++ linux/arch/i386/kernel/time.c 2004-01-09 20:33:05.000000000 +0100
@@ -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"),
};
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
Pavel Machek <[email protected]> wrote:
>
> +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!
> > +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?
Yes, good idea, I do not want to check every change in
get_cmos_time().
Pavel
Index: linux/arch/i386/kernel/time.c
===================================================================
--- linux.orig/arch/i386/kernel/time.c 2004-01-09 20:26:08.000000000 +0100
+++ linux/arch/i386/kernel/time.c 2004-01-11 12:12:48.000000000 +0100
@@ -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"),
};
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
On Sat, 2004-01-10 at 12:03, Pavel Machek wrote:
> +static int pit_suspend(struct sys_device *dev, u32 state)
[snip]
> +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]
> static struct sysdev_class pit_sysclass = {
> + .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
Hi!
> On Sat, 2004-01-10 at 12:03, Pavel Machek wrote:
> > +static int pit_suspend(struct sys_device *dev, u32 state)
> [snip]
> > +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]
> > static struct sysdev_class pit_sysclass = {
> > + .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
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
Hi!
> Pavel Machek <[email protected]> wrote:
> >
> > > 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.
> 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-PIT arch/i386/kernel/time.c
> --- 25/arch/i386/kernel/time.c~suspend-resume-for-PIT Mon Jan 12 13:49:55 2004
> +++ 25-akpm/arch/i386/kernel/time.c Mon Jan 12 13:49:55 2004
> @@ -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"),
> };
>
>
> _
--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
Pavel Machek <[email protected]> wrote:
>
> > 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(+)
diff -puN arch/i386/kernel/time.c~suspend-resume-for-PIT arch/i386/kernel/time.c
--- 25/arch/i386/kernel/time.c~suspend-resume-for-PIT Mon Jan 12 13:49:55 2004
+++ 25-akpm/arch/i386/kernel/time.c Mon Jan 12 13:49:55 2004
@@ -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"),
};
_