* [PATCH 0/4] cpuidle - support multiple drivers at a time
@ 2012-09-24 22:43 Daniel Lezcano
2012-09-24 22:43 ` [PATCH 1/4] cpuidle : move driver's refcount to cpuidle Daniel Lezcano
` (4 more replies)
0 siblings, 5 replies; 23+ messages in thread
From: Daniel Lezcano @ 2012-09-24 22:43 UTC (permalink / raw)
To: rjw, lenb
Cc: linux-pm, lorenzo.pieralisi, pdeschrijver, linux-acpi, patches,
linaro-dev
The discussion about having different cpus on the system with
different latencies bring us to a first attemp by adding a
pointer in the cpuidle_device to the states array.
But as Rafael suggested, it would make more sense to create a
driver per cpu [1].
This patch adds support for multiple cpuidle drivers.
It creates a per cpu cpuidle driver pointer.
In order to not break the different drivers, the function cpuidle_register_driver
assign for each cpu, the driver.
The multiple driver support is optional and if it is not set, the cpuide driver
core code remains the same (except some code reorganisation).
I did the following tests compiled, booted, tested without/with CONFIG_CPU_IDLE,
with/without CONFIG_CPU_IDLE_MULTIPLE_DRIVERS.
Tested on Core2 Duo T9500 with acpi_idle [and intel_idle]
Tested on ARM Dual Cortex-A9 U8500 (aka Snowball)
[1] http://www.spinics.net/lists/linux-acpi/msg37921.html
Daniel Lezcano (4):
cpuidle : move driver's refcount to cpuidle
cpuidle : move driver checking within the lock section
cpuidle - prepare the driver core to be multi drivers aware
cpuidle - support multiple drivers
drivers/cpuidle/Kconfig | 9 ++
drivers/cpuidle/cpuidle.c | 24 ++++--
drivers/cpuidle/driver.c | 194 ++++++++++++++++++++++++++++++++++++++-------
drivers/cpuidle/sysfs.c | 50 +++++++++---
include/linux/cpuidle.h | 9 ++-
5 files changed, 238 insertions(+), 48 deletions(-)
--
1.7.5.4
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 1/4] cpuidle : move driver's refcount to cpuidle 2012-09-24 22:43 [PATCH 0/4] cpuidle - support multiple drivers at a time Daniel Lezcano @ 2012-09-24 22:43 ` Daniel Lezcano 2012-09-24 22:43 ` [PATCH 2/4] cpuidle : move driver checking within the lock section Daniel Lezcano ` (3 subsequent siblings) 4 siblings, 0 replies; 23+ messages in thread From: Daniel Lezcano @ 2012-09-24 22:43 UTC (permalink / raw) To: rjw, lenb Cc: linux-pm, lorenzo.pieralisi, pdeschrijver, linux-acpi, patches, linaro-dev We want to support different cpuidle drivers co-existing together. In this case we should move the refcount to the cpuidle_driver structure to handle several drivers at a time. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/cpuidle/driver.c | 13 ++++++++----- include/linux/cpuidle.h | 1 + 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 87db387..39ba8e1 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -16,7 +16,6 @@ static struct cpuidle_driver *cpuidle_curr_driver; DEFINE_SPINLOCK(cpuidle_driver_lock); -int cpuidle_driver_refcount; static void set_power_states(struct cpuidle_driver *drv) { @@ -61,6 +60,8 @@ int cpuidle_register_driver(struct cpuidle_driver *drv) if (!drv->power_specified) set_power_states(drv); + drv->refcnt = 0; + cpuidle_curr_driver = drv; spin_unlock(&cpuidle_driver_lock); @@ -92,7 +93,7 @@ void cpuidle_unregister_driver(struct cpuidle_driver *drv) spin_lock(&cpuidle_driver_lock); - if (!WARN_ON(cpuidle_driver_refcount > 0)) + if (!WARN_ON(drv->refcnt > 0)) cpuidle_curr_driver = NULL; spin_unlock(&cpuidle_driver_lock); @@ -106,7 +107,7 @@ struct cpuidle_driver *cpuidle_driver_ref(void) spin_lock(&cpuidle_driver_lock); drv = cpuidle_curr_driver; - cpuidle_driver_refcount++; + drv->refcnt++; spin_unlock(&cpuidle_driver_lock); return drv; @@ -114,10 +115,12 @@ struct cpuidle_driver *cpuidle_driver_ref(void) void cpuidle_driver_unref(void) { + struct cpuidle_driver *drv = cpuidle_curr_driver; + spin_lock(&cpuidle_driver_lock); - if (!WARN_ON(cpuidle_driver_refcount <= 0)) - cpuidle_driver_refcount--; + if (drv && !WARN_ON(drv->refcnt <= 0)) + drv->refcnt--; spin_unlock(&cpuidle_driver_lock); } diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 279b1ea..a4ff9f8 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -131,6 +131,7 @@ static inline int cpuidle_get_last_residency(struct cpuidle_device *dev) struct cpuidle_driver { const char *name; struct module *owner; + int refcnt; unsigned int power_specified:1; /* set to 1 to use the core cpuidle time keeping (for all states). */ -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/4] cpuidle : move driver checking within the lock section 2012-09-24 22:43 [PATCH 0/4] cpuidle - support multiple drivers at a time Daniel Lezcano 2012-09-24 22:43 ` [PATCH 1/4] cpuidle : move driver's refcount to cpuidle Daniel Lezcano @ 2012-09-24 22:43 ` Daniel Lezcano [not found] ` <1348526634-19029-3-git-send-email-daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2012-09-24 22:43 ` [PATCH 3/4] cpuidle - prepare the driver core to be multi drivers aware Daniel Lezcano ` (2 subsequent siblings) 4 siblings, 1 reply; 23+ messages in thread From: Daniel Lezcano @ 2012-09-24 22:43 UTC (permalink / raw) To: rjw, lenb Cc: linux-pm, lorenzo.pieralisi, pdeschrijver, linux-acpi, patches, linaro-dev The code checks if the driver is already set without taking the lock, but, right after, it takes the lock to assign the variable. If it is safe to check the variable without lock, then it is safe to assign it without lock. If it is unsafe to assign without a lock, then it is unsafe to check it without a lock. I don't find a path in the different drivers where that could happen because the arch specific drivers are written in such way it is not possible to register a driver while it is unregistered, except maybe in a very improbable case when "intel_idle" and "processor_idle" are competing. One could unregister a driver, while the other one is registering. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/cpuidle/driver.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 39ba8e1..4a0c4ab 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -85,17 +85,17 @@ EXPORT_SYMBOL_GPL(cpuidle_get_driver); */ void cpuidle_unregister_driver(struct cpuidle_driver *drv) { + spin_lock(&cpuidle_driver_lock); + if (drv != cpuidle_curr_driver) { WARN(1, "invalid cpuidle_unregister_driver(%s)\n", drv->name); - return; + goto out; } - spin_lock(&cpuidle_driver_lock); - if (!WARN_ON(drv->refcnt > 0)) cpuidle_curr_driver = NULL; - +out: spin_unlock(&cpuidle_driver_lock); } EXPORT_SYMBOL_GPL(cpuidle_unregister_driver); -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 23+ messages in thread
[parent not found: <1348526634-19029-3-git-send-email-daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]
* Re: [PATCH 2/4] cpuidle : move driver checking within the lock section [not found] ` <1348526634-19029-3-git-send-email-daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> @ 2012-10-07 20:17 ` Rafael J. Wysocki 0 siblings, 0 replies; 23+ messages in thread From: Rafael J. Wysocki @ 2012-10-07 20:17 UTC (permalink / raw) To: Daniel Lezcano Cc: linaro-dev-cunTk1MwBs8s++Sfvej+rw, linux-pm-u79uwXL29TY76Z2rM5mHXA, pdeschrijver-DDmLM1+adcrQT0dZR+AlfA, patches-QSEj5FYQhm4dnm+yROfE0A, linux-acpi-u79uwXL29TY76Z2rM5mHXA, lenb-DgEjT+Ai2ygdnm+yROfE0A On Tuesday 25 of September 2012 00:43:52 Daniel Lezcano wrote: > The code checks if the driver is already set without taking the lock, > but, right after, it takes the lock to assign the variable. > > If it is safe to check the variable without lock, then it is safe to > assign it without lock. If it is unsafe to assign without a lock, then > it is unsafe to check it without a lock. > > I don't find a path in the different drivers where that could happen > because the arch specific drivers are written in such way it is not > possible to register a driver while it is unregistered, except maybe > in a very improbable case when "intel_idle" and "processor_idle" are > competing. One could unregister a driver, while the other one is > registering. > > Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> The code looks generally OK (a minor nit below), but I'm not really sure what you wanted to say in the changelog. It would suffice to say that the existing code is racy and either we don't need to take the spinlock in cpuidle_unregister_driver() at all, or we should put the test under the spinlock as well as the modification. > --- > drivers/cpuidle/driver.c | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c > index 39ba8e1..4a0c4ab 100644 > --- a/drivers/cpuidle/driver.c > +++ b/drivers/cpuidle/driver.c > @@ -85,17 +85,17 @@ EXPORT_SYMBOL_GPL(cpuidle_get_driver); > */ > void cpuidle_unregister_driver(struct cpuidle_driver *drv) > { > + spin_lock(&cpuidle_driver_lock); > + > if (drv != cpuidle_curr_driver) { > WARN(1, "invalid cpuidle_unregister_driver(%s)\n", > drv->name); It is not necessary to execute the WARN() under the spinlock. > - return; > + goto out; > } > > - spin_lock(&cpuidle_driver_lock); > - > if (!WARN_ON(drv->refcnt > 0)) > cpuidle_curr_driver = NULL; > - > +out: > spin_unlock(&cpuidle_driver_lock); > } > EXPORT_SYMBOL_GPL(cpuidle_unregister_driver); Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/4] cpuidle - prepare the driver core to be multi drivers aware 2012-09-24 22:43 [PATCH 0/4] cpuidle - support multiple drivers at a time Daniel Lezcano 2012-09-24 22:43 ` [PATCH 1/4] cpuidle : move driver's refcount to cpuidle Daniel Lezcano 2012-09-24 22:43 ` [PATCH 2/4] cpuidle : move driver checking within the lock section Daniel Lezcano @ 2012-09-24 22:43 ` Daniel Lezcano 2012-09-24 22:43 ` [PATCH 4/4] cpuidle - support multiple drivers Daniel Lezcano 2012-10-04 14:16 ` [PATCH 0/4] cpuidle - support multiple drivers at a time Peter De Schrijver 4 siblings, 0 replies; 23+ messages in thread From: Daniel Lezcano @ 2012-09-24 22:43 UTC (permalink / raw) To: rjw, lenb Cc: linux-pm, lorenzo.pieralisi, pdeschrijver, linux-acpi, patches, linaro-dev This patch is a preparation for the multiple drivers support. As the next patch will introduce the multiple drivers with the Kconfig option and we want to keep the code clean and understandable, this patch defines a set of functions for encapsulating some common parts and split what should be done in a lock context or not. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/cpuidle/driver.c | 71 +++++++++++++++++++++++++++++----------------- 1 files changed, 45 insertions(+), 26 deletions(-) diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 4a0c4ab..391f80f 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -39,11 +39,20 @@ static void set_power_states(struct cpuidle_driver *drv) drv->states[i].power_usage = -1 - i; } -/** - * cpuidle_register_driver - registers a driver - * @drv: the driver - */ -int cpuidle_register_driver(struct cpuidle_driver *drv) +static void __cpuidle_driver_init(struct cpuidle_driver *drv) +{ + drv->refcnt = 0; + + if (!drv->power_specified) + set_power_states(drv); +} + +static void cpuidle_set_driver(struct cpuidle_driver *drv) +{ + cpuidle_curr_driver = drv; +} + +static int __cpuidle_register_driver(struct cpuidle_driver *drv) { if (!drv || !drv->state_count) return -EINVAL; @@ -51,22 +60,41 @@ int cpuidle_register_driver(struct cpuidle_driver *drv) if (cpuidle_disabled()) return -ENODEV; - spin_lock(&cpuidle_driver_lock); - if (cpuidle_curr_driver) { - spin_unlock(&cpuidle_driver_lock); + if (cpuidle_get_driver()) return -EBUSY; - } - if (!drv->power_specified) - set_power_states(drv); + __cpuidle_driver_init(drv); - drv->refcnt = 0; + cpuidle_set_driver(drv); - cpuidle_curr_driver = drv; + return 0; +} +static void __cpuidle_unregister_driver(struct cpuidle_driver *drv) +{ + if (drv != cpuidle_get_driver()) { + WARN(1, "invalid cpuidle_unregister_driver(%s)\n", + drv->name); + return; + } + + if (!WARN_ON(drv->refcnt > 0)) + cpuidle_set_driver(NULL); +} + +/** + * cpuidle_register_driver - registers a driver + * @drv: the driver + */ +int cpuidle_register_driver(struct cpuidle_driver *drv) +{ + int ret; + + spin_lock(&cpuidle_driver_lock); + ret = __cpuidle_register_driver(drv); spin_unlock(&cpuidle_driver_lock); - return 0; + return ret; } EXPORT_SYMBOL_GPL(cpuidle_register_driver); @@ -86,16 +114,7 @@ EXPORT_SYMBOL_GPL(cpuidle_get_driver); void cpuidle_unregister_driver(struct cpuidle_driver *drv) { spin_lock(&cpuidle_driver_lock); - - if (drv != cpuidle_curr_driver) { - WARN(1, "invalid cpuidle_unregister_driver(%s)\n", - drv->name); - goto out; - } - - if (!WARN_ON(drv->refcnt > 0)) - cpuidle_curr_driver = NULL; -out: + __cpuidle_unregister_driver(drv); spin_unlock(&cpuidle_driver_lock); } EXPORT_SYMBOL_GPL(cpuidle_unregister_driver); @@ -106,7 +125,7 @@ struct cpuidle_driver *cpuidle_driver_ref(void) spin_lock(&cpuidle_driver_lock); - drv = cpuidle_curr_driver; + drv = cpuidle_get_driver(); drv->refcnt++; spin_unlock(&cpuidle_driver_lock); @@ -115,7 +134,7 @@ struct cpuidle_driver *cpuidle_driver_ref(void) void cpuidle_driver_unref(void) { - struct cpuidle_driver *drv = cpuidle_curr_driver; + struct cpuidle_driver *drv = cpuidle_get_driver(); spin_lock(&cpuidle_driver_lock); -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/4] cpuidle - support multiple drivers 2012-09-24 22:43 [PATCH 0/4] cpuidle - support multiple drivers at a time Daniel Lezcano ` (2 preceding siblings ...) 2012-09-24 22:43 ` [PATCH 3/4] cpuidle - prepare the driver core to be multi drivers aware Daniel Lezcano @ 2012-09-24 22:43 ` Daniel Lezcano [not found] ` <1348526634-19029-5-git-send-email-daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> ` (2 more replies) 2012-10-04 14:16 ` [PATCH 0/4] cpuidle - support multiple drivers at a time Peter De Schrijver 4 siblings, 3 replies; 23+ messages in thread From: Daniel Lezcano @ 2012-09-24 22:43 UTC (permalink / raw) To: rjw, lenb Cc: linux-pm, lorenzo.pieralisi, pdeschrijver, linux-acpi, patches, linaro-dev With the tegra3 and the big.LITTLE [1] new architectures, several cpus with different characteristics (latencies and states) can co-exists on the system. The cpuidle framework has the limitation of handling only identical cpus. This patch removes this limitation by introducing the multiple driver support for cpuidle. This option is configurable at compile time and should be enabled for the architectures mentioned above. So there is no impact for the other platforms if the option is disabled. The option defaults to 'n'. Note the multiple drivers support is also compatible with the existing drivers, even if just one driver is needed, all the cpu will be tied to this driver using an extra small chunk of processor memory. The multiple driver support use a per-cpu driver pointer instead of a global variable and the accessor to this variable are done from a cpu context. In order to keep the compatibility with the existing drivers, the function 'cpuidle_register_driver' and 'cpuidle_unregister_driver' will register the specified driver for all the cpus. The sysfs output for the 'current_driver' is changed when this option is set by giving the drivers per cpu. eg. cpu0: acpi_idle cpu1: acpi_idle but if the option is disabled, the output will remain the same. [1] http://lwn.net/Articles/481055/ Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/cpuidle/Kconfig | 9 +++ drivers/cpuidle/cpuidle.c | 24 ++++++-- drivers/cpuidle/driver.c | 136 +++++++++++++++++++++++++++++++++++++++++--- drivers/cpuidle/sysfs.c | 50 +++++++++++++---- include/linux/cpuidle.h | 8 ++- 5 files changed, 197 insertions(+), 30 deletions(-) diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig index a76b689..234ae65 100644 --- a/drivers/cpuidle/Kconfig +++ b/drivers/cpuidle/Kconfig @@ -9,6 +9,15 @@ config CPU_IDLE If you're using an ACPI-enabled platform, you should say Y here. +config CPU_IDLE_MULTIPLE_DRIVERS + bool "Support multiple cpuidle drivers" + depends on CPU_IDLE + default n + help + Allows the cpuidle framework to use different drivers for each CPU. + This is useful if you have a system with different CPU latencies and + states. If unsure say N. + config CPU_IDLE_GOV_LADDER bool depends on CPU_IDLE diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index e28f6ea..c581b99 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -68,7 +68,7 @@ static cpuidle_enter_t cpuidle_enter_ops; int cpuidle_play_dead(void) { struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); - struct cpuidle_driver *drv = cpuidle_get_driver(); + struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev->cpu); int i, dead_state = -1; int power_usage = -1; @@ -128,7 +128,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, int cpuidle_idle_call(void) { struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); - struct cpuidle_driver *drv = cpuidle_get_driver(); + struct cpuidle_driver *drv; int next_state, entered_state; if (off) @@ -141,6 +141,8 @@ int cpuidle_idle_call(void) if (!dev || !dev->enabled) return -EBUSY; + drv = cpuidle_get_cpu_driver(dev->cpu); + /* ask the governor for the next state */ next_state = cpuidle_curr_governor->select(drv, dev); if (need_resched()) { @@ -308,15 +310,19 @@ static void poll_idle_init(struct cpuidle_driver *drv) {} int cpuidle_enable_device(struct cpuidle_device *dev) { int ret, i; - struct cpuidle_driver *drv = cpuidle_get_driver(); + struct cpuidle_driver *drv; if (!dev) return -EINVAL; if (dev->enabled) return 0; + + drv = cpuidle_get_cpu_driver(dev->cpu); + if (!drv || !cpuidle_curr_governor) return -EIO; + if (!dev->state_count) dev->state_count = drv->state_count; @@ -368,15 +374,18 @@ EXPORT_SYMBOL_GPL(cpuidle_enable_device); */ void cpuidle_disable_device(struct cpuidle_device *dev) { + struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev->cpu); + if (!dev->enabled) return; - if (!cpuidle_get_driver() || !cpuidle_curr_governor) + + if (!drv || !cpuidle_curr_governor) return; dev->enabled = 0; if (cpuidle_curr_governor->disable) - cpuidle_curr_governor->disable(cpuidle_get_driver(), dev); + cpuidle_curr_governor->disable(drv, dev); cpuidle_remove_state_sysfs(dev); enabled_devices--; @@ -395,7 +404,8 @@ static int __cpuidle_register_device(struct cpuidle_device *dev) { int ret; struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu); - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); + struct cpuidle_driver *cpuidle_driver = + cpuidle_get_cpu_driver(dev->cpu); if (!try_module_get(cpuidle_driver->owner)) return -EINVAL; @@ -461,7 +471,7 @@ EXPORT_SYMBOL_GPL(cpuidle_register_device); void cpuidle_unregister_device(struct cpuidle_device *dev) { struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu); - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); + struct cpuidle_driver *cpuidle_driver = cpuidle_get_cpu_driver(dev->cpu); if (dev->registered == 0) return; diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 391f80f..6529b91 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -14,7 +14,11 @@ #include "cpuidle.h" +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS +DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers); +#else static struct cpuidle_driver *cpuidle_curr_driver; +#endif DEFINE_SPINLOCK(cpuidle_driver_lock); static void set_power_states(struct cpuidle_driver *drv) @@ -47,12 +51,25 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) set_power_states(drv); } -static void cpuidle_set_driver(struct cpuidle_driver *drv) +static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu) { +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS + per_cpu(cpuidle_drivers, cpu) = drv; +#else cpuidle_curr_driver = drv; +#endif } -static int __cpuidle_register_driver(struct cpuidle_driver *drv) +static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) +{ +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS + return per_cpu(cpuidle_drivers, cpu); +#else + return cpuidle_curr_driver; +#endif +} + +static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu) { if (!drv || !drv->state_count) return -EINVAL; @@ -60,26 +77,102 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv) if (cpuidle_disabled()) return -ENODEV; - if (cpuidle_get_driver()) + if (__cpuidle_get_cpu_driver(cpu)) return -EBUSY; __cpuidle_driver_init(drv); - cpuidle_set_driver(drv); + __cpuidle_set_cpu_driver(drv, cpu); return 0; } -static void __cpuidle_unregister_driver(struct cpuidle_driver *drv) +static void __cpuidle_unregister_driver(struct cpuidle_driver *drv, int cpu) { - if (drv != cpuidle_get_driver()) { + if (drv != __cpuidle_get_cpu_driver(cpu)) { WARN(1, "invalid cpuidle_unregister_driver(%s)\n", drv->name); return; } if (!WARN_ON(drv->refcnt > 0)) - cpuidle_set_driver(NULL); + __cpuidle_set_cpu_driver(NULL, cpu); +} + +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS +static void __cpuidle_unregister_all_cpu_driver(struct cpuidle_driver *drv) +{ + int cpu; + for_each_present_cpu(cpu) + __cpuidle_unregister_driver(drv, cpu); +} + +static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv) +{ + int ret = 0; + int i, cpu; + + for_each_present_cpu(cpu) { + ret = __cpuidle_register_driver(drv, cpu); + if (!ret) + continue; + for (i = cpu - 1; i >= 0; i--) + __cpuidle_unregister_driver(drv, i); + break; + } + + return ret; +} + +static int __cpuidle_for_each_driver(int (*cb)(int, struct cpuidle_driver *, + void *), void *data) +{ + struct cpuidle_driver *drv; + int cpu, ret = 0; + + for_each_present_cpu(cpu) { + drv = __cpuidle_get_cpu_driver(cpu); + ret = cb(cpu, drv, data); + if (ret < 0) + break; + } + return ret; +} + +int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu) +{ + int ret; + + spin_lock(&cpuidle_driver_lock); + ret = __cpuidle_register_driver(drv, cpu); + spin_unlock(&cpuidle_driver_lock); + + return ret; +} + +void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu) +{ + spin_lock(&cpuidle_driver_lock); + __cpuidle_unregister_driver(drv, cpu); + spin_unlock(&cpuidle_driver_lock); +} +#endif + +int cpuidle_for_each_driver(int (*cb)(int, struct cpuidle_driver *, void *), + void *data) +{ + int ret; + + spin_lock(&cpuidle_driver_lock); +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS + ret = __cpuidle_for_each_driver(cb, data); +#else + ret = cb(smp_processor_id(), + __cpuidle_get_cpu_driver(smp_processor_id()), data); +#endif + spin_unlock(&cpuidle_driver_lock); + + return ret; } /** @@ -91,7 +184,11 @@ int cpuidle_register_driver(struct cpuidle_driver *drv) int ret; spin_lock(&cpuidle_driver_lock); - ret = __cpuidle_register_driver(drv); +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS + ret = __cpuidle_register_all_cpu_driver(drv); +#else + ret = __cpuidle_register_driver(drv, smp_processor_id()); +#endif spin_unlock(&cpuidle_driver_lock); return ret; @@ -103,18 +200,37 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver); */ struct cpuidle_driver *cpuidle_get_driver(void) { - return cpuidle_curr_driver; + return __cpuidle_get_cpu_driver(smp_processor_id()); } EXPORT_SYMBOL_GPL(cpuidle_get_driver); /** + * cpuidle_get_cpu_driver - return the driver tied with a cpu + */ +struct cpuidle_driver *cpuidle_get_cpu_driver(int cpu) +{ + struct cpuidle_driver *drv; + + spin_lock(&cpuidle_driver_lock); + drv = __cpuidle_get_cpu_driver(cpu); + spin_unlock(&cpuidle_driver_lock); + + return drv; +} +EXPORT_SYMBOL_GPL(cpuidle_get_cpu_driver); + +/** * cpuidle_unregister_driver - unregisters a driver * @drv: the driver */ void cpuidle_unregister_driver(struct cpuidle_driver *drv) { spin_lock(&cpuidle_driver_lock); - __cpuidle_unregister_driver(drv); +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS + __cpuidle_unregister_all_cpu_driver(drv); +#else + __cpuidle_unregister_driver(drv, smp_processor_id()); +#endif spin_unlock(&cpuidle_driver_lock); } EXPORT_SYMBOL_GPL(cpuidle_unregister_driver); diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index 5f809e3..2596422 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -43,21 +43,46 @@ out: return i; } +struct cbarg { + char *buf; + ssize_t count; +}; + +static int each_driver_cb(int cpu, struct cpuidle_driver *drv, void *data) +{ + int ret; + struct cbarg *cbarg = data; + + if ((drv && (strlen(drv->name) + cbarg->count) >= PAGE_SIZE) || + (!drv && (strlen("none") + cbarg->count) >= PAGE_SIZE)) + return -EOVERFLOW; + +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS + ret = sprintf(cbarg->buf + cbarg->count, "cpu%d: %s\n", + cpu, drv ? drv->name : "none" ); +#else + ret = sprintf(cbarg->buf, "%s\n", drv ? drv->name : "none"); +#endif + if (ret < 0) + return ret; + + cbarg->count += ret; + + return 0; +} + static ssize_t show_current_driver(struct device *dev, struct device_attribute *attr, char *buf) { - ssize_t ret; - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); + struct cbarg cbarg = { .buf = buf }; + int ret; - spin_lock(&cpuidle_driver_lock); - if (cpuidle_driver) - ret = sprintf(buf, "%s\n", cpuidle_driver->name); - else - ret = sprintf(buf, "none\n"); - spin_unlock(&cpuidle_driver_lock); + ret = cpuidle_for_each_driver(each_driver_cb, &cbarg); + if (ret < 0) + return ret; - return ret; + return cbarg.count; } static ssize_t show_current_governor(struct device *dev, @@ -363,10 +388,10 @@ int cpuidle_add_state_sysfs(struct cpuidle_device *device) { int i, ret = -ENOMEM; struct cpuidle_state_kobj *kobj; - struct cpuidle_driver *drv = cpuidle_get_driver(); + struct cpuidle_driver *drv = cpuidle_get_cpu_driver(device->cpu); /* state statistics */ - for (i = 0; i < device->state_count; i++) { + for (i = 0; i < drv->state_count; i++) { kobj = kzalloc(sizeof(struct cpuidle_state_kobj), GFP_KERNEL); if (!kobj) goto error_state; @@ -374,7 +399,8 @@ int cpuidle_add_state_sysfs(struct cpuidle_device *device) kobj->state_usage = &device->states_usage[i]; init_completion(&kobj->kobj_unregister); - ret = kobject_init_and_add(&kobj->kobj, &ktype_state_cpuidle, &device->kobj, + ret = kobject_init_and_add(&kobj->kobj, + &ktype_state_cpuidle, &device->kobj, "state%d", i); if (ret) { kfree(kobj); diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index a4ff9f8..0e0b0ad 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -164,6 +164,13 @@ extern int cpuidle_wrap_enter(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index)); extern int cpuidle_play_dead(void); +extern int cpuidle_for_each_driver( + int (*cb)(int, struct cpuidle_driver *, void *), void *data); + +extern struct cpuidle_driver *cpuidle_get_cpu_driver(int cpu); +extern int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu); +extern void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu); + #else static inline void disable_cpuidle(void) { } static inline int cpuidle_idle_call(void) { return -ENODEV; } @@ -190,7 +197,6 @@ static inline int cpuidle_wrap_enter(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index)) { return -ENODEV; } static inline int cpuidle_play_dead(void) {return -ENODEV; } - #endif #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 23+ messages in thread
[parent not found: <1348526634-19029-5-git-send-email-daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]
* Re: [PATCH 4/4] cpuidle - support multiple drivers [not found] ` <1348526634-19029-5-git-send-email-daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> @ 2012-09-29 9:41 ` Francesco Lavra 2012-09-29 17:44 ` Daniel Lezcano 0 siblings, 1 reply; 23+ messages in thread From: Francesco Lavra @ 2012-09-29 9:41 UTC (permalink / raw) To: Daniel Lezcano Cc: linaro-dev-cunTk1MwBs8s++Sfvej+rw, linux-pm-u79uwXL29TY76Z2rM5mHXA, pdeschrijver-DDmLM1+adcrQT0dZR+AlfA, patches-QSEj5FYQhm4dnm+yROfE0A, rjw-KKrjLPT3xs0, linux-acpi-u79uwXL29TY76Z2rM5mHXA, lenb-DgEjT+Ai2ygdnm+yROfE0A Hi, On 09/25/2012 12:43 AM, Daniel Lezcano wrote: ... > diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c > index 5f809e3..2596422 100644 > --- a/drivers/cpuidle/sysfs.c > +++ b/drivers/cpuidle/sysfs.c > @@ -43,21 +43,46 @@ out: > return i; > } > > +struct cbarg { > + char *buf; > + ssize_t count; > +}; > + > +static int each_driver_cb(int cpu, struct cpuidle_driver *drv, void *data) > +{ > + int ret; > + struct cbarg *cbarg = data; > + > + if ((drv && (strlen(drv->name) + cbarg->count) >= PAGE_SIZE) || > + (!drv && (strlen("none") + cbarg->count) >= PAGE_SIZE)) > + return -EOVERFLOW; > + > +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > + ret = sprintf(cbarg->buf + cbarg->count, "cpu%d: %s\n", > + cpu, drv ? drv->name : "none" ); > +#else > + ret = sprintf(cbarg->buf, "%s\n", drv ? drv->name : "none"); > +#endif > + if (ret < 0) > + return ret; > + > + cbarg->count += ret; > + > + return 0; > +} > + > static ssize_t show_current_driver(struct device *dev, > struct device_attribute *attr, > char *buf) > { > - ssize_t ret; > - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); > + struct cbarg cbarg = { .buf = buf }; cbarg.count should be initialized to 0. > + int ret; > > - spin_lock(&cpuidle_driver_lock); > - if (cpuidle_driver) > - ret = sprintf(buf, "%s\n", cpuidle_driver->name); > - else > - ret = sprintf(buf, "none\n"); > - spin_unlock(&cpuidle_driver_lock); > + ret = cpuidle_for_each_driver(each_driver_cb, &cbarg); > + if (ret < 0) > + return ret; > > - return ret; > + return cbarg.count; > } -- Francesco ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-09-29 9:41 ` Francesco Lavra @ 2012-09-29 17:44 ` Daniel Lezcano 2012-09-29 22:07 ` Rafael J. Wysocki 0 siblings, 1 reply; 23+ messages in thread From: Daniel Lezcano @ 2012-09-29 17:44 UTC (permalink / raw) To: Francesco Lavra Cc: rjw, lenb, linaro-dev, patches, pdeschrijver, linux-pm, linux-acpi On 09/29/2012 11:41 AM, Francesco Lavra wrote: > Hi, Hi Francesco, thanks for reviewing the patch. >> static ssize_t show_current_driver(struct device *dev, >> struct device_attribute *attr, >> char *buf) >> { >> - ssize_t ret; >> - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); >> + struct cbarg cbarg = { .buf = buf }; > > cbarg.count should be initialized to 0. Actually, with this initialization, all the fields will be initialized to 0, except 'buf'. Thanks -- Daniel >> + int ret; >> >> - spin_lock(&cpuidle_driver_lock); >> - if (cpuidle_driver) >> - ret = sprintf(buf, "%s\n", cpuidle_driver->name); >> - else >> - ret = sprintf(buf, "none\n"); >> - spin_unlock(&cpuidle_driver_lock); >> + ret = cpuidle_for_each_driver(each_driver_cb, &cbarg); >> + if (ret < 0) >> + return ret; >> >> - return ret; >> + return cbarg.count; >> } > > -- > Francesco -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-09-29 17:44 ` Daniel Lezcano @ 2012-09-29 22:07 ` Rafael J. Wysocki 2012-09-30 16:34 ` Daniel Lezcano 0 siblings, 1 reply; 23+ messages in thread From: Rafael J. Wysocki @ 2012-09-29 22:07 UTC (permalink / raw) To: Daniel Lezcano Cc: Francesco Lavra, lenb, linaro-dev, patches, pdeschrijver, linux-pm, linux-acpi On Saturday, September 29, 2012, Daniel Lezcano wrote: > On 09/29/2012 11:41 AM, Francesco Lavra wrote: > > Hi, > > Hi Francesco, > > thanks for reviewing the patch. > > >> static ssize_t show_current_driver(struct device *dev, > >> struct device_attribute *attr, > >> char *buf) > >> { > >> - ssize_t ret; > >> - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); > >> + struct cbarg cbarg = { .buf = buf }; > > > > cbarg.count should be initialized to 0. > > Actually, with this initialization, all the fields will be initialized > to 0, except 'buf'. However, it would be good to initialize count explicitly so as to show that we care about the initial value of it. Thanks, Rafael ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-09-29 22:07 ` Rafael J. Wysocki @ 2012-09-30 16:34 ` Daniel Lezcano 2012-10-02 2:08 ` Rafael J. Wysocki 0 siblings, 1 reply; 23+ messages in thread From: Daniel Lezcano @ 2012-09-30 16:34 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Daniel Lezcano, linaro-dev, linux-pm, pdeschrijver, patches, linux-acpi, lenb On 09/30/2012 12:07 AM, Rafael J. Wysocki wrote: > On Saturday, September 29, 2012, Daniel Lezcano wrote: >> On 09/29/2012 11:41 AM, Francesco Lavra wrote: >>> Hi, >> Hi Francesco, >> >> thanks for reviewing the patch. >> >>>> static ssize_t show_current_driver(struct device *dev, >>>> struct device_attribute *attr, >>>> char *buf) >>>> { >>>> - ssize_t ret; >>>> - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); >>>> + struct cbarg cbarg = { .buf = buf }; >>> cbarg.count should be initialized to 0. >> Actually, with this initialization, all the fields will be initialized >> to 0, except 'buf'. > However, it would be good to initialize count explicitly so as to show > that we care about the initial value of it. Ok, I will change that in a V2. Rafael, you're right, this approach makes more sense IMO. Thanks -- Daniel -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-09-30 16:34 ` Daniel Lezcano @ 2012-10-02 2:08 ` Rafael J. Wysocki [not found] ` <3723306.GAm1cPlZaP-sKB8Sp2ER+y1GS7QM15AGw@public.gmane.org> 0 siblings, 1 reply; 23+ messages in thread From: Rafael J. Wysocki @ 2012-10-02 2:08 UTC (permalink / raw) To: Daniel Lezcano Cc: linaro-dev, linux-pm, pdeschrijver, patches, linux-acpi, lenb On Sunday 30 of September 2012 18:34:31 Daniel Lezcano wrote: > On 09/30/2012 12:07 AM, Rafael J. Wysocki wrote: > > On Saturday, September 29, 2012, Daniel Lezcano wrote: > >> On 09/29/2012 11:41 AM, Francesco Lavra wrote: > >>> Hi, > >> Hi Francesco, > >> > >> thanks for reviewing the patch. > >> > >>>> static ssize_t show_current_driver(struct device *dev, > >>>> struct device_attribute *attr, > >>>> char *buf) > >>>> { > >>>> - ssize_t ret; > >>>> - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); > >>>> + struct cbarg cbarg = { .buf = buf }; > >>> cbarg.count should be initialized to 0. > >> Actually, with this initialization, all the fields will be initialized > >> to 0, except 'buf'. > > However, it would be good to initialize count explicitly so as to show > > that we care about the initial value of it. > > Ok, I will change that in a V2. > > Rafael, you're right, this approach makes more sense IMO. Well, I'm glad that you agree. :-) I'm sorry I haven't posted any comments about the latest series, I'll do my best to review it later this week. Thanks, Rafael ^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <3723306.GAm1cPlZaP-sKB8Sp2ER+y1GS7QM15AGw@public.gmane.org>]
* Re: [PATCH 4/4] cpuidle - support multiple drivers [not found] ` <3723306.GAm1cPlZaP-sKB8Sp2ER+y1GS7QM15AGw@public.gmane.org> @ 2012-10-02 12:27 ` Daniel Lezcano 2012-10-02 14:02 ` Rafael J. Wysocki 0 siblings, 1 reply; 23+ messages in thread From: Daniel Lezcano @ 2012-10-02 12:27 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linaro-dev-cunTk1MwBs8s++Sfvej+rw, patches-QSEj5FYQhm4dnm+yROfE0A, pdeschrijver-DDmLM1+adcrQT0dZR+AlfA, linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-acpi-u79uwXL29TY76Z2rM5mHXA, lenb-DgEjT+Ai2ygdnm+yROfE0A On 10/02/2012 04:08 AM, Rafael J. Wysocki wrote: > On Sunday 30 of September 2012 18:34:31 Daniel Lezcano wrote: >> On 09/30/2012 12:07 AM, Rafael J. Wysocki wrote: >>> On Saturday, September 29, 2012, Daniel Lezcano wrote: >>>> On 09/29/2012 11:41 AM, Francesco Lavra wrote: >>>>> Hi, >>>> Hi Francesco, >>>> >>>> thanks for reviewing the patch. >>>> >>>>>> static ssize_t show_current_driver(struct device *dev, >>>>>> struct device_attribute *attr, >>>>>> char *buf) >>>>>> { >>>>>> - ssize_t ret; >>>>>> - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); >>>>>> + struct cbarg cbarg = { .buf = buf }; >>>>> cbarg.count should be initialized to 0. >>>> Actually, with this initialization, all the fields will be initialized >>>> to 0, except 'buf'. >>> However, it would be good to initialize count explicitly so as to show >>> that we care about the initial value of it. >> >> Ok, I will change that in a V2. >> >> Rafael, you're right, this approach makes more sense IMO. > > Well, I'm glad that you agree. :-) > > I'm sorry I haven't posted any comments about the latest series, > I'll do my best to review it later this week. Ok, cool. Thanks! I guess it is too late to have this material for v3.7, right ? -- Daniel -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-10-02 12:27 ` Daniel Lezcano @ 2012-10-02 14:02 ` Rafael J. Wysocki 0 siblings, 0 replies; 23+ messages in thread From: Rafael J. Wysocki @ 2012-10-02 14:02 UTC (permalink / raw) To: Daniel Lezcano Cc: linaro-dev, linux-pm, pdeschrijver, patches, linux-acpi, lenb On Tuesday 02 of October 2012 14:27:17 Daniel Lezcano wrote: > On 10/02/2012 04:08 AM, Rafael J. Wysocki wrote: > > On Sunday 30 of September 2012 18:34:31 Daniel Lezcano wrote: > >> On 09/30/2012 12:07 AM, Rafael J. Wysocki wrote: > >>> On Saturday, September 29, 2012, Daniel Lezcano wrote: > >>>> On 09/29/2012 11:41 AM, Francesco Lavra wrote: > >>>>> Hi, > >>>> Hi Francesco, > >>>> > >>>> thanks for reviewing the patch. > >>>> > >>>>>> static ssize_t show_current_driver(struct device *dev, > >>>>>> struct device_attribute *attr, > >>>>>> char *buf) > >>>>>> { > >>>>>> - ssize_t ret; > >>>>>> - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); > >>>>>> + struct cbarg cbarg = { .buf = buf }; > >>>>> cbarg.count should be initialized to 0. > >>>> Actually, with this initialization, all the fields will be initialized > >>>> to 0, except 'buf'. > >>> However, it would be good to initialize count explicitly so as to show > >>> that we care about the initial value of it. > >> > >> Ok, I will change that in a V2. > >> > >> Rafael, you're right, this approach makes more sense IMO. > > > > Well, I'm glad that you agree. :-) > > > > I'm sorry I haven't posted any comments about the latest series, > > I'll do my best to review it later this week. > > Ok, cool. Thanks! > > I guess it is too late to have this material for v3.7, right ? Yes, it is too late. Thanks, Rafael --- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-09-24 22:43 ` [PATCH 4/4] cpuidle - support multiple drivers Daniel Lezcano [not found] ` <1348526634-19029-5-git-send-email-daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> @ 2012-10-04 14:14 ` Peter De Schrijver 2012-10-04 14:26 ` Daniel Lezcano 2012-10-07 21:26 ` Rafael J. Wysocki 2 siblings, 1 reply; 23+ messages in thread From: Peter De Schrijver @ 2012-10-04 14:14 UTC (permalink / raw) To: Daniel Lezcano Cc: rjw, lenb, linux-pm, lorenzo.pieralisi, linux-acpi, patches, linaro-dev On Tue, Sep 25, 2012 at 12:43:54AM +0200, Daniel Lezcano wrote: > With the tegra3 and the big.LITTLE [1] new architectures, several cpus > with different characteristics (latencies and states) can co-exists on the > system. > > The cpuidle framework has the limitation of handling only identical cpus. > > This patch removes this limitation by introducing the multiple driver support > for cpuidle. > > This option is configurable at compile time and should be enabled for the > architectures mentioned above. So there is no impact for the other platforms > if the option is disabled. The option defaults to 'n'. Note the multiple drivers > support is also compatible with the existing drivers, even if just one driver is > needed, all the cpu will be tied to this driver using an extra small chunk of > processor memory. > > The multiple driver support use a per-cpu driver pointer instead of a global > variable and the accessor to this variable are done from a cpu context. > > In order to keep the compatibility with the existing drivers, the function > 'cpuidle_register_driver' and 'cpuidle_unregister_driver' will register > the specified driver for all the cpus. > > The sysfs output for the 'current_driver' is changed when this option is > set by giving the drivers per cpu. > > eg. > cpu0: acpi_idle > cpu1: acpi_idle > Is this allowed? I thought sysfs files can output only 1 line of data? Cheers, Peter. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-10-04 14:14 ` Peter De Schrijver @ 2012-10-04 14:26 ` Daniel Lezcano 2012-10-04 14:34 ` Peter De Schrijver 0 siblings, 1 reply; 23+ messages in thread From: Daniel Lezcano @ 2012-10-04 14:26 UTC (permalink / raw) To: Peter De Schrijver Cc: rjw, lenb, linux-pm, lorenzo.pieralisi, linux-acpi, patches, linaro-dev On 10/04/2012 04:14 PM, Peter De Schrijver wrote: > On Tue, Sep 25, 2012 at 12:43:54AM +0200, Daniel Lezcano wrote: >> With the tegra3 and the big.LITTLE [1] new architectures, several cpus >> with different characteristics (latencies and states) can co-exists on the >> system. >> >> The cpuidle framework has the limitation of handling only identical cpus. >> >> This patch removes this limitation by introducing the multiple driver support >> for cpuidle. >> >> This option is configurable at compile time and should be enabled for the >> architectures mentioned above. So there is no impact for the other platforms >> if the option is disabled. The option defaults to 'n'. Note the multiple drivers >> support is also compatible with the existing drivers, even if just one driver is >> needed, all the cpu will be tied to this driver using an extra small chunk of >> processor memory. >> >> The multiple driver support use a per-cpu driver pointer instead of a global >> variable and the accessor to this variable are done from a cpu context. >> >> In order to keep the compatibility with the existing drivers, the function >> 'cpuidle_register_driver' and 'cpuidle_unregister_driver' will register >> the specified driver for all the cpus. >> >> The sysfs output for the 'current_driver' is changed when this option is >> set by giving the drivers per cpu. >> >> eg. >> cpu0: acpi_idle >> cpu1: acpi_idle >> > > Is this allowed? I thought sysfs files can output only 1 line of data? Ah yeah :) Does it make sense to put the following files: /sys/devices/system/cpu/cpu0/cpuidle/driver => acpi_idle /sys/devices/system/cpu/cpu1/cpuidle/driver => acpi_idle and the content of the file: /sys/devices/system/cpu/cpuidle/current_driver will show the driver associated with current cpu ? -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-10-04 14:26 ` Daniel Lezcano @ 2012-10-04 14:34 ` Peter De Schrijver 2012-10-07 21:09 ` Rafael J. Wysocki 0 siblings, 1 reply; 23+ messages in thread From: Peter De Schrijver @ 2012-10-04 14:34 UTC (permalink / raw) To: Daniel Lezcano Cc: rjw, lenb, linux-pm, lorenzo.pieralisi, linux-acpi, patches, linaro-dev > >> > >> eg. > >> cpu0: acpi_idle > >> cpu1: acpi_idle > >> > > > > Is this allowed? I thought sysfs files can output only 1 line of data? > > Ah yeah :) > > Does it make sense to put the following files: > > /sys/devices/system/cpu/cpu0/cpuidle/driver > => acpi_idle > /sys/devices/system/cpu/cpu1/cpuidle/driver > => acpi_idle > Sounds like a reasonable idea. > and the content of the file: > > /sys/devices/system/cpu/cpuidle/current_driver will show the driver > associated with current cpu ? > I think that's ok. Cheers, Peter. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-10-04 14:34 ` Peter De Schrijver @ 2012-10-07 21:09 ` Rafael J. Wysocki 0 siblings, 0 replies; 23+ messages in thread From: Rafael J. Wysocki @ 2012-10-07 21:09 UTC (permalink / raw) To: Peter De Schrijver Cc: Daniel Lezcano, lenb, linux-pm, lorenzo.pieralisi, linux-acpi, patches, linaro-dev On Thursday 04 of October 2012 17:34:42 Peter De Schrijver wrote: > > >> > > >> eg. > > >> cpu0: acpi_idle > > >> cpu1: acpi_idle > > >> > > > > > > Is this allowed? I thought sysfs files can output only 1 line of data? > > > > Ah yeah :) > > > > Does it make sense to put the following files: > > > > /sys/devices/system/cpu/cpu0/cpuidle/driver > > => acpi_idle > > /sys/devices/system/cpu/cpu1/cpuidle/driver > > => acpi_idle > > > > Sounds like a reasonable idea. > > > and the content of the file: > > > > /sys/devices/system/cpu/cpuidle/current_driver will show the driver > > associated with current cpu ? > > > > I think that's ok. Yes, that sounds good. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-09-24 22:43 ` [PATCH 4/4] cpuidle - support multiple drivers Daniel Lezcano [not found] ` <1348526634-19029-5-git-send-email-daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2012-10-04 14:14 ` Peter De Schrijver @ 2012-10-07 21:26 ` Rafael J. Wysocki 2012-10-11 10:04 ` Daniel Lezcano 2 siblings, 1 reply; 23+ messages in thread From: Rafael J. Wysocki @ 2012-10-07 21:26 UTC (permalink / raw) To: Daniel Lezcano Cc: lenb, linux-pm, lorenzo.pieralisi, pdeschrijver, linux-acpi, patches, linaro-dev On Tuesday 25 of September 2012 00:43:54 Daniel Lezcano wrote: > With the tegra3 and the big.LITTLE [1] new architectures, several cpus > with different characteristics (latencies and states) can co-exists on the > system. > > The cpuidle framework has the limitation of handling only identical cpus. > > This patch removes this limitation by introducing the multiple driver support > for cpuidle. > > This option is configurable at compile time and should be enabled for the > architectures mentioned above. So there is no impact for the other platforms > if the option is disabled. The option defaults to 'n'. Note the multiple drivers > support is also compatible with the existing drivers, even if just one driver is > needed, all the cpu will be tied to this driver using an extra small chunk of > processor memory. > > The multiple driver support use a per-cpu driver pointer instead of a global > variable and the accessor to this variable are done from a cpu context. > > In order to keep the compatibility with the existing drivers, the function > 'cpuidle_register_driver' and 'cpuidle_unregister_driver' will register > the specified driver for all the cpus. > > The sysfs output for the 'current_driver' is changed when this option is > set by giving the drivers per cpu. > > eg. > cpu0: acpi_idle > cpu1: acpi_idle > > but if the option is disabled, the output will remain the same. > > [1] http://lwn.net/Articles/481055/ > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/cpuidle/Kconfig | 9 +++ > drivers/cpuidle/cpuidle.c | 24 ++++++-- > drivers/cpuidle/driver.c | 136 +++++++++++++++++++++++++++++++++++++++++--- > drivers/cpuidle/sysfs.c | 50 +++++++++++++---- > include/linux/cpuidle.h | 8 ++- > 5 files changed, 197 insertions(+), 30 deletions(-) > > diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig > index a76b689..234ae65 100644 > --- a/drivers/cpuidle/Kconfig > +++ b/drivers/cpuidle/Kconfig > @@ -9,6 +9,15 @@ config CPU_IDLE > > If you're using an ACPI-enabled platform, you should say Y here. > > +config CPU_IDLE_MULTIPLE_DRIVERS > + bool "Support multiple cpuidle drivers" > + depends on CPU_IDLE > + default n > + help > + Allows the cpuidle framework to use different drivers for each CPU. > + This is useful if you have a system with different CPU latencies and > + states. If unsure say N. > + > config CPU_IDLE_GOV_LADDER > bool > depends on CPU_IDLE > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index e28f6ea..c581b99 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -68,7 +68,7 @@ static cpuidle_enter_t cpuidle_enter_ops; > int cpuidle_play_dead(void) > { > struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); > - struct cpuidle_driver *drv = cpuidle_get_driver(); > + struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev->cpu); It would be better to change that to be called as cpuidle_get_cpu_driver(dev), since dev is a cpuidle_device already. > int i, dead_state = -1; > int power_usage = -1; > > @@ -128,7 +128,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, > int cpuidle_idle_call(void) > { > struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); > - struct cpuidle_driver *drv = cpuidle_get_driver(); > + struct cpuidle_driver *drv; > int next_state, entered_state; > > if (off) > @@ -141,6 +141,8 @@ int cpuidle_idle_call(void) > if (!dev || !dev->enabled) > return -EBUSY; > > + drv = cpuidle_get_cpu_driver(dev->cpu); > + > /* ask the governor for the next state */ > next_state = cpuidle_curr_governor->select(drv, dev); > if (need_resched()) { > @@ -308,15 +310,19 @@ static void poll_idle_init(struct cpuidle_driver *drv) {} > int cpuidle_enable_device(struct cpuidle_device *dev) > { > int ret, i; > - struct cpuidle_driver *drv = cpuidle_get_driver(); > + struct cpuidle_driver *drv; > > if (!dev) > return -EINVAL; > > if (dev->enabled) > return 0; > + > + drv = cpuidle_get_cpu_driver(dev->cpu); > + > if (!drv || !cpuidle_curr_governor) > return -EIO; > + > if (!dev->state_count) > dev->state_count = drv->state_count; > > @@ -368,15 +374,18 @@ EXPORT_SYMBOL_GPL(cpuidle_enable_device); > */ > void cpuidle_disable_device(struct cpuidle_device *dev) > { > + struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev->cpu); > + > if (!dev->enabled) > return; > - if (!cpuidle_get_driver() || !cpuidle_curr_governor) > + > + if (!drv || !cpuidle_curr_governor) > return; > > dev->enabled = 0; > > if (cpuidle_curr_governor->disable) > - cpuidle_curr_governor->disable(cpuidle_get_driver(), dev); > + cpuidle_curr_governor->disable(drv, dev); > > cpuidle_remove_state_sysfs(dev); > enabled_devices--; > @@ -395,7 +404,8 @@ static int __cpuidle_register_device(struct cpuidle_device *dev) > { > int ret; > struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu); > - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); > + struct cpuidle_driver *cpuidle_driver = > + cpuidle_get_cpu_driver(dev->cpu); > > if (!try_module_get(cpuidle_driver->owner)) > return -EINVAL; > @@ -461,7 +471,7 @@ EXPORT_SYMBOL_GPL(cpuidle_register_device); > void cpuidle_unregister_device(struct cpuidle_device *dev) > { > struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu); > - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); > + struct cpuidle_driver *cpuidle_driver = cpuidle_get_cpu_driver(dev->cpu); > > if (dev->registered == 0) > return; > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c > index 391f80f..6529b91 100644 > --- a/drivers/cpuidle/driver.c > +++ b/drivers/cpuidle/driver.c > @@ -14,7 +14,11 @@ > > #include "cpuidle.h" > > +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > +DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers); > +#else > static struct cpuidle_driver *cpuidle_curr_driver; > +#endif > DEFINE_SPINLOCK(cpuidle_driver_lock); > > static void set_power_states(struct cpuidle_driver *drv) > @@ -47,12 +51,25 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) > set_power_states(drv); > } > > -static void cpuidle_set_driver(struct cpuidle_driver *drv) > +static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu) > { > +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > + per_cpu(cpuidle_drivers, cpu) = drv; > +#else > cpuidle_curr_driver = drv; > +#endif I'm not a big fan of #ifdef blocks inside of functions. In my opinion it's better to put entire functions under #ifdef blocks. So, for example, in this particular case I would do" #ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu) { per_cpu(cpuidle_drivers, cpu) = drv; } #else static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu) { cpuidle_curr_driver = drv; } #endif > } > > -static int __cpuidle_register_driver(struct cpuidle_driver *drv) > +static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) > +{ > +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > + return per_cpu(cpuidle_drivers, cpu); > +#else > + return cpuidle_curr_driver; > +#endif > +} And here analogously. > + > +static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu) > { > if (!drv || !drv->state_count) > return -EINVAL; > @@ -60,26 +77,102 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv) > if (cpuidle_disabled()) > return -ENODEV; > > - if (cpuidle_get_driver()) > + if (__cpuidle_get_cpu_driver(cpu)) > return -EBUSY; > > __cpuidle_driver_init(drv); > > - cpuidle_set_driver(drv); > + __cpuidle_set_cpu_driver(drv, cpu); > > return 0; > } > > -static void __cpuidle_unregister_driver(struct cpuidle_driver *drv) > +static void __cpuidle_unregister_driver(struct cpuidle_driver *drv, int cpu) > { > - if (drv != cpuidle_get_driver()) { > + if (drv != __cpuidle_get_cpu_driver(cpu)) { > WARN(1, "invalid cpuidle_unregister_driver(%s)\n", > drv->name); > return; > } > > if (!WARN_ON(drv->refcnt > 0)) > - cpuidle_set_driver(NULL); > + __cpuidle_set_cpu_driver(NULL, cpu); > +} > + > +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > +static void __cpuidle_unregister_all_cpu_driver(struct cpuidle_driver *drv) > +{ > + int cpu; > + for_each_present_cpu(cpu) > + __cpuidle_unregister_driver(drv, cpu); > +} > + > +static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv) > +{ > + int ret = 0; > + int i, cpu; > + > + for_each_present_cpu(cpu) { > + ret = __cpuidle_register_driver(drv, cpu); > + if (!ret) > + continue; > + for (i = cpu - 1; i >= 0; i--) I wonder if this is going to work in all cases. For example, is there any guarantee that the CPU numbers start from 0 and that there are no gaps? > + __cpuidle_unregister_driver(drv, i); > + break; > + } > + > + return ret; > +} > + > +static int __cpuidle_for_each_driver(int (*cb)(int, struct cpuidle_driver *, > + void *), void *data) > +{ > + struct cpuidle_driver *drv; > + int cpu, ret = 0; > + > + for_each_present_cpu(cpu) { > + drv = __cpuidle_get_cpu_driver(cpu); > + ret = cb(cpu, drv, data); > + if (ret < 0) > + break; > + } > + return ret; > +} > + > +int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu) > +{ > + int ret; > + > + spin_lock(&cpuidle_driver_lock); > + ret = __cpuidle_register_driver(drv, cpu); > + spin_unlock(&cpuidle_driver_lock); > + > + return ret; > +} > + > +void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu) > +{ > + spin_lock(&cpuidle_driver_lock); > + __cpuidle_unregister_driver(drv, cpu); > + spin_unlock(&cpuidle_driver_lock); > +} > +#endif > + > +int cpuidle_for_each_driver(int (*cb)(int, struct cpuidle_driver *, void *), > + void *data) > +{ > + int ret; > + > + spin_lock(&cpuidle_driver_lock); > +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > + ret = __cpuidle_for_each_driver(cb, data); > +#else > + ret = cb(smp_processor_id(), > + __cpuidle_get_cpu_driver(smp_processor_id()), data); > +#endif Here I'd make the definition of __cpuidle_for_each_driver() depend on whether or not CONFIG_CPU_IDLE_MULTIPLE_DRIVERS is set. > + spin_unlock(&cpuidle_driver_lock); > + > + return ret; > } > > /** > @@ -91,7 +184,11 @@ int cpuidle_register_driver(struct cpuidle_driver *drv) > int ret; > > spin_lock(&cpuidle_driver_lock); > - ret = __cpuidle_register_driver(drv); > +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > + ret = __cpuidle_register_all_cpu_driver(drv); > +#else > + ret = __cpuidle_register_driver(drv, smp_processor_id()); > +#endif And analogously here. > spin_unlock(&cpuidle_driver_lock); > > return ret; > @@ -103,18 +200,37 @@ EXPORT_SYMBOL_GPL(cpuidle_register_driver); > */ > struct cpuidle_driver *cpuidle_get_driver(void) > { > - return cpuidle_curr_driver; > + return __cpuidle_get_cpu_driver(smp_processor_id()); > } > EXPORT_SYMBOL_GPL(cpuidle_get_driver); > > /** > + * cpuidle_get_cpu_driver - return the driver tied with a cpu > + */ > +struct cpuidle_driver *cpuidle_get_cpu_driver(int cpu) > +{ > + struct cpuidle_driver *drv; > + > + spin_lock(&cpuidle_driver_lock); > + drv = __cpuidle_get_cpu_driver(cpu); > + spin_unlock(&cpuidle_driver_lock); > + > + return drv; > +} > +EXPORT_SYMBOL_GPL(cpuidle_get_cpu_driver); > + > +/** > * cpuidle_unregister_driver - unregisters a driver > * @drv: the driver > */ > void cpuidle_unregister_driver(struct cpuidle_driver *drv) > { > spin_lock(&cpuidle_driver_lock); > - __cpuidle_unregister_driver(drv); > +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > + __cpuidle_unregister_all_cpu_driver(drv); > +#else > + __cpuidle_unregister_driver(drv, smp_processor_id()); > +#endif I'm slightly cautious about using smp_processor_id() above. get_cpu()/put_cpu() is the preferred way of doing this nowadays (although this particular code is under the spinlock, so it should be OK). > spin_unlock(&cpuidle_driver_lock); > } > EXPORT_SYMBOL_GPL(cpuidle_unregister_driver); > diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c > index 5f809e3..2596422 100644 > --- a/drivers/cpuidle/sysfs.c > +++ b/drivers/cpuidle/sysfs.c > @@ -43,21 +43,46 @@ out: > return i; > } > > +struct cbarg { > + char *buf; > + ssize_t count; > +}; > + > +static int each_driver_cb(int cpu, struct cpuidle_driver *drv, void *data) > +{ > + int ret; > + struct cbarg *cbarg = data; > + > + if ((drv && (strlen(drv->name) + cbarg->count) >= PAGE_SIZE) || > + (!drv && (strlen("none") + cbarg->count) >= PAGE_SIZE)) > + return -EOVERFLOW; > + > +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > + ret = sprintf(cbarg->buf + cbarg->count, "cpu%d: %s\n", > + cpu, drv ? drv->name : "none" ); > +#else > + ret = sprintf(cbarg->buf, "%s\n", drv ? drv->name : "none"); > +#endif > + if (ret < 0) > + return ret; > + > + cbarg->count += ret; > + > + return 0; > +} > + > static ssize_t show_current_driver(struct device *dev, > struct device_attribute *attr, > char *buf) > { > - ssize_t ret; > - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); > + struct cbarg cbarg = { .buf = buf }; > + int ret; > > - spin_lock(&cpuidle_driver_lock); > - if (cpuidle_driver) > - ret = sprintf(buf, "%s\n", cpuidle_driver->name); > - else > - ret = sprintf(buf, "none\n"); > - spin_unlock(&cpuidle_driver_lock); > + ret = cpuidle_for_each_driver(each_driver_cb, &cbarg); > + if (ret < 0) > + return ret; > > - return ret; > + return cbarg.count; > } > > static ssize_t show_current_governor(struct device *dev, > @@ -363,10 +388,10 @@ int cpuidle_add_state_sysfs(struct cpuidle_device *device) > { > int i, ret = -ENOMEM; > struct cpuidle_state_kobj *kobj; > - struct cpuidle_driver *drv = cpuidle_get_driver(); > + struct cpuidle_driver *drv = cpuidle_get_cpu_driver(device->cpu); > > /* state statistics */ > - for (i = 0; i < device->state_count; i++) { > + for (i = 0; i < drv->state_count; i++) { > kobj = kzalloc(sizeof(struct cpuidle_state_kobj), GFP_KERNEL); > if (!kobj) > goto error_state; > @@ -374,7 +399,8 @@ int cpuidle_add_state_sysfs(struct cpuidle_device *device) > kobj->state_usage = &device->states_usage[i]; > init_completion(&kobj->kobj_unregister); > > - ret = kobject_init_and_add(&kobj->kobj, &ktype_state_cpuidle, &device->kobj, > + ret = kobject_init_and_add(&kobj->kobj, > + &ktype_state_cpuidle, &device->kobj, > "state%d", i); > if (ret) { > kfree(kobj); > diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h > index a4ff9f8..0e0b0ad 100644 > --- a/include/linux/cpuidle.h > +++ b/include/linux/cpuidle.h > @@ -164,6 +164,13 @@ extern int cpuidle_wrap_enter(struct cpuidle_device *dev, > struct cpuidle_driver *drv, int index)); > extern int cpuidle_play_dead(void); > > +extern int cpuidle_for_each_driver( > + int (*cb)(int, struct cpuidle_driver *, void *), void *data); > + > +extern struct cpuidle_driver *cpuidle_get_cpu_driver(int cpu); > +extern int cpuidle_register_cpu_driver(struct cpuidle_driver *drv, int cpu); > +extern void cpuidle_unregister_cpu_driver(struct cpuidle_driver *drv, int cpu); > + > #else > static inline void disable_cpuidle(void) { } > static inline int cpuidle_idle_call(void) { return -ENODEV; } > @@ -190,7 +197,6 @@ static inline int cpuidle_wrap_enter(struct cpuidle_device *dev, > struct cpuidle_driver *drv, int index)) > { return -ENODEV; } > static inline int cpuidle_play_dead(void) {return -ENODEV; } > - > #endif > > #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED > Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-10-07 21:26 ` Rafael J. Wysocki @ 2012-10-11 10:04 ` Daniel Lezcano 2012-10-11 17:21 ` Rafael J. Wysocki 0 siblings, 1 reply; 23+ messages in thread From: Daniel Lezcano @ 2012-10-11 10:04 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Daniel Lezcano, linaro-dev, linux-pm, pdeschrijver, patches, linux-acpi, lenb On 10/07/2012 11:26 PM, Rafael J. Wysocki wrote: > On Tuesday 25 of September 2012 00:43:54 Daniel Lezcano wrote: >> With the tegra3 and the big.LITTLE [1] new architectures, several cpus >> with different characteristics (latencies and states) can co-exists on the >> system. >> >> The cpuidle framework has the limitation of handling only identical cpus. >> >> This patch removes this limitation by introducing the multiple driver support >> for cpuidle. >> >> This option is configurable at compile time and should be enabled for the >> architectures mentioned above. So there is no impact for the other platforms >> if the option is disabled. The option defaults to 'n'. Note the multiple drivers >> support is also compatible with the existing drivers, even if just one driver is >> needed, all the cpu will be tied to this driver using an extra small chunk of >> processor memory. >> >> The multiple driver support use a per-cpu driver pointer instead of a global >> variable and the accessor to this variable are done from a cpu context. Thanks Rafael for the review. I took into account all your remarks for the V2. [ cut ] >> +static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv) >> +{ >> + int ret = 0; >> + int i, cpu; >> + >> + for_each_present_cpu(cpu) { >> + ret = __cpuidle_register_driver(drv, cpu); >> + if (!ret) >> + continue; >> + for (i = cpu - 1; i >= 0; i--) > I wonder if this is going to work in all cases. For example, is there any > guarantee that the CPU numbers start from 0 and that there are no gaps? AFAICS, the cpumask.h is not assuming the cpu numbers start from zero and they are contiguous. I will fix this reverse loop, thanks for spotting this. [ cut ] >> void cpuidle_unregister_driver(struct cpuidle_driver *drv) >> { >> spin_lock(&cpuidle_driver_lock); >> - __cpuidle_unregister_driver(drv); >> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS >> + __cpuidle_unregister_all_cpu_driver(drv); >> +#else >> + __cpuidle_unregister_driver(drv, smp_processor_id()); >> +#endif > I'm slightly cautious about using smp_processor_id() above. > get_cpu()/put_cpu() is the preferred way of doing this nowadays (although > this particular code is under the spinlock, so it should be OK). yes, get_cpu does preempt_disable() and smp_processor_id() As spin_lock does also preempt_disable() that should be ok. But I changed the code to use the preferred way. Thanks -- Daniel -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-10-11 10:04 ` Daniel Lezcano @ 2012-10-11 17:21 ` Rafael J. Wysocki 2012-10-11 17:39 ` Daniel Lezcano 0 siblings, 1 reply; 23+ messages in thread From: Rafael J. Wysocki @ 2012-10-11 17:21 UTC (permalink / raw) To: Daniel Lezcano Cc: linaro-dev, linux-pm, pdeschrijver, patches, linux-acpi, lenb On Thursday 11 of October 2012 12:04:37 Daniel Lezcano wrote: > On 10/07/2012 11:26 PM, Rafael J. Wysocki wrote: > > On Tuesday 25 of September 2012 00:43:54 Daniel Lezcano wrote: > >> With the tegra3 and the big.LITTLE [1] new architectures, several cpus > >> with different characteristics (latencies and states) can co-exists on the > >> system. > >> > >> The cpuidle framework has the limitation of handling only identical cpus. > >> > >> This patch removes this limitation by introducing the multiple driver support > >> for cpuidle. > >> > >> This option is configurable at compile time and should be enabled for the > >> architectures mentioned above. So there is no impact for the other platforms > >> if the option is disabled. The option defaults to 'n'. Note the multiple drivers > >> support is also compatible with the existing drivers, even if just one driver is > >> needed, all the cpu will be tied to this driver using an extra small chunk of > >> processor memory. > >> > >> The multiple driver support use a per-cpu driver pointer instead of a global > >> variable and the accessor to this variable are done from a cpu context. > > Thanks Rafael for the review. > > I took into account all your remarks for the V2. > > [ cut ] > > >> +static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv) > >> +{ > >> + int ret = 0; > >> + int i, cpu; > >> + > >> + for_each_present_cpu(cpu) { > >> + ret = __cpuidle_register_driver(drv, cpu); > >> + if (!ret) > >> + continue; > >> + for (i = cpu - 1; i >= 0; i--) > > I wonder if this is going to work in all cases. For example, is there any > > guarantee that the CPU numbers start from 0 and that there are no gaps? > > AFAICS, the cpumask.h is not assuming the cpu numbers start from zero > and they are contiguous. > > I will fix this reverse loop, thanks for spotting this. > > [ cut ] > > >> void cpuidle_unregister_driver(struct cpuidle_driver *drv) > >> { > >> spin_lock(&cpuidle_driver_lock); > >> - __cpuidle_unregister_driver(drv); > >> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS > >> + __cpuidle_unregister_all_cpu_driver(drv); > >> +#else > >> + __cpuidle_unregister_driver(drv, smp_processor_id()); > >> +#endif > > I'm slightly cautious about using smp_processor_id() above. > > get_cpu()/put_cpu() is the preferred way of doing this nowadays (although > > this particular code is under the spinlock, so it should be OK). > yes, get_cpu does preempt_disable() and smp_processor_id() > As spin_lock does also preempt_disable() that should be ok. > But I changed the code to use the preferred way. Cool, thanks! I've seen your new version and I'm going to look deeper into it in the next few days, but I will be travelling from tomorrow through the end of the next week, so I may be even slower to respond than usually. Sorry about that. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] cpuidle - support multiple drivers 2012-10-11 17:21 ` Rafael J. Wysocki @ 2012-10-11 17:39 ` Daniel Lezcano 0 siblings, 0 replies; 23+ messages in thread From: Daniel Lezcano @ 2012-10-11 17:39 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Daniel Lezcano, linaro-dev, patches, pdeschrijver, linux-pm, linux-acpi, lenb On 10/11/2012 07:21 PM, Rafael J. Wysocki wrote: > On Thursday 11 of October 2012 12:04:37 Daniel Lezcano wrote: >> On 10/07/2012 11:26 PM, Rafael J. Wysocki wrote: >>> On Tuesday 25 of September 2012 00:43:54 Daniel Lezcano wrote: >>>> With the tegra3 and the big.LITTLE [1] new architectures, several cpus >>>> with different characteristics (latencies and states) can co-exists on the >>>> system. >>>> >>>> The cpuidle framework has the limitation of handling only identical cpus. >>>> >>>> This patch removes this limitation by introducing the multiple driver support >>>> for cpuidle. >>>> >>>> This option is configurable at compile time and should be enabled for the >>>> architectures mentioned above. So there is no impact for the other platforms >>>> if the option is disabled. The option defaults to 'n'. Note the multiple drivers >>>> support is also compatible with the existing drivers, even if just one driver is >>>> needed, all the cpu will be tied to this driver using an extra small chunk of >>>> processor memory. >>>> >>>> The multiple driver support use a per-cpu driver pointer instead of a global >>>> variable and the accessor to this variable are done from a cpu context. >> Thanks Rafael for the review. >> >> I took into account all your remarks for the V2. >> >> [ cut ] >> >>>> +static int __cpuidle_register_all_cpu_driver(struct cpuidle_driver *drv) >>>> +{ >>>> + int ret = 0; >>>> + int i, cpu; >>>> + >>>> + for_each_present_cpu(cpu) { >>>> + ret = __cpuidle_register_driver(drv, cpu); >>>> + if (!ret) >>>> + continue; >>>> + for (i = cpu - 1; i >= 0; i--) >>> I wonder if this is going to work in all cases. For example, is there any >>> guarantee that the CPU numbers start from 0 and that there are no gaps? >> AFAICS, the cpumask.h is not assuming the cpu numbers start from zero >> and they are contiguous. >> >> I will fix this reverse loop, thanks for spotting this. >> >> [ cut ] >> >>>> void cpuidle_unregister_driver(struct cpuidle_driver *drv) >>>> { >>>> spin_lock(&cpuidle_driver_lock); >>>> - __cpuidle_unregister_driver(drv); >>>> +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS >>>> + __cpuidle_unregister_all_cpu_driver(drv); >>>> +#else >>>> + __cpuidle_unregister_driver(drv, smp_processor_id()); >>>> +#endif >>> I'm slightly cautious about using smp_processor_id() above. >>> get_cpu()/put_cpu() is the preferred way of doing this nowadays (although >>> this particular code is under the spinlock, so it should be OK). >> yes, get_cpu does preempt_disable() and smp_processor_id() >> As spin_lock does also preempt_disable() that should be ok. >> But I changed the code to use the preferred way. > Cool, thanks! > > I've seen your new version Actually, the patchset is just sysfs cleanup for the multiple drivers support. The patches are trivial IMO. If you think they are ok and they are merged, I will send the multiple drivers patchset V2 on top of them. > and I'm going to look deeper into it in the next > few days, but I will be travelling from tomorrow through the end of the next > week, so I may be even slower to respond than usually. Sorry about that. No problem, I understand. Thanks -- Daniel -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/4] cpuidle - support multiple drivers at a time 2012-09-24 22:43 [PATCH 0/4] cpuidle - support multiple drivers at a time Daniel Lezcano ` (3 preceding siblings ...) 2012-09-24 22:43 ` [PATCH 4/4] cpuidle - support multiple drivers Daniel Lezcano @ 2012-10-04 14:16 ` Peter De Schrijver 2012-10-04 14:27 ` Daniel Lezcano 4 siblings, 1 reply; 23+ messages in thread From: Peter De Schrijver @ 2012-10-04 14:16 UTC (permalink / raw) To: Daniel Lezcano Cc: rjw, lenb, linux-pm, lorenzo.pieralisi, linux-acpi, patches, linaro-dev On Tue, Sep 25, 2012 at 12:43:50AM +0200, Daniel Lezcano wrote: > The discussion about having different cpus on the system with > different latencies bring us to a first attemp by adding a > pointer in the cpuidle_device to the states array. > > But as Rafael suggested, it would make more sense to create a > driver per cpu [1]. > > This patch adds support for multiple cpuidle drivers. > > It creates a per cpu cpuidle driver pointer. > > In order to not break the different drivers, the function cpuidle_register_driver > assign for each cpu, the driver. > > The multiple driver support is optional and if it is not set, the cpuide driver > core code remains the same (except some code reorganisation). > > I did the following tests compiled, booted, tested without/with CONFIG_CPU_IDLE, > with/without CONFIG_CPU_IDLE_MULTIPLE_DRIVERS. > > Tested on Core2 Duo T9500 with acpi_idle [and intel_idle] > Tested on ARM Dual Cortex-A9 U8500 (aka Snowball) > Tested on Cardhu (Tegra30). Besides the possibly problem with the sysfs format: Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com> Cheers, Peter. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/4] cpuidle - support multiple drivers at a time 2012-10-04 14:16 ` [PATCH 0/4] cpuidle - support multiple drivers at a time Peter De Schrijver @ 2012-10-04 14:27 ` Daniel Lezcano 0 siblings, 0 replies; 23+ messages in thread From: Daniel Lezcano @ 2012-10-04 14:27 UTC (permalink / raw) To: Peter De Schrijver Cc: rjw, lenb, linux-pm, lorenzo.pieralisi, linux-acpi, patches, linaro-dev On 10/04/2012 04:16 PM, Peter De Schrijver wrote: > On Tue, Sep 25, 2012 at 12:43:50AM +0200, Daniel Lezcano wrote: >> The discussion about having different cpus on the system with >> different latencies bring us to a first attemp by adding a >> pointer in the cpuidle_device to the states array. >> >> But as Rafael suggested, it would make more sense to create a >> driver per cpu [1]. >> >> This patch adds support for multiple cpuidle drivers. >> >> It creates a per cpu cpuidle driver pointer. >> >> In order to not break the different drivers, the function cpuidle_register_driver >> assign for each cpu, the driver. >> >> The multiple driver support is optional and if it is not set, the cpuide driver >> core code remains the same (except some code reorganisation). >> >> I did the following tests compiled, booted, tested without/with CONFIG_CPU_IDLE, >> with/without CONFIG_CPU_IDLE_MULTIPLE_DRIVERS. >> >> Tested on Core2 Duo T9500 with acpi_idle [and intel_idle] >> Tested on ARM Dual Cortex-A9 U8500 (aka Snowball) >> > > Tested on Cardhu (Tegra30). > > Besides the possibly problem with the sysfs format: > > Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com> Thanks a lot Peter for testing and reviewing the patchset. -- Daniel -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2012-10-11 17:39 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-24 22:43 [PATCH 0/4] cpuidle - support multiple drivers at a time Daniel Lezcano
2012-09-24 22:43 ` [PATCH 1/4] cpuidle : move driver's refcount to cpuidle Daniel Lezcano
2012-09-24 22:43 ` [PATCH 2/4] cpuidle : move driver checking within the lock section Daniel Lezcano
[not found] ` <1348526634-19029-3-git-send-email-daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2012-10-07 20:17 ` Rafael J. Wysocki
2012-09-24 22:43 ` [PATCH 3/4] cpuidle - prepare the driver core to be multi drivers aware Daniel Lezcano
2012-09-24 22:43 ` [PATCH 4/4] cpuidle - support multiple drivers Daniel Lezcano
[not found] ` <1348526634-19029-5-git-send-email-daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2012-09-29 9:41 ` Francesco Lavra
2012-09-29 17:44 ` Daniel Lezcano
2012-09-29 22:07 ` Rafael J. Wysocki
2012-09-30 16:34 ` Daniel Lezcano
2012-10-02 2:08 ` Rafael J. Wysocki
[not found] ` <3723306.GAm1cPlZaP-sKB8Sp2ER+y1GS7QM15AGw@public.gmane.org>
2012-10-02 12:27 ` Daniel Lezcano
2012-10-02 14:02 ` Rafael J. Wysocki
2012-10-04 14:14 ` Peter De Schrijver
2012-10-04 14:26 ` Daniel Lezcano
2012-10-04 14:34 ` Peter De Schrijver
2012-10-07 21:09 ` Rafael J. Wysocki
2012-10-07 21:26 ` Rafael J. Wysocki
2012-10-11 10:04 ` Daniel Lezcano
2012-10-11 17:21 ` Rafael J. Wysocki
2012-10-11 17:39 ` Daniel Lezcano
2012-10-04 14:16 ` [PATCH 0/4] cpuidle - support multiple drivers at a time Peter De Schrijver
2012-10-04 14:27 ` Daniel Lezcano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.