edac_mce module is an interface module that gets mcelog data and
forwards to any registered edac module that expects to receive data via
mce.
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
arch/x86/kernel/cpu/mcheck/mce.c | 12 ++++++++
drivers/edac/Kconfig | 8 ++++-
drivers/edac/Makefile | 3 +-
drivers/edac/edac_mce.c | 58 ++++++++++++++++++++++++++++++++++++++
include/linux/edac_mce.h | 31 ++++++++++++++++++++
5 files changed, 110 insertions(+), 2 deletions(-)
create mode 100644 drivers/edac/edac_mce.c
create mode 100644 include/linux/edac_mce.h
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 2f5aab2..fefbb7c 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -35,6 +35,7 @@
#include <linux/fs.h>
#include <linux/mm.h>
#include <linux/debugfs.h>
+#include <linux/edac_mce.h>
#include <asm/processor.h>
#include <asm/hw_irq.h>
@@ -135,6 +136,15 @@ void mce_log(struct mce *mce)
entry = rcu_dereference(mcelog.next);
for (;;) {
/*
+ * If edac_mce is enabled, it will check the error type
+ * and will process it, if it is a known error.
+ * Otherwise, the error will be sent through mcelog
+ * interface
+ */
+ if (edac_mce_parse(mce))
+ return;
+
+ /*
* When the buffer fills up discard new entries.
* Assume that the earlier errors are the more
* interesting ones:
@@ -193,6 +203,8 @@ static void print_mce(struct mce *m)
m->cpuvendor, m->cpuid, m->time, m->socketid,
m->apicid);
+ edac_mce_parse(m);
+
decode_mce(m);
}
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 0c8ca30..a69f69e 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -57,6 +57,9 @@ config EDAC_MM_EDAC
occurred so that a particular failing memory module can be
replaced. If unsure, select 'Y'.
+config EDAC_MCE
+ tristate
+
config EDAC_AMD64
tristate "AMD64 (Opteron, Athlon64) K8, F10h, F11h"
depends on EDAC_MM_EDAC && K8_NB && X86_64 && PCI && CPU_SUP_AMD
@@ -150,9 +153,12 @@ config EDAC_I5400
config EDAC_I7CORE
tristate "Intel i7 Core (Nehalem) processors"
depends on EDAC_MM_EDAC && PCI && X86
+ select EDAC_MCE
help
Support for error detection and correction the Intel
- i7 Core (Nehalem) Integrated Memory Controller
+ i7 Core (Nehalem) Integrated Memory Controller that exists on
+ newer processors like i7 Core, i7 Core Extreme, Xeon 35xx
+ and Xeon 55xx processors.
config EDAC_I82860
tristate "Intel 82860"
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 6f0cfe2..5ec7e9c 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -7,8 +7,9 @@
#
-obj-$(CONFIG_EDAC) := edac_stub.o
+obj-$(CONFIG_EDAC) += edac_stub.o
obj-$(CONFIG_EDAC_MM_EDAC) += edac_core.o
+obj-$(CONFIG_EDAC_MCE) += edac_mce.o
edac_core-objs := edac_mc.o edac_device.o edac_mc_sysfs.o edac_pci_sysfs.o
edac_core-objs += edac_module.o edac_device_sysfs.o
diff --git a/drivers/edac/edac_mce.c b/drivers/edac/edac_mce.c
new file mode 100644
index 0000000..b1efa8e
--- /dev/null
+++ b/drivers/edac/edac_mce.c
@@ -0,0 +1,58 @@
+/* Provides edac interface to mcelog events
+ *
+ * This file may be distributed under the terms of the
+ * GNU General Public License version 2.
+ *
+ * Copyright (c) 2009 by:
+ * Mauro Carvalho Chehab <[email protected]>
+ *
+ * Red Hat Inc. http://www.redhat.com
+ */
+
+#include <linux/module.h>
+#include <linux/edac_mce.h>
+#include <asm/mce.h>
+
+int edac_mce_enabled;
+EXPORT_SYMBOL_GPL(edac_mce_enabled);
+
+
+/*
+ * Extension interface
+ */
+
+static LIST_HEAD(edac_mce_list);
+static DEFINE_MUTEX(edac_mce_lock);
+
+int edac_mce_register(struct edac_mce *edac_mce)
+{
+ mutex_lock(&edac_mce_lock);
+ list_add_tail(&edac_mce->list, &edac_mce_list);
+ mutex_unlock(&edac_mce_lock);
+ return 0;
+}
+EXPORT_SYMBOL(edac_mce_register);
+
+void edac_mce_unregister(struct edac_mce *edac_mce)
+{
+ mutex_lock(&edac_mce_lock);
+ list_del(&edac_mce->list);
+ mutex_unlock(&edac_mce_lock);
+}
+EXPORT_SYMBOL(edac_mce_unregister);
+
+
+
+int edac_mce_queue(struct mce *mce)
+{
+ struct edac_mce *edac_mce;
+
+ list_for_each_entry(edac_mce, &edac_mce_list, list) {
+ if (edac_mce->check_error(edac_mce->priv, mce))
+ return 1;
+ }
+
+ /* Nobody queued the error */
+ return 0;
+}
+EXPORT_SYMBOL_GPL(edac_mce_queue);
diff --git a/include/linux/edac_mce.h b/include/linux/edac_mce.h
new file mode 100644
index 0000000..f974fc0
--- /dev/null
+++ b/include/linux/edac_mce.h
@@ -0,0 +1,31 @@
+/* Provides edac interface to mcelog events
+ *
+ * This file may be distributed under the terms of the
+ * GNU General Public License version 2.
+ *
+ * Copyright (c) 2009 by:
+ * Mauro Carvalho Chehab <[email protected]>
+ *
+ * Red Hat Inc. http://www.redhat.com
+ */
+
+#if defined(CONFIG_EDAC_MCE) || \
+ (defined(CONFIG_EDAC_MCE_MODULE) && defined(MODULE))
+
+#include <asm/mce.h>
+#include <linux/list.h>
+
+struct edac_mce {
+ struct list_head list;
+
+ void *priv;
+ int (*check_error)(void *priv, struct mce *mce);
+};
+
+int edac_mce_register(struct edac_mce *edac_mce);
+void edac_mce_unregister(struct edac_mce *edac_mce);
+int edac_mce_parse(struct mce *mce);
+
+#else
+#define edac_mce_parse(mce) (0)
+#endif
--
1.5.5.6
Mauro,
On Thu, Sep 24, 2009 at 07:27:27PM -0300, Mauro Carvalho Chehab wrote:
> edac_mce module is an interface module that gets mcelog data and
> forwards to any registered edac module that expects to receive data via
> mce.
>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> ---
> arch/x86/kernel/cpu/mcheck/mce.c | 12 ++++++++
> drivers/edac/Kconfig | 8 ++++-
> drivers/edac/Makefile | 3 +-
> drivers/edac/edac_mce.c | 58 ++++++++++++++++++++++++++++++++++++++
> include/linux/edac_mce.h | 31 ++++++++++++++++++++
> 5 files changed, 110 insertions(+), 2 deletions(-)
> create mode 100644 drivers/edac/edac_mce.c
> create mode 100644 include/linux/edac_mce.h
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 2f5aab2..fefbb7c 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -35,6 +35,7 @@
> #include <linux/fs.h>
> #include <linux/mm.h>
> #include <linux/debugfs.h>
> +#include <linux/edac_mce.h>
>
> #include <asm/processor.h>
> #include <asm/hw_irq.h>
> @@ -135,6 +136,15 @@ void mce_log(struct mce *mce)
> entry = rcu_dereference(mcelog.next);
> for (;;) {
> /*
> + * If edac_mce is enabled, it will check the error type
> + * and will process it, if it is a known error.
> + * Otherwise, the error will be sent through mcelog
> + * interface
> + */
> + if (edac_mce_parse(mce))
> + return;
for the third time (!): this may run in NMI context and as such does not
obey to normal kernel locking rules and you cannot safely use almost any
kernel resources involving locking. This way, your hook calls into a
module, which is a very bad idea. Please remove that hook and put in the
polling routine or somewhere more appropriate.
> +
> + /*
> * When the buffer fills up discard new entries.
> * Assume that the earlier errors are the more
> * interesting ones:
> @@ -193,6 +203,8 @@ static void print_mce(struct mce *m)
> m->cpuvendor, m->cpuid, m->time, m->socketid,
> m->apicid);
>
> + edac_mce_parse(m);
> +
> decode_mce(m);
> }
--
Regards/Gruss,
Boris.
Operating | Advanced Micro Devices GmbH
System | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. M?nchen, Germany
Research | Gesch?ftsf?hrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Center | Sitz: Dornach, Gemeinde Aschheim, Landkreis M?nchen
(OSRC) | Registergericht M?nchen, HRB Nr. 43632
Boris,
Em Fri, 25 Sep 2009 11:48:55 +0200
Borislav Petkov <[email protected]> escreveu:
> > entry = rcu_dereference(mcelog.next);
> > for (;;) {
> > /*
> > + * If edac_mce is enabled, it will check the error type
> > + * and will process it, if it is a known error.
> > + * Otherwise, the error will be sent through mcelog
> > + * interface
> > + */
> > + if (edac_mce_parse(mce))
> > + return;
>
> for the third time (!): this may run in NMI context and as such does not
> obey to normal kernel locking rules and you cannot safely use almost any
> kernel resources involving locking. This way, your hook calls into a
> module, which is a very bad idea. Please remove that hook and put in the
> polling routine or somewhere more appropriate.
I had answered you already, but let me give a more complete explanation.
For sure all the code called at this point should be carefully analyzed. So,
let's see the complete implementation:
1) edac_mce is not a module (see patch 18). So, just calling a routine on
edac_mce should be safe, even at NMI;
2) While there's nobody registered on edac_mce, it will just return 0, letting
the code proceed. There's no lock there. The called code is as simple as:
int edac_mce_parse(struct mce *mce)
{
struct edac_mce *edac_mce;
list_for_each_entry(edac_mce, &edac_mce_list, list) {
if (edac_mce->check_error(edac_mce->priv, mce))
return 1;
}
/* Nobody queued the error */
return 0;
}
3) i7core_edac will only start handling mce events after being loaded on memory
and registered on edac_mce. If an error occurs before it, normal mce handling
will happen;
4) after registered, edac_mce will call this hook, at i7core_edac:
static int i7core_mce_check_error(void *priv, struct mce *mce)
{
struct mem_ctl_info *mci = priv;
struct i7core_pvt *pvt = mci->pvt_info;
unsigned long flags;
/*
* Just let mcelog handle it if the error is
* outside the memory controller
*/
if (((mce->status & 0xffff) >> 7) != 1)
return 0;
/* Bank 8 registers are the only ones that we know how to handle */
if (mce->bank != 8)
return 0;
/* Only handle if it is the right mc controller */
if (cpu_data(mce->cpu).phys_proc_id != pvt->i7core_dev->socket) {
debugf0("mc%d: ignoring mce log for socket %d. "
"Another mc should get it.\n",
pvt->i7core_dev->socket,
cpu_data(mce->cpu).phys_proc_id);
return 0;
}
spin_lock_irqsave(&pvt->mce_lock, flags);
if (pvt->mce_count < MCE_LOG_LEN) {
memcpy(&pvt->mce_entry[pvt->mce_count], mce, sizeof(*mce));
pvt->mce_count++;
}
spin_unlock_irqrestore(&pvt->mce_lock, flags);
/* Handle fatal errors immediately */
if (mce->mcgstatus & 1)
i7core_check_error(mci);
/* Advice mcelog that the error were handled */
return 1;
}
It will basically check if the error is related to the MC, returning if not. If
the error is for the mc, it will copy the error inside a buffer on i7core_mce.
In this part, it is currently using a spinlock. It might have used an RCU code
instead, but I opted to use a simpler approach, since, on all tests, this worked fine.
Iff the error is a fatal error (Uncorrected Error), it will call the EDAC UE routine
to to display the error. For all Corrected Errors, like the mce driver, the code that
will parse the error will be called outside NMI, by edac polling code.
5) there are very few calls to kernel routines here: list_for_each_entry(),
spin_lock_irqsave(), in_unlock_irqrestore(), memcpy(). I don't think any of them will
have any problems on working even inside NMI context.
--
Cheers,
Mauro
On Fri, 25 Sep 2009 09:11:30 -0300
Mauro Carvalho Chehab <[email protected]> wrote:
> 5) there are very few calls to kernel routines here:
> list_for_each_entry(), spin_lock_irqsave(), in_unlock_irqrestore(),
> memcpy(). I don't think any of them will have any problems on working
> even inside NMI context.
spinlocks in NMI context don't do what you think they do....
_irqsave does not disable NMIs so this can deadlock.
You likely want to use trylock here....
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
Hi,
On Fri, Sep 25, 2009 at 09:11:30AM -0300, Mauro Carvalho Chehab wrote:
> > > entry = rcu_dereference(mcelog.next);
> > > for (;;) {
> > > /*
> > > + * If edac_mce is enabled, it will check the error type
> > > + * and will process it, if it is a known error.
> > > + * Otherwise, the error will be sent through mcelog
> > > + * interface
> > > + */
> > > + if (edac_mce_parse(mce))
> > > + return;
> >
> > for the third time (!): this may run in NMI context and as such does not
> > obey to normal kernel locking rules and you cannot safely use almost any
> > kernel resources involving locking. This way, your hook calls into a
> > module, which is a very bad idea. Please remove that hook and put in the
> > polling routine or somewhere more appropriate.
>
> I had answered you already, but let me give a more complete explanation.
>
> For sure all the code called at this point should be carefully analyzed. So,
> let's see the complete implementation:
>
> 1) edac_mce is not a module (see patch 18). So, just calling a routine on
> edac_mce should be safe, even at NMI;
no, I mean the ->check_error member - it could call into a module if
i7core_edac is compiled as such.
<snip the obvious non-registered module case>
> 3) i7core_edac will only start handling mce events after being loaded on memory
> and registered on edac_mce. If an error occurs before it, normal mce handling
> will happen;
>
> 4) after registered, edac_mce will call this hook, at i7core_edac:
>
> static int i7core_mce_check_error(void *priv, struct mce *mce)
> {
> struct mem_ctl_info *mci = priv;
> struct i7core_pvt *pvt = mci->pvt_info;
> unsigned long flags;
>
> /*
> * Just let mcelog handle it if the error is
> * outside the memory controller
> */
> if (((mce->status & 0xffff) >> 7) != 1)
> return 0;
>
> /* Bank 8 registers are the only ones that we know how to handle */
> if (mce->bank != 8)
> return 0;
>
> /* Only handle if it is the right mc controller */
> if (cpu_data(mce->cpu).phys_proc_id != pvt->i7core_dev->socket) {
> debugf0("mc%d: ignoring mce log for socket %d. "
> "Another mc should get it.\n",
> pvt->i7core_dev->socket,
> cpu_data(mce->cpu).phys_proc_id);
> return 0;
> }
One problem here is the debug call which is a printk() and you may
deadlock while doing a printk in an NMI context. That's why you add MCEs
to the lockless buffer in mce_log and decode them later - otherwise you
could just as well printk them here.
Generally, you need to keep the NMI handlers as short as possible and
postpone the parsing of the MCEs for later.
--
Regards/Gruss,
Boris.
Operating | Advanced Micro Devices GmbH
System | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. M?nchen, Germany
Research | Gesch?ftsf?hrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Center | Sitz: Dornach, Gemeinde Aschheim, Landkreis M?nchen
(OSRC) | Registergericht M?nchen, HRB Nr. 43632
On Fri, 25 Sep 2009 15:56:26 +0200
> > 1) edac_mce is not a module (see patch 18). So, just calling a
> > routine on edac_mce should be safe, even at NMI;
>
> no, I mean the ->check_error member - it could call into a module if
> i7core_edac is compiled as such.
calling modular code from NMI is not a fatal event though.
calling printk or taking spinlocks otoh..
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Fri, Sep 25, 2009 at 04:05:01PM +0200, Arjan van de Ven wrote:
> On Fri, 25 Sep 2009 15:56:26 +0200
> > > 1) edac_mce is not a module (see patch 18). So, just calling a
> > > routine on edac_mce should be safe, even at NMI;
> >
> > no, I mean the ->check_error member - it could call into a module if
> > i7core_edac is compiled as such.
>
> calling modular code from NMI is not a fatal event though.
No, not really. However, I remember Andi raising a stability concern one
time whether it'd be such a good idea to allow modules to hook into MCE
critical path when the system is already unstable and about to panic.
Therefore, we might want to decode critical MCEs in core kernel code and
non-critical later, at a more appropriate time (aka in modules).
Hmm?
--
Regards/Gruss,
Boris.
Operating | Advanced Micro Devices GmbH
System | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. M?nchen, Germany
Research | Gesch?ftsf?hrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Center | Sitz: Dornach, Gemeinde Aschheim, Landkreis M?nchen
(OSRC) | Registergericht M?nchen, HRB Nr. 43632
Em Fri, 25 Sep 2009 15:56:26 +0200
Borislav Petkov <[email protected]> escreveu:
> Hi,
>
> On Fri, Sep 25, 2009 at 09:11:30AM -0300, Mauro Carvalho Chehab wrote:
> > > > entry = rcu_dereference(mcelog.next);
> > > > for (;;) {
> > > > /*
> > > > + * If edac_mce is enabled, it will check the error type
> > > > + * and will process it, if it is a known error.
> > > > + * Otherwise, the error will be sent through mcelog
> > > > + * interface
> > > > + */
> > > > + if (edac_mce_parse(mce))
> > > > + return;
> > >
> > > for the third time (!): this may run in NMI context and as such does not
> > > obey to normal kernel locking rules and you cannot safely use almost any
> > > kernel resources involving locking. This way, your hook calls into a
> > > module, which is a very bad idea. Please remove that hook and put in the
> > > polling routine or somewhere more appropriate.
> >
> > I had answered you already, but let me give a more complete explanation.
> >
> > For sure all the code called at this point should be carefully analyzed. So,
> > let's see the complete implementation:
> >
> > 1) edac_mce is not a module (see patch 18). So, just calling a routine on
> > edac_mce should be safe, even at NMI;
>
> no, I mean the ->check_error member - it could call into a module if
> i7core_edac is compiled as such.
Yes, but calling a code inside a module already loaded in memory should work just fine
as calling a builtin code. As the module needs to be loaded first, in order to register
on edac_mce, there's no problem here.
> <snip the obvious non-registered module case>
>
> > 3) i7core_edac will only start handling mce events after being loaded on memory
> > and registered on edac_mce. If an error occurs before it, normal mce handling
> > will happen;
> >
> > 4) after registered, edac_mce will call this hook, at i7core_edac:
> >
> > static int i7core_mce_check_error(void *priv, struct mce *mce)
> > {
> > struct mem_ctl_info *mci = priv;
> > struct i7core_pvt *pvt = mci->pvt_info;
> > unsigned long flags;
> >
> > /*
> > * Just let mcelog handle it if the error is
> > * outside the memory controller
> > */
> > if (((mce->status & 0xffff) >> 7) != 1)
> > return 0;
> >
> > /* Bank 8 registers are the only ones that we know how to handle */
> > if (mce->bank != 8)
> > return 0;
> >
> > /* Only handle if it is the right mc controller */
> > if (cpu_data(mce->cpu).phys_proc_id != pvt->i7core_dev->socket) {
> > debugf0("mc%d: ignoring mce log for socket %d. "
> > "Another mc should get it.\n",
> > pvt->i7core_dev->socket,
> > cpu_data(mce->cpu).phys_proc_id);
> > return 0;
> > }
>
> One problem here is the debug call which is a printk() and you may
> deadlock while doing a printk in an NMI context. That's why you add MCEs
> to the lockless buffer in mce_log and decode them later - otherwise you
> could just as well printk them here.
That debug code can just be dropped. Anyway, this code disaperars if EDAC_DEBUG
is disabled.
> Generally, you need to keep the NMI handlers as short as possible and
> postpone the parsing of the MCEs for later.
True. The parser is outside the NMI called routine (except for UE, since you
may not have a chance of parsing the error outside it, as panic is called on
mce code).
--
Cheers,
Mauro
Em Fri, 25 Sep 2009 14:47:20 +0200
Arjan van de Ven <[email protected]> escreveu:
> On Fri, 25 Sep 2009 09:11:30 -0300
> Mauro Carvalho Chehab <[email protected]> wrote:
>
> > 5) there are very few calls to kernel routines here:
> > list_for_each_entry(), spin_lock_irqsave(), in_unlock_irqrestore(),
> > memcpy(). I don't think any of them will have any problems on working
> > even inside NMI context.
>
> spinlocks in NMI context don't do what you think they do....
> _irqsave does not disable NMIs so this can deadlock.
>
> You likely want to use trylock here....
I see. I'll rewrite that part of the code using trylock or RCU.
--
Cheers,
Mauro
On Fri, 25 Sep 2009 16:40:38 +0200
Borislav Petkov <[email protected]> wrote:
> On Fri, Sep 25, 2009 at 04:05:01PM +0200, Arjan van de Ven wrote:
> > On Fri, 25 Sep 2009 15:56:26 +0200
> > > > 1) edac_mce is not a module (see patch 18). So, just calling a
> > > > routine on edac_mce should be safe, even at NMI;
> > >
> > > no, I mean the ->check_error member - it could call into a module
> > > if i7core_edac is compiled as such.
> >
> > calling modular code from NMI is not a fatal event though.
>
> No, not really. However, I remember Andi raising a stability concern
> one time whether it'd be such a good idea to allow modules to hook
> into MCE critical path when the system is already unstable and about
> to panic.
module vs not does not make a difference here.
either it's good code or it's not, does not matter where it lives.
>
> Therefore, we might want to decode critical MCEs in core kernel code
> and non-critical later, at a more appropriate time (aka in modules).
>
modules are also just an artificial carve-off of the kernel, nothing
more.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org