diff mbox series

[v2,07/24] EDAC: Kill EDAC_DIMM_OFF() macro

Message ID 20190624150758.6695-8-rrichter@marvell.com
State Superseded
Headers show
Series EDAC, mc, ghes: Fixes and updates to improve memory error reporting | expand

Commit Message

Robert Richter June 24, 2019, 3:09 p.m. UTC 
We do not need to calculate the offset in the mc's dimm array, let's
just store the index in struct dimm_info and we can get rid of this
macro.

Signed-off-by: Robert Richter <rrichter@marvell.com>

---
 drivers/edac/edac_mc.c       | 13 ++++--------
 drivers/edac/edac_mc_sysfs.c | 20 ++++--------------
 include/linux/edac.h         | 41 ------------------------------------
 3 files changed, 8 insertions(+), 66 deletions(-)

-- 
2.20.1

Comments

Borislav Petkov Aug. 14, 2019, 2:52 p.m. UTC | #1 
On Mon, Jun 24, 2019 at 03:09:09PM +0000, Robert Richter wrote:
> We do not need to calculate the offset in the mc's dimm array, let's

> just store the index in struct dimm_info and we can get rid of this

> macro.

> 

> Signed-off-by: Robert Richter <rrichter@marvell.com>

> ---

>  drivers/edac/edac_mc.c       | 13 ++++--------

>  drivers/edac/edac_mc_sysfs.c | 20 ++++--------------

>  include/linux/edac.h         | 41 ------------------------------------

>  3 files changed, 8 insertions(+), 66 deletions(-)


I like this cleanup a lot. Good!

> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c

> index c959e8b1643c..c44bc83e8502 100644

> --- a/drivers/edac/edac_mc.c

> +++ b/drivers/edac/edac_mc.c

> @@ -318,7 +318,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,

>  	unsigned size, tot_dimms = 1, count = 1;

>  	unsigned tot_csrows = 1, tot_channels = 1, tot_errcount = 0;

>  	void *pvt, *p, *ptr = NULL;

> -	int i, j, row, chn, n, len, off;

> +	int idx, i, j, row, chn, n, len;

>  	bool per_rank = false;

>  

>  	BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);

> @@ -426,20 +426,15 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,

>  	memset(&pos, 0, sizeof(pos));

>  	row = 0;

>  	chn = 0;

> -	for (i = 0; i < tot_dimms; i++) {

> +	for (idx = 0; idx < tot_dimms; idx++) {

>  		chan = mci->csrows[row]->channels[chn];

> -		off = EDAC_DIMM_OFF(layer, n_layers, pos[0], pos[1], pos[2]);

> -		if (off < 0 || off >= tot_dimms) {

> -			edac_mc_printk(mci, KERN_ERR, "EDAC core bug: EDAC_DIMM_OFF is trying to do an illegal data access\n");

> -			goto error;

> -		}


Btw, right around here there's a comment:

        /*
         * Allocate and fill the dimm structs
         */

and since you're cleaning up all this, can you please add another patch
which takes all that code under the comment and see if you can carve it
out into a separate helper edac_alloc_dimms() or so. Because that
edac_mc_alloc() function is huuuge.

Btw, the code under

        /*
         * Alocate and fill the csrow/channels structs
         */

begs to be a separate function too :-)

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
diff mbox series

Patch

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index c959e8b1643c..c44bc83e8502 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -318,7 +318,7 @@  struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 	unsigned size, tot_dimms = 1, count = 1;
 	unsigned tot_csrows = 1, tot_channels = 1, tot_errcount = 0;
 	void *pvt, *p, *ptr = NULL;
-	int i, j, row, chn, n, len, off;
+	int idx, i, j, row, chn, n, len;
 	bool per_rank = false;
 
 	BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);
@@ -426,20 +426,15 @@  struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 	memset(&pos, 0, sizeof(pos));
 	row = 0;
 	chn = 0;
-	for (i = 0; i < tot_dimms; i++) {
+	for (idx = 0; idx < tot_dimms; idx++) {
 		chan = mci->csrows[row]->channels[chn];
-		off = EDAC_DIMM_OFF(layer, n_layers, pos[0], pos[1], pos[2]);
-		if (off < 0 || off >= tot_dimms) {
-			edac_mc_printk(mci, KERN_ERR, "EDAC core bug: EDAC_DIMM_OFF is trying to do an illegal data access\n");
-			goto error;
-		}
 
 		dimm = kzalloc(sizeof(**mci->dimms), GFP_KERNEL);
 		if (!dimm)
 			goto error;
-		mci->dimms[off] = dimm;
+		mci->dimms[idx] = dimm;
 		dimm->mci = mci;
-		dimm->idx = off;
+		dimm->idx = idx;
 
 		/*
 		 * Copy DIMM location and initialize it.
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 29dd9719f82f..a69e99206a6f 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -559,14 +559,8 @@  static ssize_t dimmdev_ce_count_show(struct device *dev,
 {
 	struct dimm_info *dimm = to_dimm(dev);
 	u32 count;
-	int off;
-
-	off = EDAC_DIMM_OFF(dimm->mci->layers,
-			    dimm->mci->n_layers,
-			    dimm->location[0],
-			    dimm->location[1],
-			    dimm->location[2]);
-	count = dimm->mci->ce_per_layer[dimm->mci->n_layers-1][off];
+
+	count = dimm->mci->ce_per_layer[dimm->mci->n_layers-1][dimm->idx];
 	return sprintf(data, "%u\n", count);
 }
 
@@ -576,14 +570,8 @@  static ssize_t dimmdev_ue_count_show(struct device *dev,
 {
 	struct dimm_info *dimm = to_dimm(dev);
 	u32 count;
-	int off;
-
-	off = EDAC_DIMM_OFF(dimm->mci->layers,
-			    dimm->mci->n_layers,
-			    dimm->location[0],
-			    dimm->location[1],
-			    dimm->location[2]);
-	count = dimm->mci->ue_per_layer[dimm->mci->n_layers-1][off];
+
+	count = dimm->mci->ue_per_layer[dimm->mci->n_layers-1][dimm->idx];
 	return sprintf(data, "%u\n", count);
 }
 
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 1367a3fc544f..2ee9b8598ae0 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -362,47 +362,6 @@  struct edac_mc_layer {
  */
 #define EDAC_MAX_LAYERS		3
 
-/**
- * EDAC_DIMM_OFF - Macro responsible to get a pointer offset inside a pointer
- *		   array for the element given by [layer0,layer1,layer2]
- *		   position
- *
- * @layers:	a struct edac_mc_layer array, describing how many elements
- *		were allocated for each layer
- * @nlayers:	Number of layers at the @layers array
- * @layer0:	layer0 position
- * @layer1:	layer1 position. Unused if n_layers < 2
- * @layer2:	layer2 position. Unused if n_layers < 3
- *
- * For 1 layer, this macro returns "var[layer0] - var";
- *
- * For 2 layers, this macro is similar to allocate a bi-dimensional array
- * and to return "var[layer0][layer1] - var";
- *
- * For 3 layers, this macro is similar to allocate a tri-dimensional array
- * and to return "var[layer0][layer1][layer2] - var".
- *
- * A loop could be used here to make it more generic, but, as we only have
- * 3 layers, this is a little faster.
- *
- * By design, layers can never be 0 or more than 3. If that ever happens,
- * a NULL is returned, causing an OOPS during the memory allocation routine,
- * with would point to the developer that he's doing something wrong.
- */
-#define EDAC_DIMM_OFF(layers, nlayers, layer0, layer1, layer2) ({		\
-	int __i;							\
-	if ((nlayers) == 1)						\
-		__i = layer0;						\
-	else if ((nlayers) == 2)					\
-		__i = (layer1) + ((layers[1]).size * (layer0));		\
-	else if ((nlayers) == 3)					\
-		__i = (layer2) + ((layers[2]).size * ((layer1) +	\
-			    ((layers[1]).size * (layer0))));		\
-	else								\
-		__i = -EINVAL;						\
-	__i;								\
-})
-
 struct dimm_info {
 	struct device dev;