| Message ID | 20090901113840.GH7599@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
* 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 Singh <balbir@linux.vnet.ibm.com> [2009-09-01 22:58:25]: > * 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. > Hi Balbir, Yes, your understanding is correct. Currently, x86 exports pm_idle and this pm_idle is set to cpuidle_idle_call inside cpuidle.c So instead of that x86 should just export a function called set_arch_idle() which will be called from within register_idle_function() and set pm_idle to the idle handler which is currently being registered. I have implemented this for pseries, and in the process of doing it for x86 too. > > 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? > Yes, you are right. I have missed out on this part. register/unregister_idle_function should replace install/uninstall_idle_handler at those places. Thanks. > > > @@ -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. > I don't intend to extend the meaning of idle_function_registered. Will use boolean here. > > +} > > /** > > * 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 Thanks for the review! --arun
On Tue, 2009-09-01 at 17:08 +0530, Arun R Bharadwaj wrote: > * 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. Right, and instead of fixing that, they build this cpuidle crap on top, instead of replacing the current crap with it. > 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. OK, that's a start I guess. Best would be to replace all of pm_idle with cpuidle, which is what should have been done from the very start. If cpuidle cannot fully replace the pm_idle functionality, then it needs to fix that. But having two layers of idle functions is just silly. Looking at patch 2 and 3, you're making the same mistake on power, after those patches there are multiple ways of registering idle functions, one through some native interface and one through cpuidle, this strikes me as undesirable. If cpuidle is a good idle function manager, then it should be good enough to be the sole one, if its not, then why bother with it at all.
* Balbir Singh <balbir@linux.vnet.ibm.com> [2009-09-01 22:58:25]: > * 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? > Just observed the use case for cpuidle_pause_and_lock/unlock. It is not clear as to why we need to switch back to the old idle handler and then again back to cpuidle's idle handler. Wouldn't it make more sense to just register the idle handler when the first cpuidle device is being registered and unregister the idle handler when the last cpuidle device is unregistered? --arun > > > @@ -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
* Peter Zijlstra <a.p.zijlstra@chello.nl> [2009-09-02 07:42:24]: > On Tue, 2009-09-01 at 17:08 +0530, Arun R Bharadwaj wrote: > > * 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. > > Right, and instead of fixing that, they build this cpuidle crap on top, > instead of replacing the current crap with it. > > > 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. > > OK, that's a start I guess. Best would be to replace all of pm_idle with > cpuidle, which is what should have been done from the very start. > > If cpuidle cannot fully replace the pm_idle functionality, then it needs > to fix that. But having two layers of idle functions is just silly. > > Looking at patch 2 and 3, you're making the same mistake on power, after > those patches there are multiple ways of registering idle functions, one > through some native interface and one through cpuidle, this strikes me > as undesirable. > > If cpuidle is a good idle function manager, then it should be good > enough to be the sole one, if its not, then why bother with it at all. > Okay, I'm giving this approach a shot now. i.e. trying to make cpuidle as _the_ sole idle function manager. This would mean doing away with pm_idle and ppc_md.power_save. And, cpuidle_idle_call() which is the main idle loop of cpuidle, present in drivers/cpuidle/cpuidle.c will have to be called from arch specific code of cpu_idle() Also this would mean enabling cpuidle for all platforms, even if the platform doesn't have multiple idle states. So suppose a platform doesnt have multiple states, it wouldn't want the bloated code of cpuidle governors, and would want just a simple cpuidle loop. --arun >
On Thu, 2009-09-03 at 10:12 +0530, Arun R Bharadwaj wrote: > > OK, that's a start I guess. Best would be to replace all of pm_idle with > > cpuidle, which is what should have been done from the very start. > > > > If cpuidle cannot fully replace the pm_idle functionality, then it needs > > to fix that. But having two layers of idle functions is just silly. > > > > Looking at patch 2 and 3, you're making the same mistake on power, after > > those patches there are multiple ways of registering idle functions, one > > through some native interface and one through cpuidle, this strikes me > > as undesirable. > > > > If cpuidle is a good idle function manager, then it should be good > > enough to be the sole one, if its not, then why bother with it at all. > > > > Okay, I'm giving this approach a shot now. i.e. trying to make cpuidle > as _the_ sole idle function manager. This would mean doing away with > pm_idle and ppc_md.power_save. And, cpuidle_idle_call() which is the > main idle loop of cpuidle, present in drivers/cpuidle/cpuidle.c will > have to be called from arch specific code of cpu_idle() > > Also this would mean enabling cpuidle for all platforms, even if the > platform doesn't have multiple idle states. So suppose a platform doesnt > have multiple states, it wouldn't want the bloated code of cpuidle > governors, and would want just a simple cpuidle loop. Do talk to the powerpc maintainers about this. But yes, something like that should be doable. AFAICT the whole governor thing is optional and cpuidle provides a spinning idle loop by default, and platforms can always register a simple alternative when they set up bits -- the only thing to be careful about is not creating a chicken-egg problem where the platform setup runs before cpuidle is able to register a new handler or something. I'd be delighted to see the end of pm_idle on x86.
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); } @@ -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; +} /** * 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); }
* 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. 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(-)