Undocumented and duplicated code

From: Adrian Bunk
Date: Wed May 07 2008 - 08:48:25 EST


On Tue, May 06, 2008 at 07:39:44PM -0700, Yinghai Lu wrote:
> On Tue, May 6, 2008 at 6:48 PM, Rene Herman <rene.herman@xxxxxxxxxxxx> wrote:
> > Good day.
> >
> > On 2.6.25 and below, my /proc/cpuinfo looks like:
> >
> > processor : 0
> > vendor_id : AuthenticAMD
> > cpu family : 6
> > model : 7
> > model name : AMD Duron(tm) Processor
> > stepping : 1
> > cpu MHz : 1312.969
> > cache size : 64 KB
> > fdiv_bug : no
> > hlt_bug : no
> > f00f_bug : no
> > coma_bug : no
> > fpu : yes
> > fpu_exception : yes
> > cpuid level : 1
> > wp : yes
> > flags : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca cmov
> > pat pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow ts
> > bogomips : 2628.68
> > clflush size : 32
> >
> > while on current mainline PAT and TS (Temperature Sensor) drop from the
> > feature flags:
> >
> > flags : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca cmov
> > pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow
> >
> > With respect to PAT, I guess it's 9307cacad0dfe3749f00303125c6f7f0523e5616,
> > "x86: pat cpu feature bit setting for known cpus" but what's this about?
> >
> > Did my cpuinfo lie upto this point or shouldn't the flag be cleared? The
> > commit message for that change is completely and totally unhelpful.
>
> others like to to whitebox methods, ..., please try attach patch to
> see if duron support PAT.


There surely is documentation available covering this?

And why do we need this clear_cpu_cap(c, X86_FEATURE_PAT) and then
manual setting of X86_FEATURE_PAT at all?

There's no indication in the code, and as Rene already says there's even
no description at all in commit 9307cacad0dfe3749f00303125c6f7f0523e5616

Such code really needs a comment explaining why we have to do this at all.

There must be some CPUs with the "pat" flag set but not being usable?
Which?

According to the linux-kernel discussions there might not be any broken
CPU at all - but in this case the whitelist will not fill itself, and
expecting people to note that their flags changed and complaining is not
really a good approach.

And that your commit added the same clear/set code in three different
places doesn't look good - this really deserved from the beginning being
factored out into an own function to avoid future problems when CPUs get
added (like what happens with your patch here - it touches only one
place, and since the same context is present in two places in the same
file "patch" might even choose freely where it gets applied...).

Pavel even made a similar comment on linux-kernel before the patch
got merged into Linus' tree. [1]

Guys, even if it compiles in all randconfig configurations and works on
all test machines this is exactly the kind of stuff that causes
headaches in the future.

And this patch (by the author of the code himself) is the first time
where it breaks.


> YH

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index a428ffc..81483ec 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -314,6 +314,8 @@ static void __cpuinit early_get_cap(struct cpuinfo_x86 *c)
> case X86_VENDOR_AMD:
> if (c->x86 >= 0xf && c->x86 <= 0x11)
> set_cpu_cap(c, X86_FEATURE_PAT);
> + if (c->x86 == 6 && c->x86_modes == 7)
> + set_cpu_cap(c, X86_FEATURE_PAT);
> break;
> case X86_VENDOR_INTEL:
> if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))


cu
Adrian

[1] http://lkml.org/lkml/2008/3/28/184

--

"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/