Re: [PATCH 02/13] edac: move dimm properties to struct memset_info

From: Borislav Petkov
Date: Fri Mar 30 2012 - 13:03:38 EST

Next message: Tony Luck: "Re: MCE triggered with v3.1 and v3.2 on Xeon E5"
Previous message: Jan Seiffert: "[REGRESSION][PATCH V4 2/3] bpf jit: Let the x86 jit handle negativeoffsets"
In reply to: Mauro Carvalho Chehab: "Re: [PATCH 02/13] edac: move dimm properties to struct memset_info"
Next in thread: Mauro Carvalho Chehab: "[PATCH 03/13] edac: Don't initialize csrow's first_page & friends when not needed"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Mar 29, 2012 at 01:45:35PM -0300, Mauro Carvalho Chehab wrote:
> On systems based on chip select rows, all channels need to use memories
> with the same properties, otherwise the memories on channels A and B
> won't be recognized.
>
> However, such assumption is not true for all types of memory
> controllers.
>
> Controllers for FB-DIMM's don't have such requirements.
>
> Also, modern Intel controllers seem to be capable of handling such
> differences.
>
> So, we need to get rid of storing the DIMM information into a per-csrow
> data, storing it, instead at the right place.
>
> The first step is to move grain, mtype, dtype and edac_mode to the
> per-dimm struct.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> ---
> drivers/edac/amd64_edac.c | 30 +++++++++++--------
> drivers/edac/amd76x_edac.c | 10 ++++--
> drivers/edac/cell_edac.c | 10 +++++-
> drivers/edac/cpc925_edac.c | 62 +++++++++++++++++++++------------------
> drivers/edac/e752x_edac.c | 44 +++++++++++++++-------------
> drivers/edac/e7xxx_edac.c | 44 ++++++++++++++++------------
> drivers/edac/edac_mc.c | 19 ++++++++----
> drivers/edac/edac_mc_sysfs.c | 6 ++--
> drivers/edac/i3000_edac.c | 18 ++++++-----
> drivers/edac/i3200_edac.c | 18 ++++++-----
> drivers/edac/i5000_edac.c | 24 +++++++--------
> drivers/edac/i5100_edac.c | 38 +++++++++++++-----------
> drivers/edac/i5400_edac.c | 24 ++++++---------
> drivers/edac/i7300_edac.c | 25 +++++++++------
> drivers/edac/i7core_edac.c | 27 ++++++++---------
> drivers/edac/i82443bxgx_edac.c | 13 +++++---
> drivers/edac/i82860_edac.c | 11 ++++--
> drivers/edac/i82875p_edac.c | 17 ++++++++---
> drivers/edac/i82975x_edac.c | 17 +++++++----
> drivers/edac/mpc85xx_edac.c | 13 +++++---
> drivers/edac/mv64x60_edac.c | 18 ++++++-----
> drivers/edac/pasemi_edac.c | 10 ++++--
> drivers/edac/ppc4xx_edac.c | 13 +++++---
> drivers/edac/r82600_edac.c | 10 ++++--
> drivers/edac/sb_edac.c | 31 +++++++++++---------
> drivers/edac/tile_edac.c | 13 ++++----
> drivers/edac/x38_edac.c | 17 ++++++-----
> include/linux/edac.h | 21 ++++++++-----
> 28 files changed, 340 insertions(+), 263 deletions(-)
>
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index c9eee6d..3e7bddc 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -2168,7 +2168,9 @@ static int init_csrows(struct mem_ctl_info *mci)
> struct amd64_pvt *pvt = mci->pvt_info;
> u64 input_addr_min, input_addr_max, sys_addr, base, mask;
> u32 val;
> - int i, empty = 1;
> + int i, j, empty = 1;
> + enum mem_type mtype;
> + enum edac_type edac_mode;
>
> amd64_read_pci_cfg(pvt->F3, NBCFG, &val);
>
> @@ -2202,7 +2204,21 @@ static int init_csrows(struct mem_ctl_info *mci)
> csrow->page_mask = ~mask;
> /* 8 bytes of resolution */
>
> - csrow->mtype = amd64_determine_memory_type(pvt, i);
> + mtype = amd64_determine_memory_type(pvt, i);
> +
> + /*
> + * determine whether CHIPKILL or JUST ECC or NO ECC is operating
> + */
> + if (pvt->nbcfg & NBCFG_ECC_ENABLE)
> + edac_mode = (pvt->nbcfg & NBCFG_CHIPKILL) ?
> + EDAC_S4ECD4ED : EDAC_SECDED;
> + else
> + edac_mode = EDAC_NONE;
> +
> + for (j = 0; j < pvt->channel_count; j++) {
> + csrow->channels[j].dimm->mtype = mtype;
> + csrow->channels[j].dimm->edac_mode = edac_mode;
> + }
>
> debugf1(" for MC node %d csrow %d:\n", pvt->mc_node_id, i);
> debugf1(" input_addr_min: 0x%lx input_addr_max: 0x%lx\n",
> @@ -2214,16 +2230,6 @@ static int init_csrows(struct mem_ctl_info *mci)
> "last_page: 0x%lx\n",
> (unsigned)csrow->nr_pages,
> csrow->first_page, csrow->last_page);
> -
> - /*
> - * determine whether CHIPKILL or JUST ECC or NO ECC is operating
> - */
> - if (pvt->nbcfg & NBCFG_ECC_ENABLE)
> - csrow->edac_mode =
> - (pvt->nbcfg & NBCFG_CHIPKILL) ?
> - EDAC_S4ECD4ED : EDAC_SECDED;
> - else
> - csrow->edac_mode = EDAC_NONE;

This looks like a useless code movement, please leave it where it is
now and add the for-loop after it instead of pulling it up and causing
needless churn.

< snip drivers I'm not maintaining >

> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index c03bfe7..2430ddb 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -43,7 +43,7 @@ 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->ce_count);
> + 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);
> }
> @@ -698,6 +698,7 @@ void edac_mc_handle_ce(struct mem_ctl_info *mci,
> {
> unsigned long remapped_page;
> char *label = NULL;
> + u32 grain;
>
> debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
>
> @@ -722,6 +723,7 @@ void edac_mc_handle_ce(struct mem_ctl_info *mci,
> }
>
> label = mci->csrows[row].channels[channel].dimm->label;
> + grain = mci->csrows[row].channels[channel].dimm->grain;
>
> if (edac_mc_get_log_ce())
> /* FIXME - put in DIMM location */
> @@ -729,11 +731,12 @@ void edac_mc_handle_ce(struct mem_ctl_info *mci,
> "CE page 0x%lx, offset 0x%lx, grain %d, syndrome "
> "0x%lx, row %d, channel %d, label \"%s\": %s\n",
> page_frame_number, offset_in_page,
> - mci->csrows[row].grain, syndrome, row, channel,
> + grain, syndrome, row, channel,
> label, msg);
>
> mci->ce_count++;
> mci->csrows[row].ce_count++;
> + mci->csrows[row].channels[channel].dimm->ce_count++;
> mci->csrows[row].channels[channel].ce_count++;
>
> if (mci->scrub_mode & SCRUB_SW_SRC) {
> @@ -750,8 +753,7 @@ void edac_mc_handle_ce(struct mem_ctl_info *mci,
> mci->ctl_page_to_phys(mci, page_frame_number) :
> page_frame_number;
>
> - edac_mc_scrub_block(remapped_page, offset_in_page,
> - mci->csrows[row].grain);
> + edac_mc_scrub_block(remapped_page, offset_in_page, grain);
> }
> }
> EXPORT_SYMBOL_GPL(edac_mc_handle_ce);
> @@ -777,6 +779,7 @@ void edac_mc_handle_ue(struct mem_ctl_info *mci,
> int chan;
> int chars;
> char *label = NULL;
> + u32 grain;
>
> debugf3("MC%d: %s()\n", mci->mc_idx, __func__);
>
> @@ -790,6 +793,7 @@ void edac_mc_handle_ue(struct mem_ctl_info *mci,
> return;
> }
>
> + grain = mci->csrows[row].channels[0].dimm->grain;
> label = mci->csrows[row].channels[0].dimm->label;
> chars = snprintf(pos, len + 1, "%s", label);
> len -= chars;
> @@ -807,14 +811,13 @@ void edac_mc_handle_ue(struct mem_ctl_info *mci,
> edac_mc_printk(mci, KERN_EMERG,
> "UE page 0x%lx, offset 0x%lx, grain %d, row %d, "
> "labels \"%s\": %s\n", page_frame_number,
> - offset_in_page, mci->csrows[row].grain, row,
> - labels, msg);
> + offset_in_page, grain, row, labels, msg);
>
> if (edac_mc_get_panic_on_ue())
> panic("EDAC MC%d: UE page 0x%lx, offset 0x%lx, grain %d, "
> "row %d, labels \"%s\": %s\n", mci->mc_idx,
> page_frame_number, offset_in_page,
> - mci->csrows[row].grain, row, labels, msg);
> + grain, row, labels, msg);
>
> mci->ue_count++;
> mci->csrows[row].ue_count++;
> @@ -886,6 +889,7 @@ void edac_mc_handle_fbd_ue(struct mem_ctl_info *mci,
> chars = snprintf(pos, len + 1, "%s", label);
> len -= chars;
> pos += chars;
> +

Useless \n.

> chars = snprintf(pos, len + 1, "-%s",
> mci->csrows[csrow].channels[channelb].dimm->label);
>
> @@ -939,6 +943,7 @@ void edac_mc_handle_fbd_ce(struct mem_ctl_info *mci,
>
> mci->ce_count++;
> mci->csrows[csrow].ce_count++;
> + mci->csrows[csrow].channels[channel].dimm->ce_count++;
> mci->csrows[csrow].channels[channel].ce_count++;
> }
> EXPORT_SYMBOL(edac_mc_handle_fbd_ce);
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index c83697c..d63904e 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -150,19 +150,19 @@ static ssize_t csrow_size_show(struct csrow_info *csrow, char *data,
> static ssize_t csrow_mem_type_show(struct csrow_info *csrow, char *data,
> int private)
> {
> - return sprintf(data, "%s\n", mem_types[csrow->mtype]);
> + return sprintf(data, "%s\n", mem_types[csrow->channels[0].dimm->mtype]);

^^^

This looks strange, why always channel 0, because it is always defined?

> }
>
> static ssize_t csrow_dev_type_show(struct csrow_info *csrow, char *data,
> int private)
> {
> - return sprintf(data, "%s\n", dev_types[csrow->dtype]);
> + return sprintf(data, "%s\n", dev_types[csrow->channels[0].dimm->dtype]);

ditto.

> }
>
> static ssize_t csrow_edac_mode_show(struct csrow_info *csrow, char *data,
> int private)
> {
> - return sprintf(data, "%s\n", edac_caps[csrow->edac_mode]);
> + return sprintf(data, "%s\n", edac_caps[csrow->channels[0].dimm->edac_mode]);

ditto.

<snip more drivers I don't maintain >

Reminder: you need to get yourself Acks at least from the drivers'
maintainers which are still active, at least.

> diff --git a/include/linux/edac.h b/include/linux/edac.h
> index f40b835..5244193 100644
> --- a/include/linux/edac.h
> +++ b/include/linux/edac.h
> @@ -314,6 +314,13 @@ struct dimm_info {
> unsigned memory_controller;
> unsigned csrow;
> unsigned csrow_channel;
> +
> + u32 grain; /* granularity of reported error in bytes */
> + enum dev_type dtype; /* memory device type */
> + enum mem_type mtype; /* memory dimm type */
> + enum edac_type edac_mode; /* EDAC mode for this dimm */
> +
> + u32 ce_count; /* Correctable Errors for this dimm */
> };
>
> /**
> @@ -339,19 +346,17 @@ struct rank_info {
> };
>
> struct csrow_info {
> - unsigned long first_page; /* first page number in dimm */
> - unsigned long last_page; /* last page number in dimm */
> + 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
> */
> - u32 nr_pages; /* number of pages in csrow */
> - u32 grain; /* granularity of reported error in bytes */
> - int csrow_idx; /* the chip-select row */
> - enum dev_type dtype; /* memory device type */
> + int csrow_idx; /* the chip-select row */
> +
> u32 ue_count; /* Uncorrectable Errors for this csrow */
> u32 ce_count; /* Correctable Errors for this csrow */
> - enum mem_type mtype; /* memory csrow type */
> - enum edac_type edac_mode; /* EDAC mode for this csrow */
> +
> struct mem_ctl_info *mci; /* the parent */
>
> struct kobject kobj; /* sysfs kobject 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Tony Luck: "Re: MCE triggered with v3.1 and v3.2 on Xeon E5"
Previous message: Jan Seiffert: "[REGRESSION][PATCH V4 2/3] bpf jit: Let the x86 jit handle negativeoffsets"
In reply to: Mauro Carvalho Chehab: "Re: [PATCH 02/13] edac: move dimm properties to struct memset_info"
Next in thread: Mauro Carvalho Chehab: "[PATCH 03/13] edac: Don't initialize csrow's first_page & friends when not needed"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]