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

[Borislav Petkov <bp@amd64.org>]

On Tue, Apr 24, 2012 at 09:49:59AM -0300, Mauro Carvalho Chehab wrote:
> + * EDAC_DIMM_PTR - Macro responsible to find a pointer inside a pointer array
> + *		   for the element given by [lay0,lay1,lay2] position
> + *
> + * @layers:	a struct edac_mc_layer array, describing how many elements
> + *		were allocated for each layer
> + * @var:	name of the var where we want to get the pointer
> + *		(like mci->dimms)
> + * @n_layers:	Number of layers at the @layers array
> + * @lay0:	layer0 position
> + * @lay1:	layer1 position. Unused if n_layers < 2
> + * @lay2:	layer2 position. Unused if n_layers < 3

Ok, just call them "layer", you're not saving anything by chomping
off the last two letters. Besides, "layer" actually means what it is
supposed to, versus "lay" which means something else.

:-)

> + *
> + * For 1 layer, this macro returns &var[lay0]
> + * For 2 layers, this macro is similar to allocate a bi-dimensional array
> + *		and to return "&var[lay0][lay1]"
> + * For 3 layers, this macro is similar to allocate a tri-dimensional array
> + *		and to return "&var[lay0][lay1][lay2]"
> + *
> + * A loop could be used here to make it more generic, but, as we only have
> + * 3 layers, this is a little faster.
> + * By design, layers can never be 0 or more than 3. If that ever happens,
> + * a NULL is returned, causing an OOPS during the memory allocation routine,
> + * with would point to the developer that he's doing something wrong.
> + */
> +#define EDAC_DIMM_PTR(layers, var, nlayers, lay0, lay1, lay2) ({	\
> +	typeof(var) __p;						\
> +	if ((nlayers) == 1)						\
> +		__p = &var[lay0];					\
> +	else if ((nlayers) == 2)					\
> +		__p = &var[(lay1) + ((layers[1]).size * (lay0))];	\
> +	else if ((nlayers) == 3)					\
> +		__p = &var[(lay2) + ((layers[2]).size * ((lay1) +	\
> +			    ((layers[1]).size * (lay0))))];		\
> +	else								\
> +		__p = NULL;						\
> +	__p;								\

Ok, I see it now,

@@ -2520,7 +2561,13 @@ static int amd64_init_one_instance(struct pci_dev *F2)
                goto err_siblings;

        ret = -ENOMEM;
-       mci = edac_mc_alloc(0, pvt->csels[0].b_cnt, pvt->channel_count, nid);
+       layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+       layers[0].size = pvt->csels[0].b_cnt;
+       layers[0].is_csrow = true;
+       layers[1].type = EDAC_MC_LAYER_CHANNEL;
+       layers[1].size = pvt->channel_count;
+       layers[1].is_csrow = false;
+       mci = new_edac_mc_alloc(nid, ARRAY_SIZE(layers), layers, false, 0);
        if (!mci)
                goto err_siblings;

size is not "size"! doh, but the _count_ _of_ _elements_ this layer can
have. In the example above, layer0's size is actually the amount of chip
selects you can have per channel. WTF don't you call it that way:

Your diff says

> +/**
> + * struct edac_mc_layer - describes the memory controller hierarchy
> + * @layer:           layer type
> + * @size:maximum size of the layer
> + * @is_virt_csrow:   This layer is part of the "csrow" when old API
> + *                   compatibility mode is enabled. Otherwise, it is
> + *                   a channel
> + */
> +struct edac_mc_layer {
> +     enum edac_mc_layer_type type;
> +     unsigned                size;
> +     bool                    is_virt_csrow;
> +};

WTF am I, or anyone for that matter, to understand that with "size" you
mean "num_elems" or something like that? The explanation of that struct
member "maximum size of the layer" doesn't bring me any further either!

So call this thing properly and explain properly what it means - no one
else can look in your brain and actually understand what you mean by
this non-meaning-anything "size".

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