Re: [patch 05/39] [PATCH] i386: Move phys_proc_id/early intel workaround to corr

Re: [patch 05/39] [PATCH] i386: Move phys_proc_id/early intel workaround to corr

[Chuck Ebbert]

In-Reply-To: <20060227223324.852289000@sorel.sous-sol.org>

On Mon, 27 Feb 2006 14:32:05, Chris Wright wrote:

> -stable review patch.  If anyone has any objections, please let us know.
> ------------------
> 
> early_cpu_detect only runs on the BP, but this code needs to run
> on all CPUs. This will fix problems with the powernow-k8 driver
> on dual core systems and general misdetection of AMD dual core.
> 
> Looks like a mismerge somewhere.  Also add a warning comment.
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> ---
> 
>  arch/i386/kernel/cpu/common.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> --- linux-2.6.15.4.orig/arch/i386/kernel/cpu/common.c
> +++ linux-2.6.15.4/arch/i386/kernel/cpu/common.c
> @@ -207,7 +207,10 @@ static int __devinit have_cpuid_p(void)
>  
>  /* Do minimum CPU detection early.
>     Fields really needed: vendor, cpuid_level, family, model, mask, cache alignment.
> -   The others are not touched to avoid unwanted side effects. */
> +   The others are not touched to avoid unwanted side effects.
> +
> +   WARNING: this function is only called on the BP.  Don't add code here
> +   that is supposed to run on all CPUs. */
>  static void __init early_cpu_detect(void)
>  {
>       struct cpuinfo_x86 *c = &boot_cpu_data;
> @@ -239,12 +242,6 @@ static void __init early_cpu_detect(void
>               if (cap0 & (1<<19))
>                       c->x86_cache_alignment = ((misc >> 8) & 0xff) * 8;
>       }
> -
> -     early_intel_workaround(c);

==> This has been in this location since at least 2.6.9.  If it's also needed
    in generic_identify(), fine, but it should be left here too.  I think this
    patch in 2.6.16-rc is wrong.  The comment for this function says it really
    needs accurate cache alignment information, presumably because it's needed
    for early boot setup, and without this it won't be accurate.


> -
> -#ifdef CONFIG_X86_HT
> -     phys_proc_id[smp_processor_id()] = (cpuid_ebx(1) >> 24) & 0xff;
> -#endif
>  }
>  
>  void __devinit generic_identify(struct cpuinfo_x86 * c)
> @@ -292,6 +289,12 @@ void __devinit generic_identify(struct c
>                               get_model_name(c); /* Default name */
>               }
>       }
> +
> +     early_intel_workaround(c);
> +
> +#ifdef CONFIG_X86_HT
> +     phys_proc_id[smp_processor_id()] = (cpuid_ebx(1) >> 24) & 0xff;
> +#endif
>  }
>  
>  static void __devinit squash_the_stupid_serial_number(struct cpuinfo_x86 *c)
>

-- 
Chuck
"Equations are the Devil's sentences."  --Stephen Colbert

Re: [patch 05/39] [PATCH] i386: Move phys_proc_id/early intel workaround to corr

[Andi Kleen]

> ==> This has been in this location since at least 2.6.9.  If it's also
> needed in generic_identify(), fine, but it should be left here too.  I
> think this patch in 2.6.16-rc is wrong.  The comment for this function says
> it really needs accurate cache alignment information, presumably because
> it's needed for early boot setup, and without this it won't be accurate.

Actually it's ok because early_cpu_detect() is called from setup_arch()
which is way before kmem_cache_init() which is the first user of the 
alignment information.

So I think the patch is fine. You're right the comment could need updating
though.

Thanks for the review.

-Andi