Re: [EDAC ABI v13 01/25] edac: Initialize the dimm label with theknown information

From: Borislav Petkov
Date: Mon May 07 2012 - 11:52:37 EST

Adding latest version here:

> From 50e9a89aad7045909780d635d73ab2893f8c1f90 Mon Sep 17 00:00:00 2001
> From: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> Date: Thu, 9 Feb 2012 11:05:20 -0300
> Subject: [PATCH] edac: Initialize the dimm label with the known information
>
> While userspace doesn't fill the dimm labels, add there the dimm location,
> as described by the used memory model. This could eventually match what
> is described at the dmidecode, making easier for people to identify the

in making it easier

> memory.

stick in error.

> For example, on an Intel motherboard, the memory is described as:
>
> Memory Device
> Array Handle: 0x0029
> Error Information Handle: Not Provided
> Total Width: 64 bits
> Data Width: 64 bits
> Size: 2048 MB
> Form Factor: DIMM
> Set: 1
> Locator: A1_DIMM0
> Bank Locator: A1_Node0_Channel0_Dimm0
> Type: <OUT OF SPEC>
> Type Detail: Synchronous
> Speed: 800 MHz
> Manufacturer: A1_Manufacturer0
> Serial Number: A1_SerNum0
> Asset Tag: A1_AssetTagNum0
> Part Number: A1_PartNum0
>
> After this patch, the memory label will be filled with:
> /sys/devices/system/edac/mc/mc0/dimm0/dimm_label:mc#0channel#0slot#0

This is only with the Intel-MCs, right, I still have the csrows here:

tree /sys/devices/system/edac/mc/
/sys/devices/system/edac/mc/
|-- mc0
| |-- ce_count
| |-- ce_noinfo_count
| |-- csrow0
| | |-- ce_count
| | |-- ch0_ce_count
| | |-- ch0_dimm_label
| | |-- ch1_ce_count
| | |-- ch1_dimm_label
| | |-- dev_type
| | |-- edac_mode
| | |-- mem_type
| | |-- size_mb
| | `-- ue_count
| |-- csrow1
| | |-- ce_count
| | |-- ch0_ce_count
| | |-- ch0_dimm_label
| | |-- ch1_ce_count
| | |-- ch1_dimm_label
| | |-- dev_type
| | |-- edac_mode
| | |-- mem_type
| | |-- size_mb
| | `-- ue_count
| |-- csrow2
| | |-- ce_count
| | |-- ch0_ce_count
| | |-- ch0_dimm_label
| | |-- ch1_ce_count
| | |-- ch1_dimm_label
| | |-- dev_type
| | |-- edac_mode
| | |-- mem_type
| | |-- size_mb
| | `-- ue_count
â


> With somewhat matches what it is at the Bank Locator DMI information.

I wouldn't say that - DMI is notoriously unreliable, let's look at some
boxes:

1st box:

Handle 0x0038, DMI type 17, 28 bytes
Memory Device
Array Handle: 0x0036
Error Information Handle: Not Provided
Total Width: 72 bits
Data Width: 64 bits
Size: 2048 MB
Form Factor: DIMM
Set: None
Locator: DIMM0

2nd box:

Memory Device
Array Handle: 0x0014
Error Information Handle: Not Provided
Total Width: 64 bits
Data Width: 4096 bits
Size: 9 MB
Form Factor: <OUT OF SPEC>
Set: 73
Locator: P0_DIMM_A1
Bank Locator: CHANNEL A

3rd box:

Handle 0x0033, DMI type 17, 28 bytes
Memory Device
Array Handle: 0x0031
Error Information Handle: Not Provided
Total Width: 64 bits
Data Width: 64 bits
Size: 4096 MB
Form Factor: SODIMM
Set: 2
Locator: J401
Bank Locator: Channel B

and so on.

IOW, DMI fields are almost random permutations of [a-zA-Z0-9].

> So, it is easier to associate the dimm labels, of course assuming that
> the DMI has the Bank Locator filled, and the BIOS doesn't have any bugs.
>
> Yet, even without it, several motherboards are provided with enough
> info to map from channel/slot (or branch/channel/slot) into the DIMM
> label. So, letting the EDAC core fill it, by default is a good thing.
>
> It should noticed that, as the label filling happens at the
> edac_mc_alloc(), drivers can override it to better describe the memories
> (and some actually do it).

But I guess having the info is still fine, simply remove the DMI
references in the commit message pls.

> Cc: Aristeu Rozanski <arozansk@xxxxxxxxxx>
> Cc: Doug Thompson <norsk5@xxxxxxxxx>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> ---
> drivers/edac/edac_mc.c | 25 +++++++++++++++++++------
> drivers/edac/edac_mc_sysfs.c | 8 ++++----
> include/linux/edac.h | 2 +-
> 3 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index 4c44cd298c0b..77263b33b7f0 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -210,10 +210,10 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
> struct dimm_info *dimm;
> u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
> unsigned pos[EDAC_MAX_LAYERS];
> - void *pvt, *ptr = NULL;
> unsigned size, tot_dimms = 1, count = 1;
> unsigned tot_csrows = 1, tot_channels = 1, tot_errcount = 0;
> - int i, j, err, row, chn;
> + void *pvt, *p, *ptr = NULL;
> + int i, j, err, row, chn, n, len;
> bool per_rank = false;
>
> BUG_ON(n_layers > EDAC_MAX_LAYERS || n_layers == 0);
> @@ -325,9 +325,22 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
> i, per_rank ? "rank" : "dimm", (dimm - mci->dimms),
> pos[0], pos[1], pos[2], row, chn);
>
> - /* Copy DIMM location */
> - for (j = 0; j < n_layers; j++)
> + /*
> + * Copy DIMM location and initialize the memory location

initialize it.

or do you mean two different locations?

> + */
> + len = sizeof(dimm->label);
> + p = dimm->label;
> + n = snprintf(p, len, "mc#%u", mc_num);
> + p += n;
> + len -= n;
> + for (j = 0; j < n_layers; j++) {
> + n = snprintf(p, len, "%s#%u",
> + edac_layer_name[layers[j].type],
> + pos[j]);
> + p += n;
> + len -= n;

Err, you're not checking how much len is left here, i.e.
EDAC_MC_LABEL_LEN. Or even better, each time before you do snprintf.

> dimm->location[j] = pos[j];
> + }
>
> /* Link it to the csrows old API data */
> chan->dimm = dimm;
> @@ -837,7 +850,7 @@ static void edac_inc_ce_error(struct mem_ctl_info *mci,
> {
> int i, index = 0;
>
> - mci->ce_count++;
> + mci->ce_mc++;

Oh, renaming them back, ok.

<rest snipped>

--
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/