From 23c7b54ca1cd1797ef39169ab85e6d46f1c2d061 Mon Sep 17 00:00:00 2001 From: Enric Balletbo i Serra Date: Wed, 4 Jul 2018 10:45:50 +0200 Subject: PM / devfreq: Fix devfreq_add_device() when drivers are built as modules. When the devfreq driver and the governor driver are built as modules, the call to devfreq_add_device() or governor_store() fails because the governor driver is not loaded at the time the devfreq driver loads. The devfreq driver has a build dependency on the governor but also should have a runtime dependency. We need to make sure that the governor driver is loaded before the devfreq driver. This patch fixes this bug by adding a try_then_request_governor() function. First tries to find the governor, and then, if it is not found, it requests the module and tries again. Fixes: 1b5c1be2c88e (PM / devfreq: map devfreq drivers to governor using name) Signed-off-by: Enric Balletbo i Serra Reviewed-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/devfreq.c | 53 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 4c49bb1330b5..62ead442a872 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -11,6 +11,7 @@ */ #include +#include #include #include #include @@ -221,6 +222,49 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) return ERR_PTR(-ENODEV); } +/** + * try_then_request_governor() - Try to find the governor and request the + * module if is not found. + * @name: name of the governor + * + * Search the list of devfreq governors and request the module and try again + * if is not found. This can happen when both drivers (the governor driver + * and the driver that call devfreq_add_device) are built as modules. + * devfreq_list_lock should be held by the caller. Returns the matched + * governor's pointer. + */ +static struct devfreq_governor *try_then_request_governor(const char *name) +{ + struct devfreq_governor *governor; + int err = 0; + + if (IS_ERR_OR_NULL(name)) { + pr_err("DEVFREQ: %s: Invalid parameters\n", __func__); + return ERR_PTR(-EINVAL); + } + WARN(!mutex_is_locked(&devfreq_list_lock), + "devfreq_list_lock must be locked."); + + governor = find_devfreq_governor(name); + if (IS_ERR(governor)) { + mutex_unlock(&devfreq_list_lock); + + if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, + DEVFREQ_NAME_LEN)) + err = request_module("governor_%s", "simpleondemand"); + else + err = request_module("governor_%s", name); + /* Restore previous state before return */ + mutex_lock(&devfreq_list_lock); + if (err) + return NULL; + + governor = find_devfreq_governor(name); + } + + return governor; +} + static int devfreq_notify_transition(struct devfreq *devfreq, struct devfreq_freqs *freqs, unsigned int state) { @@ -646,9 +690,8 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_unlock(&devfreq->lock); mutex_lock(&devfreq_list_lock); - list_add(&devfreq->node, &devfreq_list); - governor = find_devfreq_governor(devfreq->governor_name); + governor = try_then_request_governor(devfreq->governor_name); if (IS_ERR(governor)) { dev_err(dev, "%s: Unable to find governor for the device\n", __func__); @@ -664,12 +707,14 @@ struct devfreq *devfreq_add_device(struct device *dev, __func__); goto err_init; } + + list_add(&devfreq->node, &devfreq_list); + mutex_unlock(&devfreq_list_lock); return devfreq; err_init: - list_del(&devfreq->node); mutex_unlock(&devfreq_list_lock); device_unregister(&devfreq->dev); @@ -991,7 +1036,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr, return -EINVAL; mutex_lock(&devfreq_list_lock); - governor = find_devfreq_governor(str_governor); + governor = try_then_request_governor(str_governor); if (IS_ERR(governor)) { ret = PTR_ERR(governor); goto out; -- cgit From d0e464205b8a1fa21357aad0bbf136500d7e688d Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Tue, 24 Apr 2018 12:46:39 -0700 Subject: PM / devfreq: Drop custom MIN/MAX macros Drop the custom MIN/MAX macros in favour of the standard min/max from kernel.h Signed-off-by: Bjorn Andersson Reviewed-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/devfreq.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 62ead442a872..329c5e3f3338 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -29,9 +29,6 @@ #include #include "governor.h" -#define MAX(a,b) ((a > b) ? a : b) -#define MIN(a,b) ((a < b) ? a : b) - static struct class *devfreq_class; /* @@ -324,8 +321,8 @@ int update_devfreq(struct devfreq *devfreq) * max_freq * min_freq */ - max_freq = MIN(devfreq->scaling_max_freq, devfreq->max_freq); - min_freq = MAX(devfreq->scaling_min_freq, devfreq->min_freq); + max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq); + min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq); if (min_freq && freq < min_freq) { freq = min_freq; @@ -1197,7 +1194,7 @@ static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr, { struct devfreq *df = to_devfreq(dev); - return sprintf(buf, "%lu\n", MAX(df->scaling_min_freq, df->min_freq)); + return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq)); } static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, @@ -1233,7 +1230,7 @@ static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr, { struct devfreq *df = to_devfreq(dev); - return sprintf(buf, "%lu\n", MIN(df->scaling_max_freq, df->max_freq)); + return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq)); } static DEVICE_ATTR_RW(max_freq); -- cgit From df5cf4a36178c5d4f2b8b9469cb2f722e64cd102 Mon Sep 17 00:00:00 2001 From: Matthias Kaehlcke Date: Fri, 3 Aug 2018 13:05:09 -0700 Subject: PM / devfreq: Fix handling of min/max_freq == 0 Commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when adding the devfreq device") initializes df->min/max_freq with the min/max OPP when the device is added. Later commit f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency") adds df->scaling_min/max_freq and the following to the frequency adjustment code: max_freq = MIN(devfreq->scaling_max_freq, devfreq->max_freq); With the current handling of min/max_freq this is incorrect: Even though df->max_freq is now initialized to a value != 0 user space can still set it to 0, in this case max_freq would be 0 instead of df->scaling_max_freq as intended. In consequence the frequency adjustment is not performed: if (max_freq && freq > max_freq) { freq = max_freq; To fix this set df->min/max freq to the min/max OPP in max/max_freq_store, when the user passes a value of 0. This also prevents df->max_freq from being set below the min OPP when df->min_freq is 0, and similar for min_freq. Since it is now guaranteed that df->min/max_freq can't be 0 the checks for this case can be removed. Fixes: f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency") Signed-off-by: Matthias Kaehlcke Reviewed-by: Brian Norris Reviewed-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/devfreq.c | 42 ++++++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 329c5e3f3338..b5e7af60723c 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -324,11 +324,11 @@ int update_devfreq(struct devfreq *devfreq) max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq); min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq); - if (min_freq && freq < min_freq) { + if (freq < min_freq) { freq = min_freq; flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */ } - if (max_freq && freq > max_freq) { + if (freq > max_freq) { freq = max_freq; flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */ } @@ -1168,17 +1168,26 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, struct devfreq *df = to_devfreq(dev); unsigned long value; int ret; - unsigned long max; ret = sscanf(buf, "%lu", &value); if (ret != 1) return -EINVAL; mutex_lock(&df->lock); - max = df->max_freq; - if (value && max && value > max) { - ret = -EINVAL; - goto unlock; + + if (value) { + if (value > df->max_freq) { + ret = -EINVAL; + goto unlock; + } + } else { + unsigned long *freq_table = df->profile->freq_table; + + /* Get minimum frequency according to sorting order */ + if (freq_table[0] < freq_table[df->profile->max_state - 1]) + value = freq_table[0]; + else + value = freq_table[df->profile->max_state - 1]; } df->min_freq = value; @@ -1203,17 +1212,26 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, struct devfreq *df = to_devfreq(dev); unsigned long value; int ret; - unsigned long min; ret = sscanf(buf, "%lu", &value); if (ret != 1) return -EINVAL; mutex_lock(&df->lock); - min = df->min_freq; - if (value && min && value < min) { - ret = -EINVAL; - goto unlock; + + if (value) { + if (value < df->min_freq) { + ret = -EINVAL; + goto unlock; + } + } else { + unsigned long *freq_table = df->profile->freq_table; + + /* Get maximum frequency according to sorting order */ + if (freq_table[0] < freq_table[df->profile->max_state - 1]) + value = freq_table[df->profile->max_state - 1]; + else + value = freq_table[0]; } df->max_freq = value; -- cgit From 6ff66e2a008337b8a005fd0ae2037bed716262cc Mon Sep 17 00:00:00 2001 From: Matthias Kaehlcke Date: Fri, 3 Aug 2018 13:05:10 -0700 Subject: PM / devfreq: Don't adjust to user limits in governors Several governors use the user space limits df->min/max_freq to adjust the target frequency. This is not necessary, since update_devfreq() already takes care of this. Instead the governor can request the available min/max frequency by setting the target frequency to DEVFREQ_MIN/MAX_FREQ and let update_devfreq() take care of any adjustments. Signed-off-by: Matthias Kaehlcke Reviewed-by: Brian Norris Reviewed-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/governor.h | 3 +++ drivers/devfreq/governor_performance.c | 5 +---- drivers/devfreq/governor_powersave.c | 2 +- drivers/devfreq/governor_simpleondemand.c | 12 +++--------- drivers/devfreq/governor_userspace.c | 16 ++++------------ 5 files changed, 12 insertions(+), 26 deletions(-) diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h index cfc50a61a90d..b81700244ce3 100644 --- a/drivers/devfreq/governor.h +++ b/drivers/devfreq/governor.h @@ -25,6 +25,9 @@ #define DEVFREQ_GOV_SUSPEND 0x4 #define DEVFREQ_GOV_RESUME 0x5 +#define DEVFREQ_MIN_FREQ 0 +#define DEVFREQ_MAX_FREQ ULONG_MAX + /** * struct devfreq_governor - Devfreq policy governor * @node: list node - contains registered devfreq governors diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c index 4d23ecfbd948..ded429fd51be 100644 --- a/drivers/devfreq/governor_performance.c +++ b/drivers/devfreq/governor_performance.c @@ -20,10 +20,7 @@ static int devfreq_performance_func(struct devfreq *df, * target callback should be able to get floor value as * said in devfreq.h */ - if (!df->max_freq) - *freq = UINT_MAX; - else - *freq = df->max_freq; + *freq = DEVFREQ_MAX_FREQ; return 0; } diff --git a/drivers/devfreq/governor_powersave.c b/drivers/devfreq/governor_powersave.c index 0c42f23249ef..9e8897f5ac42 100644 --- a/drivers/devfreq/governor_powersave.c +++ b/drivers/devfreq/governor_powersave.c @@ -20,7 +20,7 @@ static int devfreq_powersave_func(struct devfreq *df, * target callback should be able to get ceiling value as * said in devfreq.h */ - *freq = df->min_freq; + *freq = DEVFREQ_MIN_FREQ; return 0; } diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c index 28e0f2de7100..c0417f0e081e 100644 --- a/drivers/devfreq/governor_simpleondemand.c +++ b/drivers/devfreq/governor_simpleondemand.c @@ -27,7 +27,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD; unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; struct devfreq_simple_ondemand_data *data = df->data; - unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX; err = devfreq_update_stats(df); if (err) @@ -47,7 +46,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, /* Assume MAX if it is going to be divided by zero */ if (stat->total_time == 0) { - *freq = max; + *freq = DEVFREQ_MAX_FREQ; return 0; } @@ -60,13 +59,13 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, /* Set MAX if it's busy enough */ if (stat->busy_time * 100 > stat->total_time * dfso_upthreshold) { - *freq = max; + *freq = DEVFREQ_MAX_FREQ; return 0; } /* Set MAX if we do not know the initial frequency */ if (stat->current_frequency == 0) { - *freq = max; + *freq = DEVFREQ_MAX_FREQ; return 0; } @@ -85,11 +84,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2)); *freq = (unsigned long) b; - if (df->min_freq && *freq < df->min_freq) - *freq = df->min_freq; - if (df->max_freq && *freq > df->max_freq) - *freq = df->max_freq; - return 0; } diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c index 080607c3f34d..378d84c011df 100644 --- a/drivers/devfreq/governor_userspace.c +++ b/drivers/devfreq/governor_userspace.c @@ -26,19 +26,11 @@ static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq) { struct userspace_data *data = df->data; - if (data->valid) { - unsigned long adjusted_freq = data->user_frequency; - - if (df->max_freq && adjusted_freq > df->max_freq) - adjusted_freq = df->max_freq; - - if (df->min_freq && adjusted_freq < df->min_freq) - adjusted_freq = df->min_freq; - - *freq = adjusted_freq; - } else { + if (data->valid) + *freq = data->user_frequency; + else *freq = df->previous_freq; /* No user freq specified yet */ - } + return 0; } -- cgit From b596d895fa2957e136a6b398b97b06bd42b51291 Mon Sep 17 00:00:00 2001 From: Matthias Kaehlcke Date: Fri, 3 Aug 2018 13:05:11 -0700 Subject: PM / devfreq: Make update_devfreq() public Currently update_devfreq() is only visible to devfreq governors outside of devfreq.c. Make it public to allow drivers that adjust devfreq policies to cause a re-evaluation of the frequency after a policy change. Signed-off-by: Matthias Kaehlcke Reviewed-by: Brian Norris Reviewed-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/governor.h | 3 --- include/linux/devfreq.h | 8 ++++++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h index b81700244ce3..f53339ca610f 100644 --- a/drivers/devfreq/governor.h +++ b/drivers/devfreq/governor.h @@ -57,9 +57,6 @@ struct devfreq_governor { unsigned int event, void *data); }; -/* Caution: devfreq->lock must be locked before calling update_devfreq */ -extern int update_devfreq(struct devfreq *devfreq); - extern void devfreq_monitor_start(struct devfreq *devfreq); extern void devfreq_monitor_stop(struct devfreq *devfreq); extern void devfreq_monitor_suspend(struct devfreq *devfreq); diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index 3aae5b3af87c..e4963b0f45da 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -198,6 +198,14 @@ extern void devm_devfreq_remove_device(struct device *dev, extern int devfreq_suspend_device(struct devfreq *devfreq); extern int devfreq_resume_device(struct devfreq *devfreq); +/** + * update_devfreq() - Reevaluate the device and configure frequency + * @devfreq: the devfreq device + * + * Note: devfreq->lock must be held + */ +extern int update_devfreq(struct devfreq *devfreq); + /* Helper functions for devfreq user device driver with OPP. */ extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq, u32 flags); -- cgit From f037eb8c1f476bc903d99695c1eb9f99ccb46b27 Mon Sep 17 00:00:00 2001 From: Rob Herring Date: Mon, 27 Aug 2018 20:52:16 -0500 Subject: PM / devfreq: Convert to using %pOFn instead of device_node.name In preparation to remove the node name pointer from struct device_node, convert printf users to use the %pOFn format specifier. Signed-off-by: Rob Herring Acked-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/event/exynos-ppmu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c index a9c64f0d3284..c61de0bdf053 100644 --- a/drivers/devfreq/event/exynos-ppmu.c +++ b/drivers/devfreq/event/exynos-ppmu.c @@ -535,8 +535,8 @@ static int of_get_devfreq_events(struct device_node *np, if (i == ARRAY_SIZE(ppmu_events)) { dev_warn(dev, - "don't know how to configure events : %s\n", - node->name); + "don't know how to configure events : %pOFn\n", + node); continue; } -- cgit From 2f061fd0c2d852e32e03a903fccd810663c5c31e Mon Sep 17 00:00:00 2001 From: Vincent Donnefort Date: Mon, 3 Sep 2018 09:02:07 +0900 Subject: PM / devfreq: stopping the governor before device_unregister() device_release() is freeing the resources before calling the device specific release callback which is, in the case of devfreq, stopping the governor. It is a problem as some governors are using the device resources. e.g. simpleondemand which is using the devfreq deferrable monitoring work. If it is not stopped before the resources are freed, it might lead to a use after free. Signed-off-by: Vincent Donnefort Reviewed-by: John Einar Reitan [cw00.choi: Fix merge conflict] Reviewed-by: Chanwoo Choi Signed-off-by: MyungJoo Ham --- drivers/devfreq/devfreq.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index b5e7af60723c..1868423355d9 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -575,10 +575,6 @@ static void devfreq_dev_release(struct device *dev) list_del(&devfreq->node); mutex_unlock(&devfreq_list_lock); - if (devfreq->governor) - devfreq->governor->event_handler(devfreq, - DEVFREQ_GOV_STOP, NULL); - if (devfreq->profile->exit) devfreq->profile->exit(devfreq->dev.parent); @@ -714,7 +710,7 @@ struct devfreq *devfreq_add_device(struct device *dev, err_init: mutex_unlock(&devfreq_list_lock); - device_unregister(&devfreq->dev); + devfreq_remove_device(devfreq); devfreq = NULL; err_dev: if (devfreq) @@ -735,6 +731,9 @@ int devfreq_remove_device(struct devfreq *devfreq) if (!devfreq) return -EINVAL; + if (devfreq->governor) + devfreq->governor->event_handler(devfreq, + DEVFREQ_GOV_STOP, NULL); device_unregister(&devfreq->dev); return 0; -- cgit From 8188b154f95014dae4d19892fefb202c8df3f885 Mon Sep 17 00:00:00 2001 From: zhong jiang Date: Fri, 21 Sep 2018 21:18:43 +0800 Subject: PM / devfreq: remove redundant null pointer check before kfree kfree has taken the null pointer into account. hence it is safe to remove the redundant null pointer check before kfree. Signed-off-by: zhong jiang Signed-off-by: MyungJoo Ham --- drivers/devfreq/devfreq.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 1868423355d9..141413067b5c 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -713,8 +713,7 @@ err_init: devfreq_remove_device(devfreq); devfreq = NULL; err_dev: - if (devfreq) - kfree(devfreq); + kfree(devfreq); err_out: return ERR_PTR(err); } -- cgit