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    [from the cache]
©2003-2011 Jasper Spaans