linux-kernel - Re: [PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps in a macro

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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 17 Mar 2016 22:15:34 +1100
From:	Paul Mackerras <paulus@...abs.org>
To:	"Shreyas B. Prabhu" <shreyas@...ux.vnet.ibm.com>
Cc:	mpe@...erman.id.au, linuxppc-dev@...ts.ozlabs.org,
	linux-kernel@...r.kernel.org, benh@...nel.crashing.org,
	mahesh@...ux.vnet.ibm.com, maddy@...ux.vnet.ibm.com
Subject: Re: [PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps
 in a macro

On Mon, Feb 29, 2016 at 05:52:59PM +0530, Shreyas B. Prabhu wrote:
> Before entering any idle state which can result in a state loss
> we currently save the context in the stack before entering idle.
> Encapsulate these steps in a macro IDLE_STATE_PREP. Move this
> and other macros to commonly accessible location.

There are two problems with this.  First, your new macro does much
more than create a stack frame and save some registers.  It also
messes with interrupts and potentially executes a blr instruction.
That is not what people would expect from the name of the macro or the
comments around it.  It also means that it would be hard to reuse the
macro in another place.

Secondly, I don't think this change helps readability.  Since the
macro is only used in one place, it doesn't reduce the total number of
lines of code, in fact it increases it slightly.  Having the macro
just means that someone reading the code has to look in two places to
see what power7_powersave_common is actually doing, rather than having
all the code in one place.  If what your macro did was a single thing
conceptually that took several instructions, then it might be helpful,
but as it is, it doesn't help readability.

Paul.