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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <1251344649.20467.13.camel@pasglop>
Date:	Thu, 27 Aug 2009 13:44:09 +1000
From:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	arun@...ux.vnet.ibm.com, Joel Schopp <jschopp@...tin.ibm.com>,
	Paul Mackerras <paulus@...ba.org>, Ingo Molnar <mingo@...e.hu>,
	Vaidyanathan Srinivasan <svaidy@...ux.vnet.ibm.com>,
	Dipankar Sarma <dipankar@...ibm.com>,
	Balbir Singh <balbir@...ux.vnet.ibm.com>,
	Gautham R Shenoy <ego@...ibm.com>,
	linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [v2 PATCH 2/2]: pseries: Implement Pseries Processor Idle idle
 module.

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/