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. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
