On 03/30/2012 04:18 PM, Daniel Lezcano wrote:
> The usual cpuidle initialization routines are to register the
> driver, then register a cpuidle device per cpu.
>
> With the device's state count default initialization with the
> driver's state count, the code initialization remains mostly the
> same in the different drivers.
>
> We can then add a new function 'cpuidle_register' where we register
> the driver and the devices. These devices can be defined in a global
> static variable in cpuidle.c. We will be able to factor out and
> remove a lot of duplicate lines of code.
>
> As we still have some drivers, with different initialization routines,
> we keep 'cpuidle_register_driver' and 'cpuidle_register_device' as low
> level initialization routines to do some specific operations on the
> cpuidle devices.
>
> Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
> ---
> drivers/cpuidle/cpuidle.c | 34 ++++++++++++++++++++++++++++++++++
> include/linux/cpuidle.h | 3 +++
> 2 files changed, 37 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index b8a1faf..2a174e8 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -23,6 +23,7 @@
> #include "cpuidle.h"
>
> DEFINE_PER_CPU(struct cpuidle_device *, cpuidle_devices);
> +DEFINE_PER_CPU(struct cpuidle_device, cpuidle_device);
>
> DEFINE_MUTEX(cpuidle_lock);
> LIST_HEAD(cpuidle_detected_devices);
> @@ -391,6 +392,39 @@ int cpuidle_register_device(struct cpuidle_device *dev)
>
> EXPORT_SYMBOL_GPL(cpuidle_register_device);
>
> +int cpuidle_register(struct cpuidle_driver *drv)
> +{
> + int ret, cpu;
> + struct cpuidle_device *dev;
> +
> + ret = cpuidle_register_driver(drv);
> + if (ret)
> + return ret;
> +
> + for_each_online_cpu(cpu) {
> + dev = &per_cpu(cpuidle_device, cpu);
> + dev->cpu = cpu;
> +
> + ret = cpuidle_register_device(dev);
> + if (ret)
> + goto out_unregister;
> + }
> +
Isn't this racy with respect to CPU hotplug?
> +out:
> + return ret;
> +
> +out_unregister:
> + for_each_online_cpu(cpu) {
> + dev = &per_cpu(cpuidle_device, cpu);
> + cpuidle_unregister_device(dev);
> + }
> +
This could be improved I guess.. What if the registration fails
for the first cpu itself? Then looping over entire online cpumask
would be a waste of time..
Here is a discussion on some very similar code:
https://lkml.org/lkml/2012/3/22/72
https://lkml.org/lkml/2012/3/22/143
> + cpuidle_unregister_driver(drv);
> +
> + goto out;
> +}
> +EXPORT_SYMBOL_GPL(cpuidle_register);
> +
> /**
> * cpuidle_unregister_device - unregisters a CPU's idle PM feature
> * @dev: the cpu
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index f3ebbba..17e3d33 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -133,6 +133,7 @@ struct cpuidle_driver {
> #ifdef CONFIG_CPU_IDLE
> extern void disable_cpuidle(void);
> extern int cpuidle_idle_call(void);
> +extern int cpuidle_register(struct cpuidle_driver *drv);
> extern int cpuidle_register_driver(struct cpuidle_driver *drv);
> struct cpuidle_driver *cpuidle_get_driver(void);
> extern void cpuidle_unregister_driver(struct cpuidle_driver *drv);
> @@ -150,6 +151,8 @@ extern int cpuidle_wrap_enter(struct cpuidle_device *dev,
> #else
> static inline void disable_cpuidle(void) { }
> static inline int cpuidle_idle_call(void) { return -ENODEV; }
> +static inline int cpuidle_register(struct cpuidle_driver *drv)
> +{return -ENODEV; }
> static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
> {return -ENODEV; }
> static inline struct cpuidle_driver *cpuidle_get_driver(void) {return NULL; }
Regards,
Srivatsa S. Bhat
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
