From: Borislav Petkov <[email protected]>
Hi,
here's the conversion of ghes_edac to a proper module using a notifier.
This is only the bare minimum stuff and we should do all outstanding
work ontop. Which right now is:
* platform whitelist
* Tony: "Additional cleanup: edac_ghes.c should really call
cper_mem_err_location() instead of doing pretty much the same thing but
with subtle formatting differences ("node:%d" vs. "node: %d")."
Yes, I think we should take a look at doing that eventually as a
sensible cleanup.
* Tony: "This driver isn't going to set the EDAC error counts since it
just does:
e->top_layer = -1;
e->mid_layer = -1;
e->low_layer = -1;
"
Right, that we should address by making layer 0 EDAC_MC_LAYER_SLOT and
no other layers as we can't get any channel, branch or whatever DIMM
topology from GHES. At least I'm not aware of any.
But that should be enough as this info is good enough for pinpointing
the DIMM. From our discussion with Tony:
>> The FRU identifier is:
>>
>> "The module number of the memory error location. (NODE, CARD, and MODULE
>> should provide the information necessary to identify the failing FRU)."
>
> Sounds pretty good. Generally people don't care about the channel. They
> want to know which DIMM to replace. This will give them what they want.
Thanks.
Borislav Petkov (3):
EDAC: Add edac_pr_err/info macros
ACPI/GHES: Add an EDAC notifier chain
EDAC, ghes: Make it a proper module
drivers/acpi/apei/ghes.c | 32 +++++++-----
drivers/edac/Kconfig | 4 +-
drivers/edac/edac_mc.h | 3 ++
drivers/edac/ghes_edac.c | 129 ++++++++++++++++++++++++-----------------------
include/acpi/ghes.h | 27 +---------
5 files changed, 93 insertions(+), 102 deletions(-)
--
2.14.0.rc0
From: Borislav Petkov <[email protected]>
Analogue to the generic pr_* variants but issuing the EDAC prefix.
No functionality change.
Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/edac/edac_mc.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index 5357800e418d..6d46f30dc657 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -60,6 +60,9 @@
#define edac_pci_printk(ctl, level, fmt, arg...) \
printk(level "EDAC PCI%d: " fmt, ctl->pci_idx, ##arg)
+#define edac_pr_err(fmt, arg...) edac_printk(KERN_ERR, "", fmt, ##arg)
+#define edac_pr_info(fmt, arg...) edac_printk(KERN_INFO, "", fmt, ##arg)
+
/* prefixes for edac_printk() and edac_mc_printk() */
#define EDAC_MC "MC"
#define EDAC_PCI "PCI"
--
2.14.0.rc0
From: Borislav Petkov <[email protected]>
... so that GHES can dump memory errors into it and consumers like
ghes_edac can register.
Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/acpi/apei/ghes.c | 14 ++++++++++++++
include/acpi/ghes.h | 2 ++
2 files changed, 16 insertions(+)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index d661d452b238..3f05f5ff3461 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -140,6 +140,20 @@ static atomic_t ghes_estatus_cache_alloced;
static int ghes_panic_timeout __read_mostly = 30;
+static ATOMIC_NOTIFIER_HEAD(ghes_edac_chain);
+
+void ghes_register_edac_chain(struct notifier_block *nb)
+{
+ atomic_notifier_chain_register(&ghes_edac_chain, nb);
+}
+EXPORT_SYMBOL_GPL(ghes_register_edac_chain);
+
+void ghes_unregister_edac_chain(struct notifier_block *nb)
+{
+ atomic_notifier_chain_unregister(&ghes_edac_chain, nb);
+}
+EXPORT_SYMBOL_GPL(ghes_unregister_edac_chain);
+
static int ghes_ioremap_init(void)
{
ghes_ioremap_area = __get_vm_area(PAGE_SIZE * GHES_IOREMAP_PAGES,
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 9f26e01186ae..506b6a197738 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -76,6 +76,8 @@ static inline void ghes_edac_unregister(struct ghes *ghes)
{
}
#endif
+void ghes_register_edac_chain(struct notifier_block *nb);
+void ghes_unregister_edac_chain(struct notifier_block *nb);
static inline int acpi_hest_get_version(struct acpi_hest_generic_data *gdata)
{
--
2.14.0.rc0
From: Borislav Petkov <[email protected]>
Register with the GHES notifier chain so that there's no need to call
into the module with ghes_edac_report_mem_error().
Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/acpi/apei/ghes.c | 18 +++----
drivers/edac/Kconfig | 4 +-
drivers/edac/ghes_edac.c | 129 ++++++++++++++++++++++++-----------------------
include/acpi/ghes.h | 25 ---------
4 files changed, 74 insertions(+), 102 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 3f05f5ff3461..37cd698cacd2 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -475,11 +475,11 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int
static void ghes_do_proc(struct ghes *ghes,
const struct acpi_hest_generic_status *estatus)
{
- int sev, sec_sev;
struct acpi_hest_generic_data *gdata;
guid_t *sec_type;
guid_t *fru_id = &NULL_UUID_LE;
char *fru_text = "";
+ int sev, sec_sev;
sev = ghes_severity(estatus->error_severity);
apei_estatus_for_each_section(estatus, gdata) {
@@ -494,7 +494,8 @@ static void ghes_do_proc(struct ghes *ghes,
if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) {
struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
- ghes_edac_report_mem_error(ghes, sev, mem_err);
+
+ atomic_notifier_call_chain(&ghes_edac_chain, sev, &mem_err);
arch_apei_report_mem_error(sev, mem_err);
ghes_handle_memory_failure(gdata, sev);
@@ -1153,10 +1154,6 @@ static int ghes_probe(struct platform_device *ghes_dev)
goto err;
}
- rc = ghes_edac_register(ghes, &ghes_dev->dev);
- if (rc < 0)
- goto err;
-
switch (generic->notify.type) {
case ACPI_HEST_NOTIFY_POLLED:
setup_deferrable_timer(&ghes->timer, ghes_poll_func,
@@ -1169,13 +1166,13 @@ static int ghes_probe(struct platform_device *ghes_dev)
if (rc) {
pr_err(GHES_PFX "Failed to map GSI to IRQ for generic hardware error source: %d\n",
generic->header.source_id);
- goto err_edac_unreg;
+ goto err;
}
rc = request_irq(ghes->irq, ghes_irq_func, 0, "GHES IRQ", ghes);
if (rc) {
pr_err(GHES_PFX "Failed to register IRQ for generic hardware error source: %d\n",
generic->header.source_id);
- goto err_edac_unreg;
+ goto err;
}
break;
@@ -1204,8 +1201,7 @@ static int ghes_probe(struct platform_device *ghes_dev)
ghes_proc(ghes);
return 0;
-err_edac_unreg:
- ghes_edac_unregister(ghes);
+
err:
if (ghes) {
ghes_fini(ghes);
@@ -1255,8 +1251,6 @@ static int ghes_remove(struct platform_device *ghes_dev)
ghes_fini(ghes);
- ghes_edac_unregister(ghes);
-
kfree(ghes);
platform_set_drvdata(ghes_dev, NULL);
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 96afb2aeed18..fdd8278ca89a 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -53,8 +53,8 @@ config EDAC_DECODE_MCE
has been initialized.
config EDAC_GHES
- bool "Output ACPI APEI/GHES BIOS detected errors via EDAC"
- depends on ACPI_APEI_GHES && (EDAC=y)
+ tristate "Output ACPI APEI/GHES BIOS detected errors via EDAC"
+ depends on ACPI_APEI_GHES
help
Not all machines support hardware-driven error report. Some of those
provide a BIOS-driven error report mechanism via ACPI, using the
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 6f80eb65c26c..ecd34b8bd27e 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -5,6 +5,9 @@
* License version 2.
*
* Copyright (c) 2013 by Mauro Carvalho Chehab
+ * (c) 2017 Borislav Petkov
+ *
+ * Borislav Petkov: turn it into a proper module.
*
* Red Hat Inc. http://www.redhat.com
*/
@@ -28,10 +31,10 @@ struct ghes_edac_pvt {
char msg[80];
};
-static LIST_HEAD(ghes_reglist);
-static DEFINE_MUTEX(ghes_edac_lock);
-static int ghes_edac_mc_num;
+static struct ghes_edac_pvt *ghes_pvt;
+/* Hand it into EDAC's core so that we have a device to operate on. */
+static struct device dummy_dev;
/* Memory Device - Type 17 of SMBIOS spec */
struct memdev_dmi_entry {
@@ -163,24 +166,21 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
}
}
-void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
- struct cper_sec_mem_err *mem_err)
+static int report_mem_error(struct notifier_block *nb, unsigned long sev, void *data)
{
+ struct cper_sec_mem_err *mem_err = data;
enum hw_event_mc_err_type type;
struct edac_raw_error_desc *e;
struct mem_ctl_info *mci;
- struct ghes_edac_pvt *pvt = NULL;
- char *p;
+ struct ghes_edac_pvt *pvt = ghes_pvt;
u8 grain_bits;
+ char *p;
- list_for_each_entry(pvt, &ghes_reglist, list) {
- if (ghes == pvt->ghes)
- break;
- }
if (!pvt) {
- pr_err("Internal error: Can't find EDAC structure\n");
- return;
+ edac_pr_err("Internal error: Can't find EDAC structure\n");
+ return NOTIFY_DONE;
}
+
mci = pvt->mci;
e = &mci->error_desc;
@@ -400,23 +400,40 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
/* Report the error via EDAC API */
edac_raw_mc_handle_error(type, mci, e);
+
+ return NOTIFY_DONE;
}
-EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error);
-int ghes_edac_register(struct ghes *ghes, struct device *dev)
+static struct notifier_block ghes_nb = {
+ .notifier_call = report_mem_error,
+};
+
+static const char * const fake_msg =
+"This EDAC driver relies on BIOS to enumerate memory and get error reports.\n"
+"Unfortunately, not all BIOSes reflect the memory layout correctly.\n"
+"So, the end result of using this driver varies from vendor to vendor.\n"
+"If you find incorrect reports, please contact your hardware vendor\n"
+"to correct its BIOS.";
+
+static const char * const super_crap_msg =
+"This system has a very crappy BIOS: It doesn't even list the DIMMS.\n"
+"Its SMBIOS info is wrong. It is doubtful that the error report would\n"
+"work on such system. Use this driver with caution.";
+
+static int __init ghes_edac_register(void)
{
+ struct ghes_edac_pvt *pvt = ghes_pvt;
bool fake = false;
int rc, num_dimm = 0;
struct mem_ctl_info *mci;
struct edac_mc_layer layers[1];
- struct ghes_edac_pvt *pvt;
struct ghes_edac_dimm_fill dimm_fill;
/* Get the number of DIMMs */
dmi_walk(ghes_edac_count_dimms, &num_dimm);
/* Check if we've got a bogus BIOS */
- if (num_dimm == 0) {
+ if (!num_dimm) {
fake = true;
num_dimm = 1;
}
@@ -429,21 +446,17 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
* We need to serialize edac_mc_alloc() and edac_mc_add_mc(),
* to avoid duplicated memory controller numbers
*/
- mutex_lock(&ghes_edac_lock);
- mci = edac_mc_alloc(ghes_edac_mc_num, ARRAY_SIZE(layers), layers,
- sizeof(*pvt));
+ mci = edac_mc_alloc(1, ARRAY_SIZE(layers), layers, sizeof(*pvt));
if (!mci) {
- pr_info("Can't allocate memory for EDAC data\n");
- mutex_unlock(&ghes_edac_lock);
+ edac_pr_err("Can't allocate memory for EDAC data\n");
return -ENOMEM;
}
pvt = mci->pvt_info;
memset(pvt, 0, sizeof(*pvt));
- list_add_tail(&pvt->list, &ghes_reglist);
- pvt->ghes = ghes;
pvt->mci = mci;
- mci->pdev = dev;
+
+ mci->pdev = &dummy_dev;
mci->mtype_cap = MEM_FLAG_EMPTY;
mci->edac_ctl_cap = EDAC_FLAG_NONE;
@@ -452,21 +465,12 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
mci->ctl_name = "ghes_edac";
mci->dev_name = "ghes";
- if (!ghes_edac_mc_num) {
- if (!fake) {
- pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
- pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
- pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
- pr_info("If you find incorrect reports, please contact your hardware vendor\n");
- pr_info("to correct its BIOS.\n");
- pr_info("This system has %d DIMM sockets.\n",
- num_dimm);
- } else {
- pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
- pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
- pr_info("work on such system. Use this driver with caution\n");
- }
- }
+ if (!fake)
+ edac_pr_info("%s\n", fake_msg);
+ else
+ edac_pr_info("%s\n", super_crap_msg);
+
+ edac_pr_info("This system has %d DIMM sockets.\n", num_dimm);
if (!fake) {
/*
@@ -475,13 +479,11 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
* Keep it in blank for the other memory controllers, as
* there's no reliable way to properly credit each DIMM to
* the memory controller, as different BIOSes fill the
- * DMI bank location fields on different ways
+ * DMI bank location fields in different ways.
*/
- if (!ghes_edac_mc_num) {
- dimm_fill.count = 0;
- dimm_fill.mci = mci;
- dmi_walk(ghes_edac_dmidecode, &dimm_fill);
- }
+ dimm_fill.count = 0;
+ dimm_fill.mci = mci;
+ dmi_walk(ghes_edac_dmidecode, &dimm_fill);
} else {
struct dimm_info *dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
mci->n_layers, 0, 0, 0);
@@ -495,30 +497,31 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
rc = edac_mc_add_mc(mci);
if (rc < 0) {
- pr_info("Can't register at EDAC core\n");
+ edac_pr_err("Can't register with EDAC core\n");
edac_mc_free(mci);
- mutex_unlock(&ghes_edac_lock);
return -ENODEV;
}
- ghes_edac_mc_num++;
- mutex_unlock(&ghes_edac_lock);
+ ghes_register_edac_chain(&ghes_nb);
+
return 0;
}
-EXPORT_SYMBOL_GPL(ghes_edac_register);
+module_init(ghes_edac_register);
-void ghes_edac_unregister(struct ghes *ghes)
+static void __exit ghes_edac_unregister(void)
{
struct mem_ctl_info *mci;
- struct ghes_edac_pvt *pvt, *tmp;
-
- list_for_each_entry_safe(pvt, tmp, &ghes_reglist, list) {
- if (ghes == pvt->ghes) {
- mci = pvt->mci;
- edac_mc_del_mc(mci->pdev);
- edac_mc_free(mci);
- list_del(&pvt->list);
- }
- }
+
+ ghes_unregister_edac_chain(&ghes_nb);
+
+ mci = find_mci_by_dev(&dummy_dev);
+ WARN_ON(!mci);
+
+ edac_mc_del_mc(mci->pdev);
+ edac_mc_free(mci);
+
}
-EXPORT_SYMBOL_GPL(ghes_edac_unregister);
+module_exit(ghes_edac_unregister);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("GHES error decoding module");
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 506b6a197738..c02b8eb91bd6 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -51,31 +51,6 @@ enum {
GHES_SEV_PANIC = 0x3,
};
-/* From drivers/edac/ghes_edac.c */
-
-#ifdef CONFIG_EDAC_GHES
-void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
- struct cper_sec_mem_err *mem_err);
-
-int ghes_edac_register(struct ghes *ghes, struct device *dev);
-
-void ghes_edac_unregister(struct ghes *ghes);
-
-#else
-static inline void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
- struct cper_sec_mem_err *mem_err)
-{
-}
-
-static inline int ghes_edac_register(struct ghes *ghes, struct device *dev)
-{
- return 0;
-}
-
-static inline void ghes_edac_unregister(struct ghes *ghes)
-{
-}
-#endif
void ghes_register_edac_chain(struct notifier_block *nb);
void ghes_unregister_edac_chain(struct notifier_block *nb);
--
2.14.0.rc0
Em Wed, 26 Jul 2017 10:48:27 +0200
Borislav Petkov <[email protected]> escreveu:
> From: Borislav Petkov <[email protected]>
>
> Register with the GHES notifier chain so that there's no need to call
> into the module with ghes_edac_report_mem_error().
Hmm... I'm not seeing any implementation that would allow setting
between firmware first, hardware first or "auto", as we've discussed.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> drivers/acpi/apei/ghes.c | 18 +++----
> drivers/edac/Kconfig | 4 +-
> drivers/edac/ghes_edac.c | 129 ++++++++++++++++++++++++-----------------------
> include/acpi/ghes.h | 25 ---------
> 4 files changed, 74 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 3f05f5ff3461..37cd698cacd2 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -475,11 +475,11 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int
> static void ghes_do_proc(struct ghes *ghes,
> const struct acpi_hest_generic_status *estatus)
> {
> - int sev, sec_sev;
> struct acpi_hest_generic_data *gdata;
> guid_t *sec_type;
> guid_t *fru_id = &NULL_UUID_LE;
> char *fru_text = "";
> + int sev, sec_sev;
>
> sev = ghes_severity(estatus->error_severity);
> apei_estatus_for_each_section(estatus, gdata) {
> @@ -494,7 +494,8 @@ static void ghes_do_proc(struct ghes *ghes,
> if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) {
> struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
>
> - ghes_edac_report_mem_error(ghes, sev, mem_err);
> +
> + atomic_notifier_call_chain(&ghes_edac_chain, sev, &mem_err);
>
> arch_apei_report_mem_error(sev, mem_err);
> ghes_handle_memory_failure(gdata, sev);
> @@ -1153,10 +1154,6 @@ static int ghes_probe(struct platform_device *ghes_dev)
> goto err;
> }
>
> - rc = ghes_edac_register(ghes, &ghes_dev->dev);
> - if (rc < 0)
> - goto err;
> -
> switch (generic->notify.type) {
> case ACPI_HEST_NOTIFY_POLLED:
> setup_deferrable_timer(&ghes->timer, ghes_poll_func,
> @@ -1169,13 +1166,13 @@ static int ghes_probe(struct platform_device *ghes_dev)
> if (rc) {
> pr_err(GHES_PFX "Failed to map GSI to IRQ for generic hardware error source: %d\n",
> generic->header.source_id);
> - goto err_edac_unreg;
> + goto err;
> }
> rc = request_irq(ghes->irq, ghes_irq_func, 0, "GHES IRQ", ghes);
> if (rc) {
> pr_err(GHES_PFX "Failed to register IRQ for generic hardware error source: %d\n",
> generic->header.source_id);
> - goto err_edac_unreg;
> + goto err;
> }
> break;
>
> @@ -1204,8 +1201,7 @@ static int ghes_probe(struct platform_device *ghes_dev)
> ghes_proc(ghes);
>
> return 0;
> -err_edac_unreg:
> - ghes_edac_unregister(ghes);
> +
> err:
> if (ghes) {
> ghes_fini(ghes);
> @@ -1255,8 +1251,6 @@ static int ghes_remove(struct platform_device *ghes_dev)
>
> ghes_fini(ghes);
>
> - ghes_edac_unregister(ghes);
> -
> kfree(ghes);
>
> platform_set_drvdata(ghes_dev, NULL);
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 96afb2aeed18..fdd8278ca89a 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -53,8 +53,8 @@ config EDAC_DECODE_MCE
> has been initialized.
>
> config EDAC_GHES
> - bool "Output ACPI APEI/GHES BIOS detected errors via EDAC"
> - depends on ACPI_APEI_GHES && (EDAC=y)
> + tristate "Output ACPI APEI/GHES BIOS detected errors via EDAC"
> + depends on ACPI_APEI_GHES
> help
> Not all machines support hardware-driven error report. Some of those
> provide a BIOS-driven error report mechanism via ACPI, using the
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index 6f80eb65c26c..ecd34b8bd27e 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -5,6 +5,9 @@
> * License version 2.
> *
> * Copyright (c) 2013 by Mauro Carvalho Chehab
> + * (c) 2017 Borislav Petkov
> + *
> + * Borislav Petkov: turn it into a proper module.
> *
> * Red Hat Inc. http://www.redhat.com
> */
> @@ -28,10 +31,10 @@ struct ghes_edac_pvt {
> char msg[80];
> };
>
> -static LIST_HEAD(ghes_reglist);
> -static DEFINE_MUTEX(ghes_edac_lock);
> -static int ghes_edac_mc_num;
> +static struct ghes_edac_pvt *ghes_pvt;
>
> +/* Hand it into EDAC's core so that we have a device to operate on. */
> +static struct device dummy_dev;
>
> /* Memory Device - Type 17 of SMBIOS spec */
> struct memdev_dmi_entry {
> @@ -163,24 +166,21 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg)
> }
> }
>
> -void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
> - struct cper_sec_mem_err *mem_err)
> +static int report_mem_error(struct notifier_block *nb, unsigned long sev, void *data)
> {
> + struct cper_sec_mem_err *mem_err = data;
> enum hw_event_mc_err_type type;
> struct edac_raw_error_desc *e;
> struct mem_ctl_info *mci;
> - struct ghes_edac_pvt *pvt = NULL;
> - char *p;
> + struct ghes_edac_pvt *pvt = ghes_pvt;
> u8 grain_bits;
> + char *p;
>
> - list_for_each_entry(pvt, &ghes_reglist, list) {
> - if (ghes == pvt->ghes)
> - break;
> - }
> if (!pvt) {
> - pr_err("Internal error: Can't find EDAC structure\n");
> - return;
> + edac_pr_err("Internal error: Can't find EDAC structure\n");
> + return NOTIFY_DONE;
> }
> +
> mci = pvt->mci;
> e = &mci->error_desc;
>
> @@ -400,23 +400,40 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
>
> /* Report the error via EDAC API */
> edac_raw_mc_handle_error(type, mci, e);
> +
> + return NOTIFY_DONE;
> }
> -EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error);
>
> -int ghes_edac_register(struct ghes *ghes, struct device *dev)
> +static struct notifier_block ghes_nb = {
> + .notifier_call = report_mem_error,
> +};
> +
> +static const char * const fake_msg =
> +"This EDAC driver relies on BIOS to enumerate memory and get error reports.\n"
> +"Unfortunately, not all BIOSes reflect the memory layout correctly.\n"
> +"So, the end result of using this driver varies from vendor to vendor.\n"
> +"If you find incorrect reports, please contact your hardware vendor\n"
> +"to correct its BIOS.";
> +
> +static const char * const super_crap_msg =
> +"This system has a very crappy BIOS: It doesn't even list the DIMMS.\n"
> +"Its SMBIOS info is wrong. It is doubtful that the error report would\n"
> +"work on such system. Use this driver with caution.";
I would also add a note saying that the BIOS may be masking errors.
> +
> +static int __init ghes_edac_register(void)
> {
> + struct ghes_edac_pvt *pvt = ghes_pvt;
> bool fake = false;
> int rc, num_dimm = 0;
> struct mem_ctl_info *mci;
> struct edac_mc_layer layers[1];
> - struct ghes_edac_pvt *pvt;
> struct ghes_edac_dimm_fill dimm_fill;
>
> /* Get the number of DIMMs */
> dmi_walk(ghes_edac_count_dimms, &num_dimm);
>
> /* Check if we've got a bogus BIOS */
> - if (num_dimm == 0) {
> + if (!num_dimm) {
> fake = true;
> num_dimm = 1;
> }
> @@ -429,21 +446,17 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
> * We need to serialize edac_mc_alloc() and edac_mc_add_mc(),
> * to avoid duplicated memory controller numbers
> */
> - mutex_lock(&ghes_edac_lock);
> - mci = edac_mc_alloc(ghes_edac_mc_num, ARRAY_SIZE(layers), layers,
> - sizeof(*pvt));
> + mci = edac_mc_alloc(1, ARRAY_SIZE(layers), layers, sizeof(*pvt));
> if (!mci) {
> - pr_info("Can't allocate memory for EDAC data\n");
> - mutex_unlock(&ghes_edac_lock);
> + edac_pr_err("Can't allocate memory for EDAC data\n");
> return -ENOMEM;
> }
>
> pvt = mci->pvt_info;
> memset(pvt, 0, sizeof(*pvt));
> - list_add_tail(&pvt->list, &ghes_reglist);
> - pvt->ghes = ghes;
> pvt->mci = mci;
> - mci->pdev = dev;
> +
> + mci->pdev = &dummy_dev;
>
> mci->mtype_cap = MEM_FLAG_EMPTY;
> mci->edac_ctl_cap = EDAC_FLAG_NONE;
> @@ -452,21 +465,12 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
> mci->ctl_name = "ghes_edac";
> mci->dev_name = "ghes";
>
> - if (!ghes_edac_mc_num) {
> - if (!fake) {
> - pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
> - pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
> - pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
> - pr_info("If you find incorrect reports, please contact your hardware vendor\n");
> - pr_info("to correct its BIOS.\n");
> - pr_info("This system has %d DIMM sockets.\n",
> - num_dimm);
> - } else {
> - pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
> - pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
> - pr_info("work on such system. Use this driver with caution\n");
> - }
> - }
> + if (!fake)
> + edac_pr_info("%s\n", fake_msg);
> + else
> + edac_pr_info("%s\n", super_crap_msg);
> +
> + edac_pr_info("This system has %d DIMM sockets.\n", num_dimm);
>
> if (!fake) {
> /*
> @@ -475,13 +479,11 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
> * Keep it in blank for the other memory controllers, as
> * there's no reliable way to properly credit each DIMM to
> * the memory controller, as different BIOSes fill the
> - * DMI bank location fields on different ways
> + * DMI bank location fields in different ways.
> */
> - if (!ghes_edac_mc_num) {
> - dimm_fill.count = 0;
> - dimm_fill.mci = mci;
> - dmi_walk(ghes_edac_dmidecode, &dimm_fill);
> - }
> + dimm_fill.count = 0;
> + dimm_fill.mci = mci;
> + dmi_walk(ghes_edac_dmidecode, &dimm_fill);
> } else {
> struct dimm_info *dimm = EDAC_DIMM_PTR(mci->layers, mci->dimms,
> mci->n_layers, 0, 0, 0);
> @@ -495,30 +497,31 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
>
> rc = edac_mc_add_mc(mci);
> if (rc < 0) {
> - pr_info("Can't register at EDAC core\n");
> + edac_pr_err("Can't register with EDAC core\n");
> edac_mc_free(mci);
> - mutex_unlock(&ghes_edac_lock);
> return -ENODEV;
> }
>
> - ghes_edac_mc_num++;
> - mutex_unlock(&ghes_edac_lock);
> + ghes_register_edac_chain(&ghes_nb);
> +
> return 0;
> }
> -EXPORT_SYMBOL_GPL(ghes_edac_register);
> +module_init(ghes_edac_register);
>
> -void ghes_edac_unregister(struct ghes *ghes)
> +static void __exit ghes_edac_unregister(void)
> {
> struct mem_ctl_info *mci;
> - struct ghes_edac_pvt *pvt, *tmp;
> -
> - list_for_each_entry_safe(pvt, tmp, &ghes_reglist, list) {
> - if (ghes == pvt->ghes) {
> - mci = pvt->mci;
> - edac_mc_del_mc(mci->pdev);
> - edac_mc_free(mci);
> - list_del(&pvt->list);
> - }
> - }
> +
> + ghes_unregister_edac_chain(&ghes_nb);
> +
> + mci = find_mci_by_dev(&dummy_dev);
> + WARN_ON(!mci);
> +
> + edac_mc_del_mc(mci->pdev);
> + edac_mc_free(mci);
> +
> }
> -EXPORT_SYMBOL_GPL(ghes_edac_unregister);
> +module_exit(ghes_edac_unregister);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("GHES error decoding module");
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index 506b6a197738..c02b8eb91bd6 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -51,31 +51,6 @@ enum {
> GHES_SEV_PANIC = 0x3,
> };
>
> -/* From drivers/edac/ghes_edac.c */
> -
> -#ifdef CONFIG_EDAC_GHES
> -void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
> - struct cper_sec_mem_err *mem_err);
> -
> -int ghes_edac_register(struct ghes *ghes, struct device *dev);
> -
> -void ghes_edac_unregister(struct ghes *ghes);
> -
> -#else
> -static inline void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
> - struct cper_sec_mem_err *mem_err)
> -{
> -}
> -
> -static inline int ghes_edac_register(struct ghes *ghes, struct device *dev)
> -{
> - return 0;
> -}
> -
> -static inline void ghes_edac_unregister(struct ghes *ghes)
> -{
> -}
> -#endif
> void ghes_register_edac_chain(struct notifier_block *nb);
> void ghes_unregister_edac_chain(struct notifier_block *nb);
>
Thanks,
Mauro
On Wed, Jul 26, 2017 at 07:24:04AM -0300, Mauro Carvalho Chehab wrote:
> Hmm... I'm not seeing any implementation that would allow setting
> between firmware first, hardware first or "auto", as we've discussed.
This is all coming up. As the 0/3 message said, these 3 patches are the
bare minimum of reorganizing stuff only and should serve as a base.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
Em Wed, 26 Jul 2017 12:37:08 +0200
Borislav Petkov <[email protected]> escreveu:
> On Wed, Jul 26, 2017 at 07:24:04AM -0300, Mauro Carvalho Chehab wrote:
> > Hmm... I'm not seeing any implementation that would allow setting
> > between firmware first, hardware first or "auto", as we've discussed.
>
> This is all coming up. As the 0/3 message said, these 3 patches are the
> bare minimum of reorganizing stuff only and should serve as a base.
I'll then wait for such patch before acking this series.
Thanks,
Mauro
> > > Hmm... I'm not seeing any implementation that would allow setting
> > > between firmware first, hardware first or "auto", as we've discussed.
> >
> > This is all coming up. As the 0/3 message said, these 3 patches are the
> > bare minimum of reorganizing stuff only and should serve as a base.
>
> I'll then wait for such patch before acking this series.
I didn't think that a BIOS that set "firmware first" gave the OS any choice about this.
What exactly is this option going to do? Fiddle with ACPI OSC??
-Tony
Em Wed, 26 Jul 2017 17:27:12 +0000
"Luck, Tony" <[email protected]> escreveu:
> > > > Hmm... I'm not seeing any implementation that would allow setting
> > > > between firmware first, hardware first or "auto", as we've discussed.
> > >
> > > This is all coming up. As the 0/3 message said, these 3 patches are the
> > > bare minimum of reorganizing stuff only and should serve as a base.
> >
> > I'll then wait for such patch before acking this series.
>
> I didn't think that a BIOS that set "firmware first" gave the OS any choice about this.
>
> What exactly is this option going to do? Fiddle with ACPI OSC??
Currently, my HP server that I use to build the Kernel is FF:
[ 3.783803] GHES: APEI firmware first mode is enabled by APEI bit and WHEA _OSC.
I didn't try to disable FF on its BIOS. Not sure if it is even possible.
Still, EDAC is working there using sb_edac. As I pointed before, one of the
MC channels is not being detected, but I don't use it on this machine.
Except for that, EDAC seems to be working fine there:
$ ras-mc-ctl --layout
+-----------------------------------------------------------------------+
| mc0 | mc1 |
| channel0 | channel1 | channel2 | channel0 | channel1 | channel2 |
-------+-----------------------------------------------------------------------+
slot2: | 0 MB | 0 MB | 0 MB | 0 MB | 0 MB | 0 MB |
slot1: | 0 MB | 0 MB | 0 MB | 0 MB | 0 MB | 0 MB |
slot0: | 16384 MB | 0 MB | 16384 MB | 16384 MB | 0 MB | 16384 MB |
-------+---------------------------------------------------------------------------+
# ras-mc-ctl --guess-labels
memory stick 'PROC 1 DIMM 1' is located at 'Not Specified'
memory stick 'PROC 1 DIMM 2' is located at 'Not Specified'
memory stick 'PROC 1 DIMM 3' is located at 'Not Specified'
memory stick 'PROC 1 DIMM 4' is located at 'Not Specified'
memory stick 'PROC 1 DIMM 5' is located at 'Not Specified'
memory stick 'PROC 1 DIMM 6' is located at 'Not Specified'
memory stick 'PROC 1 DIMM 7' is located at 'Not Specified'
memory stick 'PROC 1 DIMM 8' is located at 'Not Specified'
memory stick 'PROC 1 DIMM 9' is located at 'Not Specified'
memory stick 'PROC 1 DIMM 10' is located at 'Not Specified'
memory stick 'PROC 1 DIMM 11' is located at 'Not Specified'
memory stick 'PROC 1 DIMM 12' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 1' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 2' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 3' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 4' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 5' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 6' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 7' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 8' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 9' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 10' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 11' is located at 'Not Specified'
memory stick 'PROC 2 DIMM 12' is located at 'Not Specified'
I didn't try to inject an error, as I'm not sure if EINJ feature is
enabled on this BIOS. Probably not.
At least on this machine, I very much prefer to use sb_edac driver.
As I explained earlier in the previous thread, I just don't if the
BIOS would be doing the right thing for CE, as I don't know its
internal algorithm.
Also, as I'm maintaining the EDAC userspace tools (rasdaemon),
I would really love to get a few CE error reports there from time to
time, as it could be used to check if rasdaemon is doing do the right
thing to them.
So, I very much prefer to not have any threshold at all there at BIOS.
Thanks,
Mauro
On Wed, 2017-07-26 at 15:17 -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 26 Jul 2017 17:27:12 +0000
:
> I didn't try to inject an error, as I'm not sure if EINJ feature is
> enabled on this BIOS. Probably not.
I believe it has EINJ support.
> At least on this machine, I very much prefer to use sb_edac driver.
>
> As I explained earlier in the previous thread, I just don't if the
> BIOS would be doing the right thing for CE, as I don't know its
> internal algorithm.
>
> Also, as I'm maintaining the EDAC userspace tools (rasdaemon),
> I would really love to get a few CE error reports there from time to
> time, as it could be used to check if rasdaemon is doing do the right
> thing to them.
>
> So, I very much prefer to not have any threshold at all there at
> BIOS.
Using sb_edac does not change the fact that it is FF. I do not think
you'd see normal CEs on your box.
Thanks,
-Toshi
On Wed, 2017-07-26 at 07:24 -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 26 Jul 2017 10:48:27 +0200
> Borislav Petkov <[email protected]> escreveu:
>
> > From: Borislav Petkov <[email protected]>
> >
> > Register with the GHES notifier chain so that there's no need to
> > call into the module with ghes_edac_report_mem_error().
>
> Hmm... I'm not seeing any implementation that would allow setting
> between firmware first, hardware first or "auto", as we've discussed.
A minor nit for terminology - hardware is always the first one to
detect an error. The difference is whether an error is first reported
to the kernel or firmware. So, "kernel first" or "os first" would be
more appropriate as an alternative to "firmware first". That said,
such wording may be misleading since it does not change the mode.
Thanks,
-Toshi
On Wed, Jul 26, 2017 at 07:24:59PM +0000, Kani, Toshimitsu wrote:
> Using sb_edac does not change the fact that it is FF. I do not think
> you'd see normal CEs on your box.
I guess we should add some blurb to EDAC to say that on FF systems,
error counts are unreliable or even non-existent.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Wed, Jul 26, 2017 at 10:48:24AM +0200, Borislav Petkov wrote:
> EDAC: Add edac_pr_err/info macros
> ACPI/GHES: Add an EDAC notifier chain
> EDAC, ghes: Make it a proper module
Pushed here:
https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Wed, 2017-07-26 at 10:48 +0200, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> Register with the GHES notifier chain so that there's no need to call
> into the module with ghes_edac_report_mem_error().
>
> Signed-off-by: Borislav Petkov <[email protected]>
:
> +static int report_mem_error(struct notifier_block *nb, unsigned long
> sev, void *data)
> {
> + struct cper_sec_mem_err *mem_err = data;
> enum hw_event_mc_err_type type;
> struct edac_raw_error_desc *e;
> struct mem_ctl_info *mci;
> - struct ghes_edac_pvt *pvt = NULL;
> - char *p;
> + struct ghes_edac_pvt *pvt = ghes_pvt;
> u8 grain_bits;
> + char *p;
>
> - list_for_each_entry(pvt, &ghes_reglist, list) {
> - if (ghes == pvt->ghes)
> - break;
> - }
> if (!pvt) {
I think it always hits this error condition. See below.
> - pr_err("Internal error: Can't find EDAC
> structure\n");
> - return;
> + edac_pr_err("Internal error: Can't find EDAC
> structure\n");
> + return NOTIFY_DONE;
> }
> +
:
> +static int __init ghes_edac_register(void)
> {
> + struct ghes_edac_pvt *pvt = ghes_pvt;
This simply sets NULL to pvt, and does not initialize ghes_pvt.
> bool fake = false;
> int rc, num_dimm = 0;
> struct mem_ctl_info *mci;
> struct edac_mc_layer layers[1];
> - struct ghes_edac_pvt *pvt;
> struct ghes_edac_dimm_fill dimm_fill;
>
:
> -EXPORT_SYMBOL_GPL(ghes_edac_register);
> +module_init(ghes_edac_register);
Since this patch has removed the GHES-presence check and ordering hack
for ghes_edac, it now registers itself per the module_init() ordering
regardless of the presence of GHES. As Mauro pointed out, some type of
GHES check needs to be in place before making this change.
Thanks,
-Toshi
On Fri, Jul 28, 2017 at 06:50:56PM +0000, Kani, Toshimitsu wrote:
> This simply sets NULL to pvt, and does not initialize ghes_pvt.
Yeah, I guess we need this ontop:
---
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index ecd34b8bd27e..e158bf4ee337 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -422,7 +422,6 @@ static const char * const super_crap_msg =
static int __init ghes_edac_register(void)
{
- struct ghes_edac_pvt *pvt = ghes_pvt;
bool fake = false;
int rc, num_dimm = 0;
struct mem_ctl_info *mci;
@@ -446,15 +445,15 @@ static int __init ghes_edac_register(void)
* We need to serialize edac_mc_alloc() and edac_mc_add_mc(),
* to avoid duplicated memory controller numbers
*/
- mci = edac_mc_alloc(1, ARRAY_SIZE(layers), layers, sizeof(*pvt));
+ mci = edac_mc_alloc(1, ARRAY_SIZE(layers), layers, sizeof(struct ghes_edac_pvt));
if (!mci) {
edac_pr_err("Can't allocate memory for EDAC data\n");
return -ENOMEM;
}
- pvt = mci->pvt_info;
- memset(pvt, 0, sizeof(*pvt));
- pvt->mci = mci;
+ ghes_pvt = mci->pvt_info;
+ memset(ghes_pvt, 0, sizeof(struct ghes_edac_pvt));
+ ghes_pvt->mci = mci;
mci->pdev = &dummy_dev;
---
> As Mauro pointed out, some type of GHES check needs to be in place
> before making this change.
Your whitelist I guess.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Sat, 2017-07-29 at 08:47 +0200, Borislav Petkov wrote:
> On Fri, Jul 28, 2017 at 06:50:56PM +0000, Kani, Toshimitsu wrote:
> > This simply sets NULL to pvt, and does not initialize ghes_pvt.
>
> Yeah, I guess we need this ontop:
Yes, this fix looks good.
> > As Mauro pointed out, some type of GHES check needs to be in place
> > before making this change.
>
> Your whitelist I guess.
We still need the two mechanisms provided by the existing code below.
The whitelist simply compliments 1.
1. GHES-presence check. (We can add APEI OSC bit check as well.)
2. Module priority. ghes_edac has higher priority for registration.
I'd prefer to add the whitelist check to ghes_edac first. This makes
the existing code to work. We can then work on refactoring changes
like this on top of it without breaking the functionality.
Thanks,
-Toshi
On Mon, Jul 31, 2017 at 08:19:32PM +0000, Kani, Toshimitsu wrote:
> I'd prefer to add the whitelist check to ghes_edac first. This makes
> the existing code to work. We can then work on refactoring changes
> like this on top of it without breaking the functionality.
Yes, but we want only the whitelist - not the FF testing because, as we
said, BIOS is notoriously buggy so we're going to load ghes_edac only on
known-good platforms.
Which brings the question about the priority.
And I *think* the easiest would be if the whitelist were in the core
edac.ko module, perhaps in edac_module.c (even though it doesn't really
matter, technically).
There we can set a "use_ghes" or so bool which the x86 platform drivers
would query through accessor functions and determine whether to load or
not.
In any case, something like that. I'm always open for better
suggestions, though.
I've pushed a rebased branch here:
https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=ghes
feel free to base your changes ontop.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Tue, 2017-08-01 at 11:46 +0200, Borislav Petkov wrote:
> On Mon, Jul 31, 2017 at 08:19:32PM +0000, Kani, Toshimitsu wrote:
> > I'd prefer to add the whitelist check to ghes_edac first. This
> > makes the existing code to work. We can then work on refactoring
> > changes like this on top of it without breaking the functionality.
>
> Yes, but we want only the whitelist - not the FF testing because, as
> we said, BIOS is notoriously buggy so we're going to load ghes_edac
> only on known-good platforms.
This GHES-probe itself is appropriate and should remain. Since not all
GHES firmware can be trusted, we will add the white-list as an
additional condition to complement this check.
> Which brings the question about the priority.
>
> And I *think* the easiest would be if the whitelist were in the core
> edac.ko module, perhaps in edac_module.c (even though it doesn't
> really matter, technically).
I agree that adding the white-list into the core edac module is the
easiest when we make the change on top of yours. Thinking further on
this, though, I now think that keeping the current implementation is
more reasonable with the reasons below.
1. Device-probing-logic should belong to a driver, and should remain
private to a driver. When we add the white-list, it should be added to
ghes_edac.
2. ghes_edac is an extension to the ghes driver as they both are
specific to ghes. ghes_edac is merely ghes driver's edac error-
reporting wrapper than an independent edac driver. It looks OK to let
ghes_edac get registered as part of ghes_probe() and leave it as an
unconventional edac driver.
3. EDAC does not have its managed probe-chain. All edac drivers are
called from module_init list. They independently probe the hardware
and get unloaded when not needed. The core edac is simply a set of
library to them. I think it's good to keep them independent, and not
to introduce a new central mechanism for a special case like ghes_edac.
Thanks,
-Toshi
On Wed, Aug 02, 2017 at 12:19:29AM +0000, Kani, Toshimitsu wrote:
> 1. Device-probing-logic should belong to a driver, and should remain
> private to a driver. When we add the white-list, it should be added to
> ghes_edac.
Nonsense. There are a lot of examples where driver probing depends on
outside modalities like built-in quirks and such.
> 2. ghes_edac is an extension to the ghes driver as they both are
> specific to ghes. ghes_edac is merely ghes driver's edac error-
> reporting wrapper than an independent edac driver. It looks OK to let
> ghes_edac get registered as part of ghes_probe() and leave it as an
> unconventional edac driver.
Except that GHES wants to report into the EDAC infrastructure so it
better has a wrapper for it.
One of the directions I explored when looking at this is to stick
ghes_edac functionality into ghes.c or so and make it completely
independent from EDAC. Would've been much cleaner.
> 3. EDAC does not have its managed probe-chain. All edac drivers are
> called from module_init list. They independently probe the hardware
> and get unloaded when not needed. The core edac is simply a set of
> library to them. I think it's good to keep them independent, and not
> to introduce a new central mechanism for a special case like ghes_edac.
They're independent because before GHES we needed to load one driver per
system. Until the bolted-on thing came. And it is bolted on because the
already overwhelmed firmware decided to do error reporting too.
So the only real reason why I'm fine with keeping the current situation
is the whitelist. Because then, we can at least control what loads and
what not.
But then we need:
1. A clean mechanism for the platform drivers to query whether another
agent is loaded (ghes_edac) and not do any probing then.
2. ghes_edac needs to drop that multiple probing thing as its
dmi_walk(ghes_edac_count_dimms, &num_dimm) already probes *all* DIMMs on
the system so no need to do that multiple times.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Wed, 2017-08-02 at 05:18 +0200, Borislav Petkov wrote:
> On Wed, Aug 02, 2017 at 12:19:29AM +0000, Kani, Toshimitsu wrote:
> > 1. Device-probing-logic should belong to a driver, and should
> > remain private to a driver. When we add the white-list, it should
> > be added to ghes_edac.
>
> Nonsense. There are a lot of examples where driver probing depends on
> outside modalities like built-in quirks and such.
>
> > 2. ghes_edac is an extension to the ghes driver as they both are
> > specific to ghes. ghes_edac is merely ghes driver's edac error-
> > reporting wrapper than an independent edac driver. It looks OK to
> > let ghes_edac get registered as part of ghes_probe() and leave it
> > as an unconventional edac driver.
>
> Except that GHES wants to report into the EDAC infrastructure so it
> better has a wrapper for it.
>
> One of the directions I explored when looking at this is to stick
> ghes_edac functionality into ghes.c or so and make it completely
> independent from EDAC. Would've been much cleaner.
Agreed. I think the current model aimed at this direction while it was
needed to depend on EDAC.
> > 3. EDAC does not have its managed probe-chain. All edac drivers
> > are called from module_init list. They independently probe the
> > hardware and get unloaded when not needed. The core edac is simply
> > a set of library to them. I think it's good to keep them
> > independent, and not to introduce a new central mechanism for a
> > special case like ghes_edac.
>
> They're independent because before GHES we needed to load one driver
> per system. Until the bolted-on thing came. And it is bolted on
> because the already overwhelmed firmware decided to do error
> reporting too.
>
> So the only real reason why I'm fine with keeping the current
> situation is the whitelist. Because then, we can at least control
> what loads and what not.
>
> But then we need:
>
> 1. A clean mechanism for the platform drivers to query whether
> another agent is loaded (ghes_edac) and not do any probing then.
>
> 2. ghes_edac needs to drop that multiple probing thing as its
> dmi_walk(ghes_edac_count_dimms, &num_dimm) already probes *all* DIMMs
> on the system so no need to do that multiple times.
Sounds good. I will keep the current model and address the above
points.
Thanks,
-Toshi