Linux EDAC development
 help / color / mirror / Atom feed
From: "Luck, Tony" <tony.luck@intel.com>
To: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Cc: Borislav Petkov <bp@alien8.de>,
	Aristeu Rozanski <aris@redhat.com>,
	Mauro Carvalho Chehab <mchehab@s-opensource.com>,
	linux-edac@vger.kernel.org, linux-acpi@vger.kernel.org,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Subject: [3/3] EDAC, skx_edac: Add address translation for non-volatile DIMMs
Date: Wed, 10 Oct 2018 16:04:57 -0700	[thread overview]
Message-ID: <20181010230457.GA8392@agluck-desk> (raw)

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: ",

             reply	other threads:[~2018-10-10 23:04 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-10 23:04 Luck, Tony [this message]
  -- strict thread matches above, loose matches on Subject: below --
2018-10-11 16:32 [3/3] EDAC, skx_edac: Add address translation for non-volatile DIMMs Luck, Tony
2018-10-11  5:36 Qiuxu Zhuo
2018-10-11  2:59 Qiuxu Zhuo
2018-10-09 18:33 Luck, Tony

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181010230457.GA8392@agluck-desk \
    --to=tony.luck@intel.com \
    --cc=aris@redhat.com \
    --cc=bp@alien8.de \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-edac@vger.kernel.org \
    --cc=mchehab@s-opensource.com \
    --cc=qiuxu.zhuo@intel.com \
    --cc=rafael.j.wysocki@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox