Skip to contentSkip to conversation

EDAC/amd64: Stop allocating ECC settings separately

PATCH Applied Dmitry Torokhov 2 participants 3 messages Sep 26 – Nov 5

Subsystems (2)
EDAC-core EDAC-AMD64
lore
Share & tools
lore

Dmitry Torokhov

author
#0 lore raw

PATCH v2 1/1 EDAC/amd64: Stop allocating ECC settings separately

There is no reason to allocate and manage ECC settings separately when struct ecc_settings can be embedded directly into the driver-private struct amd64_pvt.

Reviewed-by Yazen Ghannam <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>

---
Changes in v2:
- Return -ENOMEM directly on allocation failure in probe_one_instance() and amd64_edac_init() instead of using error labels (Yazen Ghannam)
- Keep the blank line after clearing pci_ctl_dev in amd64_edac_exit() (Yazen Ghannam)
- Make the comment above struct ecc_settings a single-line comment (Yazen Ghannam)
- Link to v1: https://lore.kernel.org/r/[email protected]

 drivers/edac/amd64_edac.c | 42 +++++----------------------------------
 drivers/edac/amd64_edac.h | 25 ++++++++++++-----------
 2 files changed, 18 insertions(+), 49 deletions(-)
drivers/edac/amd64_edac.c +5 -37
index 475235c402e8..6b9d7bb11d76 100644
@@ -30,9 +30,6 @@ static inline u32 get_umc_reg(struct amd64_pvt *pvt, u32 reg)
 	return 0;
 }
 
-/* Per-node stuff */
-static struct ecc_settings **ecc_stngs;
-
 /* Device for the PCI component */
 static struct device *pci_ctl_dev;
 

@@ -3999,19 +3996,11 @@ static int probe_one_instance(unsigned int nid)
 {
 	struct pci_dev *F3 = node_to_amd_nb(nid)->misc;
 	struct amd64_pvt *pvt = NULL;
-	struct ecc_settings *s;
 	int ret;
 
-	ret = -ENOMEM;
-	s = kzalloc_obj(struct ecc_settings);
-	if (!s)
-		goto err_out;
-
-	ecc_stngs[nid] = s;
-
 	pvt = kzalloc_obj(struct amd64_pvt);
 	if (!pvt)
-		goto err_settings;
+		return -ENOMEM;
 
 	pvt->mc_node_id	= nid;
 	pvt->F3 = F3;

@@ -4042,7 +4031,7 @@ static int probe_one_instance(unsigned int nid)
 		} else
 			amd64_warn("Forcing ECC on!\n");
 
-		if (!enable_ecc_error_reporting(s, nid, F3))
+		if (!enable_ecc_error_reporting(&pvt->ecc, nid, F3))
 			goto err_enable;
 	}
 

@@ -4051,7 +4040,7 @@ static int probe_one_instance(unsigned int nid)
 		amd64_err("Error probing instance: %d\n", nid);
 
 		if (boot_cpu_data.x86 < 0x17)
-			restore_ecc_error_reporting(s, nid, F3);
+			restore_ecc_error_reporting(&pvt->ecc, nid, F3);
 
 		goto err_enable;
 	}

@@ -4067,18 +4056,12 @@ static int probe_one_instance(unsigned int nid)
 	hw_info_put(pvt);
 	kfree(pvt);
 
-err_settings:
-	kfree(s);
-	ecc_stngs[nid] = NULL;
-
-err_out:
 	return ret;
 }
 
 static void remove_one_instance(unsigned int nid)
 {
 	struct pci_dev *F3 = node_to_amd_nb(nid)->misc;
-	struct ecc_settings *s = ecc_stngs[nid];
 	struct mem_ctl_info *mci;
 	struct amd64_pvt *pvt;
 

@@ -4089,10 +4072,7 @@ static void remove_one_instance(unsigned int nid)
 
 	pvt = mci->pvt_info;
 
-	restore_ecc_error_reporting(s, nid, F3);
-
-	kfree(ecc_stngs[nid]);
-	ecc_stngs[nid] = NULL;
+	restore_ecc_error_reporting(&pvt->ecc, nid, F3);
 
 	/* Free the EDAC CORE resources */
 	mci->pvt_info = NULL;

@@ -4148,14 +4128,9 @@ static int __init amd64_edac_init(void)
 
 	opstate_init();
 
-	err = -ENOMEM;
-	ecc_stngs = kzalloc_objs(ecc_stngs[0], amd_nb_num());
-	if (!ecc_stngs)
-		goto err_free;
-
 	msrs = msrs_alloc();
 	if (!msrs)
-		goto err_free;
+		return -ENOMEM;
 
 	for (i = 0; i < amd_nb_num(); i++) {
 		err = probe_one_instance(i);

@@ -4195,10 +4170,6 @@ static int __init amd64_edac_init(void)
 	msrs_free(msrs);
 	msrs = NULL;
 
-err_free:
-	kfree(ecc_stngs);
-	ecc_stngs = NULL;
-
 	return err;
 }
 

@@ -4218,9 +4189,6 @@ static void __exit amd64_edac_exit(void)
 	for (i = 0; i < amd_nb_num(); i++)
 		remove_one_instance(i);
 
-	kfree(ecc_stngs);
-	ecc_stngs = NULL;
-
 	pci_ctl_dev = NULL;
 
 	msrs_free(msrs);
drivers/edac/amd64_edac.h +13 -12
index 1757c1b99fc8..2c6b0429916e 100644
@@ -322,6 +322,17 @@ struct amd64_family_flags {
 	      __reserved	: 63;
 };
 
+/* per-node ECC settings descriptor */
+struct ecc_settings {
+	u32 old_nbctl;
+	bool nbctl_valid;
+
+	struct flags {
+		unsigned long nb_mce_enable:1;
+		unsigned long nb_ecc_prev:1;
+	} flags;
+};
+
 struct amd64_pvt {
 	struct low_ops *ops;
 

@@ -372,6 +383,8 @@ struct amd64_pvt {
 	/* place to store error injection parameters prior to issue */
 	struct error_injection injection;
 
+	struct ecc_settings ecc;
+
 	/*
 	 * cache the dram_type
 	 *

@@ -441,18 +454,6 @@ static inline u8 dct_sel_interleave_addr(struct amd64_pvt *pvt)
 
 	return	((pvt)->dct_sel_lo >> 6) & 0x3;
 }
-/*
- * per-node ECC settings descriptor
- */
-struct ecc_settings {
-	u32 old_nbctl;
-	bool nbctl_valid;
-
-	struct flags {
-		unsigned long nb_mce_enable:1;
-		unsigned long nb_ecc_prev:1;
-	} flags;
-};
 
 /*
  * Each of the PCI Device IDs types have their own set of hardware accessor
Signature · 5 lines
2.56.0.rc1.315.gc6ed9934b7-goog


-- 
Dmitry
maintainer Applied
#2 lore raw
Quoted 11 lines · Dmitry Torokhov

Hi Borislav, Tony,

Quoted 6 lines · Dmitry Torokhov

There is no reason to allocate and manage ECC settings separately when struct ecc_settings can be embedded directly into the driver-private struct amd64_pvt.

Reviewed-by Yazen Ghannam <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>

Just a gentle ping on this patch. Could it be picked up?

Applied, thanks.

Signature · 4 lines
Regards/Gruss,
    Boris.

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