Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

Ingo Molnar
2009-04-29T19:23:52Z

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 29 Apr 2009 20:22:55 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
>
> > > + 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.
>
> These identifers (or at least, DctSelBaseOffsetLong, which is the
> only one I googled for) come straight out of the AMD "BIOS and
> Kernel Developer's Guide".
>
> Sucky though they are, there's value in making the kernel code
> match up with the documentation.

I'm generally resisting patches that hungarinize arch/x86/ (and heck
there's been many attempts ...) but there's some conflicting advice
here. I've Cc:-ed Linus, maybe he has an opinion about this.

My gut reaction would be 'hell no'. There's other, structural
problems with this code too, and doing some saner naming would
mostly be a sed job and would take minimal amount of time. The
naming can still be intuitive. The symbols from the documentation
can perhaps be mentioned in a couple of comments to establish a
mapping.

Ingo