From 75f029c3a83f3e7a1d0d928efa4fe47dd6a8a9eb Mon Sep 17 00:00:00 2001 From: Arvind Yadav Date: Thu, 21 Sep 2017 12:16:56 +0200 Subject: [PATCH 01/13] EDAC: Handle return value of kasprintf() kasprintf() can fail and we must check its return value. Signed-off-by: Arvind Yadav Cc: linux-edac@vger.kernel.org [ Merged into a single patch, small formatting fixups. ] Signed-off-by: Borislav Petkov --- drivers/edac/i7core_edac.c | 11 +++++++++-- drivers/edac/sb_edac.c | 5 +++++ drivers/edac/skx_edac.c | 9 +++++++-- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/drivers/edac/i7core_edac.c b/drivers/edac/i7core_edac.c index c16c3b931b3d..8c5540160a23 100644 --- a/drivers/edac/i7core_edac.c +++ b/drivers/edac/i7core_edac.c @@ -2159,8 +2159,13 @@ static int i7core_register_mci(struct i7core_dev *i7core_dev) mci->edac_ctl_cap = EDAC_FLAG_NONE; mci->edac_cap = EDAC_FLAG_NONE; mci->mod_name = "i7core_edac.c"; - mci->ctl_name = kasprintf(GFP_KERNEL, "i7 core #%d", - i7core_dev->socket); + + mci->ctl_name = kasprintf(GFP_KERNEL, "i7 core #%d", i7core_dev->socket); + if (!mci->ctl_name) { + rc = -ENOMEM; + goto fail1; + } + mci->dev_name = pci_name(i7core_dev->pdev[0]); mci->ctl_page_to_phys = NULL; @@ -2214,6 +2219,8 @@ static int i7core_register_mci(struct i7core_dev *i7core_dev) fail0: kfree(mci->ctl_name); + +fail1: edac_mc_free(mci); i7core_dev->mci = NULL; return rc; diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c index dc0591654011..2078ee414568 100644 --- a/drivers/edac/sb_edac.c +++ b/drivers/edac/sb_edac.c @@ -3287,6 +3287,11 @@ static int sbridge_register_mci(struct sbridge_dev *sbridge_dev, enum type type) break; } + if (!mci->ctl_name) { + rc = -ENOMEM; + goto fail0; + } + /* Get dimm basic config and the memory layout */ rc = get_dimm_config(mci); if (rc < 0) { diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c index 16dea97568a1..3fe85b0ac274 100644 --- a/drivers/edac/skx_edac.c +++ b/drivers/edac/skx_edac.c @@ -464,8 +464,12 @@ static int skx_register_mci(struct skx_imc *imc) pvt = mci->pvt_info; pvt->imc = imc; - mci->ctl_name = kasprintf(GFP_KERNEL, "Skylake Socket#%d IMC#%d", - imc->node_id, imc->lmc); + mci->ctl_name = kasprintf(GFP_KERNEL, "Skylake Socket#%d IMC#%d", imc->node_id, imc->lmc); + if (!mci->ctl_name) { + rc = -ENOMEM; + goto fail0; + } + mci->mtype_cap = MEM_FLAG_DDR4; mci->edac_ctl_cap = EDAC_FLAG_NONE; mci->edac_cap = EDAC_FLAG_NONE; @@ -491,6 +495,7 @@ static int skx_register_mci(struct skx_imc *imc) fail: kfree(mci->ctl_name); +fail0: edac_mc_free(mci); imc->mci = NULL; return rc; From c9c8b4d6d0259ac914309f8fbcef022792c99a89 Mon Sep 17 00:00:00 2001 From: Borislav Petkov Date: Wed, 16 Aug 2017 19:41:52 +0200 Subject: [PATCH 02/13] EDAC, ghes: Remove symbol exports They're called from builtin code so no need for the exports. Signed-off-by: Borislav Petkov --- drivers/edac/ghes_edac.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index 6f80eb65c26c..8acdf29cf3a4 100644 --- a/drivers/edac/ghes_edac.c +++ b/drivers/edac/ghes_edac.c @@ -401,7 +401,6 @@ 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); } -EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error); int ghes_edac_register(struct ghes *ghes, struct device *dev) { @@ -505,7 +504,6 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) mutex_unlock(&ghes_edac_lock); return 0; } -EXPORT_SYMBOL_GPL(ghes_edac_register); void ghes_edac_unregister(struct ghes *ghes) { @@ -521,4 +519,3 @@ void ghes_edac_unregister(struct ghes *ghes) } } } -EXPORT_SYMBOL_GPL(ghes_edac_unregister); From 0fe5f281f749f1d4e462a282ef8ba76407a11fd1 Mon Sep 17 00:00:00 2001 From: Borislav Petkov Date: Wed, 16 Aug 2017 10:33:44 +0200 Subject: [PATCH 03/13] EDAC, ghes: Model a single, logical memory controller We're enumerating the DIMMs through a DMI walk and since we can't get any more detailed topological information about which DIMMs belong to which memory controller, convert it to a single, logical controller which contains all the DIMMs. The error reporting path from GHES ghes_edac_report_mem_error() doesn't get called in NMI context but add a warning about it to catch any changes in the future as if so, our locking scheme will be insufficient then. Signed-off-by: Borislav Petkov --- drivers/edac/ghes_edac.c | 114 +++++++++++++++++---------------------- 1 file changed, 50 insertions(+), 64 deletions(-) diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index 8acdf29cf3a4..8d904df75ad8 100644 --- a/drivers/edac/ghes_edac.c +++ b/drivers/edac/ghes_edac.c @@ -28,10 +28,15 @@ 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 atomic_t ghes_init = ATOMIC_INIT(0); +static struct ghes_edac_pvt *ghes_pvt; +/* + * Sync with other, potentially concurrent callers of + * ghes_edac_report_mem_error(). We don't know what the + * "inventive" firmware would do. + */ +static DEFINE_SPINLOCK(ghes_lock); /* Memory Device - Type 17 of SMBIOS spec */ struct memdev_dmi_entry { @@ -169,18 +174,26 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev, enum hw_event_mc_err_type type; struct edac_raw_error_desc *e; struct mem_ctl_info *mci; - struct ghes_edac_pvt *pvt = NULL; + struct ghes_edac_pvt *pvt = ghes_pvt; + unsigned long flags; char *p; u8 grain_bits; - 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; } + + /* + * We can do the locking below because GHES defers error processing + * from NMI to IRQ context. Whenever that changes, we'd at least + * know. + */ + if (WARN_ON_ONCE(in_nmi())) + return; + + spin_lock_irqsave(&ghes_lock, flags); + mci = pvt->mci; e = &mci->error_desc; @@ -398,8 +411,8 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev, (e->page_frame_number << PAGE_SHIFT) | e->offset_in_page, grain_bits, e->syndrome, pvt->detail_location); - /* Report the error via EDAC API */ edac_raw_mc_handle_error(type, mci, e); + spin_unlock_irqrestore(&ghes_lock, flags); } int ghes_edac_register(struct ghes *ghes, struct device *dev) @@ -408,9 +421,14 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) 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; + /* + * We have only one logical memory controller to which all DIMMs belong. + */ + if (atomic_inc_return(&ghes_init) > 1) + return 0; + /* Get the number of DIMMs */ dmi_walk(ghes_edac_count_dimms, &num_dimm); @@ -424,26 +442,17 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) layers[0].size = num_dimm; layers[0].is_virt_csrow = true; - /* - * 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(0, ARRAY_SIZE(layers), layers, sizeof(struct ghes_edac_pvt)); if (!mci) { pr_info("Can't allocate memory for EDAC data\n"); - mutex_unlock(&ghes_edac_lock); 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; + ghes_pvt = mci->pvt_info; + ghes_pvt->ghes = ghes; + ghes_pvt->mci = mci; + mci->pdev = dev; mci->mtype_cap = MEM_FLAG_EMPTY; mci->edac_ctl_cap = EDAC_FLAG_NONE; mci->edac_cap = EDAC_FLAG_NONE; @@ -451,36 +460,23 @@ 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) { + 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) { - /* - * Fill DIMM info from DMI for the memory controller #0 - * - * 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 - */ - 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); @@ -496,26 +492,16 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) if (rc < 0) { pr_info("Can't register at EDAC core\n"); edac_mc_free(mci); - mutex_unlock(&ghes_edac_lock); return -ENODEV; } - - ghes_edac_mc_num++; - mutex_unlock(&ghes_edac_lock); return 0; } void ghes_edac_unregister(struct ghes *ghes) { 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); - } - } + mci = ghes_pvt->mci; + edac_mc_del_mc(mci->pdev); + edac_mc_free(mci); } From 5deed6b6a479ad5851d7ead6412dc6faa84a694e Mon Sep 17 00:00:00 2001 From: Toshi Kani Date: Wed, 23 Aug 2017 16:54:45 -0600 Subject: [PATCH 04/13] EDAC, ghes: Add platform check The ghes_edac driver was introduced in 2013 [1], but it has not been enabled by any distro yet. This driver obtains error info from firmware interfaces (APEI), which are not properly implemented on many platforms, as the driver says on load: This EDAC driver relies on BIOS to enumerate memory and get error reports. Unfortunately, not all BIOSes reflect the memory layout correctly. So, the end result of using this driver varies from vendor to vendor. If you find incorrect reports, please contact your hardware vendor to correct its BIOS. To get out from this situation, add a platform check to selectively enable the driver on platforms that are known to have proper APEI firmware implementation. "ghes_edac.force_load=1" skips this platform check. [1]: https://lkml.kernel.org/r/cover.1360931635.git.mchehab@redhat.com Signed-off-by: Toshi Kani Cc: Mauro Carvalho Chehab Cc: Tony Luck Cc: linux-acpi@vger.kernel.org Cc: linux-edac Link: http://lkml.kernel.org/r/20170823225447.15608-4-toshi.kani@hpe.com Signed-off-by: Borislav Petkov --- drivers/edac/ghes_edac.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index 8d904df75ad8..68b6ee18bea6 100644 --- a/drivers/edac/ghes_edac.c +++ b/drivers/edac/ghes_edac.c @@ -38,6 +38,10 @@ static struct ghes_edac_pvt *ghes_pvt; */ static DEFINE_SPINLOCK(ghes_lock); +/* "ghes_edac.force_load=1" skips the platform check */ +static bool __read_mostly force_load; +module_param(force_load, bool, 0); + /* Memory Device - Type 17 of SMBIOS spec */ struct memdev_dmi_entry { u8 type; @@ -415,6 +419,14 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev, spin_unlock_irqrestore(&ghes_lock, flags); } +/* + * Known systems that are safe to enable this module. + */ +static struct acpi_platform_list plat_list[] = { + {"HPE ", "Server ", 0, ACPI_SIG_FADT, all_versions}, + { } /* End */ +}; + int ghes_edac_register(struct ghes *ghes, struct device *dev) { bool fake = false; @@ -422,6 +434,12 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) struct mem_ctl_info *mci; struct edac_mc_layer layers[1]; struct ghes_edac_dimm_fill dimm_fill; + int idx; + + /* Check if safe to enable on this system */ + idx = acpi_match_platform_list(plat_list); + if (!force_load && idx < 0) + return 0; /* * We have only one logical memory controller to which all DIMMs belong. @@ -460,17 +478,17 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev) mci->ctl_name = "ghes_edac"; mci->dev_name = "ghes"; - if (!fake) { + if (fake) { + 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"); + } else if (idx < 0) { 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) { From 3877c7d1e24c05eeb1c57ade04d2527d511f25a1 Mon Sep 17 00:00:00 2001 From: Toshi Kani Date: Wed, 23 Aug 2017 16:54:46 -0600 Subject: [PATCH 05/13] EDAC: Add helper which returns the loaded platform driver Only a single EDAC platform driver can be loaded. When ghes_edac is enabled, an EDAC platform driver still attempts to register itself and fails in edac_mc_add_mc(). Add edac_get_owner() so that EDAC platform drivers can check the owner first. Signed-off-by: Toshi Kani Suggested-by: Borislav Petkov Cc: Mauro Carvalho Chehab Cc: Tony Luck Cc: linux-edac Link: http://lkml.kernel.org/r/20170823225447.15608-5-toshi.kani@hpe.com [ Massage commit message. ] Signed-off-by: Borislav Petkov --- drivers/edac/edac_mc.c | 7 ++++++- drivers/edac/edac_mc.h | 8 ++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c index 480072139b7a..48193f5f3b56 100644 --- a/drivers/edac/edac_mc.c +++ b/drivers/edac/edac_mc.c @@ -53,7 +53,7 @@ static LIST_HEAD(mc_devices); * Used to lock EDAC MC to just one module, avoiding two drivers e. g. * apei/ghes and i7core_edac to be used at the same time. */ -static void const *edac_mc_owner; +static const char *edac_mc_owner; static struct bus_type mc_bus[EDAC_MAX_MCS]; @@ -701,6 +701,11 @@ unlock: } EXPORT_SYMBOL(edac_mc_find); +const char *edac_get_owner(void) +{ + return edac_mc_owner; +} +EXPORT_SYMBOL_GPL(edac_get_owner); /* FIXME - should a warning be printed if no error detection? correction? */ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci, diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h index 5357800e418d..4165e15995ad 100644 --- a/drivers/edac/edac_mc.h +++ b/drivers/edac/edac_mc.h @@ -128,6 +128,14 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num, unsigned sz_pvt); /** + * edac_get_owner - Return the owner's mod_name of EDAC MC + * + * Returns: + * Pointer to mod_name string when EDAC MC is owned. NULL otherwise. + */ +extern const char *edac_get_owner(void); + +/* * edac_mc_add_mc_with_groups() - Insert the @mci structure into the mci * global list and create sysfs entries associated with @mci structure. * From 301375e764324b8048704eaf2c46fe1ee290830e Mon Sep 17 00:00:00 2001 From: Toshi Kani Date: Wed, 23 Aug 2017 16:54:47 -0600 Subject: [PATCH 06/13] EDAC: Add owner check to the x86 platform drivers Change x86 EDAC platform drivers to verify the module owner at the beginning of their module init functions. This allows them to fail their init immediately when ghes_edac is enabled. Similar change can be made to other edac drivers if necessary. Also, remove ".c" from module names of pnp2_edac, sb_edac, and skx_edac. Signed-off-by: Toshi Kani Suggested-by: Borislav Petkov Cc: Mauro Carvalho Chehab Cc: Tony Luck Cc: linux-edac Link: http://lkml.kernel.org/r/20170823225447.15608-6-toshi.kani@hpe.com Signed-off-by: Borislav Petkov --- drivers/edac/amd64_edac.c | 5 +++++ drivers/edac/pnd2_edac.c | 9 ++++++++- drivers/edac/sb_edac.c | 9 +++++++-- drivers/edac/skx_edac.c | 9 ++++++++- 4 files changed, 28 insertions(+), 4 deletions(-) diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index ac2f30295efe..8b16ec595fa7 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -3434,9 +3434,14 @@ MODULE_DEVICE_TABLE(x86cpu, amd64_cpuids); static int __init amd64_edac_init(void) { + const char *owner; int err = -ENODEV; int i; + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + if (!x86_match_cpu(amd64_cpuids)) return -ENODEV; diff --git a/drivers/edac/pnd2_edac.c b/drivers/edac/pnd2_edac.c index 4395c84cdcbf..df28b65358d2 100644 --- a/drivers/edac/pnd2_edac.c +++ b/drivers/edac/pnd2_edac.c @@ -45,6 +45,8 @@ #include "edac_module.h" #include "pnd2_edac.h" +#define EDAC_MOD_STR "pnd2_edac" + #define APL_NUM_CHANNELS 4 #define DNV_NUM_CHANNELS 2 #define DNV_MAX_DIMMS 2 /* Max DIMMs per channel */ @@ -1355,7 +1357,7 @@ static int pnd2_register_mci(struct mem_ctl_info **ppmci) pvt = mci->pvt_info; memset(pvt, 0, sizeof(*pvt)); - mci->mod_name = "pnd2_edac.c"; + mci->mod_name = EDAC_MOD_STR; mci->dev_name = ops->name; mci->ctl_name = "Pondicherry2"; @@ -1547,10 +1549,15 @@ MODULE_DEVICE_TABLE(x86cpu, pnd2_cpuids); static int __init pnd2_init(void) { const struct x86_cpu_id *id; + const char *owner; int rc; edac_dbg(2, "\n"); + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + id = x86_match_cpu(pnd2_cpuids); if (!id) return -ENODEV; diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c index 2078ee414568..3ea90fc5978b 100644 --- a/drivers/edac/sb_edac.c +++ b/drivers/edac/sb_edac.c @@ -36,7 +36,7 @@ static LIST_HEAD(sbridge_edac_list); * Alter this version for the module when modifications are made */ #define SBRIDGE_REVISION " Ver: 1.1.2 " -#define EDAC_MOD_STR "sbridge_edac" +#define EDAC_MOD_STR "sb_edac" /* * Debug macros @@ -3155,7 +3155,7 @@ static int sbridge_register_mci(struct sbridge_dev *sbridge_dev, enum type type) MEM_FLAG_DDR4 : MEM_FLAG_DDR3; mci->edac_ctl_cap = EDAC_FLAG_NONE; mci->edac_cap = EDAC_FLAG_NONE; - mci->mod_name = "sb_edac.c"; + mci->mod_name = EDAC_MOD_STR; mci->dev_name = pci_name(pdev); mci->ctl_page_to_phys = NULL; @@ -3407,10 +3407,15 @@ static void sbridge_remove(void) static int __init sbridge_init(void) { const struct x86_cpu_id *id; + const char *owner; int rc; edac_dbg(2, "\n"); + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + id = x86_match_cpu(sbridge_cpuids); if (!id) return -ENODEV; diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c index 3fe85b0ac274..e8e570e73796 100644 --- a/drivers/edac/skx_edac.c +++ b/drivers/edac/skx_edac.c @@ -31,6 +31,8 @@ #include "edac_module.h" +#define EDAC_MOD_STR "skx_edac" + /* * Debug macros */ @@ -473,7 +475,7 @@ static int skx_register_mci(struct skx_imc *imc) mci->mtype_cap = MEM_FLAG_DDR4; mci->edac_ctl_cap = EDAC_FLAG_NONE; mci->edac_cap = EDAC_FLAG_NONE; - mci->mod_name = "skx_edac.c"; + mci->mod_name = EDAC_MOD_STR; mci->dev_name = pci_name(imc->chan[0].cdev); mci->ctl_page_to_phys = NULL; @@ -1044,12 +1046,17 @@ static int __init skx_init(void) { const struct x86_cpu_id *id; const struct munit *m; + const char *owner; int rc = 0, i; u8 mc = 0, src_id, node_id; struct skx_dev *d; edac_dbg(2, "\n"); + owner = edac_get_owner(); + if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR))) + return -EBUSY; + id = x86_match_cpu(skx_cpuids); if (!id) return -ENODEV; From 15cc3ae001873845b5d842e212478a6570c7d938 Mon Sep 17 00:00:00 2001 From: Qiuxu Zhuo Date: Wed, 13 Sep 2017 18:42:14 +0800 Subject: [PATCH 07/13] EDAC, sb_edac: Don't create a second memory controller if HA1 is not present Yi Zhang reported the following failure on a 2-socket Haswell (E5-2603v3) server (DELL PowerEdge 730xd): EDAC sbridge: Some needed devices are missing EDAC MC: Removed device 0 for sb_edac.c Haswell SrcID#0_Ha#0: DEV 0000:7f:12.0 EDAC MC: Removed device 1 for sb_edac.c Haswell SrcID#1_Ha#0: DEV 0000:ff:12.0 EDAC sbridge: Couldn't find mci handler EDAC sbridge: Couldn't find mci handler EDAC sbridge: Failed to register device with error -19. The refactored sb_edac driver creates the IMC1 (the 2nd memory controller) if any IMC1 device is present. In this case only HA1_TA of IMC1 was present, but the driver expected to find HA1/HA1_TM/HA1_TAD[0-3] devices too, leading to the above failure. The document [1] says the 'E5-2603 v3' CPU has 4 memory channels max. Yi Zhang inserted one DIMM per channel for each CPU, and did random error address injection test with this patch: 4024 addresses fell in TOLM hole area 12715 addresses fell in CPU_SrcID#0_Ha#0_Chan#0_DIMM#0 12774 addresses fell in CPU_SrcID#0_Ha#0_Chan#1_DIMM#0 12798 addresses fell in CPU_SrcID#0_Ha#0_Chan#2_DIMM#0 12913 addresses fell in CPU_SrcID#0_Ha#0_Chan#3_DIMM#0 12674 addresses fell in CPU_SrcID#1_Ha#0_Chan#0_DIMM#0 12686 addresses fell in CPU_SrcID#1_Ha#0_Chan#1_DIMM#0 12882 addresses fell in CPU_SrcID#1_Ha#0_Chan#2_DIMM#0 12934 addresses fell in CPU_SrcID#1_Ha#0_Chan#3_DIMM#0 106400 addresses were injected totally. The test result shows that all the 4 channels belong to IMC0 per CPU, so the server really only has one IMC per CPU. In the 1st page of chapter 2 in datasheet [2], it also says 'E5-2600 v3' implements either one or two IMCs. For CPUs with one IMC, IMC1 is not used and should be ignored. Thus, do not create a second memory controller if the key HA1 is absent. [1] http://ark.intel.com/products/83349/Intel-Xeon-Processor-E5-2603-v3-15M-Cache-1_60-GHz [2] https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-v3-datasheet-vol-2.pdf Reported-and-tested-by: Yi Zhang Signed-off-by: Qiuxu Zhuo Cc: Tony Luck Cc: linux-edac Fixes: e2f747b1f42a ("EDAC, sb_edac: Assign EDAC memory controller per h/w controller") Link: http://lkml.kernel.org/r/20170913104214.7325-1-qiuxu.zhuo@intel.com [ Massage commit message. ] Signed-off-by: Borislav Petkov --- drivers/edac/sb_edac.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c index 3ea90fc5978b..54d9d179cb77 100644 --- a/drivers/edac/sb_edac.c +++ b/drivers/edac/sb_edac.c @@ -462,6 +462,7 @@ static const struct pci_id_table pci_dev_descr_sbridge_table[] = { static const struct pci_id_descr pci_dev_descr_ibridge[] = { /* Processor Home Agent */ { PCI_DESCR(PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA0, 0, IMC0) }, + { PCI_DESCR(PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA1, 1, IMC1) }, /* Memory controller */ { PCI_DESCR(PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA0_TA, 0, IMC0) }, @@ -472,7 +473,6 @@ static const struct pci_id_descr pci_dev_descr_ibridge[] = { { PCI_DESCR(PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA0_TAD3, 0, IMC0) }, /* Optional, mode 2HA */ - { PCI_DESCR(PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA1, 1, IMC1) }, { PCI_DESCR(PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA1_TA, 1, IMC1) }, { PCI_DESCR(PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA1_RAS, 1, IMC1) }, { PCI_DESCR(PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA1_TAD0, 1, IMC1) }, @@ -2291,6 +2291,13 @@ static int sbridge_get_onedevice(struct pci_dev **prev, next_imc: sbridge_dev = get_sbridge_dev(bus, dev_descr->dom, multi_bus, sbridge_dev); if (!sbridge_dev) { + /* If the HA1 wasn't found, don't create EDAC second memory controller */ + if (dev_descr->dom == IMC1 && devno != 1) { + edac_dbg(0, "Skip IMC1: %04x:%04x (since HA1 was absent)\n", + PCI_VENDOR_ID_INTEL, dev_descr->dev_id); + pci_dev_put(pdev); + return 0; + } if (dev_descr->dom == SOCK) goto out_imc; From a9c0a10888614603cc38b6f60250e92932f24a37 Mon Sep 17 00:00:00 2001 From: Charles Rose Date: Mon, 25 Sep 2017 10:10:43 -0700 Subject: [PATCH 08/13] EDAC, skx_edac: Fix detection of single-rank DIMMs Single-rank DIMMs did not get detected on Skylake/Kabylake systems due to wrong limit check. The single rank DIMM check is a simple typo. "0" is a legal value in this field meaning single rank. Signed-off-by: Charles Rose Cc: Aristeu Rozanski Cc: Mauro Carvalho Chehab Cc: Qiuxu Zhuo Cc: linux-edac Link: http://lkml.kernel.org/r/66df72d327c265fbf92fe25df96daa228a35f076.1506358467.git.tony.luck@intel.com [ Also fix debug message to print number of ranks. ] Signed-off-by: Tony Luck [ Expand commit message. ] Signed-off-by: Borislav Petkov --- drivers/edac/skx_edac.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c index e8e570e73796..9ca40a3fe03f 100644 --- a/drivers/edac/skx_edac.c +++ b/drivers/edac/skx_edac.c @@ -301,7 +301,7 @@ static int get_dimm_attr(u32 reg, int lobit, int hibit, int add, int minval, #define IS_DIMM_PRESENT(mtr) GET_BITFIELD((mtr), 15, 15) -#define numrank(reg) get_dimm_attr((reg), 12, 13, 0, 1, 2, "ranks") +#define numrank(reg) get_dimm_attr((reg), 12, 13, 0, 0, 2, "ranks") #define numrow(reg) get_dimm_attr((reg), 2, 4, 12, 1, 6, "rows") #define numcol(reg) get_dimm_attr((reg), 0, 1, 10, 0, 2, "cols") @@ -362,7 +362,7 @@ static int get_dimm_info(u32 mtr, u32 amap, struct dimm_info *dimm, edac_dbg(0, "mc#%d: channel %d, dimm %d, %lld Mb (%d pages) bank: %d, rank: %d, row: %#x, col: %#x\n", imc->mc, chan, dimmno, size, npages, - banks, ranks, rows, cols); + banks, 1 << ranks, rows, cols); imc->chan[chan].dimms[dimmno].close_pg = GET_BITFIELD(mtr, 0, 0); imc->chan[chan].dimms[dimmno].bank_xor_enable = GET_BITFIELD(mtr, 9, 9); From f821fe8cc7e51b238585e71b688e7093bfec355c Mon Sep 17 00:00:00 2001 From: Jan Glauber Date: Mon, 25 Sep 2017 14:34:56 +0200 Subject: [PATCH 09/13] EDAC, thunderx: Remove suspend/resume support The memory controller on ThunderX/OcteonTX systems does not support power management. Therefore remove the suspend/resume callbacks. Signed-off-by: Jan Glauber Cc: David Daney Cc: Jan Glauber Cc: Mark Rutland Cc: Mauro Carvalho Chehab Cc: Ralf Baechle Cc: Suzuki K Poulose Cc: Will Deacon Cc: Zhangshaokun Cc: linux-arm-kernel@lists.infradead.org Cc: linux-edac Cc: linux-mips@linux-mips.org Link: http://lkml.kernel.org/r/20170925123502.17289-2-jglauber@cavium.com Signed-off-by: Borislav Petkov --- drivers/edac/thunderx_edac.c | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/drivers/edac/thunderx_edac.c b/drivers/edac/thunderx_edac.c index f35d87519a3e..4803c6468bab 100644 --- a/drivers/edac/thunderx_edac.c +++ b/drivers/edac/thunderx_edac.c @@ -639,27 +639,6 @@ err_free: return ret; } -#ifdef CONFIG_PM -static int thunderx_lmc_suspend(struct pci_dev *pdev, pm_message_t state) -{ - pci_save_state(pdev); - pci_disable_device(pdev); - - pci_set_power_state(pdev, pci_choose_state(pdev, state)); - - return 0; -} - -static int thunderx_lmc_resume(struct pci_dev *pdev) -{ - pci_set_power_state(pdev, PCI_D0); - pci_enable_wake(pdev, PCI_D0, 0); - pci_restore_state(pdev); - - return 0; -} -#endif - static const struct pci_device_id thunderx_lmc_pci_tbl[] = { { PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDER_LMC) }, { 0, }, @@ -834,10 +813,6 @@ static struct pci_driver thunderx_lmc_driver = { .name = "thunderx_lmc_edac", .probe = thunderx_lmc_probe, .remove = thunderx_lmc_remove, -#ifdef CONFIG_PM - .suspend = thunderx_lmc_suspend, - .resume = thunderx_lmc_resume, -#endif .id_table = thunderx_lmc_pci_tbl, }; From 88ae80aa609c7db3e3cc98e97e05badbcc6347dc Mon Sep 17 00:00:00 2001 From: Tony Luck Date: Mon, 25 Sep 2017 10:10:42 -0700 Subject: [PATCH 10/13] EDAC, skx_edac: Handle systems with segmented PCI busses Large systems separate their PCI busses into segments since the limit of only 256 PCI busses can be too restrictive. Extend this driver to check whether matches when deciding how to group memory controller PCI devices to CPU sockets. Signed-off-by: Tony Luck Cc: Aristeu Rozanski Cc: Charles Rose Cc: Mauro Carvalho Chehab Cc: Qiuxu Zhuo Cc: linux-edac Link: http://lkml.kernel.org/r/f58abfd10bf73c8bc5adc1fe4de7408128b00625.1506358467.git.tony.luck@intel.com [ Make skx_dev.seg an int. ] Signed-off-by: Borislav Petkov --- drivers/edac/skx_edac.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c index 9ca40a3fe03f..912c4930c9ef 100644 --- a/drivers/edac/skx_edac.c +++ b/drivers/edac/skx_edac.c @@ -67,6 +67,7 @@ static u64 skx_tolm, skx_tohm; struct skx_dev { struct list_head list; u8 bus[4]; + int seg; struct pci_dev *sad_all; struct pci_dev *util_all; u32 mcroute; @@ -112,12 +113,12 @@ struct decoded_addr { int bank_group; }; -static struct skx_dev *get_skx_dev(u8 bus, u8 idx) +static struct skx_dev *get_skx_dev(struct pci_bus *bus, u8 idx) { struct skx_dev *d; list_for_each_entry(d, &skx_edac_list, list) { - if (d->bus[idx] == bus) + if (d->seg == pci_domain_nr(bus) && d->bus[idx] == bus->number) return d; } @@ -174,6 +175,7 @@ static int get_all_bus_mappings(void) pci_dev_put(pdev); return -ENOMEM; } + d->seg = pci_domain_nr(pdev->bus); pci_read_config_dword(pdev, 0xCC, ®); d->bus[0] = GET_BITFIELD(reg, 0, 7); d->bus[1] = GET_BITFIELD(reg, 8, 15); @@ -209,7 +211,7 @@ static int get_all_munits(const struct munit *m) if (i == NUM_IMC) goto fail; } - d = get_skx_dev(pdev->bus->number, m->busidx); + d = get_skx_dev(pdev->bus, m->busidx); if (!d) goto fail; From 24281a2f4ca569d44e44e4cbc500cf08e7bb4c36 Mon Sep 17 00:00:00 2001 From: Luis Felipe Sandoval Castro Date: Thu, 28 Sep 2017 08:54:42 -0500 Subject: [PATCH 11/13] EDAC, sb_edac: Fix missing DIMM sysfs entries with KNL SNC2/SNC4 mode When figuring out the size of the DIMMs and the cluster mode is SNC2 or SNC4 the current algorithm ignores the contribution of some of the channels resulting in EDAC never knowing of the existence of some DIMMs attached to such channels (thus sysfs is not populated). Instead of selectively iterating from 0 to interlv_ways when looking for all the participants in the interleave, do an exhaustive search and iterate from 0 to KNL_MAX_CHANNELS. The algorithm is already smart enough to consider participants only one time. This works fine in all KNL cluster modes and even when there are missing DIMMs as the contribution of those channels is 0. Signed-off-by: Luis Felipe Sandoval Castro Acked-by: Tony Luck Cc: Mauro Carvalho Chehab Cc: arozansk@redhat.com Cc: linux-edac Cc: qiuxu.zhuo@intel.com Link: http://lkml.kernel.org/r/1506606882-90521-1-git-send-email-luis.felipe.sandoval.castro@intel.com Signed-off-by: Borislav Petkov --- drivers/edac/sb_edac.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c index 54d9d179cb77..72b98a081d2b 100644 --- a/drivers/edac/sb_edac.c +++ b/drivers/edac/sb_edac.c @@ -1318,9 +1318,7 @@ static int knl_get_dimm_capacity(struct sbridge_pvt *pvt, u64 *mc_sizes) int cur_reg_start; int mc; int channel; - int way; int participants[KNL_MAX_CHANNELS]; - int participant_count = 0; for (i = 0; i < KNL_MAX_CHANNELS; i++) mc_sizes[i] = 0; @@ -1495,21 +1493,14 @@ static int knl_get_dimm_capacity(struct sbridge_pvt *pvt, u64 *mc_sizes) * this channel mapped to the given target? */ for (channel = 0; channel < KNL_MAX_CHANNELS; channel++) { - for (way = 0; way < intrlv_ways; way++) { - int target; - int cha; - - if (KNL_MOD3(dram_rule)) - target = way; - else - target = 0x7 & sad_pkg( - pvt->info.interleave_pkg, interleave_reg, way); + int target; + int cha; + for (target = 0; target < KNL_MAX_CHANNELS; target++) { for (cha = 0; cha < KNL_MAX_CHAS; cha++) { if (knl_get_mc_route(target, mc_route_reg[cha]) == channel && !participants[channel]) { - participant_count++; participants[channel] = 1; break; } @@ -1517,10 +1508,6 @@ static int knl_get_dimm_capacity(struct sbridge_pvt *pvt, u64 *mc_sizes) } } - if (participant_count != intrlv_ways) - edac_dbg(0, "participant_count (%d) != interleave_ways (%d): DIMM size may be incorrect\n", - participant_count, intrlv_ways); - for (channel = 0; channel < KNL_MAX_CHANNELS; channel++) { mc = knl_channel_mc(channel); if (participants[channel]) { From 7339605aa1b5fc551eb23a9563b8b84ba33632f0 Mon Sep 17 00:00:00 2001 From: Jan Glauber Date: Thu, 19 Oct 2017 08:41:29 +0200 Subject: [PATCH 12/13] MAINTAINERS: Split Cavium EDAC entry and add myself Split the Cavium EDAC entry into MIPS and ARM drivers because they have different maintainers and mailing lists. Add myself as additional maintainer to the ThunderX driver. Signed-off-by: Jan Glauber Cc: David Daney Cc: Ralf Baechle Cc: Sergey Temerkhanov Cc: linux-edac Link: http://lkml.kernel.org/r/20171019064129.5064-1-jglauber@cavium.com Signed-off-by: Borislav Petkov --- MAINTAINERS | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 2281af4b41b6..0eedf0bd1b7d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4895,13 +4895,19 @@ L: linux-edac@vger.kernel.org S: Maintained F: drivers/edac/highbank* -EDAC-CAVIUM +EDAC-CAVIUM OCTEON M: Ralf Baechle M: David Daney L: linux-edac@vger.kernel.org L: linux-mips@linux-mips.org S: Supported F: drivers/edac/octeon_edac* + +EDAC-CAVIUM THUNDERX +M: David Daney +M: Jan Glauber +L: linux-edac@vger.kernel.org +S: Supported F: drivers/edac/thunderx_edac* EDAC-CORE From a8e9b186f153a44690ad0363a56716e7077ad28c Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Mon, 16 Oct 2017 12:40:29 -0500 Subject: [PATCH 13/13] EDAC, sb_edac: Fix missing break in switch Add missing break statement in order to prevent the code from falling through. Signed-off-by: Gustavo A. R. Silva Cc: Qiuxu Zhuo Cc: linux-edac Link: http://lkml.kernel.org/r/20171016174029.GA19757@embeddedor.com Signed-off-by: Borislav Petkov --- drivers/edac/sb_edac.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c index 72b98a081d2b..f34430f99fd8 100644 --- a/drivers/edac/sb_edac.c +++ b/drivers/edac/sb_edac.c @@ -2485,6 +2485,7 @@ static int ibridge_mci_bind_devs(struct mem_ctl_info *mci, case PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA0_TA: case PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA1_TA: pvt->pci_ta = pdev; + break; case PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA0_RAS: case PCI_DEVICE_ID_INTEL_IBRIDGE_IMC_HA1_RAS: pvt->pci_ras = pdev;