lkml.org 
[lkml]   [2009]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [v4 PATCH 1/5]: cpuidle: Cleanup drivers/cpuidle/cpuidle.c
* Arun R B <arun@linux.vnet.ibm.com> [2009-09-01 17:08:40]:

> * Arun R Bharadwaj <arun@linux.vnet.ibm.com> [2009-09-01 17:07:04]:
>
> Cleanup drivers/cpuidle/cpuidle.c
>
> Cpuidle maintains a pm_idle_old void pointer because, currently in x86
> there is no clean way of registering and unregistering a idle function.
>
> So remove pm_idle_old and leave the responsibility of maintaining the
> list of registered idle loops to the architecture specific code. If the
> architecture registers cpuidle_idle_call as its idle loop, only then
> this loop is called.
>

It sounds as if there is a side-effect of this
patch on x86 (am I reading it incorrectly), which can be fixed, but
it will need a patch or so to get back the old behaviour on x86.

> Also remove unwanted functions cpuidle_[un]install_idle_handler,
> cpuidle_kick_cpus()
>
> Signed-off-by: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
> ---
> drivers/cpuidle/cpuidle.c | 51 +++++++++++++++------------------------------
> drivers/cpuidle/governor.c | 3 --
> 2 files changed, 17 insertions(+), 37 deletions(-)
>
> Index: linux.trees.git/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- linux.trees.git.orig/drivers/cpuidle/cpuidle.c
> +++ linux.trees.git/drivers/cpuidle/cpuidle.c
> @@ -24,9 +24,14 @@ DEFINE_PER_CPU(struct cpuidle_device *,
>
> DEFINE_MUTEX(cpuidle_lock);
> LIST_HEAD(cpuidle_detected_devices);
> -static void (*pm_idle_old)(void);
>
> static int enabled_devices;
> +static int idle_function_registered;
> +
> +struct idle_function_desc cpuidle_idle_desc = {
> + .name = "cpuidle_loop",
> + .idle_func = cpuidle_idle_call,
> +};
>
> #if defined(CONFIG_ARCH_HAS_CPU_IDLE_WAIT)
> static void cpuidle_kick_cpus(void)
> @@ -54,13 +59,10 @@ static void cpuidle_idle_call(void)
>
> /* check if the device is ready */
> if (!dev || !dev->enabled) {
> - if (pm_idle_old)
> - pm_idle_old();
> - else
> #if defined(CONFIG_ARCH_HAS_DEFAULT_IDLE)
> - default_idle();
> + default_idle();
> #else
> - local_irq_enable();
> + local_irq_enable();
> #endif
> return;
> }
> @@ -94,35 +96,11 @@ static void cpuidle_idle_call(void)
> }
>
> /**
> - * cpuidle_install_idle_handler - installs the cpuidle idle loop handler
> - */
> -void cpuidle_install_idle_handler(void)
> -{
> - if (enabled_devices && (pm_idle != cpuidle_idle_call)) {
> - /* Make sure all changes finished before we switch to new idle */
> - smp_wmb();
> - pm_idle = cpuidle_idle_call;
> - }
> -}
> -
> -/**
> - * cpuidle_uninstall_idle_handler - uninstalls the cpuidle idle loop handler
> - */
> -void cpuidle_uninstall_idle_handler(void)
> -{
> - if (enabled_devices && pm_idle_old && (pm_idle != pm_idle_old)) {
> - pm_idle = pm_idle_old;
> - cpuidle_kick_cpus();
> - }
> -}
> -
> -/**
> * cpuidle_pause_and_lock - temporarily disables CPUIDLE
> */
> void cpuidle_pause_and_lock(void)
> {
> mutex_lock(&cpuidle_lock);
> - cpuidle_uninstall_idle_handler();
> }
>
> EXPORT_SYMBOL_GPL(cpuidle_pause_and_lock);
> @@ -132,7 +110,6 @@ EXPORT_SYMBOL_GPL(cpuidle_pause_and_lock
> */
> void cpuidle_resume_and_unlock(void)
> {
> - cpuidle_install_idle_handler();
> mutex_unlock(&cpuidle_lock);
> }
>

What does this mean for users of cpuidle_pause_and_lock/unlock?
Should we be calling register/unregister_idle_function here?


> @@ -287,6 +264,12 @@ static int __cpuidle_register_device(str
> return 0;
> }
>
> +static void register_cpuidle_idle_function(void)
> +{
> + register_idle_function(&cpuidle_idle_desc);
> +
> + idle_function_registered = 1;

Use booleans if possible, unless you intend to extend the meaning of
registered someday.

> +}
> /**
> * cpuidle_register_device - registers a CPU's idle PM feature
> * @dev: the cpu
> @@ -303,7 +286,9 @@ int cpuidle_register_device(struct cpuid
> }
>
> cpuidle_enable_device(dev);
> - cpuidle_install_idle_handler();
> +
> + if (!idle_function_registered)
> + register_cpuidle_idle_function();
>
> mutex_unlock(&cpuidle_lock);
>
> @@ -382,8 +367,6 @@ static int __init cpuidle_init(void)
> {
> int ret;
>
> - pm_idle_old = pm_idle;
> -
> ret = cpuidle_add_class_sysfs(&cpu_sysdev_class);
> if (ret)
> return ret;
> Index: linux.trees.git/drivers/cpuidle/governor.c
> ===================================================================
> --- linux.trees.git.orig/drivers/cpuidle/governor.c
> +++ linux.trees.git/drivers/cpuidle/governor.c
> @@ -48,8 +48,6 @@ int cpuidle_switch_governor(struct cpuid
> if (gov == cpuidle_curr_governor)
> return 0;
>
> - cpuidle_uninstall_idle_handler();
> -
> if (cpuidle_curr_governor) {
> list_for_each_entry(dev, &cpuidle_detected_devices, device_list)
> cpuidle_disable_device(dev);
> @@ -63,7 +61,6 @@ int cpuidle_switch_governor(struct cpuid
> return -EINVAL;
> list_for_each_entry(dev, &cpuidle_detected_devices, device_list)
> cpuidle_enable_device(dev);
> - cpuidle_install_idle_handler();
> printk(KERN_INFO "cpuidle: using governor %s\n", gov->name);
> }
>

--
Balbir


\
 
 \ /
  Last update: 2009-09-02 06:17    [from the cache]
©2003-2011 Jasper Spaans