lkml.org 
[lkml]   [2009]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

    * Ingo Molnar <mingo@elte.hu> wrote:

    >
    > * Borislav Petkov <borislav.petkov@amd.com> wrote:
    >
    > > From: Doug Thompson <dougthompson@xmission.com>
    > >
    > > Signed-off-by: Doug Thompson <dougthompson@xmission.com>
    > > Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
    > > ---
    > > drivers/edac/amd64_edac.c | 318 +++++++++++++++++++++++++++++++++++++++++++++
    > > 1 files changed, 318 insertions(+), 0 deletions(-)
    > >
    > > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
    > > index fe2342c..84075c0 100644
    > > --- a/drivers/edac/amd64_edac.c
    > > +++ b/drivers/edac/amd64_edac.c
    > > @@ -2726,4 +2726,322 @@ static int f10_lookup_addr_in_dct(u32 InputAddr, u32 NodeID, u32 ChannelSelect)
    > > return CSFound;
    > > }
    > >
    > > +/*
    > > + * f10_match_to_this_node
    > > + *
    > > + * For a given 'DramRange' value, check if 'SystemAddr' fall within this value
    > > + */
    > > +static int f10_match_to_this_node(struct amd64_pvt *pvt, int DramRange,
    > > + u64 SystemAddr,
    > > + int *node_id,
    > > + int *channel_select)
    > > +{
    > > + int CSFound = -1;
    > > + int NodeID;
    > > + int HiRangeSelected;
    > > + u32 IntlvEn, IntlvSel;
    > > + u32 DramEn;
    > > + u32 Ilog;
    > > + u32 HoleOffset, HoleEn;
    > > + u32 InputAddr, Temp;
    > > + u32 DctSelBaseAddr, DctSelIntLvAddr;
    > > + u32 DctSelHi;
    > > + u32 ChannelSelect;
    > > + u64 DramBaseLong, DramLimitLong;
    > > + u64 DctSelBaseOffsetLong, ChannelAddrLong;
    > > +
    > > + /* DRAM Base value for this DRAM instance */
    > > + DramBaseLong = pvt->dram_base[DramRange];
    > > + DramEn = pvt->dram_rw_en[DramRange];
    > > + IntlvEn = pvt->dram_IntlvEn[DramRange];
    > > +
    > > + /* DRAM Limit value for this DRAM instance */
    > > + DramLimitLong = pvt->dram_limit[DramRange];
    > > + NodeID = pvt->dram_DstNode[DramRange];
    > > + IntlvSel = pvt->dram_IntlvSel[DramRange];
    > > +
    > > + debugf1("%s(dram=%d) Base=0x%llx SystemAddr= 0x%llx Limit=0x%llx\n",
    > > + __func__, DramRange, DramBaseLong, SystemAddr, DramLimitLong);
    > > +
    > > + /* This assumes that one node's DHAR is the same as
    > > + * all the other node's DHARs
    > > + */
    > > + HoleEn = pvt->dhar;
    > > + HoleOffset = (HoleEn & 0x0000FF80);
    > > + HoleEn = (HoleEn & 0x00000003);
    > > +
    > > + debugf1(" HoleOffset=0x%x HoleEn=0x%x IntlvSel=0x%x\n",
    > > + HoleOffset, HoleEn, IntlvSel);
    > > +
    > > + if ((IntlvEn == 0) || IntlvSel == ((SystemAddr >> 12) & IntlvEn)) {
    > > +
    > > + Ilog = f10_map_IntlvEn_to_shift(IntlvEn);
    > > +
    > > + Temp = pvt->dram_ctl_select_low;
    > > + DctSelBaseOffsetLong = pvt->dram_ctl_select_high << 16;
    > > +
    > > + DctSelHi = (Temp >> 1) & 1;
    > > + DctSelIntLvAddr = dct_sel_interleave_addr(pvt);
    > > + DctSelBaseAddr = dct_sel_baseaddr(pvt);
    > > +
    > > + if (dct_high_range_enabled(pvt) &&
    > > + !dct_ganging_enabled(pvt) &&
    > > + ((SystemAddr >> 27) >= (DctSelBaseAddr >> 11)))
    > > + HiRangeSelected = 1;
    > > + else
    > > + HiRangeSelected = 0;
    > > +
    > > + ChannelSelect = f10_determine_channel(pvt, SystemAddr,
    > > + HiRangeSelected, IntlvEn);
    > > +
    > > + ChannelAddrLong = f10_determine_base_addr_offset(
    > > + SystemAddr,
    > > + HiRangeSelected,
    > > + DctSelBaseAddr,
    > > + DctSelBaseOffsetLong,
    > > + HoleEn,
    > > + HoleOffset,
    > > + DramBaseLong);
    > > +
    > > + /* Remove Node ID (in case of processor interleaving) */
    > > + Temp = ChannelAddrLong & 0xFC0;
    > > +
    > > + ChannelAddrLong = ((ChannelAddrLong >> Ilog) &
    > > + 0xFFFFFFFFF000ULL) | Temp;
    > > +
    > > + /* Remove Channel interleave and hash */
    > > + if (dct_interleave_enabled(pvt) &&
    > > + !dct_high_range_enabled(pvt) &&
    > > + !dct_ganging_enabled(pvt)) {
    > > + if (DctSelIntLvAddr != 1)
    > > + ChannelAddrLong =
    > > + (ChannelAddrLong >> 1) &
    > > + 0xFFFFFFFFFFFFFFC0ULL;
    > > + else {
    > > + Temp = ChannelAddrLong & 0xFC0;
    > > + ChannelAddrLong =
    > > + ((ChannelAddrLong &
    > > + 0xFFFFFFFFFFFFC000ULL)
    > > + >> 1) | Temp;
    > > + }
    > > + }
    > > +
    > > + /* Form a normalize InputAddr (Move bits 36:8 down to 28:0
    > > + * which will set it up to match the DCT Base register
    > > + */
    > > + InputAddr = ChannelAddrLong >> 8;
    > > +
    > > + debugf1(" (ChannelAddrLong=0x%llx) >> 8 becomes "
    > > + "InputAddr=0x%x\n", ChannelAddrLong, InputAddr);
    > > +
    > > + /* Iterate over the DRAM DCTs looking for a
    > > + * match for InputAddr on the selected NodeID
    > > + */
    > > + CSFound = f10_lookup_addr_in_dct(InputAddr,
    > > + NodeID, ChannelSelect);
    > > +
    > > + if (CSFound >= 0) {
    > > + *node_id = NodeID;
    > > + *channel_select = ChannelSelect;
    > > + }
    > > + }
    > > +
    > > + return CSFound;
    > > +}
    >
    > this function is probably too large, and also it uses some weird
    > hungarian notation coding style. Please dont do that! It's
    > completely unacceptable.
    >
    > this condition:
    >
    > > + if ((IntlvEn == 0) || IntlvSel == ((SystemAddr >> 12) & IntlvEn)) {
    >
    > could be inverted and an early "return cs_found" could be done -
    > saving an indentitation level for most of the above code.
    >
    > etc. etc.
    >
    > Please look at the function in a really large xterm, from far
    > away. If the shape does not look 'good', and the structure is not
    > an obvious pattern seen a hundred times elsewhere in the kernel,
    > there's something weird going on with the function. It should be
    > split up, cleaned up, simplified. Variable names could become
    > shorter, etc. etc.

    ... and this general observation about variable naming and general
    structure holds for the rest of the patches as well. This really
    needs to be sorted out before a more detailed review can be done.

    Ingo


    \
     
     \ /
      Last update: 2009-04-29 20:27    [W:0.062 / U:60.280 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site