Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753901AbcCSAVn (ORCPT <rfc822;w@1wt.eu>);
	Fri, 18 Mar 2016 20:21:43 -0400
Received: from ozlabs.org ([103.22.144.67]:48146 "EHLO ozlabs.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751802AbcCSAVe (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Fri, 18 Mar 2016 20:21:34 -0400
Date: Sat, 19 Mar 2016 11:21:22 +1100
From: Paul Mackerras <paulus@ozlabs.org>
To: Shreyas B Prabhu <shreyas@linux.vnet.ibm.com>
Cc: mpe@ellerman.id.au, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org, benh@kernel.crashing.org,
        mahesh@linux.vnet.ibm.com, maddy@linux.vnet.ibm.com
Subject: Re: [PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps
 in a macro
Message-ID: <20160319002122.GA27126@fergus.ozlabs.ibm.com>
References: <1456748580-10519-1-git-send-email-shreyas@linux.vnet.ibm.com>
 <1456748580-10519-3-git-send-email-shreyas@linux.vnet.ibm.com>
 <20160317111534.GD28728@fergus.ozlabs.ibm.com>
 <56EC1664.8020905@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <56EC1664.8020905@linux.vnet.ibm.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1702
Lines: 34

On Fri, Mar 18, 2016 at 08:23:24PM +0530, Shreyas B Prabhu wrote:
> Hi Paul,
> 
> On 03/17/2016 04:45 PM, Paul Mackerras wrote:
> > 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. 
> 
> This patch was in preparation for support for new POWER ISA v3 idle
> states. The idea was to have the common idle preparation steps in a
> macro which be reused while adding support for the new idle states. With
> this context do you think this macro with better comments make sense?

No, it still does too many disparate things.  In particular it's a bad
idea to embed a blr inside a macro unless the name makes it very clear
that the macro can cause a return (e.g. the macro name is
RETURN_IF_<something>).  Yours would need to be called
MAKE_STACK_FRAME_AND_SAVE_SPRS_AND_HARD_DISABLE_AND_RETURN_IF_IRQ_OCCURRED
or something. :)

Paul.