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    [W:0.027 / U:66.328 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site