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]

Date:	Wed, 29 Apr 2009 21:23:26 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	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


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

> On Wed, 29 Apr 2009 20:22:55 +0200
> Ingo Molnar <mingo@...e.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
--
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/