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

[Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>]

* Peter Zijlstra <peterz@infradead.org> [2009-08-28 09:01:12]:

> On Fri, 2009-08-28 at 08:48 +0200, Peter Zijlstra wrote:
> > 
> > > 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?
> 
> Argh, now I see, it installs itself as the platform idle handler.
> 
> so cpuidle_install_idle_handler() pokes at the unmanaged pm_idle pointer
> to make cpuidle take control.
> 
> On module load it does:
> 
>  pm_idle_old = pm_idle;
> 
> then in the actual idle loop it does:
> 
>         if (!dev || !dev->enabled) {
>                 if (pm_idle_old)
>                         pm_idle_old();
> 
> who is to say that the pointer stored at module init time is still
> around at that time?
> 
> So cpuidle recognised the pm_idle stuff was a flaky, but instead of
> fixing it, they build a whole new layer on top of it. Brilliant.
> 
> /me goes mark this whole thread read, I've got enough things to do.

Hi Peter,

I understand that you are frustrated with the mess.  We are willing
to clean up the pm_idle pointer at the moment before the cpuidle
framework is exploited my more archs.

At this moment we need your suggestions on what interface should we
call 'clean' and safe.

cpuidle.c and the arch independent cpuidle subsystem is not a module
and its cpuidle_idle_call() routine is valid and can be safely called
from arch dependent process.c

The fragile part is how cpuidle_idle_call() is hooked onto arch
specific cpu_idle() function at runtime.  x86 has the pm_idle pointer
exported while powerpc has ppc_md.power_save pointer being called.

At cpuidle init time we can override the platform idle function, but
that will mean we are including arch specific code in cpuidle.c

Do you think having an exported function in cpuidle.c to give us the
correct pointer to arch code be better than the current situation:

in drivers/cpuidle/cpuidle.c

void (*get_cpuidle_handler(void))(void)
{
        return cpuidle_pm_idle;
}
EXPORT_SYMBOL(get_cpuidle_handler);

and from pseries/processor_idle.c,

ppc_md.power_save = get_cpuidle_handler();

--Vaidy