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

[Mauro Carvalho Chehab]

Em 16-04-2012 08:02, Borislav Petkov escreveu:
> On Mon, Apr 16, 2012 at 05:41:33AM -0300, Mauro Carvalho Chehab wrote:
>> This is not a double loop.
> 
> But it is, actually.
> 
> Let's look at the code:
> 
>         /* 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;
> 
>         for (row = 0; row < nr_csrows; row++) {					<-- A1
>                 csrow = &csi[row];
>                 csrow->csrow_idx = row;
>                 csrow->mci = mci;
>                 csrow->nr_channels = nr_chans;
>                 chp = &chi[row * nr_chans];
>                 csrow->channels = chp;
> 
>                 for (chn = 0; chn < nr_chans; chn++) {				<-- B1
>                         chan = &chp[chn];
>                         chan->chan_idx = chn;
>                         chan->csrow = csrow;
>                 }
>         }
> 
>         /*
>          * 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++) {				<-- A2
>                 for (chn = 0; chn < mci->csrows[row].nr_channels; chn++) {	<-- B2
>                         mci->csrows[row].channels[chn].dimm = dimm;
>                         dimm->csrow = row;
>                         dimm->csrow_channel = chn;
>                         dimm++;
>                         mci->nr_dimms++;
>                 }
>         }
> 
> So the lines tagged with A1 and A2 iterate over the nr_csrows, while
> lines tagged with B1 and B2 iterate over nr_chans. In B2, loop termination is
> 
> 	mci->csrows[row].nr_channels
> 
> which is assigned in the first loop above to
> 
> 	csrow->nr_channels = nr_chans
> 
> In B1, it is simply nr_chans.

Yes, this is true, on this patchset, because it is a preparation for a bigger
change, but this won't be true after changeset 5/13, where the dimm_info will
get a real life.

> 
> So how about we merge those two:
> 
> 	...
> 	dimm = mci->dimms;
> 
>         for (row = 0; row < nr_csrows; row++) {					<-- A1
>                 csrow = &csi[row];
>                 csrow->csrow_idx = row;
>                 csrow->mci = mci;
>                 csrow->nr_channels = nr_chans;
>                 chp = &chi[row * nr_chans];
>                 csrow->channels = chp;
> 
>                 for (chn = 0; chn < nr_chans; chn++) {				<-- B1
>                         chan = &chp[chn];
>                         chan->chan_idx = chn;
>                         chan->csrow = csrow;
> 
> 			 /* second loop */
>                         csrow->channels[chn].dimm = dimm;
>                         dimm->csrow = row;
>                         dimm->csrow_channel = chn;
>                         dimm++;
>                         mci->nr_dimms++;
>                 }
>         }
> 
> So it is only 5 lines of code instead of another loop.

Because this will make patch 5/13 even bigger and messy. Each of those
loops have different functions: the first one initializes the legacy API
data structures for virtual csrows/channels, while the second one 
initializes the struct that contains the real DIMM or rank information.

Patches 1 to 4 are just a preparation for patch 5/13, cleaning what's
possible before the big change.

While it is possible to do the above merge on this patch alone, such
cleanup doesn't make sense at the patch series (and should be reverted
on patch 5/13 anyway), as what we want is to separate the DIMM information 
on a data structure that won't mix it with a memory layout-dependent 
information, as not all drivers use csrows/channels.

Regards,
Mauro.


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