lkml.org 
[lkml]   [2009]   [Jun]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: fishy code in arch/x86/kernel/tsc.c:time_cpufreq_notifier()
    On Mon, 1 Jun 2009, Christoph Hellwig wrote:

    > Just notice the following error from gcc 4.4:
    >
    > arch/x86/kernel/tsc.c: In function 'time_cpufreq_notifier':
    > arch/x86/kernel/tsc.c:634: warning: 'dummy' may be used uninitialized in this function
    >
    > dummy is only used in the following way in this function:
    >
    > lpj = &dummy;
    >
    > and then dummy might be overriden in the following odd way:
    >
    > if (!(freq->flags & CPUFREQ_CONST_LOOPS))
    > #ifdef CONFIG_SMP
    > lpj = &cpu_data(freq->cpu).loops_per_jiffy;
    > #else
    > lpj = &boot_cpu_data.loops_per_jiffy;
    > #endif

    This is misindented; the if applies to both CONFIG_SMP and otherwise. For
    that matter, cpu_data(anything) == boot_cpu_data if !CONFIG_SMP. So the
    current code is equivalent to:

    if (!(freq->flags & CPUFREQ_CONST_LOOPS))
    lpj = &cpu_data(freq->cpu).loops_per_jiffy;

    > and then is used in
    >
    > if (!ref_freq) {
    > ref_freq = freq->old;
    > loops_per_jiffy_ref = *lpj;
    > tsc_khz_ref = tsc_khz;
    > }
    >
    > to me that looks like it can indeed be used unitialized for the case
    > where we do have CONFIG_SMP set, freq->flags & CPUFREQ_CONST_LOOPS is
    > true and ref_freq is false.
    >
    > Can that case actually happen?

    Looks to me like loops_per_jiffy_ref is only used to compute a new value
    for *lpj. So the case that matters is if this function can be called the
    first time with freq->flags & CPUFREQ_CONST_LOOPS and then without;
    otherwise, the uninitialized values only contribute to a dead assignment
    (and the junk in the static variable).

    I'd guess that, if freq->flags & CPUFREQ_CONST_LOOPS, no processor's
    loops_per_jiffy should get scaled, so the current code is essentially
    correct, although it's hard to read and far too hard for the compiler to
    analyze.

    Probably the right answer is to move the *lpj = ... in with
    mark_tsc_unstable and drop the earlier if and dummy.

    -Daniel
    *This .sig left intentionally blank*


    \
     
     \ /
      Last update: 2009-06-02 00:57    [W:2.928 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site