lkml.org 
[lkml]   [2009]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [GIT pull] timers fixes for 2.6.31

* Linus Torvalds <torvalds@linux-foundation.org> 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);
}

\
 
 \ /
  Last update: 2009-07-31 14:39    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site