Re: [v2 PATCH 2/2]: pseries: Implement Pseries Processor Idle idle module.

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2009-08-26 at 13:27 +0200, Peter Zijlstra wrote:
> On Wed, 2009-08-26 at 16:40 +0530, Arun R Bharadwaj wrote:
> > +void (*pm_idle)(void);
> > +EXPORT_SYMBOL_GPL(pm_idle);
> 
> Seriously.. this caused plenty problems over on x86 and you're doing the
> exact same dumb thing?

I already said I didn't want this export.

First thing first: We already have a ppc_md.power_save() callback,
filled by the platform. which implements the "low level" part of idle on
powerpc (ie, the actual putting of the CPU into some kind of wait state)
and is called by our idle loop.

We -also- have a higher level ppc_md.idle_loop() which allows the
platform to completely override the idle loop, though we rarely do it (I
think only iSeries does it nowadays).

So I see no need to -add- another callback here. I'm not entirely sure
what the cpuidle framework does, it's no obvious from Arun commit
messages I must say, but it doesn't look like the right approach for
integration. In fact, pSeries already have a choice between different
powersave models depending on what kind of hypervisor is there.

So Arun, please try to fit nicely within the existing interfaces, or if
you want to add a new one, please justify very precisely what design
decisions lead you to that.

Finally, there's also a problem with your first patch 1/2: You are
setting a Kconfig flag unconditionally indicating that the arch supports
some idle wait function, but you only implement it somewhere in
arch/powerpc/platform/pseries, so you'll break the build of any other
platform. You also prevent another platform to implement a different one
and be built in the same kernel.

For such generic callbacks, if it's justified (and only if it is), you
can add a ppc_md. hook for the platform to fill, and you need to cater
for platforms that don't.

Cheers,
Ben.