On 07/07/16 15:49, Rafael J. Wysocki wrote:

On Thursday, July 07, 2016 02:34:36 PM Sudeep Holla wrote:

quoted

On 07/07/16 14:21, Rafael J. Wysocki wrote:

quoted

On Tuesday, June 28, 2016 02:55:50 PM Sudeep Holla wrote:

quoted

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" <redacted>
Cc: Daniel Lezcano <redacted>
Cc: Lorenzo Pieralisi <redacted>
Signed-off-by: Sudeep Holla <redacted>
---
   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. :-)

I know and I am all for getting rid of that.

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.

Sounds good. Thanks for the suggestion. I will respin the series with
this change then.

-- 
Regards,
Sudeep