Re: [PATCH v2 1/5] cpu_pm: Add cpu power management notifiers

[Andrew Morton <akpm@linux-foundation.org>]

On Sat, 3 Sep 2011 20:09:11 +0530
Santosh Shilimkar <santosh.shilimkar@ti.com> wrote:

> From: Colin Cross <ccross@android.com>
> 
> During some CPU power modes entered during idle, hotplug and
> suspend, peripherals located in the CPU power domain, such as
> the GIC, localtimers, and VFP, may be powered down.  Add a
> notifier chain that allows drivers for those peripherals to
> be notified before and after they may be reset.

Have you identified which indivudual you hope/expect to merge this into
mainline?

The code is presumably and hopefully applicable to architectures other
than ARM, yes?  Can you suggest likely candidate architectures so we
can go off and bug the relevant maintainers to review it?

>
> ...
>
> +/*
> + * When a CPU goes to a low power state that turns off power to the CPU's
> + * power domain, the contents of some blocks (floating point coprocessors,
> + * interrutp controllers, caches, timers) in the same power domain can

s/interrutp/interrupt/

> + * be lost.  The cpm_pm notifiers provide a method for platform idle, suspend,
> + * and hotplug implementations to notify the drivers for these blocks that
> + * they may be reset.
> + *
> + * All cpu_pm notifications must be called with interrupts disabled.
> + *
> + * The notifications are split into two classes, CPU notifications and CPU

s/,/:/

> + * cluster notifications.
> + *
> + * CPU notifications apply to a single CPU, and must be called on the affected

s/,// ;)

> + * CPU.  They are used to save per-cpu context for affected blocks.
> + *
> + * CPU cluster notifications apply to all CPUs in a single power domain. They
> + * are used to save any global context for affected blocks, and must be called
> + * after all the CPUs in the power domain have been notified of the low power
> + * state.
> + *

Remove this line.

> + */
> +
>
> ...
>
> +/*
> + * cpm_pm_enter
> + *
> + * Notifies listeners that a single cpu is entering a low power state that may
> + * cause some blocks in the same power domain as the cpu to reset.
> + *
> + * Must be called on the affected cpu with interrupts disabled.  Platform is
> + * responsible for ensuring that cpu_pm_enter is not called twice on the same
> + * cpu before cpu_pm_exit is called.
> + */

It's unconventional to put the documentation over the declarations in the
.h file.  It's not a *bad* idea per-se, but we generally don't do it. 
People will look at the definition in .c for the documentation and it
if isn't there, some will assume that documentation doesn't exist.

Plus: I don't know about others, but I don't configure ctags to lead me
to declarations.  So finding the documentation for cpm_pm_enter() is a
single keystroke if it's in the .c file, and a big PITA if it is in the
.h file.

Also, this documentation could trivially be converted into kerneldoc
format - you may as well do this?

> +int cpu_pm_enter(void);

An actual design question: the interface assumes that CPU PM is a
boolean state: on or off.  "a CPU goes to a low power state that turns
off power to the CPU's power domain".

Will that always be true for all CPUs?  Or should the interface have
the capability of notifying clients of multi-level power state
transitions?

> +
> +/*
> + * cpm_pm_exit
> + *
> + * Notifies listeners that a single cpu is exiting a low power state that may
> + * have caused some blocks in the same power domain as the cpu to reset.
> + *
> + * Must be called on the affected cpu with interrupts disabled.

It's unobvious (to little old me) why all these things need to be
called under local_irq_disable().  I suggest the addition of a code
comment and changelog update so that others are not similarly
mystified.

> + */
> +int cpu_pm_exit(void);
>
> ...
>
> +int cpu_cluster_pm_enter(void)
> +{
> +	int nr_calls;
> +	int ret = 0;
> +
> +	read_lock(&cpu_pm_notifier_lock);
> +	ret = cpu_pm_notify(CPU_CLUSTER_PM_ENTER, -1, &nr_calls);
> +	if (ret)
> +		cpu_pm_notify(CPU_CLUSTER_PM_ENTER_FAILED, nr_calls - 1, NULL);

What's going on with nr_calls?  Avoiding calling the most recently
registered callback?  It is unclear why.  Some explanation here would
be good.

> +	read_unlock(&cpu_pm_notifier_lock);
> +
> +	return ret;
> +}
>
> ...
>
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -235,3 +235,7 @@ config PM_GENERIC_DOMAINS
>  config PM_GENERIC_DOMAINS_RUNTIME
>  	def_bool y
>  	depends on PM_RUNTIME && PM_GENERIC_DOMAINS
> +
> +config CPU_PM
> +	def_bool y
> +	depends on SUSPEND || CPU_IDLE

This will unconditionally include kernel/cpu_pm.o in x86 kernels, and
it's all dead code.  Fix, please!