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

["Luck, Tony" <tony.luck@xxxxxxxxx>]

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

---

diff --git a/drivers/edac/skx_edac.c b/drivers/edac/skx_edac.c
index 2989ebad8970..c4b3e5a90e84 100644
--- a/drivers/edac/skx_edac.c
+++ b/drivers/edac/skx_edac.c
@@ -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: ",