From 9c9a43ed2734081124407c779b36a4761c41139b Mon Sep 17 00:00:00 2001 From: Mattia Dongili Date: Wed, 5 Jul 2006 23:12:20 +0200 Subject: [CPUFREQ] return error when failing to set minfreq I just stumbled on this bug/feature, this is how to reproduce it: # echo 450000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq # echo 450000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq # echo powersave > /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor # cpufreq-info -p 450000 450000 powersave # echo 1800000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq ; echo $? 0 # cpufreq-info -p 450000 450000 powersave Here it is. The kernel refuses to set a min_freq higher than the max_freq but it allows a max_freq lower than min_freq (lowering min_freq also). This behaviour is pretty straightforward (but undocumented) and it doesn't return an error altough failing to accomplish the requested action (set min_freq). The problem (IMO) is basically that userspace is not allowed to set a full policy atomically while the kernel always does that thus it must enforce an ordering on operations. The attached patch returns -EINVAL if trying to increase frequencies starting from scaling_min_freq and documents the correct ordering of writes. Signed-off-by: Mattia Dongili Signed-off-by: Dominik Brodowski Signed-off-by: Dave Jones -- --- drivers/cpufreq/cpufreq.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'drivers/cpufreq/cpufreq.c') diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index bc1088d9b379..ad996c772c8b 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1343,6 +1343,11 @@ static int __cpufreq_set_policy(struct cpufreq_policy *data, struct cpufreq_poli memcpy(&policy->cpuinfo, &data->cpuinfo, sizeof(struct cpufreq_cpuinfo)); + if (policy->min > data->min && policy->min > policy->max) { + ret = -EINVAL; + goto error_out; + } + /* verify the cpu speed can be set within this limit */ ret = cpufreq_driver->verify(policy); if (ret) -- cgit v1.2.2 From 3bcb09a35641f2840bd59d8f82154f830dca282c Mon Sep 17 00:00:00 2001 From: Jeremy Fitzhardinge Date: Thu, 6 Jul 2006 12:30:26 -0700 Subject: [CPUFREQ] [1/2] add __find_governor helper and clean up some error handling. Adds a __find_governor() helper function to look up a governor by name. Also restructures some error handling to conform to the "single-exit" model which is generally preferred for kernel code. Signed-off-by: Jeremy Fitzhardinge Signed-off-by: Dave Jones --- drivers/cpufreq/cpufreq.c | 57 ++++++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 23 deletions(-) (limited to 'drivers/cpufreq/cpufreq.c') diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index ad996c772c8b..9b416372a8e4 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -284,39 +284,52 @@ EXPORT_SYMBOL_GPL(cpufreq_notify_transition); * SYSFS INTERFACE * *********************************************************************/ +static struct cpufreq_governor *__find_governor(const char *str_governor) +{ + struct cpufreq_governor *t; + + list_for_each_entry(t, &cpufreq_governor_list, governor_list) + if (!strnicmp(str_governor,t->name,CPUFREQ_NAME_LEN)) + return t; + + return NULL; +} + /** * cpufreq_parse_governor - parse a governor string */ static int cpufreq_parse_governor (char *str_governor, unsigned int *policy, struct cpufreq_governor **governor) { + int err = -EINVAL; + if (!cpufreq_driver) - return -EINVAL; + goto out; + if (cpufreq_driver->setpolicy) { if (!strnicmp(str_governor, "performance", CPUFREQ_NAME_LEN)) { *policy = CPUFREQ_POLICY_PERFORMANCE; - return 0; + err = 0; } else if (!strnicmp(str_governor, "powersave", CPUFREQ_NAME_LEN)) { *policy = CPUFREQ_POLICY_POWERSAVE; - return 0; + err = 0; } - return -EINVAL; - } else { + } else if (cpufreq_driver->target) { struct cpufreq_governor *t; + mutex_lock(&cpufreq_governor_mutex); - if (!cpufreq_driver || !cpufreq_driver->target) - goto out; - list_for_each_entry(t, &cpufreq_governor_list, governor_list) { - if (!strnicmp(str_governor,t->name,CPUFREQ_NAME_LEN)) { - *governor = t; - mutex_unlock(&cpufreq_governor_mutex); - return 0; - } + + t = __find_governor(str_governor); + + if (t != NULL) { + *governor = t; + err = 0; } -out: + mutex_unlock(&cpufreq_governor_mutex); } - return -EINVAL; + out: + return err; } @@ -1265,23 +1278,21 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event) int cpufreq_register_governor(struct cpufreq_governor *governor) { - struct cpufreq_governor *t; + int err; if (!governor) return -EINVAL; mutex_lock(&cpufreq_governor_mutex); - list_for_each_entry(t, &cpufreq_governor_list, governor_list) { - if (!strnicmp(governor->name,t->name,CPUFREQ_NAME_LEN)) { - mutex_unlock(&cpufreq_governor_mutex); - return -EBUSY; - } + err = -EBUSY; + if (__find_governor(governor->name) == NULL) { + err = 0; + list_add(&governor->governor_list, &cpufreq_governor_list); } - list_add(&governor->governor_list, &cpufreq_governor_list); mutex_unlock(&cpufreq_governor_mutex); - return 0; + return err; } EXPORT_SYMBOL_GPL(cpufreq_register_governor); -- cgit v1.2.2 From ea71497020c55cd39221e0abad5c1752ac6e3f47 Mon Sep 17 00:00:00 2001 From: Jeremy Fitzhardinge Date: Thu, 6 Jul 2006 12:32:01 -0700 Subject: [CPUFREQ] [2/2] demand load governor modules. Demand-load cpufreq governor modules if needed. Signed-off-by: Jeremy Fitzhardinge Signed-off-by: Dave Jones --- drivers/cpufreq/cpufreq.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) (limited to 'drivers/cpufreq/cpufreq.c') diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 9b416372a8e4..b3df613ae4ec 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -321,6 +321,23 @@ static int cpufreq_parse_governor (char *str_governor, unsigned int *policy, t = __find_governor(str_governor); + if (t == NULL) { + char *name = kasprintf(GFP_KERNEL, "cpufreq_%s", str_governor); + + if (name) { + int ret; + + mutex_unlock(&cpufreq_governor_mutex); + ret = request_module(name); + mutex_lock(&cpufreq_governor_mutex); + + if (ret == 0) + t = __find_governor(str_governor); + } + + kfree(name); + } + if (t != NULL) { *governor = t; err = 0; -- cgit v1.2.2