Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1617699 > unrolled thread

[PATCH 02/11] x86/nmi, EDAC: Get rid of DRAM error reporting thru PCI SERR NMI

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 02/11] x86/nmi, EDAC: Get rid of DRAM error reporting thru PCI SERR NMI Borislav Petkov <bp@alien8.de> - 2017-04-06 11:10 +0200
    Re: [PATCH 02/11] x86/nmi, EDAC: Get rid of DRAM error reporting  thru PCI SERR NMI Thomas Gleixner <tglx@linutronix.de> - 2017-04-10 15:50 +0200
      Re: [PATCH 02/11] x86/nmi, EDAC: Get rid of DRAM error reporting  thru PCI SERR NMI Borislav Petkov <bp@alien8.de> - 2017-04-10 17:10 +0200

#1617699 — [PATCH 02/11] x86/nmi, EDAC: Get rid of DRAM error reporting thru PCI SERR NMI

From: Borislav Petkov <bp@suse.de>

Apparently, some machines used to report DRAM errors through a PCI SERR
NMI. This is why we have a call into EDAC in the NMI handler. See

  c0d121720220 ("drivers/edac: add new nmi rescan").

From looking at the patch above, that's two drivers: e752x_edac.c and
e7xxx_edac.c. Now, I wanna say those are old machines which are probably
decommissioned already.

Tony says that "[t]the newest CPU supported by either of those drivers
is the Xeon E7520 (a.k.a. "Nehalem") released in Q1'2010. Possibly some
folks are still using these ... but people that hold onto h/w for 7
years generally cling to old s/w too ... so I'd guess it unlikely that
we will get complaints for breaking these in upstream."

So even if there is a small number still in use, we did load EDAC with
edac_op_state == EDAC_OPSTATE_POLL by default (we still do, in fact)
which means a default EDAC setup without any parameters supplied on the
command line or otherwise would never even log the error in the NMI
handler because we're polling by default:

  inline int edac_handler_set(void)
  {
         if (edac_op_state == EDAC_OPSTATE_POLL)
                 return 0;

         return atomic_read(&edac_handlers);
  }

So, long story short, I'd like to get rid of that nastiness called
edac_stub.c and confine all the EDAC drivers solely to drivers/edac/. If
we ever have to do stuff like that again, it should be notifiers we're
using and not some insanity like this one.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/nmi.c    | 11 -----------
 drivers/edac/edac_stub.c | 22 ----------------------
 include/linux/edac.h     |  2 --
 3 files changed, 35 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index f088ea4c66e7..f0c4c890f71b 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -224,17 +224,6 @@ pci_serr_error(unsigned char reason, struct pt_regs *regs)
 	pr_emerg("NMI: PCI system error (SERR) for reason %02x on CPU %d.\n",
 		 reason, smp_processor_id());
 
-	/*
-	 * On some machines, PCI SERR line is used to report memory
-	 * errors. EDAC makes use of it.
-	 */
-#if defined(CONFIG_EDAC)
-	if (edac_handler_set()) {
-		edac_atomic_assert_error();
-		return;
-	}
-#endif
-
 	if (panic_on_unrecovered_nmi)
 		nmi_panic(regs, "NMI: Not continuing");
 
diff --git a/drivers/edac/edac_stub.c b/drivers/edac/edac_stub.c
index 952e411f01f2..d1389e4b7989 100644
--- a/drivers/edac/edac_stub.c
+++ b/drivers/edac/edac_stub.c
@@ -44,25 +44,3 @@ static int __init edac_report_setup(char *str)
 	return 0;
 }
 __setup("edac_report=", edac_report_setup);
-
-/*
- * called to determine if there is an EDAC driver interested in
- * knowing an event (such as NMI) occurred
- */
-int edac_handler_set(void)
-{
-	if (edac_op_state == EDAC_OPSTATE_POLL)
-		return 0;
-
-	return atomic_read(&edac_handlers);
-}
-EXPORT_SYMBOL_GPL(edac_handler_set);
-
-/*
- * handler for NMI type of interrupts to assert error
- */
-void edac_atomic_assert_error(void)
-{
-	edac_err_assert++;
-}
-EXPORT_SYMBOL_GPL(edac_atomic_assert_error);
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 5b6adf964248..bf8daabf3d51 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -31,8 +31,6 @@ extern int edac_op_state;
 extern int edac_err_assert;
 extern atomic_t edac_handlers;
 
-extern int edac_handler_set(void);
-extern void edac_atomic_assert_error(void);
 extern struct bus_type *edac_get_sysfs_subsys(void);
 
 enum {
-- 
2.11.0

[toc] | [next] | [standalone]

#1619947 — Re: [PATCH 02/11] x86/nmi, EDAC: Get rid of DRAM error reporting thru PCI SERR NMI

On Thu, 6 Apr 2017, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Apparently, some machines used to report DRAM errors through a PCI SERR
> NMI. This is why we have a call into EDAC in the NMI handler. See
> 
>   c0d121720220 ("drivers/edac: add new nmi rescan").
> 
> >From looking at the patch above, that's two drivers: e752x_edac.c and

Stray '>'

> e7xxx_edac.c. Now, I wanna say those are old machines which are probably
> decommissioned already.
> 
> Tony says that "[t]the newest CPU supported by either of those drivers
> is the Xeon E7520 (a.k.a. "Nehalem") released in Q1'2010. Possibly some
> folks are still using these ... but people that hold onto h/w for 7
> years generally cling to old s/w too ... so I'd guess it unlikely that
> we will get complaints for breaking these in upstream."
> 
> So even if there is a small number still in use, we did load EDAC with
> edac_op_state == EDAC_OPSTATE_POLL by default (we still do, in fact)
> which means a default EDAC setup without any parameters supplied on the
> command line or otherwise would never even log the error in the NMI
> handler because we're polling by default:
> 
>   inline int edac_handler_set(void)
>   {
>          if (edac_op_state == EDAC_OPSTATE_POLL)
>                  return 0;
> 
>          return atomic_read(&edac_handlers);
>   }
> 
> So, long story short, I'd like to get rid of that nastiness called
> edac_stub.c and confine all the EDAC drivers solely to drivers/edac/. If
> we ever have to do stuff like that again, it should be notifiers we're

Notifiers? You mean a proper NMI handler, right?

Other than that: Acked-by: Thomas Gleixner <tglx@linutronix.de>

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1620009 — Re: [PATCH 02/11] x86/nmi, EDAC: Get rid of DRAM error reporting thru PCI SERR NMI

On Mon, Apr 10, 2017 at 03:39:55PM +0200, Thomas Gleixner wrote:
> Notifiers? You mean a proper NMI handler, right?

I mean notifiers in the sense, the NMI handler should queue the logged
error on the decoding chain x86_mce_decoder_chain like the rest of RAS
code does instead of calling straight into EDAC which can be a module
and which causes all this ugliness I'm removing now :-)

> Other than that: Acked-by: Thomas Gleixner <tglx@linutronix.de>

Thanks!

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web