From: Magnus Damm <[email protected]>
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 <[email protected]>
---
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);
}
On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
> From: Magnus Damm <[email protected]>
>
> 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 <[email protected]>
Acked-by: John Stultz <[email protected]>
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);
> }
On Fri, Jun 19, 2009 at 4:17 AM, john stultz<[email protected]> wrote:
> On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
>> From: Magnus Damm <[email protected]>
>>
>> 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 <[email protected]>
>
> Acked-by: John Stultz <[email protected]>
>
> Thomas, Andrew, please push this for 2.6.31.
This one is slowly making it's way in I suppose?
/ magnus
Commit-ID: a52c77102626951144165c53a6b54f1e812360d5
Gitweb: http://git.kernel.org/tip/a52c77102626951144165c53a6b54f1e812360d5
Author: Magnus Damm <[email protected]>
AuthorDate: Tue, 28 Jul 2009 14:09:55 -0700
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
LKML-Reference: <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
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);
}
Commit-ID: c7121843685de2bf7f3afd3ae1d6a146010bf1fc
Gitweb: http://git.kernel.org/tip/c7121843685de2bf7f3afd3ae1d6a146010bf1fc
Author: Magnus Damm <[email protected]>
AuthorDate: Tue, 28 Jul 2009 14:09:55 -0700
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
LKML-Reference: <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
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);
}
* Magnus Damm <[email protected]> wrote:
> On Fri, Jun 19, 2009 at 4:17 AM, john stultz<[email protected]> wrote:
> > On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
> >> From: Magnus Damm <[email protected]>
> >>
> >> 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 <[email protected]>
> >
> > Acked-by: John Stultz <[email protected]>
> >
> > 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
On Fri, Jul 31, 2009 at 9:18 PM, Ingo Molnar<[email protected]> wrote:
>
> * Magnus Damm <[email protected]> wrote:
>
>> On Fri, Jun 19, 2009 at 4:17 AM, john stultz<[email protected]> wrote:
>> > On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
>> >> From: Magnus Damm <[email protected]>
>> >>
>> >> 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 <[email protected]>
>> >
>> > Acked-by: John Stultz <[email protected]>
>> >
>> > 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
On Fri, 2009-07-31 at 14:18 +0200, Ingo Molnar wrote:
> * Magnus Damm <[email protected]> wrote:
>
> > On Fri, Jun 19, 2009 at 4:17 AM, john stultz<[email protected]> wrote:
> > > On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
> > >> From: Magnus Damm <[email protected]>
> > >>
> > >> 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 <[email protected]>
> > >
> > > Acked-by: John Stultz <[email protected]>
> > >
> > > 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
On Fri, 2009-07-31 at 23:23 +0900, Magnus Damm wrote:
> On Fri, Jul 31, 2009 at 9:18 PM, Ingo Molnar<[email protected]> wrote:
> >
> > * Magnus Damm <[email protected]> wrote:
> >
> >> On Fri, Jun 19, 2009 at 4:17 AM, john stultz<[email protected]> wrote:
> >> > On Fri, 2009-06-19 at 00:24 +0900, Magnus Damm wrote:
> >> >> From: Magnus Damm <[email protected]>
> >> >>
> >> >> 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 <[email protected]>
> >> >
> >> > Acked-by: John Stultz <[email protected]>
> >> >
> >> > 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