* Peter Zijlstra [off-list ref] [2009-08-27 14:53:27]:

On Thu, 2009-08-27 at 17:23 +0530, Arun R Bharadwaj wrote:

quoted

* Arun R Bharadwaj [off-list ref] [2009-08-27 17:19:08]:

Cpuidle infrastructure assumes pm_idle as the default idle routine.
But, ppc_md.power_save is the default idle callback in case of pSeries.

So, create a more generic, architecture independent cpuidle_pm_idle
function pointer in driver/cpuidle/cpuidle.c and allow the idle routines
of architectures to be set to cpuidle_pm_idle.

Signed-off-by: Arun R Bharadwaj <redacted>
---
 drivers/cpuidle/cpuidle.c |   12 +++++++-----
 include/linux/cpuidle.h   |    7 +++++++
 2 files changed, 14 insertions(+), 5 deletions(-)

Index: linux.trees.git/drivers/cpuidle/cpuidle.c
===================================================================

--- linux.trees.git.orig/drivers/cpuidle/cpuidle.c
+++ linux.trees.git/drivers/cpuidle/cpuidle.c

@@ -25,6 +25,7 @@ DEFINE_PER_CPU(struct cpuidle_device *, 
 DEFINE_MUTEX(cpuidle_lock);
 LIST_HEAD(cpuidle_detected_devices);
 static void (*pm_idle_old)(void);
+void (*cpuidle_pm_idle)(void);
 
 static int enabled_devices;
 

@@ -98,10 +99,10 @@ static void cpuidle_idle_call(void)
  */
 void cpuidle_install_idle_handler(void)
 {
-	if (enabled_devices && (pm_idle != cpuidle_idle_call)) {
+	if (enabled_devices && (cpuidle_pm_idle != cpuidle_idle_call)) {
 		/* Make sure all changes finished before we switch to new idle */
 		smp_wmb();
-		pm_idle = cpuidle_idle_call;
+		cpuidle_pm_idle = cpuidle_idle_call;
 	}
 }
 

@@ -110,8 +111,9 @@ void cpuidle_install_idle_handler(void)
  */
 void cpuidle_uninstall_idle_handler(void)
 {
-	if (enabled_devices && pm_idle_old && (pm_idle != pm_idle_old)) {
-		pm_idle = pm_idle_old;
+	if (enabled_devices && pm_idle_old &&
+			(cpuidle_pm_idle != pm_idle_old)) {
+		cpuidle_pm_idle = pm_idle_old;
 		cpuidle_kick_cpus();
 	}
 }

@@ -382,7 +384,7 @@ static int __init cpuidle_init(void)
 {
 	int ret;
 
-	pm_idle_old = pm_idle;
+	pm_idle_old = cpuidle_pm_idle;
 
 	ret = cpuidle_add_class_sysfs(&cpu_sysdev_class);
 	if (ret)

Index: linux.trees.git/include/linux/cpuidle.h
===================================================================

--- linux.trees.git.orig/include/linux/cpuidle.h
+++ linux.trees.git/include/linux/cpuidle.h

@@ -188,4 +188,11 @@ static inline void cpuidle_unregister_go
 #define CPUIDLE_DRIVER_STATE_START	0
 #endif
 
+/*
+ * Idle callback used by cpuidle to call the cpuidle_idle_call().
+ * Platform drivers can use this to register to cpuidle's idle loop.
+ */
+
+extern void (*cpuidle_pm_idle)(void);
+
 #endif /* _LINUX_CPUIDLE_H */

I'm not quite seeing how this makes anything any better. Not we have 3
function pointers, where 1 should suffice.

Not really. We already do have pm_idle in case of x86 and
ppc_md.power_save in case of POWER. So here I'm only introducing
cpuidle_pm_idle which can be used by doing a

ppc_md.power_save = cpuidle_pm_idle;

/me wonders what's wrong with something like:

struct idle_func_desc {
	int		 power;
	int		 latency;
	void		 (*idle)(void);
	struct list_head list;
};

static void spin_idle(void)
{
	for (;;)
		cpu_relax();
}

static idle_func_desc default_idle_func = {
	power = 0, 	   /* doesn't safe any power */
	latency = INT_MAX, /* has max latency */
	idle = spin_idle,
	list = INIT_LIST_HEAD(default_idle_func.list),
};

void (*idle_func)(void);
static struct list_head idle_func_list;

static void pick_idle_func(void)
{
	struct idle_func_desc *desc, *idle = &default_idle_desc;

	list_for_each_entry(desc, &idle_func_list, list) {
		if (desc->power < idle->power)
			continue;
		if (desc->latency > target_latency);
			continue;
		idle = desc;
	}

	pm_idle = idle->idle;
}

This only does the job of picking the right idle loop for current
latency and power requirement. This is already done in ladder/menu
governors under the routines menu_select()/ladder_select().
I'm not sure whats the purpose of it here.

Here we are only concerned about the main idle loop, which is
pm_idle/ppc_md.power_save. After setting the main idle loop to
cpuidle_pm_idle, that would call cpuidle_idle_call() which would do
the job of picking the right low level idle loop based on latency and
other requirements.

void register_idle_func(struct idle_func_desc *desc)
{
	WARN_ON_ONCE(!list_empty(&desc->list));

	list_add_tail(&idle_func_list, &desc->list);
	pick_idle_func();
}

void unregister_idle_func(struct idle_func_desc *desc)
{
	WARN_ON_ONCE(list_empty(&desc->list));

	list_del_init(&desc->list);
	if (idle_func == desc->idle) 
		pick_idle_func();
}