Re: [PATCH -tip] x86: cpu/intel.c cleanup

[Ingo Molnar <mingo@elte.hu>]

* Vegard Nossum <vegard.nossum@gmail.com> wrote:

> 2009/3/14 Jaswinder Singh Rajput <jaswinder@kernel.org>:
> > From: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
> > Date: Sat, 14 Mar 2009 17:47:38 +0530
> > Subject: [PATCH] x86: cpu/intel.c cleanup
> >
> > - fix various style problems
> >  - fix header files issues
> >
> 
> [...]
> 
> >  static void __cpuinit early_init_intel(struct cpuinfo_x86 *c)
> >  {
> > +       u64 misc_enable;
> > +
> >        /* Unmask CPUID levels if masked: */
> >        if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {
> > -               u64 misc_enable;
> >
> >                rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> >
> > @@ -44,16 +45,16 @@ static void __cpuinit early_init_intel(struct cpuinfo_x86 *c)
> >        }
> >
> >        if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> > -               (c->x86 == 0x6 && c->x86_model >= 0x0e))
> > +           (c->x86 == 0x6 && c->x86_model >= 0x0e))
> >                set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> >
> >  #ifdef CONFIG_X86_64
> >        set_cpu_cap(c, X86_FEATURE_SYSENTER32);
> > -#else
> > +#else /* CONFIG_X86_64 */
> >        /* Netburst reports 64 bytes clflush size, but does IO in 128 bytes */
> >        if (c->x86 == 15 && c->x86_cache_alignment == 64)
> >                c->x86_cache_alignment = 128;
> > -#endif
> > +#endif /* CONFIG_X86_64 */
> >
> >        /* CPUID workaround for 0F33/0F34 CPU */
> >        if (c->x86 == 0xF && c->x86_model == 0x3
> > @@ -96,19 +97,18 @@ static void __cpuinit early_init_intel(struct cpuinfo_x86 *c)
> >         * Ingo Molnar reported a Pentium D (model 6) and a Xeon
> >         * (model 2) with the same problem.
> >         */
> > -       if (c->x86 == 15) {
> > -               u64 misc_enable;
> > +       if (c->x86 != 15)
> > +               return;
> >
> > -               rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> > +       rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> >
> > -               if (misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING) {
> > -                       printk(KERN_INFO "kmemcheck: Disabling fast string operations\n");
> > +       if (misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING) {
> > +               pr_info("kmemcheck: Disabling fast string operations\n");
> >
> > -                       misc_enable &= ~MSR_IA32_MISC_ENABLE_FAST_STRING;
> > -                       wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> > -               }
> > +               misc_enable &= ~MSR_IA32_MISC_ENABLE_FAST_STRING;
> > +               wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> >        }
> > -#endif
> > +#endif /* CONFIG_KMEMCHECK */
> >  }
> 
> I don't really like this change (last hunk). Doesn't it seem a 
> bit pointless? It breaks the symmetry with the masked CPUID 
> levels at the beginning of the function. If somebody wants to 
> add something else to this function, it might have to be 
> reindented again. Or is there a problem with too long lines 
> here?

yes, it would be cleaner to put the whole family 15 branch into 
a helper inline function instead.

	Ingo