On Thu, May 03, 2012 at 11:16:54AM -0300, Mauro Carvalho Chehab wrote:
> >> + bool enable_filter,
> >> + unsigned pos[EDAC_MAX_LAYERS])
> >
> > Passing the whole array as an argument instead of only a pointer to it?
>
> This is C, and not C++ or Pascal. Only the pointer is passed here. The size
> of the array is used for type check only.
Right, and you can see where he still has trouble. And by "he" I mean me :).
[ … ]
> >> +void edac_mc_handle_error(const enum hw_event_mc_err_type type,
> >> + struct mem_ctl_info *mci,
> >> + const unsigned long page_frame_number,
> >> + const unsigned long offset_in_page,
> >> + const unsigned long syndrome,
> >> + const int layer0,
> >> + const int layer1,
> >> + const int layer2,
> >
> > Instead of passing each layer as an arg, you can prepare the array pos[]
> > in each edac_mc_hanlde_*() and pass around a pointer to it - you need it
> > anyway in the edac_mc_inc*() functions.
>
> Yes, but the changes at the drivers will be more complex, without any reason:
> before each call to this function, they would need to create and fill a
> temporary
> array.
>
> As there are only 3 layers, in the worse case, this way is simpler and more
> efficient. We can review it, if we ever need more than 3 layers.
I see, the edac_mc_handle_error is the main interface for all edac drivers, ok.
[ … ]
> >> + bool enable_filter = false;
> >
> > What does this enable_filter thing mean:
> >
> > if (pos[i] >= 0)
> > enable_filter = true;
> >
> > This absolutely needs explanation and better naming!
>
> Renamed it to "enable_per_layer_report".
Or "detailed_dimm_report" or ...
> The code that implement it seems self-explained:
>
> ..
> if (enable_filter && dimm->nr_pages) {
> if (p != label) {
> strcpy(p, OTHER_LABEL);
> p += strlen(OTHER_LABEL);
> }
> strcpy(p, dimm->label);
> p += strlen(p);
> *p = '\0';
>
> ..
>
> if (!enable_filter) {
> strcpy(label, "any memory");
> } else {
> debugf4("%s: csrow/channel to increment: (%d,%d)\n",
> __func__, row, chan);
> if (p == label)
> strcpy(label, "unknown memory");
> if (type == HW_EVENT_ERR_CORRECTED) {
> if (row >= 0) {
> mci->csrows[row].ce_count++;
> if (chan >= 0)
>
> mci->csrows[row].channels[chan].ce_count++;
> }
> } else
> if (row >= 0)
> mci->csrows[row].ue_count++;
> }
>
> Theis flag indicates if is there any useful information about the affected
> DIMM(s) provided by the EDAC driver. If this is provided, the DIMM location
> labels are
> filtered and reported, and the per-layer error counters are incremented.
>
> As it was discussed on previous reviews, with FB-DIMM MCs, and/or when mirror
> mode/lockstep mode is enabled, the memory controller points errors to 2 DIMMs
> (or 4 DIMMs, when both mirror mode and lockstep mode are enabled) on most
> memory
> controllers, under some conditions. The edac_mc_handle_fbd_ue() function call
> were
> created due to that.
>
> When comparing with the old code, "enable_filter = false" would be equivalent
> to call
> edac_mc_handle_ce_no_info/edac_mc_handle_ue_no_info.
>
> I'm adding a comment about it.
Much better, thanks.
Btw, I have to admit, this is a pretty strange way of handling the case
where layers are { -1, -1, -1 }, i.e. edac_mc_handle_error is called
with the "no info" hint.
I'm wondering whether it wouldn't be more readable if you could do
edac_mc_handle_error(HW_EVENT_ERR_INFO_INVALID | ..)
or similar and define such a flag which simply states that. But you'll
have to change enum hw_event_mc_err_type to a bitfield to allow more
than one set bit.
Hmm.
[ … ]
> > The SCRUB_SW_SRC piece can be another function.
>
> It is now part of the edac_ce_error().
Hm, I can't find this function on your "experimental" branch on
infradead but it is mentioned in the inlined patch below, what's going
on? Which patch should I be looking at now?
[ … ]
> The following patch addresses the pointed issues. I've updated them
> on my experimental branch at infradead:
> git://git.infradead.org/users/mchehab/edac.git experimental
Ok, I checked this one out but can't find the edac_ce_error() function
as already stated above, pls check.
> They'll also be soon available at:
> git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-edac.git
> hw_events_v18
Will review the patch below now and reply in another mail.
Thanks.
>
> Regards,
> Mauro
>
> -
>
> edac: Change internal representation to work with layers
>
> From: Mauro Carvalho Chehab <mche...@redhat.com>
>
> Change the EDAC internal representation to work with non-csrow
> based memory controllers.
--
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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
