Re: [PATCH v2] edac_mc: Cleanup per-dimm_info debug messages

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em 30-04-2012 12:02, Mauro Carvalho Chehab escreveu:
> The edac_mc_alloc() routine allocates one dimm_info device for all
> possible memories, including the non-filled ones. The debug messages
> there are somewhat confusing. So, cleans them, by moving the code
> that prints the memory location to edac_mc, and using it on both
> edac_mc_sysfs and edac_mc.
> 
> After this patch, a dimm-based memory controller will print the debug
> info as:
> 
> [  728.430828] EDAC DEBUG: edac_mc_dump_dimm: 	dimm2: channel 0 slot 2 mapped as virtual row 0, chan 2
> [  728.430834] EDAC DEBUG: edac_mc_dump_dimm: 	dimm->label = 'mc#0channel#0slot#2'
> [  728.430839] EDAC DEBUG: edac_mc_dump_dimm: 	dimm->nr_pages = 0x0
> [  728.430846] EDAC DEBUG: edac_mc_dump_dimm: 	dimm->grain = 0
> [  728.430850] EDAC DEBUG: edac_mc_dump_dimm: 	dimm->nr_pages = 0x0

Hmm... just noticed that, just like the per-csrow register dumps, this routine
is called even when empty memories are used (in this case: nr_pages = 0).

IMHO, as this is a registers dump, the better is to keep it as-is, but it would
be simple to add a test there - and at edac_mc_dump_csrow() - to just dump it
when dimm->nr_pages is not 0.

What do you think?

Regards,
Mauro

> 
> (a rank-based memory controller would print, instead, "rank2"
>  on the above debug info)
> 
> Cc: Doug Thompson <norsk5@yahoo.com>
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
> 
> v2: rebase this patch to apply after:
>     http://git.infradead.org/users/mchehab/edac.git/commitdiff/a93f4f2a667afa1ff2f983b12d2062d7259c6c44
>     As Joe's patches renamed debugf[0-4] to edac_dbg([0-4], and both patches
>     would conflict otherwise.
> 
>  drivers/edac/edac_mc.c       |   44 ++++++++++++++++++++++++++---------------
>  drivers/edac/edac_mc_sysfs.c |   11 +---------
>  drivers/edac/edac_module.h   |    3 ++
>  3 files changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index f48011f..ecb23e5 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -40,6 +40,25 @@
>  static DEFINE_MUTEX(mem_ctls_mutex);
>  static LIST_HEAD(mc_devices);
>  
> +unsigned edac_dimm_info_location(struct dimm_info *dimm, char *buf,
> +			         int len)
> +{
> +	struct mem_ctl_info *mci = dimm->mci;
> +	int i, n, count = 0;
> +	char *p = buf;
> +
> +	for (i = 0; i < mci->n_layers; i++) {
> +		n = snprintf(p, len, "%s %d ",
> +			      edac_layer_name[mci->layers[i].type],
> +			      dimm->location[i]);
> +		p += n;
> +		len -= n;
> +		count += n;
> +	}
> +
> +	return count;
> +}
> +
>  #ifdef CONFIG_EDAC_DEBUG
>  
>  static void edac_mc_dump_channel(struct rank_info *chan)
> @@ -50,20 +69,19 @@ static void edac_mc_dump_channel(struct rank_info *chan)
>  	edac_dbg(4, "\tchannel->dimm = %p\n", chan->dimm);
>  }
>  
> -static void edac_mc_dump_dimm(struct dimm_info *dimm)
> +static void edac_mc_dump_dimm(struct dimm_info *dimm, int number)
>  {
> -	int i;
> +	char location[80];
> +
> +	edac_dimm_info_location(dimm, location, sizeof(location));
>  
>  	edac_dbg(4, "\tdimm = %p\n", dimm);
> +	edac_dbg(4, "\tdimm = %p\n", dimm);
> +	edac_dbg(4, "\t%s%i: %smapped as virtual row %d, chan %d\n",
> +		 dimm->mci->mem_is_per_rank ? "rank" : "dimm",
> +		 number, location, dimm->csrow, dimm->cschannel);
>  	edac_dbg(4, "\tdimm->label = '%s'\n", dimm->label);
>  	edac_dbg(4, "\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
> -	edac_dbg(4, "\tdimm location ");
> -	for (i = 0; i < dimm->mci->n_layers; i++) {
> -		printk(KERN_CONT "%d", dimm->location[i]);
> -		if (i < dimm->mci->n_layers - 1)
> -			printk(KERN_CONT ".");
> -	}
> -	printk(KERN_CONT "\n");
>  	edac_dbg(4, "\tdimm->grain = %d\n", dimm->grain);
>  	edac_dbg(4, "\tdimm->nr_pages = 0x%x\n", dimm->nr_pages);
>  }
> @@ -338,8 +356,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
>  	memset(&pos, 0, sizeof(pos));
>  	row = 0;
>  	chn = 0;
> -	edac_dbg(4, "initializing %d %s\n",
> -		 tot_dimms, per_rank ? "ranks" : "dimms");
>  	for (i = 0; i < tot_dimms; i++) {
>  		chan = mci->csrows[row]->channels[chn];
>  		off = EDAC_DIMM_OFF(layer, n_layers, pos[0], pos[1], pos[2]);
> @@ -352,10 +368,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
>  		mci->dimms[off] = dimm;
>  		dimm->mci = mci;
>  
> -		edac_dbg(2, "%d: %s%i (%d:%d:%d): row %d, chan %d\n",
> -			 i, per_rank ? "rank" : "dimm", off,
> -			 pos[0], pos[1], pos[2], row, chn);
> -
>  		/*
>  		 * Copy DIMM location and initialize the memory location
>  		 */
> @@ -731,7 +743,7 @@ int edac_mc_add_mc(struct mem_ctl_info *mci)
>  				edac_mc_dump_channel(mci->csrows[i]->channels[j]);
>  		}
>  		for (i = 0; i < mci->tot_dimms; i++)
> -			edac_mc_dump_dimm(mci->dimms[i]);
> +			edac_mc_dump_dimm(mci->dimms[i], i);
>  	}
>  #endif
>  	mutex_lock(&mem_ctls_mutex);
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index fe2d922..95865a0 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -485,17 +485,8 @@ static ssize_t dimmdev_location_show(struct device *dev,
>  				     struct device_attribute *mattr, char *data)
>  {
>  	struct dimm_info *dimm = to_dimm(dev);
> -	struct mem_ctl_info *mci = dimm->mci;
> -	int i;
> -	char *p = data;
> -
> -	for (i = 0; i < mci->n_layers; i++) {
> -		p += sprintf(p, "%s %d ",
> -			     edac_layer_name[mci->layers[i].type],
> -			     dimm->location[i]);
> -	}
>  
> -	return p - data;
> +	return edac_dimm_info_location(dimm, data, PAGE_SIZE);
>  }
>  
>  static ssize_t dimmdev_label_show(struct device *dev,
> diff --git a/drivers/edac/edac_module.h b/drivers/edac/edac_module.h
> index 1af1367..de92756 100644
> --- a/drivers/edac/edac_module.h
> +++ b/drivers/edac/edac_module.h
> @@ -34,6 +34,9 @@ extern int edac_mc_get_panic_on_ue(void);
>  extern int edac_get_poll_msec(void);
>  extern int edac_mc_get_poll_msec(void);
>  
> +unsigned edac_dimm_info_location(struct dimm_info *dimm, char *buf,
> +				 int len);
> +
>  	/* on edac_device.c */
>  extern int edac_device_register_sysfs_main_kobj(
>  				struct edac_device_ctl_info *edac_dev);