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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090429204730.GA24298@elte.hu>
Date:	Wed, 29 Apr 2009 22:47:30 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	torvalds@...ux-foundation.org, borislav.petkov@....com,
	greg@...ah.com, tglx@...utronix.de, hpa@...or.com,
	dougthompson@...ssion.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3


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

> 
> * Andrew Morton <akpm@...ux-foundation.org> wrote:
> 
> > On Wed, 29 Apr 2009 21:23:26 +0200
> > Ingo Molnar <mingo@...e.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/