Messages in this thread |  | | | Date | Fri, 30 Mar 2012 10:26:13 -0300 | | From | Mauro Carvalho Chehab <> | | Subject | Re: [PATCH 01/13] edac: Create a dimm struct and move the labels into it |
| |
Em 30-03-2012 07:50, Borislav Petkov escreveu: > On Thu, Mar 29, 2012 at 01:45:34PM -0300, Mauro Carvalho Chehab wrote: >> The way a DIMM is currently represented implies that they're >> linked into a per-csrow struct. However, some drivers don't see >> csrows, as they're ridden behind some chip like the AMB's >> on FBDIMM's, for example. >> >> This forced drivers to fake a csrow struct, and to create >> a mess under csrow/channel original's concept. >> >> Move the DIMM labels into a per-DIMM struct, and add there >> the real location of the socket, in terms of csrow/channel. >> Latter patches will modify the location to properly represent the >> memory architecture. >> >> All other drivers will use a per-csrow type of location. >> Some of those drivers will require a latter conversion, as >> they also fake the csrows internally. >> >> TODO: While this patch doesn't change the existing behavior, on >> csrows-based memory controllers, a csrow/channel pair points to a memory >> rank. There's a known bug at the EDAC core that allows having different >> labels for the same DIMM, if it has more than one rank. A latter patch >> is need to merge the several ranks for a DIMM into the same dimm_info >> struct, in order to avoid having different labels for the same DIMM. >> >> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> >> --- >> drivers/edac/edac_mc.c | 50 +++++++++++++++++++++++++++++++---------- >> drivers/edac/edac_mc_sysfs.c | 11 ++++----- >> drivers/edac/i5100_edac.c | 8 +++--- >> drivers/edac/i7core_edac.c | 4 +- >> drivers/edac/i82975x_edac.c | 2 +- >> drivers/edac/sb_edac.c | 4 +- >> include/linux/edac.h | 28 +++++++++++++++++++---- >> 7 files changed, 75 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c >> index 690cbf1..c03bfe7 100644 >> --- a/drivers/edac/edac_mc.c >> +++ b/drivers/edac/edac_mc.c >> @@ -44,7 +44,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->label = '%s'\n", chan->label); >> + debugf4("\tchannel->label = '%s'\n", chan->dimm->label); >> debugf4("\tchannel->csrow = %p\n\n", chan->csrow); >> } >> >> @@ -157,6 +157,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows, >> struct mem_ctl_info *mci; >> struct csrow_info *csi, *csrow; >> struct rank_info *chi, *chp, *chan; >> + struct dimm_info *dimm; >> void *pvt; >> unsigned size; >> int row, chn; >> @@ -170,7 +171,8 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows, >> mci = (struct mem_ctl_info *)0; >> csi = edac_align_ptr(&mci[1], sizeof(*csi)); >> chi = edac_align_ptr(&csi[nr_csrows], sizeof(*chi)); >> - pvt = edac_align_ptr(&chi[nr_chans * nr_csrows], sz_pvt); >> + dimm = edac_align_ptr(&chi[nr_chans * nr_csrows], sizeof(*dimm)); >> + pvt = edac_align_ptr(&dimm[nr_chans * nr_csrows], sz_pvt); >> size = ((unsigned long)pvt) + sz_pvt; >> >> mci = kzalloc(size, GFP_KERNEL); >> @@ -182,11 +184,13 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows, >> */ >> csi = (struct csrow_info *)(((char *)mci) + ((unsigned long)csi)); >> chi = (struct rank_info *)(((char *)mci) + ((unsigned long)chi)); >> + dimm = (struct dimm_info *)(((char *)mci) + ((unsigned long)dimm)); >> pvt = sz_pvt ? (((char *)mci) + ((unsigned long)pvt)) : NULL; >> >> /* setup index and various internal pointers */ >> mci->mc_idx = edac_index; >> mci->csrows = csi; >> + mci->dimms = dimm; >> mci->pvt_info = pvt; >> mci->nr_csrows = nr_csrows; >> >> @@ -205,6 +209,21 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows, >> } >> } >> >> + /* >> + * By default, assumes that a per-csrow arrangement will be used, >> + * as most drivers are based on such assumption. >> + */ >> + dimm = mci->dimms; >> + for (row = 0; row < mci->nr_csrows; row++) { >> + for (chn = 0; chn < mci->csrows[row].nr_channels; chn++) { >> + mci->csrows[row].channels[chn].dimm = dimm; >> + dimm->csrow = row; >> + dimm->csrow_channel = chn; >> + dimm++; >> + mci->nr_dimms++; >> + } >> + } > > There's a double loop above this one which iterates over the same > things: rows and then channels in each row. So merge that loop with the > one above instead of repeating it here. >
The first loop will disappear on the patch I'm writing right now with the edac_mc_alloc() changes to per-csrow/per-dimm/per-channel kzalloc().
Regards, Mauro
|  |