lkml.org 
[lkml]   [2012]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 00/13] Convert EDAC internal strutures to support all types of Memory Controllers
    On Mon, Apr 16, 2012 at 09:58:23AM -0300, Mauro Carvalho Chehab wrote:
    > That's weird. Maybe it was just a temporary error at vger. I'll contact vger
    > maintainers in order to double check what's happening there.

    No, not weird. You're probably hitting some limit on patch mail size (it
    was 40K AFAICR) and your patch is 156K.

    > > And what a patch it is: almost 5000 lines.
    >
    > No. It is half of it (2449 lines):

    $ wc -l 05-edac-fix-fore-support-for-mcs-that-see-dimms.patch
    4642 05-edac-fix-fore-support-for-mcs-that-see-dimms.patch

    [..]

    > No way. Applying just the include/linux/edac.h changes:
    >
    > drivers/edac/edac_mc.c: In function ‘edac_mc_dump_channel’:
    > drivers/edac/edac_mc.c:47:2: error: ‘struct dimm_info’ has no member named ‘ce_count’

    [..]

    > The changes at edac.h are replacing the csrow-dependent broken
    > internal ABI to a csrow-independent one. Due to that single change,
    > all existing code needs to be touched.

    Ok let me spell it:

    * Add a patch which only _adds_ the changes to <include/linux/edac.h>, i.e.:

    enum hw_event_mc_err_type, edac_mc_layer_type, GET_POS macro and that's it -
    this is your first patch out of this monster and the changes there are easily
    verifiable when looking at it.

    * Then, add a patch which introduces dimm_info->location[] array along with code
    that touches it (if possible).

    * Then, add a patch which adds dimm_info->mci which is the parent (it
    should be called parent_mci btw) and all code that touches it.

    ...

    * Add a patch which adds ->csrow and ->cschannel _without_ removing
    ->ce_count so that drivers still build.

    * Then, after you've switched code to use ->csrow and ->cschannel,
    remove ->ce_count.

    * Then, after you've converted the data structures properly, you can
    always adjust the functions in later patches.

    * Then, documentation pointers to memory controllers can go into a
    different patch.

    * edac_mc_dump_dimm() is yet another patch.

    ...

    Do you catch my drift?

    All I'm trying to explain to you is that a reviewer needs small
    patches, each patch touching a _single_ thing so that it is easily
    understandable what you're changing.

    Then, it is much easily debuggable than with a 5000 lines single
    monster.

    Also, look at Documentation/SubmittingPatches which has some more good
    advice. Hint: "If you cannot condense your patch set into a smaller set
    of patches, then only post say 15 or so at a time and wait for review
    and integration."

    > > * changes to edac_mc_alloc
    >
    > Those are also related with the edac.h changes: the data got moved
    > from one place to another one, some fields disappeared, others
    > appeared.

    As said above, split changing of those members in single patches.

    And so on...

    --
    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-04-16 16:09    [W:0.025 / U:29.884 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site