Re: [PATCH] edac.h: Add generic layers for describing a memory location

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

Em 24-04-2012 10:38, Borislav Petkov escreveu:
> On Tue, Apr 24, 2012 at 10:22:09AM -0300, Mauro Carvalho Chehab wrote:
>> The count of elements of a layer is the size of the layer. The Kernel macro
>> that gets the number of elements of an array is called "ARRAY_SIZE", and not
>> "ARRAY_N_ELEMS".
>>
>> layers->size is the dimension of the layer. So, the term "size" fits better.
>> For example, according with [1], size means:
>> 	"the spatial dimensions, proportions, magnitude, or bulk of anything:
>> 	 the size of a farm; the size of the fish you caught."
>>
>> so, "size" fits better for a "dimension" measure.
>>
>> I don't mind renaming it to n_elems, if this makes you happy.
> 
> Ok, let's do a simple comparison:
> 
> 1. Imagine you look at ARRAY_SIZE(), what does it mean? Well, the size
> of an array is pretty well defined to be the number of elements in it.
> Easy.
> 
> 2. Now imagine a struct member ->size. Out of context it could mean
> anything, the size of something this struct represents, what the hell do
> I know...

The size of a memory architecture layer is the number of physical components 
on it:

So, for example (from i7300_edac, just to get an example with 3 layers):

...
#define MAX_SLOTS		8
#define MAX_BRANCHES		2
#define MAX_CH_PER_BRANCH	2
...
	layers[0].type = EDAC_MC_LAYER_BRANCH;
	layers[0].size = MAX_BRANCHES;
	layers[0].is_csrow = false;
	layers[1].type = EDAC_MC_LAYER_CHANNEL;
	layers[1].size = MAX_CH_PER_BRANCH;
	layers[1].is_csrow = true;
	layers[2].type = EDAC_MC_LAYER_SLOT;
	layers[2].size = MAX_SLOTS;
	layers[2].is_csrow = true;

This means that there are 2 branches at the branch layer, 2 channels
per branch, at the channel layer, and 8 DIMM slots, at the slot layer.

The maximum number of DIMMs on this MCU is 32 DIMMs.

Calling "2 channels" as n_elems = 2 doesn't sound nice.

> 
> Now let's put it in context, layer->size: It could mean the size of the
> layer in MB, it could mean how thick the layer is in meters, it could
> ... I can go on with these forever.
> 
> So your example with ARRAY_SIZE does not apply here.
> 
> If it is a badly explained struct and size is "maximum size" then this
> means sh*t. So either leave it "size" but make sure to explain it
> thoroughly what exactly size means here or change its name to something
> more meaningful.

Ok, I'll apply then the explanation below:

diff --git a/include/linux/edac.h b/include/linux/edac.h
index 671b27b..934c196 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -348,7 +348,8 @@ enum edac_mc_layer_type {
 /**
  * struct edac_mc_layer - describes the memory controller hierarchy
  * @layer:		layer type
- * @size:maximum size of the layer
+ * @size:		number of components on that layer. For example,
+ *			if the channel layer have two channels, size = 2
  * @is_virt_csrow:	This layer is part of the "csrow" when old API
  *			compatibility mode is enabled. Otherwise, it is
  *			a channel