Messages in this thread | | | Date | Wed, 29 Apr 2009 22:47:30 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3 |
| |
* Ingo Molnar <mingo@elte.hu> wrote:
> > * Andrew Morton <akpm@linux-foundation.org> wrote: > > > On Wed, 29 Apr 2009 21:23:26 +0200 > > Ingo Molnar <mingo@elte.hu> wrote: > > > > > > > > + 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. > > > > I think I disagree. For those identifiers which map 1:1 with the > > manufacturer's document, the ugliness involved in exactly copying > > the manufacturer's chosen identifiers is outweighed by the benefit > > of exactly copying the manufacturer's chosen identifiers. > > > > Of course, we don't have to use StinkyIdentifiers anywhere else. > > And the nice thing about that is that when one reads the code and > > comes across a StinkyIdentifier, one immeditely knows that it's an > > AMD-provided thing rather than a Linux-provided thing. > > > > Zillions of StinkyIdentifiers get merged via this logic. > > Andrew, for heaven's sake, please review the patchset - as i did.
Let me apologize for this rude reply ... it appears we do agree, i just didnt properly read your paragraphs above :-/
What i point out below is precisely what you say is ineligible under:
> > Of course, we don't have to use StinkyIdentifiers anywhere else.
I'd extend that rule to say that StinkyIdentifiers should only be used for hw API definitions/constants - macros, enums - not really local variable names. The moment they are allowed into local variables the stuff below happens.
Thanks,
Ingo
> > The thing is, up to 12/21, the patches look like normal Linux > patches. (there's problems with them too, but on a different level) > > Then do the StinkyIdentifiers show up, in full force: > > +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; > > Tell me, how is 'SystemAddr' or 'Temp' or 'Ilog' an AMD document > thing? > > I have a much simpler explanation really: someone got really bored > at converting some code written For Another OS, somewhere in the > middle - and started plopping Other OS Code into a Linux driver ... > > I dont mind the occasional _constant_ that tells us a hw API detail > in whatever externally dictated style - but this thing stinks > HeadToToe ... ;-) > > Ingo
| |