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    [W:0.061 / U:30.324 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site