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
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?
On Mon, Jun 01, 2009 at 10:21:04AM -0400, 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
>
> where we do have CONFIG_SMP set, freq->flags & CPUFREQ_CONST_LOOPS is
> true and ref_freq is false.
>
> Can that case actually happen?
I think you're right, though the circumstances for hitting it are really
low. Nearly all SMP capable cpufreq drivers set CPUFREQ_CONST_LOOPS.
powernow-k8 is really the only exception. The older CPUs were typically
only ever UP. (powernow-k7 never supported SMP for eg)
It's probably worth fixing.
I think the patch below is closer to the actual intent, with the bonus
of being a lot more readable.
Sanity check?
Dave
Fix possible uninitialized use of dummy, by just removing it,
and making the setting of lpj more obvious.
Signed-off-by: Dave Jones <[email protected]>
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index d57de05..78c54ea 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -631,17 +631,15 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
void *data)
{
struct cpufreq_freqs *freq = data;
- unsigned long *lpj, dummy;
+ unsigned long *lpj;
if (cpu_has(&cpu_data(freq->cpu), X86_FEATURE_CONSTANT_TSC))
return 0;
- lpj = &dummy;
- if (!(freq->flags & CPUFREQ_CONST_LOOPS))
+ lpj = &boot_cpu_data.loops_per_jiffy;
#ifdef CONFIG_SMP
+ if (!(freq->flags & CPUFREQ_CONST_LOOPS))
lpj = &cpu_data(freq->cpu).loops_per_jiffy;
-#else
- lpj = &boot_cpu_data.loops_per_jiffy;
#endif
if (!ref_freq) {
On Mon, Jun 01, 2009 at 12:29:55PM -0400, Dave Jones wrote:
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index d57de05..78c54ea 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -631,17 +631,15 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
> void *data)
> {
> struct cpufreq_freqs *freq = data;
> - unsigned long *lpj, dummy;
> + unsigned long *lpj;
>
> if (cpu_has(&cpu_data(freq->cpu), X86_FEATURE_CONSTANT_TSC))
> return 0;
>
> - lpj = &dummy;
> - if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> + lpj = &boot_cpu_data.loops_per_jiffy;
> #ifdef CONFIG_SMP
> + if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> lpj = &cpu_data(freq->cpu).loops_per_jiffy;
> -#else
> - lpj = &boot_cpu_data.loops_per_jiffy;
> #endif
This makes the code look a lot more sane and should fix the potential
issue.
On Mon, 2009-06-01 at 12:39 -0400, Christoph Hellwig wrote:
> On Mon, Jun 01, 2009 at 12:29:55PM -0400, Dave Jones wrote:
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index d57de05..78c54ea 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -631,17 +631,15 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
> > void *data)
> > {
> > struct cpufreq_freqs *freq = data;
> > - unsigned long *lpj, dummy;
> > + unsigned long *lpj;
> >
> > if (cpu_has(&cpu_data(freq->cpu), X86_FEATURE_CONSTANT_TSC))
> > return 0;
> >
> > - lpj = &dummy;
> > - if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > + lpj = &boot_cpu_data.loops_per_jiffy;
> > #ifdef CONFIG_SMP
> > + if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > lpj = &cpu_data(freq->cpu).loops_per_jiffy;
> > -#else
> > - lpj = &boot_cpu_data.loops_per_jiffy;
> > #endif
>
> This makes the code look a lot more sane and should fix the potential
> issue.
Tiny niggle that you wind up setting lpj (loops per jiffy) twice if
you're on SMP and have CPUFREQ_CONST_LOOPS.
Jon.
On Mon June 1 2009, Jon Masters wrote:
> On Mon, 2009-06-01 at 12:39 -0400, Christoph Hellwig wrote:
> > On Mon, Jun 01, 2009 at 12:29:55PM -0400, Dave Jones wrote:
> > > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > > index d57de05..78c54ea 100644
> > > --- a/arch/x86/kernel/tsc.c
> > > +++ b/arch/x86/kernel/tsc.c
> > > @@ -631,17 +631,15 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
> > > void *data)
> > > {
> > > struct cpufreq_freqs *freq = data;
> > > - unsigned long *lpj, dummy;
> > > + unsigned long *lpj;
> > >
> > > if (cpu_has(&cpu_data(freq->cpu), X86_FEATURE_CONSTANT_TSC))
> > > return 0;
> > >
> > > - lpj = &dummy;
> > > - if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > > + lpj = &boot_cpu_data.loops_per_jiffy;
> > > #ifdef CONFIG_SMP
> > > + if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > > lpj = &cpu_data(freq->cpu).loops_per_jiffy;
> > > -#else
> > > - lpj = &boot_cpu_data.loops_per_jiffy;
> > > #endif
> >
> > This makes the code look a lot more sane and should fix the potential
> > issue.
>
> Tiny niggle that you wind up setting lpj (loops per jiffy) twice if
> you're on SMP and have CPUFREQ_CONST_LOOPS.
>
At least it is consistent. ;)
You set lpj twice if your not on SMP and have CPUFREG_CONST_LOOPS.
Mike
> Jon.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
On Mon June 1 2009, Michael S. Zick wrote:
Just ignore the last - I hit the "send" rather than "discard".
Mike
> On Mon June 1 2009, Jon Masters wrote:
> > On Mon, 2009-06-01 at 12:39 -0400, Christoph Hellwig wrote:
> > > On Mon, Jun 01, 2009 at 12:29:55PM -0400, Dave Jones wrote:
> > > > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > > > index d57de05..78c54ea 100644
> > > > --- a/arch/x86/kernel/tsc.c
> > > > +++ b/arch/x86/kernel/tsc.c
> > > > @@ -631,17 +631,15 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
> > > > void *data)
> > > > {
> > > > struct cpufreq_freqs *freq = data;
> > > > - unsigned long *lpj, dummy;
> > > > + unsigned long *lpj;
> > > >
> > > > if (cpu_has(&cpu_data(freq->cpu), X86_FEATURE_CONSTANT_TSC))
> > > > return 0;
> > > >
> > > > - lpj = &dummy;
> > > > - if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > > > + lpj = &boot_cpu_data.loops_per_jiffy;
> > > > #ifdef CONFIG_SMP
> > > > + if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > > > lpj = &cpu_data(freq->cpu).loops_per_jiffy;
> > > > -#else
> > > > - lpj = &boot_cpu_data.loops_per_jiffy;
> > > > #endif
> > >
> > > This makes the code look a lot more sane and should fix the potential
> > > issue.
> >
> > Tiny niggle that you wind up setting lpj (loops per jiffy) twice if
> > you're on SMP and have CPUFREQ_CONST_LOOPS.
> >
>
> At least it is consistent. ;)
> You set lpj twice if your not on SMP and have CPUFREG_CONST_LOOPS.
>
> Mike
> > Jon.
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
> >
>
>
>
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*