Skip to contentSkip to conversation

EDAC, skx_edac: Add address translation for non-volatile DIMMs

PATCH New Luck, Tony 1 participant 1 message Oct 10, 2018

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

Luck, Tony

author maintainer
#0 lore raw

PATCH 3/3 EDAC, skx_edac: Add address translation for non-volatile DIMMs

Qiuxu,

I just made the quick change from what you mailed to me earlier for the "const char * const" bits. I did some more review and came up with these changes:

1) Just allocate the array for the values once when setting up, not on each decode (its not very big, and avoids risk of not having memory available at decode time).

2) Add an enum to define names for the offsets in the component_indices[] array

3) Ignore unused fields returned by adxl_decode() (those with value 0xffffffffffffffff.

patch (on top of the one I posted) below.

-Tony

drivers/edac/skx_edac.c +30 -23
index 2989ebad8970..c4b3e5a90e84 100644
@@ -57,11 +57,20 @@ static LIST_HEAD(skx_edac_list);
 
 static u64 skx_tolm, skx_tohm;
 static int nvdimm_count;
+static u64 *adxl_values;
+
+enum {
+	INDEX_SOCKET,
+	INDEX_MEMCTRL,
+	INDEX_CHANNEL,
+	INDEX_DIMM,
+	INDEX_MAX
+};
 static char component_names[][32] = {
-	"ProcessorSocketId",
-	"MemoryControllerId",
-	"ChannelId",
-	"DimmSlotId",
+	[INDEX_SOCKET]	= "ProcessorSocketId",
+	[INDEX_MEMCTRL]	= "MemoryControllerId",
+	[INDEX_CHANNEL]	= "ChannelId",
+	[INDEX_DIMM]	= "DimmSlotId",
 };
 
 static int component_indices[ARRAY_SIZE(component_names)];

@@ -914,7 +923,6 @@ static bool skx_dsm_decode(u64 addr, char *msg, int msglen,
 			   u64 *chan, u64 *dimm)
 
 {
-	u64 *values;
 	int i, len = 0;
 
 	if (addr >= skx_tohm || (addr >= skx_tolm && addr < BIT_ULL(32))) {

@@ -922,32 +930,26 @@ static bool skx_dsm_decode(u64 addr, char *msg, int msglen,
 		return false;
 	}
 
-	values = kcalloc(adxl_component_count, sizeof(*values), GFP_KERNEL);
-	if (!values) {
-		edac_dbg(0, "Out of memory\n");
-		return false;
-	}
-
-	if (adxl_decode(addr, values)) {
+	if (adxl_decode(addr, adxl_values)) {
 		edac_dbg(0, "Failed to decode 0x%llx\n", addr);
-		kfree(values);
 		return false;
 	}
 
-	*sock = values[component_indices[0]];
-	*imc = values[component_indices[1]];
-	*chan = values[component_indices[2]];
-	*dimm = values[component_indices[3]];
+	*sock = adxl_values[component_indices[INDEX_SOCKET]];
+	*imc = adxl_values[component_indices[INDEX_MEMCTRL]];
+	*chan = adxl_values[component_indices[INDEX_CHANNEL]];
+	*dimm = adxl_values[component_indices[INDEX_DIMM]];
 
 	for (i = 0; i < adxl_component_count; i++) {
+		if (adxl_values[i] == ~0x0ull)
+			continue;
 		len += snprintf(msg + len, msglen, " %s:0x%llx",
-				adxl_component_names[i], values[i]);
+				adxl_component_names[i], adxl_values[i]);
 
 		if (msglen - len <= 0)
 			break;
 	}
 
-	kfree(values);
 	return true;
 }
 

@@ -1195,17 +1197,16 @@ static void skx_remove(void)
 static void __init skx_dsm_get(void)
 {
 	const char * const *names;
-	int i, j, n;
+	int i, j;
 
 	names = adxl_get_component_names();
 	if (!names) {
-		skx_printk(KERN_NOTICE, "No firmware supports for address translation.");
+		skx_printk(KERN_NOTICE, "No firmware support for address translation.");
 		skx_printk(KERN_CONT, " Only decoding DDR4 address!\n");
 		return;
 	}
 
-	n = ARRAY_SIZE(component_names);
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < INDEX_MAX; i++) {
 		for (j = 0; names[j]; j++) {
 			if (!strcmp(component_names[i], names[j])) {
 				component_indices[i] = j;

@@ -1221,6 +1222,12 @@ static void __init skx_dsm_get(void)
 	while (*names++)
 		adxl_component_count++;
 
+	adxl_values = kcalloc(adxl_component_count, sizeof(*adxl_values), GFP_KERNEL);
+	if (!adxl_values) {
+		adxl_component_count = 0;
+		edac_dbg(0, "No memory for adxl_decode()\n");
+	}
+
 	return;
 err:
 	skx_printk(KERN_ERR, "'%s' is not matched from DSM parameters: ",