linux-kernel - Re: [PATCHv7 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller.

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: <20140708120432.GB8628@amd.pavel.ucw.cz>
Date:	Tue, 8 Jul 2014 14:04:32 +0200
From:	Pavel Machek <pavel@...x.de>
To:	Borislav Petkov <bp@...en8.de>
Cc:	tthayer@...era.com, robherring2@...il.com, pawel.moll@....com,
	mark.rutland@....com, ijc+devicetree@...lion.org.uk,
	galak@...eaurora.org, rob@...dley.net, linux@....linux.org.uk,
	dinguyen@...era.com, dougthompson@...ssion.com,
	grant.likely@...aro.org, devicetree@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	tthayer.linux@...il.com, linux-arm-kernel@...ts.infradead.org,
	linux-edac@...r.kernel.org
Subject: Re: [PATCHv7 3/3] edac: altera: Add EDAC support for Altera SoC
 SDRAM Controller.

On Tue 2014-07-08 13:54:58, Borislav Petkov wrote:
> On Tue, Jul 08, 2014 at 01:52:05PM +0200, Pavel Machek wrote:
> > I'm not joking. Try to understand and verify the code above. You
> > can't. The "descriptive macro names" are useless; all the code does is
> > split register in pieces. With the numbers it would be  very obvious.
> 
> No, you need to fix the names not switch to naked numbers.

Hiding numbers that are used just once into defines to put them out of
sight does not really help readability.

Compare:

+#define DRAMADDRW_COLBIT_MASK	0x001F
+#define DRAMADDRW_COLBIT_LSB	0
+#define DRAMADDRW_ROWBIT_MASK	0x02E0
+#define DRAMADDRW_ROWBIT_LSB	5
+#define DRAMADDRW_BANKBIT_MASK	0x1C00
+#define DRAMADDRW_BANKBIT_LSB	10
+#define DRAMADDRW_CSBIT_MASK	0xE000
+#define DRAMADDRW_CSBIT_LSB	13

(Insert few screens of code so that you have to scroll or split
window).

What information do the define names have? None, I already know how
hardware registers work.

+	col = (read_reg & DRAMADDRW_COLBIT_MASK) >>
+	     DRAMADDRW_COLBIT_LSB;
+	row = (read_reg & DRAMADDRW_ROWBIT_MASK) >>
+	     	 DRAMADDRW_ROWBIT_LSB;
+	bank = (read_reg & DRAMADDRW_BANKBIT_MASK) >>
+		      DRAMADDRW_BANKBIT_LSB;
+	cs = (read_reg & DRAMADDRW_CSBIT_MASK) >>
+		      	 DRAMADDRW_CSBIT_LSB;

Instead, we could have:

	 col = read_reg & 0x001F;
	 row = (read_reg & 0x02E0) >> 5;
	 bank = (read_reg & 0x1C00) >> 10;
	 cs = (read_reg & 0xE000) >> 13;

All information is preserved, code is shorter and easier to understand.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
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/