suspend/resume support for PIT (time.c)

suspend/resume support for PIT (time.c)

[Pavel Machek]

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?]

Re: suspend/resume support for PIT (time.c)

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> 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?

Re: suspend/resume support for PIT (time.c)

[Pavel Machek]

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?]

Re: suspend/resume support for PIT (time.c)

[john stultz]

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

Re: suspend/resume support for PIT (time.c)

[Pavel Machek]

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?]

Re: suspend/resume support for PIT (time.c)

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> 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 <pavel@ucw.cz>

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"),
 };
 

_

Re: suspend/resume support for PIT (time.c)

[Pavel Machek]

Hi!

> Pavel Machek <pavel@ucw.cz> 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 <pavel@ucw.cz>
> 
> 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?]