Re: [PATCH 2/4]: CPUIDLE: Introduce architecture independent cpuidle_pm_idle in drivers/cpuidle/cpuidle.c

[Balbir Singh <balbir@linux.vnet.ibm.com>]

* Peter Zijlstra <a.p.zijlstra@chello.nl> [2009-08-28 08:48:05]:

> On Fri, 2009-08-28 at 11:44 +0530, Arun R Bharadwaj wrote:
> > * Peter Zijlstra <a.p.zijlstra@chello.nl> [2009-08-27 14:53:27]:
> > 
> > Hi Peter, Ben,
> > 
> > I've put the whole thing in a sort of a block diagram. Hope it
> > explains things more clearly.
> > 
> > 
> > 
> > 
> > 
> > 
> > 				----------------
> > 				|    CPUIDLE   |  (Select idle states like
> > 				|   GOVERNORS  |   C1, C1e, C6 etc in case
> > 				| (Menu/Ladder)|   x86 & nap, snooze in
> > 				|	       |   case of POWER - based on
> > 				----------------   latency & power req)
> > 					^
> > 					|
> > 					|
> > 					|
> > 					|
> > 					|
> >   ----------			-----------------	         -------------
> >   |	   |			|		|	         |  PSERIES  |
> >   |  ACPI  |------------------>	|    CPUIDLE	| <--------------|   IDLE    |
> >   |	   |			|		|	         |           |
> >   ----------			-----------------	         -------------
> > 
> > Main idle routine- pm_idle()			         Main idle routine-
> > 							     ppc_md.power_save()
> > 
> > pm_idle = cpuidle_pm_idle;			         ppc_md.power_save =
> > (start using cpuidle's idle				      cpuidle_pm_idle();
> >  loop, which internally calls
> >  governor to select the right
> >  state to go into).
> > 
> > 
> > Relavent code snippet from drivers/cpuidle/cpuidle.c
> > -------------------------------------
> > 
> > static void cpuidle_idle_call(void)
> > {
> > 	............
> > 	............
> > 
> > 	/* Call the menu_select() to select the idle state to enter. */
> > 	next_state = cpuidle_curr_governor->select(dev);
> > 
> > 	............
> > 	............
> > 
> > 	/*
> > 	 * Enter the idle state previously selected. target_state->enter
> > 	 * would call pseries_cpuidle_loop() which selects nap/snooze
> > 	 * /
> > 	dev->last_residency = target_state->enter(dev, target_state);
> > }
> > 
> > void cpuidle_install_idle_handler(void)
> > {
> > 	.........
> > 	.........
> > 	cpuidle_pm_idle = cpuidle_idle_call;
> > }
> 
> All I'm seeing here is a frigging mess.
> 
> How on earths can something called: cpuidle_install_idle_handler() have
> a void argument, _WHAT_ handler is it going to install?
>

Peter, I think that is a typo, we need a function pointer like your
snippet of code showed
 
> So somehow you added to the ACPI mess by now having 3 wild function
> pointers, that's _NOT_ progress.
>

The goal, IIUC is to integrate three modules

1. CPUIdle - for idle CPU management
2. Architecture specific code for idle detection
3. CPUIdle governor

Again, if IIUC

The architecture specific idle code would call an idle loop that would
call into the CPUIdle framework, which inturn would depend on the
governor selected.

Passing void* is a mess, we need function pointer registration
and a framework so that we can query what is registered.


 

-- 
	Balbir