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

[Vegard Nossum <vegard.nossum@gmail.com>]

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?

But it's just a question of taste -- if this is the preferred style,
then it's fine.


Vegard