lkml.org 
[lkml]   [2012]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [EDAC ABI v13 01/25] edac: Initialize the dimm label with the known information
    Review comments to the one below are still not addressed, maybe slipped
    through the cracks?

    On Mon, May 07, 2012 at 05:52:26PM +0200, Borislav Petkov wrote:
    > Adding latest version here:
    >
    > > From 50e9a89aad7045909780d635d73ab2893f8c1f90 Mon Sep 17 00:00:00 2001
    > > From: Mauro Carvalho Chehab <mchehab@redhat.com>
    > > 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@redhat.com>
    > > Cc: Doug Thompson <norsk5@yahoo.com>
    > > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
    > > ---
    > > 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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-05-14 15:22    [W:0.039 / U:1.608 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site