Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1361837

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

Show all headers | View raw

On 03/19/2016 05:51 AM, Paul Mackerras wrote:
> 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. :)
> 

Ok :) . I'll drop this patch and work this differently.

Thanks,
Shreyas

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

Re: [PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps  in a macro Paul Mackerras <paulus@ozlabs.org> - 2016-03-17 22:30 +0100
  Re: [PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps  in a macro Shreyas B Prabhu <shreyas@linux.vnet.ibm.com> - 2016-03-18 16:00 +0100
    Re: [PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps  in a macro Paul Mackerras <paulus@ozlabs.org> - 2016-03-19 01:30 +0100
      Re: [PATCH 2/3] powerpc/powernv: Encapsulate idle preparation steps  in a macro Shreyas B Prabhu <shreyas@linux.vnet.ibm.com> - 2016-03-21 14:30 +0100

csiph-web