Re: [PATCH 04/13] edac: move nr_pages to dimm struct

[Borislav Petkov <bp@amd64.org>]

On Thu, Mar 29, 2012 at 01:45:37PM -0300, Mauro Carvalho Chehab wrote:
> The number of pages is a dimm property. Move it to the dimm struct.
> 
> After this change, it is possible to add sysfs nodes for the DIMM's that

							       DIMMs

> will properly represent the DIMM stick properties, including its size.
> 
> A TODO fix here is to properly represent dual-rank/quad-rank DIMMs when
> the memory controller represents the memory via chip select rows.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
>  drivers/edac/amd64_edac.c      |   13 ++++------
>  drivers/edac/amd76x_edac.c     |    6 ++--
>  drivers/edac/cell_edac.c       |    8 ++++--
>  drivers/edac/cpc925_edac.c     |    8 ++++--
>  drivers/edac/e752x_edac.c      |    6 +++-
>  drivers/edac/e7xxx_edac.c      |    5 ++-
>  drivers/edac/edac_mc.c         |   28 ++++++++++++---------
>  drivers/edac/edac_mc_sysfs.c   |   52 ++++++++++++++++++++++++++++++----------
>  drivers/edac/i3000_edac.c      |    6 +++-
>  drivers/edac/i3200_edac.c      |    3 +-
>  drivers/edac/i5000_edac.c      |   14 ++++++----
>  drivers/edac/i5100_edac.c      |   22 ++++++++++-------
>  drivers/edac/i5400_edac.c      |    9 ++----
>  drivers/edac/i7300_edac.c      |   22 ++++------------
>  drivers/edac/i7core_edac.c     |   10 ++-----
>  drivers/edac/i82443bxgx_edac.c |    2 +-
>  drivers/edac/i82860_edac.c     |    2 +-
>  drivers/edac/i82875p_edac.c    |    5 ++-
>  drivers/edac/i82975x_edac.c    |   11 ++++++--
>  drivers/edac/mpc85xx_edac.c    |    3 +-
>  drivers/edac/mv64x60_edac.c    |    3 +-
>  drivers/edac/pasemi_edac.c     |   14 +++++-----
>  drivers/edac/ppc4xx_edac.c     |    5 ++-
>  drivers/edac/r82600_edac.c     |    3 +-
>  drivers/edac/sb_edac.c         |    8 +----
>  drivers/edac/tile_edac.c       |    2 +-
>  drivers/edac/x38_edac.c        |    4 +-
>  include/linux/edac.h           |   10 ++++---
>  28 files changed, 158 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index b1b1551..ad0376e 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -2126,12 +2126,6 @@ static u32 amd64_csrow_nr_pages(struct amd64_pvt *pvt, u8 dct, int csrow_nr)
>  
>  	nr_pages = pvt->ops->dbam_to_cs(pvt, dct, cs_mode) << (20 - PAGE_SHIFT);
>  
> -	/*
> -	 * If dual channel then double the memory size of single channel.
> -	 * Channel count is 1 or 2
> -	 */
> -	nr_pages <<= (pvt->channel_count - 1);
> -
>  	debugf0("  (csrow=%d) DBAM map index= %d\n", csrow_nr, cs_mode);
>  	debugf0("    nr_pages= %u  channel-count = %d\n",
>  		nr_pages, pvt->channel_count);
> @@ -2152,6 +2146,7 @@ static int init_csrows(struct mem_ctl_info *mci)
>  	int i, j, empty = 1;
>  	enum mem_type mtype;
>  	enum edac_type edac_mode;
> +	int nr_pages;
>  
>  	amd64_read_pci_cfg(pvt->F3, NBCFG, &val);
>  
> @@ -2174,7 +2169,7 @@ static int init_csrows(struct mem_ctl_info *mci)
>  			i, pvt->mc_node_id);
>  
>  		empty = 0;
> -		csrow->nr_pages = amd64_csrow_nr_pages(pvt, 0, i);
> +		nr_pages = amd64_csrow_nr_pages(pvt, 0, i);
>  		get_cs_base_and_mask(pvt, i, 0, &base, &mask);
>  		/* 8 bytes of resolution */
>  
> @@ -2192,10 +2187,12 @@ static int init_csrows(struct mem_ctl_info *mci)
>  		for (j = 0; j < pvt->channel_count; j++) {
>  			csrow->channels[j].dimm->mtype = mtype;
>  			csrow->channels[j].dimm->edac_mode = edac_mode;
> +			csrow->channels[j].dimm->nr_pages = nr_pages;
> +

One newline too many.

>  		}
>  
>  		debugf1("  for MC node %d csrow %d:\n", pvt->mc_node_id, i);
> -		debugf1("    nr_pages: %u\n", csrow->nr_pages);
> +		debugf1("    nr_pages: %u\n", nr_pages);
>  	}
>  
>  	return empty;


<snip>

> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index 2430ddb..02263c3 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -43,22 +43,22 @@ static void edac_mc_dump_channel(struct rank_info *chan)
>  {
>  	debugf4("\tchannel = %p\n", chan);
>  	debugf4("\tchannel->chan_idx = %d\n", chan->chan_idx);
> -	debugf4("\tchannel->ce_count = %d\n", chan->dimm->ce_count);
> -	debugf4("\tchannel->label = '%s'\n", chan->dimm->label);
>  	debugf4("\tchannel->csrow = %p\n\n", chan->csrow);
> +	debugf4("\tdimm->ce_count = %d\n", chan->dimm->ce_count);
> +	debugf4("\tdimm->label = '%s'\n", chan->dimm->label);
> +	debugf4("\tdimm->nr_pages = 0x%x\n", chan->dimm->nr_pages);
>  }
>  
>  static void edac_mc_dump_csrow(struct csrow_info *csrow)
>  {
>  	debugf4("\tcsrow = %p\n", csrow);
>  	debugf4("\tcsrow->csrow_idx = %d\n", csrow->csrow_idx);
> -	debugf4("\tcsrow->first_page = 0x%lx\n", csrow->first_page);
> -	debugf4("\tcsrow->last_page = 0x%lx\n", csrow->last_page);
> -	debugf4("\tcsrow->page_mask = 0x%lx\n", csrow->page_mask);
> -	debugf4("\tcsrow->nr_pages = 0x%x\n", csrow->nr_pages);
>  	debugf4("\tcsrow->nr_channels = %d\n", csrow->nr_channels);
>  	debugf4("\tcsrow->channels = %p\n", csrow->channels);
>  	debugf4("\tcsrow->mci = %p\n\n", csrow->mci);
> +	debugf4("\tcsrow->first_page = 0x%lx\n", csrow->first_page);
> +	debugf4("\tcsrow->last_page = 0x%lx\n", csrow->last_page);
> +	debugf4("\tcsrow->page_mask = 0x%lx\n", csrow->page_mask);

Useless code churn: simply remove the ->nr_pages line.

>  }
>  
>  static void edac_mc_dump_mci(struct mem_ctl_info *mci)
> @@ -655,15 +655,19 @@ static void edac_mc_scrub_block(unsigned long page, unsigned long offset,
>  int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci, unsigned long page)
>  {
>  	struct csrow_info *csrows = mci->csrows;
> -	int row, i;
> +	int row, i, j, n;
>  
>  	debugf1("MC%d: %s(): 0x%lx\n", mci->mc_idx, __func__, page);
>  	row = -1;
>  
>  	for (i = 0; i < mci->nr_csrows; i++) {
>  		struct csrow_info *csrow = &csrows[i];
> -
> -		if (csrow->nr_pages == 0)
> +		n = 0;
> +		for (j = 0; j < csrow->nr_channels; j++) {
> +			struct dimm_info *dimm = csrow->channels[j].dimm;
> +			n += dimm->nr_pages;
> +		}
> +		if (n == 0)
>  			continue;
>  
>  		debugf3("MC%d: %s(): first(0x%lx) page(0x%lx) last(0x%lx) "
> @@ -672,9 +676,9 @@ int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci, unsigned long page)
>  			csrow->page_mask);
>  
>  		if ((page >= csrow->first_page) &&
> -		    (page <= csrow->last_page) &&
> -		    ((page & csrow->page_mask) ==
> -		     (csrow->first_page & csrow->page_mask))) {
> +		(page <= csrow->last_page) &&
> +		((page & csrow->page_mask) ==
> +		(csrow->first_page & csrow->page_mask))) {

Useless and wrong formatting change, please retain the original formatting.

>  			row = i;
>  			break;
>  		}
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index d63904e..52c56cf 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -144,7 +144,13 @@ static ssize_t csrow_ce_count_show(struct csrow_info *csrow, char *data,
>  static ssize_t csrow_size_show(struct csrow_info *csrow, char *data,
>  				int private)
>  {
> -	return sprintf(data, "%u\n", PAGES_TO_MiB(csrow->nr_pages));
> +	int i;
> +	u32 nr_pages = 0;
> +
> +	for (i = 0; i < csrow->nr_channels; i++)
> +		nr_pages += csrow->channels[i].dimm->nr_pages;
> +
> +	return sprintf(data, "%u\n", PAGES_TO_MiB(nr_pages));
>  }
>  
>  static ssize_t csrow_mem_type_show(struct csrow_info *csrow, char *data,
> @@ -519,16 +525,17 @@ static ssize_t mci_ctl_name_show(struct mem_ctl_info *mci, char *data)
>  
>  static ssize_t mci_size_mb_show(struct mem_ctl_info *mci, char *data)
>  {
> -	int total_pages, csrow_idx;
> +	int total_pages, csrow_idx, j;
>  
>  	for (total_pages = csrow_idx = 0; csrow_idx < mci->nr_csrows;
> -		csrow_idx++) {
> +	     csrow_idx++) {

You can do the total_pages initialization in the declaration above and
thus don't have to break the loop across two lines:

	int total_pages = 0;
	...

	for (csrow_idx = 0; csrow_idx < mci->nr_csrows; csrow_idx++) {
	...

>  		struct csrow_info *csrow = &mci->csrows[csrow_idx];
>  
> -		if (!csrow->nr_pages)
> -			continue;
> +		for (j = 0; j < csrow->nr_channels; j++) {
> +			struct dimm_info *dimm = csrow->channels[j].dimm;
>  
> -		total_pages += csrow->nr_pages;
> +			total_pages += dimm->nr_pages;
> +		}
>  	}
>  
>  	return sprintf(data, "%u\n", PAGES_TO_MiB(total_pages));
> @@ -900,7 +907,7 @@ static void edac_remove_mci_instance_attributes(struct mem_ctl_info *mci,
>   */
>  int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
>  {
> -	int i;
> +	int i, j;
>  	int err;
>  	struct csrow_info *csrow;
>  	struct kobject *kobj_mci = &mci->edac_mci_kobj;
> @@ -934,10 +941,15 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
>  	/* Make directories for each CSROW object under the mc<id> kobject
>  	 */
>  	for (i = 0; i < mci->nr_csrows; i++) {
> +		int n = 0;

Let's call this variable with a bit more descriptive name like
'nr_pages' or 'pages_total' or whatever but not simply n.

> +
>  		csrow = &mci->csrows[i];
> +		for (j = 0; j < csrow->nr_channels; j++) {
> +			struct dimm_info *dimm = csrow->channels[j].dimm;
> +			n += dimm->nr_pages;
> +		}

Btw, you can make the loop a single-statement one and drop the curly
braces like so:

		for (j = 0; j < csrow->nr_channels; j++)
			nr_pages += csrow->channels[j].dimm->nr_pages;

The same applies for all the other occurrences of when you iterate over
the channels, accumulating the pages per dimm.

>  
> -		/* Only expose populated CSROWs */
> -		if (csrow->nr_pages > 0) {
> +		if (n > 0) {
>  			err = edac_create_csrow_object(mci, csrow, i);
>  			if (err) {
>  				debugf1("%s() failure: create csrow %d obj\n",
> @@ -949,10 +961,16 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
>  
>  	return 0;
>  
> -	/* CSROW error: backout what has already been registered,  */
>  fail1:
>  	for (i--; i >= 0; i--) {
> -		if (mci->csrows[i].nr_pages > 0)
> +		int n = 0;

ditto.

> +
> +		csrow = &mci->csrows[i];
> +		for (j = 0; j < csrow->nr_channels; j++) {
> +			struct dimm_info *dimm = csrow->channels[j].dimm;
> +			n += dimm->nr_pages;
> +		}
> +		if (n > 0)
>  			kobject_put(&mci->csrows[i].kobj);
>  	}
>  
> @@ -972,14 +990,22 @@ fail0:
>   */
>  void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
>  {
> -	int i;
> +	struct csrow_info *csrow;
> +	int i, j;
>  
>  	debugf0("%s()\n", __func__);
>  
>  	/* remove all csrow kobjects */
>  	debugf4("%s()  unregister this mci kobj\n", __func__);
>  	for (i = 0; i < mci->nr_csrows; i++) {
> -		if (mci->csrows[i].nr_pages > 0) {
> +		int n = 0;

ditto.

> +
> +		csrow = &mci->csrows[i];
> +		for (j = 0; j < csrow->nr_channels; j++) {
> +			struct dimm_info *dimm = csrow->channels[j].dimm;
> +			n += dimm->nr_pages;
> +		}
> +		if (n > 0) {
>  			debugf0("%s()  unreg csrow-%d\n", __func__, i);
>  			kobject_put(&mci->csrows[i].kobj);
>  		}

<snip>

> diff --git a/include/linux/edac.h b/include/linux/edac.h
> index 5244193..de22d4c 100644
> --- a/include/linux/edac.h
> +++ b/include/linux/edac.h
> @@ -320,6 +320,8 @@ struct dimm_info {
>  	enum mem_type mtype;	/* memory dimm type */
>  	enum edac_type edac_mode;	/* EDAC mode for this dimm */
>  
> +	u32 nr_pages;			/* number of pages in csrow */
> +
>  	u32 ce_count;		/* Correctable Errors for this dimm */
>  };
>  
> @@ -346,13 +348,13 @@ struct rank_info {
>  };
>  
>  struct csrow_info {
> +	int csrow_idx;			/* the chip-select row */
> +
> +	/* Used only by edac_mc_find_csrow_by_page() */
>  	unsigned long first_page;	/* first page number in csrow */
>  	unsigned long last_page;	/* last page number in csrow */
> -	u32 nr_pages;			/* number of pages in csrow */
>  	unsigned long page_mask;	/* used for interleaving -
> -					 * 0UL for non intlv
> -					 */
> -	int csrow_idx;			/* the chip-select row */
> +					 * 0UL for non intlv */

Useless moving of code around.

>  
>  	u32 ue_count;		/* Uncorrectable Errors for this csrow */
>  	u32 ce_count;		/* Correctable Errors for this csrow */
> -- 
> 1.7.8

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551