[PATCH] clocksource: save mult_orig in clocksource_disable()

[PATCH] clocksource: save mult_orig in clocksource_disable()

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

Save clocksource mult_orig in clocksource_disable().

To fix the common case where ->enable() does not setup
mult, make sure mult_orig is saved in mult on disable.

Also add comments to explain why we do this.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 include/linux/clocksource.h |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

--- 0001/include/linux/clocksource.h
+++ work/include/linux/clocksource.h	2009-06-19 00:12:16.000000000 +0900
@@ -293,7 +293,11 @@ static inline int clocksource_enable(str
 	if (cs->enable)
 		ret = cs->enable(cs);
 
-	/* save mult_orig on enable */
+	/* The frequency may have changed while the clocksource
+	 * was disabled. If so the code in ->enable() must update
+	 * the mult value to reflect the new frequency. Make sure
+	 * mult_orig follows this change.
+	 */
 	cs->mult_orig = cs->mult;
 
 	return ret;
@@ -309,6 +313,12 @@ static inline int clocksource_enable(str
  */
 static inline void clocksource_disable(struct clocksource *cs)
 {
+	/* Save mult_orig in mult so clocksource_enable() can
+	 * restore the value regardless if ->enable() updates
+	 * the value of mult or not.
+	 */
+	cs->mult = cs->mult_orig;
+
 	if (cs->disable)
 		cs->disable(cs);
 }

Re: [PATCH] clocksource: save mult_orig in clocksource_disable()

[john stultz]

On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> Save clocksource mult_orig in clocksource_disable().
> 
> To fix the common case where ->enable() does not setup
> mult, make sure mult_orig is saved in mult on disable.
> 
> Also add comments to explain why we do this.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>

Acked-by: John Stultz <johnstul@us.ibm.com>

Thomas, Andrew, please push this for 2.6.31.

thanks
-john


> ---
> 
>  include/linux/clocksource.h |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> --- 0001/include/linux/clocksource.h
> +++ work/include/linux/clocksource.h	2009-06-19 00:12:16.000000000 +0900
> @@ -293,7 +293,11 @@ static inline int clocksource_enable(str
>  	if (cs->enable)
>  		ret = cs->enable(cs);
> 
> -	/* save mult_orig on enable */
> +	/* The frequency may have changed while the clocksource
> +	 * was disabled. If so the code in ->enable() must update
> +	 * the mult value to reflect the new frequency. Make sure
> +	 * mult_orig follows this change.
> +	 */
>  	cs->mult_orig = cs->mult;
> 
>  	return ret;
> @@ -309,6 +313,12 @@ static inline int clocksource_enable(str
>   */
>  static inline void clocksource_disable(struct clocksource *cs)
>  {
> +	/* Save mult_orig in mult so clocksource_enable() can
> +	 * restore the value regardless if ->enable() updates
> +	 * the value of mult or not.
> +	 */
> +	cs->mult = cs->mult_orig;
> +
>  	if (cs->disable)
>  		cs->disable(cs);
>  }

Re: [PATCH] clocksource: save mult_orig in clocksource_disable()

[Magnus Damm]

On Fri, Jun 19, 2009 at 4:17 AM, john stultz<johnstul@us.ibm.com> wrote:
> On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> Save clocksource mult_orig in clocksource_disable().
>>
>> To fix the common case where ->enable() does not setup
>> mult, make sure mult_orig is saved in mult on disable.
>>
>> Also add comments to explain why we do this.
>>
>> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>
> Acked-by: John Stultz <johnstul@us.ibm.com>
>
> Thomas, Andrew, please push this for 2.6.31.

This one is slowly making it's way in I suppose?

/ magnus

Re: [PATCH] clocksource: save mult_orig in clocksource_disable()

[Ingo Molnar]

* Magnus Damm <magnus.damm@gmail.com> wrote:

> On Fri, Jun 19, 2009 at 4:17 AM, john stultz<johnstul@us.ibm.com> wrote:
> > On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
> >> From: Magnus Damm <damm@igel.co.jp>
> >>
> >> Save clocksource mult_orig in clocksource_disable().
> >>
> >> To fix the common case where ->enable() does not setup
> >> mult, make sure mult_orig is saved in mult on disable.
> >>
> >> Also add comments to explain why we do this.
> >>
> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >
> > Acked-by: John Stultz <johnstul@us.ibm.com>
> >
> > Thomas, Andrew, please push this for 2.6.31.
> 
> This one is slowly making it's way in I suppose?

Btw., what specific issue does this fix? The commit description is 
generic, there's no bugzilla link and no other information either 
that could give me an idea about precisely what incarnation of the 
bug you have hit.

Thanks,

	Ingo

Re: [PATCH] clocksource: save mult_orig in clocksource_disable()

[Magnus Damm]

On Fri, Jul 31, 2009 at 9:18 PM, Ingo Molnar<mingo@elte.hu> wrote:
>
> * Magnus Damm <magnus.damm@gmail.com> wrote:
>
>> On Fri, Jun 19, 2009 at 4:17 AM, john stultz<johnstul@us.ibm.com> wrote:
>> > On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
>> >> From: Magnus Damm <damm@igel.co.jp>
>> >>
>> >> Save clocksource mult_orig in clocksource_disable().
>> >>
>> >> To fix the common case where ->enable() does not setup
>> >> mult, make sure mult_orig is saved in mult on disable.
>> >>
>> >> Also add comments to explain why we do this.
>> >>
>> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> >
>> > Acked-by: John Stultz <johnstul@us.ibm.com>
>> >
>> > Thomas, Andrew, please push this for 2.6.31.
>>
>> This one is slowly making it's way in I suppose?
>
> Btw., what specific issue does this fix? The commit description is
> generic, there's no bugzilla link and no other information either
> that could give me an idea about precisely what incarnation of the
> bug you have hit.

The comments in the actual code gives more details, but that's
probably not where you want this to be explained. I can resend a
version with more verbose commit message early next week if you'd
like. Please let me know if so.

I'm not aware of any bugzilla links, maybe John knows?

Cheers,

/ magnus

Re: [PATCH] clocksource: save mult_orig in clocksource_disable()

[john stultz]

On Fri, 2009-07-31 at 23:23 +0900, Magnus Damm wrote:
> On Fri, Jul 31, 2009 at 9:18 PM, Ingo Molnar<mingo@elte.hu> wrote:
> >
> > * Magnus Damm <magnus.damm@gmail.com> wrote:
> >
> >> On Fri, Jun 19, 2009 at 4:17 AM, john stultz<johnstul@us.ibm.com> wrote:
> >> > On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
> >> >> From: Magnus Damm <damm@igel.co.jp>
> >> >>
> >> >> Save clocksource mult_orig in clocksource_disable().
> >> >>
> >> >> To fix the common case where ->enable() does not setup
> >> >> mult, make sure mult_orig is saved in mult on disable.
> >> >>
> >> >> Also add comments to explain why we do this.
> >> >>
> >> >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> >> >
> >> > Acked-by: John Stultz <johnstul@us.ibm.com>
> >> >
> >> > Thomas, Andrew, please push this for 2.6.31.
> >>
> >> This one is slowly making it's way in I suppose?
> >
> > Btw., what specific issue does this fix? The commit description is
> > generic, there's no bugzilla link and no other information either
> > that could give me an idea about precisely what incarnation of the
> > bug you have hit.
> 
> The comments in the actual code gives more details, but that's
> probably not where you want this to be explained. I can resend a
> version with more verbose commit message early next week if you'd
> like. Please let me know if so.
> 
> I'm not aware of any bugzilla links, maybe John knows?

No bugzilla link. I found this while reviewing the patch, however at
that point it had already been pulled into -tip and my objections
somehow never made it to anyone's eyes before it got pulled into
mainline.

thanks
-john

Re: [PATCH] clocksource: save mult_orig in clocksource_disable()

[john stultz]

On Fri, 2009-07-31 at 14:18 +0200, Ingo Molnar wrote:
> * Magnus Damm <magnus.damm@gmail.com> wrote:
> 
> > On Fri, Jun 19, 2009 at 4:17 AM, john stultz<johnstul@us.ibm.com> wrote:
> > > On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
> > >> From: Magnus Damm <damm@igel.co.jp>
> > >>
> > >> Save clocksource mult_orig in clocksource_disable().
> > >>
> > >> To fix the common case where ->enable() does not setup
> > >> mult, make sure mult_orig is saved in mult on disable.
> > >>
> > >> Also add comments to explain why we do this.
> > >>
> > >> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> > >
> > > Acked-by: John Stultz <johnstul@us.ibm.com>
> > >
> > > Thomas, Andrew, please push this for 2.6.31.
> > 
> > This one is slowly making it's way in I suppose?
> 
> Btw., what specific issue does this fix? The commit description is 
> generic, there's no bugzilla link and no other information either 
> that could give me an idea about precisely what incarnation of the 
> bug you have hit.

Magnus' earlier change to mult_orig makes it so mult_orig is written on
enable, instead of when we register the clocksource. This causes
problems as enable() might be called multiple times while a system is
running, where as the register would only happen once.

If a user switches away from a clocksource and back to a clocksource,
the mult_orig will no longer be the original mult, as we will over write
it with an NTP adjusted mult.

Its a minor detail, but it causes problems when I do NTP testing across
different clocksources, as the ppm drift factor does not stay constant
per clocksource.

This fix makes sure we store mult_orig over mult on disable() so we
restore the correct value on the next enable().

thanks
-john

[tip:timers/urgent] clocksource: save mult_orig in clocksource_disable()

[tip-bot for Magnus Damm]

Commit-ID:  a52c77102626951144165c53a6b54f1e812360d5
Gitweb:     http://git.kernel.org/tip/a52c77102626951144165c53a6b54f1e812360d5
Author:     Magnus Damm <magnus.damm@gmail.com>
AuthorDate: Tue, 28 Jul 2009 14:09:55 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 30 Jul 2009 21:55:47 +0200

clocksource: save mult_orig in clocksource_disable()

Save clocksource mult_orig in clocksource_disable().

To fix the common case where ->enable() does not setup
mult, make sure mult_orig is saved in mult on disable.

Also add comments to explain why we do this.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
Cc: johnstul@us.ibm.com
Cc: lethal@linux-sh.org
Cc: akpm@linux-foundation.org
LKML-Reference: <20090618152432.10136.9932.sendpatchset@rx1.opensource.se>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>


---
 include/linux/clocksource.h |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index c56457c..aff9f01 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -293,7 +293,11 @@ static inline int clocksource_enable(struct clocksource *cs)
 	if (cs->enable)
 		ret = cs->enable(cs);
 
-	/* save mult_orig on enable */
+	/* The frequency may have changed while the clocksource
+	 * was disabled. If so the code in ->enable() must update
+	 * the mult value to reflect the new frequency. Make sure
+	 * mult_orig follows this change.
+	 */
 	cs->mult_orig = cs->mult;
 
 	return ret;
@@ -309,6 +313,12 @@ static inline int clocksource_enable(struct clocksource *cs)
  */
 static inline void clocksource_disable(struct clocksource *cs)
 {
+	/* Save mult_orig in mult so clocksource_enable() can
+	 * restore the value regardless if ->enable() updates
+	 * the value of mult or not.
+	 */
+	cs->mult = cs->mult_orig;
+
 	if (cs->disable)
 		cs->disable(cs);
 }

[tip:timers/urgent] clocksource: Save mult_orig in clocksource_disable()

[tip-bot for Magnus Damm]

Commit-ID:  c7121843685de2bf7f3afd3ae1d6a146010bf1fc
Gitweb:     http://git.kernel.org/tip/c7121843685de2bf7f3afd3ae1d6a146010bf1fc
Author:     Magnus Damm <magnus.damm@gmail.com>
AuthorDate: Tue, 28 Jul 2009 14:09:55 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 31 Jul 2009 14:12:36 +0200

clocksource: Save mult_orig in clocksource_disable()

To fix the common case where ->enable() does not set up
mult, make sure mult_orig is saved in mult on disable.

Also add comments to explain why we do this.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
Cc: johnstul@us.ibm.com
Cc: lethal@linux-sh.org
Cc: akpm@linux-foundation.org
LKML-Reference: <20090618152432.10136.9932.sendpatchset@rx1.opensource.se>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>


---
 include/linux/clocksource.h |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index c56457c..1219be4 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -293,7 +293,12 @@ static inline int clocksource_enable(struct clocksource *cs)
 	if (cs->enable)
 		ret = cs->enable(cs);
 
-	/* save mult_orig on enable */
+	/*
+	 * The frequency may have changed while the clocksource
+	 * was disabled. If so the code in ->enable() must update
+	 * the mult value to reflect the new frequency. Make sure
+	 * mult_orig follows this change.
+	 */
 	cs->mult_orig = cs->mult;
 
 	return ret;
@@ -309,6 +314,13 @@ static inline int clocksource_enable(struct clocksource *cs)
  */
 static inline void clocksource_disable(struct clocksource *cs)
 {
+	/*
+	 * Save mult_orig in mult so clocksource_enable() can
+	 * restore the value regardless if ->enable() updates
+	 * the value of mult or not.
+	 */
+	cs->mult = cs->mult_orig;
+
 	if (cs->disable)
 		cs->disable(cs);
 }