Re: [GIT pull] timers fixes for 2.6.31

[Ingo Molnar]

* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> 
> 
> On Thu, 30 Jul 2009, Thomas Gleixner wrote:
> > 
> > Please pull the latest timers-fixes-for-linus git tree from:
> > 
> >    git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git timers-fixes-for-linus
> > 
> > Thanks,
> > 
> > 	tglx
> > 
> > ------------------>
> > Magnus Damm (1):
> >       clocksource: save mult_orig in clocksource_disable()
> > 
> >  include/linux/clocksource.h |   12 +++++++++++-
> >  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> Hmm. You seem to have forgotten to push this out. I realize you're 
> off to vacation, do you have time to check that?

I've pushed out the fix. I did two minor (non-functional) edits to 
the commit: fixed the comment style to match that of the rest of 
clocksource.h and improved the commit log a tiny bit. Find the 
updated pull request below.

[ Markus, John: the fix looks right for .31 but i think there's two
  small structural problems with this code, which might have
  contributed to the bug to happen to begin with:

  Firstly, ->mult_orig is a slight misnomer - if it was named 
  properly we wouldnt even need the comments to explain how to use 
  and update it. It's the unadjusted multiplicator while _orig 
  patterns in the kernel generally suggest some sort of save/restore 
  pattern (which this is not).

  I'd suggest to rename it to ->mult_unadjusted, ->mult_raw or 
  ->mult_static instead. This field has not gotten into many 
  clocksource drivers yet so it's easy to do. For .32 obviously.

  Secondly, the broader design question is: why are clocksource
  drivers mucking around with NTP details? Whether NTP is running 
  should be a transparent detail to drivers and if such details are 
  visible in low level driver (which they are in 
  arch/arm/plat-omap/common.c et al) that's sign of uncleanliness. 
  Mind improving that? (for .32 too)

  Also, a commit message quality nit: the commit description lacks 
  details about precisely what bug was hit in practice. While this 
  fix is eligible even without that info, it's not good to omit it. 
]

Linus, please pull the latest timers-fixes-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git timers-fixes-for-linus

 Thanks,

	Ingo

------------------>
Magnus Damm (1):
      clocksource: Save mult_orig in clocksource_disable()


 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);
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/