lkml.org 
[lkml]   [2012]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] edac.h: Add generic layers for describing a memory location
    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


    \
     
     \ /
      Last update: 2012-04-24 15:13    [W:0.031 / U:60.796 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site