Re: [tip:x86/mce] x86, mce: Make xeon75xx memory driver dependent onPCI

[Ingo Molnar]

* tip-bot for Andi Kleen <andi@xxxxxxxxxxxxxx> wrote:

> Commit-ID:  757fd770c649b0dfa6eeefc2d5e2ea3119b6be9c
> Gitweb:     http://git.kernel.org/tip/757fd770c649b0dfa6eeefc2d5e2ea3119b6be9c
> Author:     Andi Kleen <andi@xxxxxxxxxxxxxx>
> AuthorDate: Sat, 23 Jan 2010 12:33:59 +0100
> Committer:  H. Peter Anvin <hpa@xxxxxxxxx>
> CommitDate: Fri, 5 Feb 2010 14:51:53 -0800
> 
> x86, mce: Make xeon75xx memory driver dependent on PCI
> 
> Found by Ingo Molnar's automated tester.
> 
> Reported-by: Ingo Molnar <mingo@xxxxxxx>
> Signed-off-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> LKML-Reference: <20100123113359.GA29555@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: H. Peter Anvin <hpa@xxxxxxxxx>

As an x86 maintainer i'm NAK-ing your Nehalem MCE patches. I really dont want 
to see this code in v2.6.34, please work on it some more - this should be 
implemented _properly_ and _cleanly_.

Andi, you've ignored my repeated complaints about the cleanliness of the MCE 
code:

  http://linux.derkeiler.com/Mailing-Lists/Kernel/2010-01/msg08454.html

  ( There's been earlier remarks from me on this topic, months ago, the first 
    one more than a year ago, so it's not like you didnt have any advance 
    warning. I suspect i should have NAK-ed earlier. )

and you've ignored what the EDAC developers such as Mauro tried to explain to 
you in that same thread. Your design to expose Intel hardware features sucks 
(because it's essentially non-existent, and because it exposes so little and 
gives users so little benefits) and you should know it.

Please work with Mauro on the Nehalem EDAC bits, they seem rather advanced to 
me for v2.6.34, and _far_ cleaner and more capable as well. See those Intel 
support bits at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/i7core.git master

  Documentation/edac.txt              |  151 +++
  arch/x86/include/asm/pci_x86.h      |    2 +
  arch/x86/kernel/cpu/mcheck/mce_64.c | 1200 +++++++++++++++++++++
  arch/x86/pci/legacy.c               |   43 +-
  drivers/edac/Kconfig                |   19 +
  drivers/edac/Makefile               |    7 +
  drivers/edac/edac_core.h            |    5 +
  drivers/edac/edac_mc_sysfs.c        |    4 +
  drivers/edac/edac_mce.c             |   61 ++
  drivers/edac/i7core_edac.c          | 1966 +++++++++++++++++++++++++++++++++++
  include/linux/edac_mce.h            |   31 +
  include/linux/pci.h                 |    1 +
  include/linux/pci_ids.h             |   19 +
  13 files changed, 3492 insertions(+), 17 deletions(-)

It's a big step forward for Intel CPU features in my opinion, as it is a 
proper, clean driver interface, and i find it highly curious why you are not 
coding for that feature instead.

Once that is done the EDAC code can be pushed by all distros as a common 
interface, as recent Intel CPUs would be supported by it.

I really do not understand why you are trying to keep Intel CPUs in the dark 
ages while most other CPUs are properly supported by the EDAC code ... It 
seems hugely counter-intuitive to me.

Note, any missing functionality on the EDAC side needs a proper driver 
interface to arch/x86 - not this kind of ugly butchered-in 700 lines piece of 
ugly crap that you are trying to push here ... I'd welcome patches for such 
interfaces as they'd further simplify (and also enhance) arch/x86.

( And as i suggested you might also want to work on representing machine
  events as part of the perf events framework. )

	Ingo
--
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/