Skip to contentSkip to conversation

Fix i10nm_edac driver load failure

PATCH In review Tony Luck 2 participants 5 messages Apr 24, 2020 – Apr 27, 2020

Subsystems (3)
EDAC-core EDAC-skylake EDAC-I10NM
lore
Share & tools
lore

Tony Luck

author maintainer
#0 lore raw

cover Fix i10nm_edac driver load failure

The root cause is that one of the configuration registers needed by EDAC changed offset between pre-production and final stepping of some CPU models. Ordinarily we might fix this by just making the driver only support the production stepping. But there are still significant numbers of the pre-production parts in use (both within Intel and with customers), so we need to adapt to use the right offset for the register depending on both model and stepping.

Patch 1: Adds a configuration structure to make this, and future changes, possible.
Patch 2: Adjusts the register offset

Qiuxu Zhuo (2):
EDAC, {skx,i10nm}: Make some configurations CPU model specific EDAC, i10nm: Fix i10nm_edac loading failure on some servers

 drivers/edac/i10nm_base.c | 28 ++++++++++++++++++++++++----
 drivers/edac/skx_base.c   | 12 ++++++++++--
 drivers/edac/skx_common.c | 11 +++++------
 drivers/edac/skx_common.h | 11 +++++++++--
 4 files changed, 48 insertions(+), 14 deletions(-)
Signature · 1 line
2.21.1
author maintainer
#1 lore raw

PATCH 1/2 EDAC, {skx,i10nm}: Make some configurations CPU model specific

From: Qiuxu Zhuo <[email protected]>

The device ID for configuration agent PCI device and the offset for bus number configuration register can be CPU model specific. So add a new structure res_config to make them configurable and pass res_config to {skx,i10nm}_init() and skx_get_all_bus_mappings() for use.

Signed-off-by Qiuxu Zhuo <[email protected]>
Signed-off-by Tony Luck <[email protected]>
---
 drivers/edac/i10nm_base.c | 16 ++++++++++++----
 drivers/edac/skx_base.c   | 12 ++++++++++--
 drivers/edac/skx_common.c | 11 +++++------
 drivers/edac/skx_common.h | 11 +++++++++--
 4 files changed, 36 insertions(+), 14 deletions(-)
drivers/edac/i10nm_base.c +12 -4
index df08de963d10..ba4578c6ef2b 100644
@@ -122,10 +122,16 @@ static int i10nm_get_all_munits(void)
 	return 0;
 }
 
+static struct res_config i10nm_cfg = {
+	.type			= I10NM,
+	.decs_did		= 0x3452,
+	.busno_cfg_offset	= 0xcc,
+};
+
 static const struct x86_cpu_id i10nm_cpuids[] = {
-	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_D,	NULL),
-	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		NULL),
-	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,		NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_D,	&i10nm_cfg),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		&i10nm_cfg),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,		&i10nm_cfg),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, i10nm_cpuids);

@@ -234,6 +240,7 @@ static int __init i10nm_init(void)
 {
 	u8 mc = 0, src_id = 0, node_id = 0;
 	const struct x86_cpu_id *id;
+	struct res_config *cfg;
 	const char *owner;
 	struct skx_dev *d;
 	int rc, i, off[3] = {0xd0, 0xc8, 0xcc};

@@ -248,12 +255,13 @@ static int __init i10nm_init(void)
 	id = x86_match_cpu(i10nm_cpuids);
 	if (!id)
 		return -ENODEV;
+	cfg = (struct res_config *)id->driver_data;
 
 	rc = skx_get_hi_lo(0x09a2, off, &tolm, &tohm);
 	if (rc)
 		return rc;
 
-	rc = skx_get_all_bus_mappings(0x3452, 0xcc, I10NM, &i10nm_edac_list);
+	rc = skx_get_all_bus_mappings(cfg, &i10nm_edac_list);
 	if (rc < 0)
 		goto fail;
 	if (rc == 0) {
drivers/edac/skx_base.c +10 -2
index 46a3a3440f5e..ae2c2b516adc 100644
@@ -157,8 +157,14 @@ static int get_all_munits(const struct munit *m)
 	return -ENODEV;
 }
 
+static struct res_config skx_cfg = {
+	.type			= SKX,
+	.decs_did		= 0x2016,
+	.busno_cfg_offset	= 0xcc,
+};
+
 static const struct x86_cpu_id skx_cpuids[] = {
-	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X,	NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X,	&skx_cfg),
 	{ }
 };
 MODULE_DEVICE_TABLE(x86cpu, skx_cpuids);

@@ -641,6 +647,7 @@ static inline void teardown_skx_debug(void) {}
 static int __init skx_init(void)
 {
 	const struct x86_cpu_id *id;
+	struct res_config *cfg;
 	const struct munit *m;
 	const char *owner;
 	int rc = 0, i, off[3] = {0xd0, 0xd4, 0xd8};

@@ -656,12 +663,13 @@ static int __init skx_init(void)
 	id = x86_match_cpu(skx_cpuids);
 	if (!id)
 		return -ENODEV;
+	cfg = (struct res_config *)id->driver_data;
 
 	rc = skx_get_hi_lo(0x2034, off, &skx_tolm, &skx_tohm);
 	if (rc)
 		return rc;
 
-	rc = skx_get_all_bus_mappings(0x2016, 0xcc, SKX, &skx_edac_list);
+	rc = skx_get_all_bus_mappings(cfg, &skx_edac_list);
 	if (rc < 0)
 		goto fail;
 	if (rc == 0) {
drivers/edac/skx_common.c +5 -6
index 99bbaf629b8d..abc9ddd2b7d1 100644
@@ -197,12 +197,11 @@ static int get_width(u32 mtr)
 }
 
 /*
- * We use the per-socket device @did to count how many sockets are present,
+ * We use the per-socket device @cfg->did to count how many sockets are present,
  * and to detemine which PCI buses are associated with each socket. Allocate
  * and build the full list of all the skx_dev structures that we need here.
  */
-int skx_get_all_bus_mappings(unsigned int did, int off, enum type type,
-			     struct list_head **list)
+int skx_get_all_bus_mappings(struct res_config *cfg, struct list_head **list)
 {
 	struct pci_dev *pdev, *prev;
 	struct skx_dev *d;

@@ -211,7 +210,7 @@ int skx_get_all_bus_mappings(unsigned int did, int off, enum type type,
 
 	prev = NULL;
 	for (;;) {
-		pdev = pci_get_device(PCI_VENDOR_ID_INTEL, did, prev);
+		pdev = pci_get_device(PCI_VENDOR_ID_INTEL, cfg->decs_did, prev);
 		if (!pdev)
 			break;
 		ndev++;

@@ -221,7 +220,7 @@ int skx_get_all_bus_mappings(unsigned int did, int off, enum type type,
 			return -ENOMEM;
 		}
 
-		if (pci_read_config_dword(pdev, off, &reg)) {
+		if (pci_read_config_dword(pdev, cfg->busno_cfg_offset, &reg)) {
 			kfree(d);
 			pci_dev_put(pdev);
 			skx_printk(KERN_ERR, "Failed to read bus idx\n");

@@ -230,7 +229,7 @@ int skx_get_all_bus_mappings(unsigned int did, int off, enum type type,
 
 		d->bus[0] = GET_BITFIELD(reg, 0, 7);
 		d->bus[1] = GET_BITFIELD(reg, 8, 15);
-		if (type == SKX) {
+		if (cfg->type == SKX) {
 			d->seg = pci_domain_nr(pdev->bus);
 			d->bus[2] = GET_BITFIELD(reg, 16, 23);
 			d->bus[3] = GET_BITFIELD(reg, 24, 31);
drivers/edac/skx_common.h +9 -2
index 60d1ea669afd..19dd8c099520 100644
@@ -112,6 +112,14 @@ struct decoded_addr {
 	int	bank_group;
 };
 
+struct res_config {
+	enum type type;
+	/* Configuration agent device ID */
+	unsigned int decs_did;
+	/* Default bus number configuration register offset */
+	int busno_cfg_offset;
+};
+
 typedef int (*get_dimm_config_f)(struct mem_ctl_info *mci);
 typedef bool (*skx_decode_f)(struct decoded_addr *res);
 typedef void (*skx_show_retry_log_f)(struct decoded_addr *res, char *msg, int len);

@@ -123,8 +131,7 @@ void skx_set_decode(skx_decode_f decode, skx_show_retry_log_f show_retry_log);
 int skx_get_src_id(struct skx_dev *d, int off, u8 *id);
 int skx_get_node_id(struct skx_dev *d, u8 *id);
 
-int skx_get_all_bus_mappings(unsigned int did, int off, enum type,
-			     struct list_head **list);
+int skx_get_all_bus_mappings(struct res_config *cfg, struct list_head **list);
 
 int skx_get_hi_lo(unsigned int did, int off[], u64 *tolm, u64 *tohm);
Signature · 1 line
2.21.1
maintainer Reviewed-by on v1 1/2
#2 lore raw

Re: [PATCH 1/2] EDAC, {skx,i10nm}: Make some configurations CPU model specific

Quoted 49 lines · Tony Luck

From: Qiuxu Zhuo <[email protected]>

The device ID for configuration agent PCI device and the offset for bus number configuration register can be CPU model specific. So add a new structure res_config to make them configurable and pass res_config to {skx,i10nm}_init() and skx_get_all_bus_mappings() for use.

Signed-off-by Qiuxu Zhuo <[email protected]>
Signed-off-by Tony Luck <[email protected]>
---
 drivers/edac/i10nm_base.c | 16 ++++++++++++----
 drivers/edac/skx_base.c   | 12 ++++++++++--
 drivers/edac/skx_common.c | 11 +++++------
 drivers/edac/skx_common.h | 11 +++++++++--
 4 files changed, 36 insertions(+), 14 deletions(-)
drivers/edac/i10nm_base.c +10 -3
index df08de963d10..ba4578c6ef2b 100644
@@ -122,10 +122,16 @@ static int i10nm_get_all_munits(void)
 	return 0;
 }
 
+static struct res_config i10nm_cfg = {
+	.type			= I10NM,
+	.decs_did		= 0x3452,
+	.busno_cfg_offset	= 0xcc,
+};
+
 static const struct x86_cpu_id i10nm_cpuids[] = {
-	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_D,	NULL),
-	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		NULL),
-	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,		NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_D,	&i10nm_cfg),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		&i10nm_cfg),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,		&i10nm_cfg),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, i10nm_cpuids);

@@ -234,6 +240,7 @@ static int __init i10nm_init(void)
 {
 	u8 mc = 0, src_id = 0, node_id = 0;
 	const struct x86_cpu_id *id;
+	struct res_config *cfg;
 	const char *owner;
 	struct skx_dev *d;
 	int rc, i, off[3] = {0xd0, 0xc8, 0xcc};

drivers/edac/i10nm_base.c static int __init i10nm_init(void) · line 257

Quoted 3 lines
drivers/edac/i10nm_base.c +0 -0
@@ -248,12 +255,13 @@ static int __init i10nm_init(void)
 	id = x86_match_cpu(i10nm_cpuids);
 	if (!id)
 		return -ENODEV;

<---- newline here.

Quoted 40 lines
drivers/edac/i10nm_base.c +2 -1
+	cfg = (struct res_config *)id->driver_data;
 
 	rc = skx_get_hi_lo(0x09a2, off, &tolm, &tohm);
 	if (rc)
 		return rc;
 
-	rc = skx_get_all_bus_mappings(0x3452, 0xcc, I10NM, &i10nm_edac_list);
+	rc = skx_get_all_bus_mappings(cfg, &i10nm_edac_list);
 	if (rc < 0)
 		goto fail;
 	if (rc == 0) {
drivers/edac/skx_base.c +8 -1
index 46a3a3440f5e..ae2c2b516adc 100644
@@ -157,8 +157,14 @@ static int get_all_munits(const struct munit *m)
 	return -ENODEV;
 }
 
+static struct res_config skx_cfg = {
+	.type			= SKX,
+	.decs_did		= 0x2016,
+	.busno_cfg_offset	= 0xcc,
+};
+
 static const struct x86_cpu_id skx_cpuids[] = {
-	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X,	NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X,	&skx_cfg),
 	{ }
 };
 MODULE_DEVICE_TABLE(x86cpu, skx_cpuids);

@@ -641,6 +647,7 @@ static inline void teardown_skx_debug(void) {}
 static int __init skx_init(void)
 {
 	const struct x86_cpu_id *id;
+	struct res_config *cfg;
 	const struct munit *m;
 	const char *owner;
 	int rc = 0, i, off[3] = {0xd0, 0xd4, 0xd8};

drivers/edac/skx_base.c static int __init skx_init(void) · line 665

Quoted 3 lines
drivers/edac/skx_base.c +0 -0
@@ -656,12 +663,13 @@ static int __init skx_init(void)
 	id = x86_match_cpu(skx_cpuids);
 	if (!id)
 		return -ENODEV;

<---- newline here.

other than that:

Reviewed-by Borislav Petkov <[email protected]>
Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
author maintainer
#3 lore raw

PATCH 2/2 EDAC, i10nm: Fix i10nm_edac loading failure on some servers

In reply to Tony Luck

From: Qiuxu Zhuo <[email protected]>

It failed to load the i10nm_edac driver on Ice Lake and Tremont/Jacobsville servers if their CPU stepping >= 4 and failed on Ice Lake-D servers from stepping 0. The root cause was that for Ice Lake and Tremont/Jacobsville servers with CPU stepping >=4, the offset for bus number configuration register was updated from 0xcc to 0xd0. For Ice Lake-D servers, all the steppings use the updated 0xd0 offset.

Fix the issue by using the appropriate offset for bus number configuration register according to the CPU model number and stepping.

Reported-by Jerry Chen <[email protected]>
Reported-and-tested-by Jin Wen <[email protected]>
Signed-off-by Qiuxu Zhuo <[email protected]>
Signed-off-by Tony Luck <[email protected]>
---
 drivers/edac/i10nm_base.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)
drivers/edac/i10nm_base.c +16 -4
index ba4578c6ef2b..ebb45738c11b 100644
@@ -122,16 +122,24 @@ static int i10nm_get_all_munits(void)
 	return 0;
 }
 
-static struct res_config i10nm_cfg = {
+/* ATOM_TREMONT_D, ICELAKE_X */
+static struct res_config i10nm_cfg0 = {
 	.type			= I10NM,
 	.decs_did		= 0x3452,
 	.busno_cfg_offset	= 0xcc,
 };
 
+/* ICELAKE_D */
+static struct res_config i10nm_cfg1 = {
+	.type			= I10NM,
+	.decs_did		= 0x3452,
+	.busno_cfg_offset	= 0xd0,
+};
+
 static const struct x86_cpu_id i10nm_cpuids[] = {
-	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_D,	&i10nm_cfg),
-	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		&i10nm_cfg),
-	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,		&i10nm_cfg),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_D,	&i10nm_cfg0),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		&i10nm_cfg0),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,		&i10nm_cfg1),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, i10nm_cpuids);

@@ -257,6 +265,10 @@ static int __init i10nm_init(void)
 		return -ENODEV;
 	cfg = (struct res_config *)id->driver_data;
 
+	/* Newer steppings have different offset for ATOM_TREMONT_D/ICELAKE_X */
+	if (boot_cpu_data.x86_stepping >= 4)
+		cfg->busno_cfg_offset = 0xd0;
+
 	rc = skx_get_hi_lo(0x09a2, off, &tolm, &tohm);
 	if (rc)
 		return rc;
Signature · 1 line
2.21.1
maintainer Reviewed-by on v1 2/2
#4 lore raw

Re: [PATCH 2/2] EDAC, i10nm: Fix i10nm_edac loading failure on some servers

Quoted 1 line · Tony Luck

Subject: Re: [PATCH 2/2] EDAC, i10nm: Fix i10nm_edac loading failure on some

Please use "EDAC/<driver>:" prefix.

And I guess that patch name should be something like:

EDAC/i10nm: Update driver to support different bus number config register offsets

or so.

Quoted 3 lines

From: Qiuxu Zhuo <[email protected]>

It failed to load the i10nm_edac driver on Ice Lake and

"The i10nm_edac driver failed to load ... "

Quoted 23 lines

Tremont/Jacobsville servers if their CPU stepping >= 4 and failed on Ice Lake-D servers from stepping 0. The root cause was that for Ice Lake and Tremont/Jacobsville servers with CPU stepping >=4, the offset for bus number configuration register was updated from 0xcc to 0xd0. For Ice Lake-D servers, all the steppings use the updated 0xd0 offset.

Fix the issue by using the appropriate offset for bus number configuration register according to the CPU model number and stepping.

Reported-by Jerry Chen <[email protected]>
Reported-and-tested-by Jin Wen <[email protected]>
Signed-off-by Qiuxu Zhuo <[email protected]>
Signed-off-by Tony Luck <[email protected]>
---
 drivers/edac/i10nm_base.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)
drivers/edac/i10nm_base.c +0 -0
index ba4578c6ef2b..ebb45738c11b 100644

drivers/edac/i10nm_base.c static int i10nm_get_all_munits(void) · line 125

Quoted 5 lines
drivers/edac/i10nm_base.c +1 -1
@@ -122,16 +122,24 @@ static int i10nm_get_all_munits(void)
 	return 0;
 }
 
-static struct res_config i10nm_cfg = {
+/* ATOM_TREMONT_D, ICELAKE_X */

That comment...

drivers/edac/i10nm_base.c static int i10nm_get_all_munits(void) · line 132

Quoted 7 lines
drivers/edac/i10nm_base.c +2 -0
+static struct res_config i10nm_cfg0 = {
 	.type			= I10NM,
 	.decs_did		= 0x3452,
 	.busno_cfg_offset	= 0xcc,
 };
 
+/* ICELAKE_D */

... and that one are kinda redundant...

drivers/edac/i10nm_base.c static int i10nm_get_all_munits(void) · line 142

Quoted 13 lines
drivers/edac/i10nm_base.c +9 -3
+static struct res_config i10nm_cfg1 = {
+	.type			= I10NM,
+	.decs_did		= 0x3452,
+	.busno_cfg_offset	= 0xd0,
+};
+
 static const struct x86_cpu_id i10nm_cpuids[] = {
-	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_D,	&i10nm_cfg),
-	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		&i10nm_cfg),
-	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,		&i10nm_cfg),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_TREMONT_D,	&i10nm_cfg0),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X,		&i10nm_cfg0),
+	X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_D,		&i10nm_cfg1),

... since it is written here what what is.

Anyway, just nitpicks. Other than that:

Reviewed-by Borislav Petkov <[email protected]>
Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette