Re: [PATCH v7 4/6] cpuidle: introduce HAVE_GENERIC_CPUIDLE_ENTER for ARM{32,64} platforms

[Rafael J. Wysocki]

On Thursday, July 07, 2016 02:34:36 PM Sudeep Holla wrote:
> 
> On 07/07/16 14:21, Rafael J. Wysocki wrote:
> > On Tuesday, June 28, 2016 02:55:50 PM Sudeep Holla wrote:
> >> The function arm_enter_idle_state is exactly the same in both generic
> >> ARM{32,64} CPUIdle driver and will be the same even on ARM64 backend
> >> for ACPI processor idle driver. So we can unify it and move it as
> >> generic_cpuidle_enter by introducing HAVE_GENERIC_CPUIDLE_ENTER and
> >> enabling the same on both ARM{32,64}.
> >>
> >> This is in preparation of reuse of the generic cpuidle entry function
> >> for ACPI LPI support on ARM64.
> >>
> >> Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> >> Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> >> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
> >> ---
> >>   arch/arm/Kconfig              |  1 +
> >>   arch/arm/kernel/cpuidle.c     |  4 ++--
> >>   arch/arm64/Kconfig            |  1 +
> >>   arch/arm64/kernel/cpuidle.c   |  6 +++---
> >>   drivers/cpuidle/Kconfig       |  3 +++
> >>   drivers/cpuidle/cpuidle-arm.c | 21 +--------------------
> >>   drivers/cpuidle/cpuidle.c     | 35 +++++++++++++++++++++++++++++++++++
> >>   include/linux/cpuidle.h       |  8 ++++++++
> >>   8 files changed, 54 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >> index 90542db1220d..52b3dca0381c 100644
> >> --- a/arch/arm/Kconfig
> >> +++ b/arch/arm/Kconfig
> >> @@ -54,6 +54,7 @@ config ARM
> >>   	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
> >>   	select HAVE_FUNCTION_GRAPH_TRACER if (!THUMB2_KERNEL)
> >>   	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
> >> +	select HAVE_GENERIC_CPUIDLE_ENTER
> >
> > That "generic" part in the name concerns me a bit, because the thing is not
> > really generic.  It is "common on ARM" rather.
> >
> 
> I agree and that's exactly what I told Daniel. It's rather just
> *ARM Generic*. Any preference on the name ? I had it header file under
> include/linu/cpuidle-arm.h in the previous version. Do you prefer that ?

Well, I got confused by these names which probably means that they really
are confusing. :-)

So the underlying observation is that ->enter() callbacks in some ARM code
tend to do the same thing, ie. wrap the cpu_pm_enter()/exit() pair around
the actual "low-level enter" routine, so the idea is to move the wrapping
to the core and add the symbol plus standard header for the "low-level enter"
thing.

But then ->enter has to point to the wrapper and that just invokes a static
function defined somewhere.

So in fact what you want is to avoid code duplication in the source, but not
in the binary.

For that, I'd use a macro like this:

#define CPU_IDLE_ENTER_WRAPPED(low_level_idle_enter, idx)	\
({								\
	int __ret;						\
								\
	if (!idx) {						\
		cpu_do_idle();					\
		return idx;					\
	}							\
								\
	__ret = cpu_pm_enter();					\
	if (!__ret) {						\
		__ret = low_level_idle_enter(idx);		\
		cpu_pm_exit();					\
	}							\
								\
	__ret ? -1 : idx;					\
})

and then, whoever want's to generate a "wrapped" callback, will need to
define the low_level_idle_enter thing, say my_low_level_idle_enter() and
then do

int idle_enter(int idx)
{
	return CPU_IDLE_ENTER_WRAPPED(my_low_level_idle_enter, idx);
}

and point the ->enter callback to idle_enter().

No need for extra symbols, confusing function names and similar.

And the macro can go into cpuidle.h if you want.

Thanks,
Rafael