Re: [PATCH 01/13] edac: Create a dimm struct and move the labels into it

[Mauro Carvalho Chehab <mchehab@redhat.com>]

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