Hi Rafael,
This is what I came up with in reply to: http://www.spinics.net/lists/arm-kernel/msg380065.html
The issue was first reported by Stefan Wahren, where he got warnings for duplicate OPP entries while he tried to insert/remove cpufreq-dt.ko multiple times.
This set fixes it by first marking each OPP entry as static (created from DT) or dynamic. And then freeing only static ones from the ->exit() path of cpufreq drivers. An API is also provided to remove the dynamics ones, but no one is using it currently.
This also modifies bunch of cpufreq drivers which were using OPPs created from DT.
At last, thanks to Paul and You to clarify my doubts on RCU. Hope I understood them correctly :)
Pushed here: git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/opp-remove-v1 Rebased over: pm/linux-next Tested-on: Exynos 5250, dual cortex A15 with cpufreq-dt.c.
Cc: Paul McKenney paulmck@linux.vnet.ibm.com
Viresh Kumar (8): opp: rename 'head' as 'rcu_head' or 'srcu_head' based on its type opp: don't match for existing OPPs when list is empty opp: mark OPPs as 'static' or 'dynamic' opp: Introduce APIs to remove OPPs arm_big_little: free OPP table created during ->init() cpufreq-dt: free OPP table created during ->init() exynos5440: free OPP table created during ->init() imx6q: free OPP table created during ->init()
drivers/base/power/opp.c | 196 +++++++++++++++++++++++++++-------- drivers/cpufreq/arm_big_little.c | 7 +- drivers/cpufreq/arm_big_little.h | 5 +- drivers/cpufreq/arm_big_little_dt.c | 1 + drivers/cpufreq/cpufreq-dt.c | 6 +- drivers/cpufreq/exynos5440-cpufreq.c | 5 +- drivers/cpufreq/imx6q-cpufreq.c | 11 +- include/linux/pm_opp.h | 12 ++- 8 files changed, 195 insertions(+), 48 deletions(-)
Both 'struct dev_pm_opp' and 'struct device_opp' have member with name 'head' but with different types. This leads to confusion while reading the code.
Name them 'rcu_head' and 'srcu_head'.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 89ced95..76ae6fd 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -53,7 +53,7 @@ * @rate: Frequency in hertz * @u_volt: Nominal voltage in microvolts corresponding to this OPP * @dev_opp: points back to the device_opp struct this opp belongs to - * @head: RCU callback head used for deferred freeing + * @rcu_head: RCU callback head used for deferred freeing * * This structure stores the OPP information for a given device. */ @@ -65,7 +65,7 @@ struct dev_pm_opp { unsigned long u_volt;
struct device_opp *dev_opp; - struct rcu_head head; + struct rcu_head rcu_head; };
/** @@ -76,7 +76,7 @@ struct dev_pm_opp { * RCU usage: nodes are not modified in the list of device_opp, * however addition is possible and is secured by dev_opp_list_lock * @dev: device pointer - * @head: notifier head to notify the OPP availability changes. + * @srcu_head: notifier head to notify the OPP availability changes. * @opp_list: list of opps * * This is an internal data structure maintaining the link to opps attached to @@ -87,7 +87,7 @@ struct device_opp { struct list_head node;
struct device *dev; - struct srcu_notifier_head head; + struct srcu_notifier_head srcu_head; struct list_head opp_list; };
@@ -436,7 +436,7 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) }
dev_opp->dev = dev; - srcu_init_notifier_head(&dev_opp->head); + srcu_init_notifier_head(&dev_opp->srcu_head); INIT_LIST_HEAD(&dev_opp->opp_list);
/* Secure the device list modification */ @@ -481,7 +481,7 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) * Notify the changes in the availability of the operable * frequency/voltage list. */ - srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_ADD, new_opp); + srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp); return 0; } EXPORT_SYMBOL_GPL(dev_pm_opp_add); @@ -557,14 +557,14 @@ static int opp_set_availability(struct device *dev, unsigned long freq,
list_replace_rcu(&opp->node, &new_opp->node); mutex_unlock(&dev_opp_list_lock); - kfree_rcu(opp, head); + kfree_rcu(opp, rcu_head);
/* Notify the change of the OPP availability */ if (availability_req) - srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_ENABLE, + srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ENABLE, new_opp); else - srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_DISABLE, + srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_DISABLE, new_opp);
return 0; @@ -629,7 +629,7 @@ struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev) if (IS_ERR(dev_opp)) return ERR_CAST(dev_opp); /* matching type */
- return &dev_opp->head; + return &dev_opp->srcu_head; }
#ifdef CONFIG_OF
OPP list is guaranteed to be empty when 'dev_opp' is created. And so we don't need to run the comparison loop with existing OPPs.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 76ae6fd..a333e2ef 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -417,6 +417,12 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) /* Hold our list modification lock here */ mutex_lock(&dev_opp_list_lock);
+ /* populate the opp table */ + new_opp->dev_opp = dev_opp; + new_opp->rate = freq; + new_opp->u_volt = u_volt; + new_opp->available = true; + /* Check for existing list for 'dev' */ dev_opp = find_device_opp(dev); if (IS_ERR(dev_opp)) { @@ -441,14 +447,10 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
/* Secure the device list modification */ list_add_rcu(&dev_opp->node, &dev_opp_list); + head = &dev_opp->opp_list; + goto list_add; }
- /* populate the opp table */ - new_opp->dev_opp = dev_opp; - new_opp->rate = freq; - new_opp->u_volt = u_volt; - new_opp->available = true; - /* * Insert new OPP in order of increasing frequency * and discard if already present @@ -474,6 +476,7 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) return ret; }
+list_add: list_add_rcu(&new_opp->node, head); mutex_unlock(&dev_opp_list_lock);
Static OPPs are the ones created from Device Tree entries and dynamic are the ones created at runtime by calling dev_pm_opp_add().
There is a need to distinguish them as we need to free static OPPs from cpufreq drivers when they are removed.
So, add another field 'dynamic' in 'struct dev_pm_opp' to keep this information.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 59 ++++++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 25 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index a333e2ef..b249b01 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -49,6 +49,7 @@ * are protected by the dev_opp_list_lock for integrity. * IMPORTANT: the opp nodes should be maintained in increasing * order. + * @dynamic: not-created from static DT entries. * @available: true/false - marks if this OPP as available or not * @rate: Frequency in hertz * @u_volt: Nominal voltage in microvolts corresponding to this OPP @@ -61,6 +62,7 @@ struct dev_pm_opp { struct list_head node;
bool available; + bool dynamic; unsigned long rate; unsigned long u_volt;
@@ -378,30 +380,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev, } EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
-/** - * dev_pm_opp_add() - Add an OPP table from a table definitions - * @dev: device for which we do this operation - * @freq: Frequency in Hz for this OPP - * @u_volt: Voltage in uVolts for this OPP - * - * This function adds an opp definition to the opp list and returns status. - * The opp is made available by default and it can be controlled using - * dev_pm_opp_enable/disable functions. - * - * Locking: The internal device_opp and opp structures are RCU protected. - * Hence this function internally uses RCU updater strategy with mutex locks - * to keep the integrity of the internal data structures. Callers should ensure - * that this function is *NOT* called under RCU protection or in contexts where - * mutex cannot be locked. - * - * Return: - * 0: On success OR - * Duplicate OPPs (both freq and volt are same) and opp->available - * -EEXIST: Freq are same and volt are different OR - * Duplicate OPPs (both freq and volt are same) and !opp->available - * -ENOMEM: Memory allocation failure - */ -int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) +static int dev_pm_opp_add_dynamic(struct device *dev, unsigned long freq, + unsigned long u_volt, bool dynamic) { struct device_opp *dev_opp = NULL; struct dev_pm_opp *opp, *new_opp; @@ -422,6 +402,7 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) new_opp->rate = freq; new_opp->u_volt = u_volt; new_opp->available = true; + new_opp->dynamic = dynamic;
/* Check for existing list for 'dev' */ dev_opp = find_device_opp(dev); @@ -487,6 +468,34 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_ADD, new_opp); return 0; } + +/** + * dev_pm_opp_add() - Add an OPP table from a table definitions + * @dev: device for which we do this operation + * @freq: Frequency in Hz for this OPP + * @u_volt: Voltage in uVolts for this OPP + * + * This function adds an opp definition to the opp list and returns status. + * The opp is made available by default and it can be controlled using + * dev_pm_opp_enable/disable functions. + * + * Locking: The internal device_opp and opp structures are RCU protected. + * Hence this function internally uses RCU updater strategy with mutex locks + * to keep the integrity of the internal data structures. Callers should ensure + * that this function is *NOT* called under RCU protection or in contexts where + * mutex cannot be locked. + * + * Return: + * 0: On success OR + * Duplicate OPPs (both freq and volt are same) and opp->available + * -EEXIST: Freq are same and volt are different OR + * Duplicate OPPs (both freq and volt are same) and !opp->available + * -ENOMEM: Memory allocation failure + */ +int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) +{ + return dev_pm_opp_add_dynamic(dev, freq, u_volt, true); +} EXPORT_SYMBOL_GPL(dev_pm_opp_add);
/** @@ -669,7 +678,7 @@ int of_init_opp_table(struct device *dev) unsigned long freq = be32_to_cpup(val++) * 1000; unsigned long volt = be32_to_cpup(val++);
- if (dev_pm_opp_add(dev, freq, volt)) + if (dev_pm_opp_add_dynamic(dev, freq, volt, false)) dev_warn(dev, "%s: Failed to add OPP %ld\n", __func__, freq); nr -= 2;
OPPs are created statically (from DT) or dynamically. Currently we don't free OPPs that are created statically, when the module unloads. And so if the module is inserted back again, we get warning for duplicate OPPs as the same were already present.
Also, there might be a need to remove dynamic OPPs in future and so API for that is also added.
This patch adds helper APIs to remove/free existing static and dynamic OPPs.
Cc: Paul McKenney paulmck@linux.vnet.ibm.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
--- @Paul/Rafael: Do we need to use call_srcu() instead of kfree_rcu() in opp_set_availability()? I left it as it looked a bit different. srcu_notifier_call_chain() is getting called after deleting the node.
drivers/base/power/opp.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 12 +++++- 2 files changed, 113 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index b249b01..7ae4db5 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -79,6 +79,7 @@ struct dev_pm_opp { * however addition is possible and is secured by dev_opp_list_lock * @dev: device pointer * @srcu_head: notifier head to notify the OPP availability changes. + * @rcu_head: RCU callback head used for deferred freeing * @opp_list: list of opps * * This is an internal data structure maintaining the link to opps attached to @@ -90,6 +91,7 @@ struct device_opp {
struct device *dev; struct srcu_notifier_head srcu_head; + struct rcu_head rcu_head; struct list_head opp_list; };
@@ -498,6 +500,76 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) } EXPORT_SYMBOL_GPL(dev_pm_opp_add);
+static void kfree_opp_rcu(struct rcu_head *head) +{ + struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head); + + kfree(opp); +} + +static void kfree_device_rcu(struct rcu_head *head) +{ + struct device_opp *device_opp = container_of(head, struct device_opp, rcu_head); + + kfree(device_opp); +} + +void __dev_pm_opp_remove(struct device_opp *dev_opp, struct dev_pm_opp *opp) +{ + /* + * Notify the changes in the availability of the operable + * frequency/voltage list. + */ + srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp); + list_del_rcu(&opp->node); + call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, kfree_opp_rcu); + + if (list_empty(&dev_opp->opp_list)) { + list_del_rcu(&dev_opp->node); + call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head, + kfree_device_rcu); + } +} + +/** + * dev_pm_opp_remove() - Remove an OPP from OPP list + * @dev: device for which we do this operation + * @freq: OPP to remove with matching 'freq' + * + * This function removes an opp from the opp list. + */ +void dev_pm_opp_remove(struct device *dev, unsigned long freq) +{ + struct dev_pm_opp *opp; + struct device_opp *dev_opp; + bool found = false; + + /* Hold our list modification lock here */ + mutex_lock(&dev_opp_list_lock); + + dev_opp = find_device_opp(dev); + if (IS_ERR(dev_opp)) + goto unlock; + + list_for_each_entry(opp, &dev_opp->opp_list, node) { + if (opp->rate == freq) { + found = true; + break; + } + } + + if (!found) { + dev_warn(dev, "%s: Couldn't find OPP with freq: %lu\n", + __func__, freq); + goto unlock; + } + + __dev_pm_opp_remove(dev_opp, opp); +unlock: + mutex_unlock(&dev_opp_list_lock); +} +EXPORT_SYMBOL_GPL(dev_pm_opp_remove); + /** * opp_set_availability() - helper to set the availability of an opp * @dev: device for which we do this operation @@ -687,4 +759,34 @@ int of_init_opp_table(struct device *dev) return 0; } EXPORT_SYMBOL_GPL(of_init_opp_table); + +/** + * of_free_opp_table() - Free OPP table entries created from static DT entries + * @dev: device pointer used to lookup device OPPs. + * + * Free OPPs created using static entries present in DT. + */ +void of_free_opp_table(struct device *dev) +{ + struct device_opp *dev_opp = find_device_opp(dev); + struct dev_pm_opp *opp, *tmp; + + /* Check for existing list for 'dev' */ + dev_opp = find_device_opp(dev); + if (WARN(IS_ERR(dev_opp), "%s: dev_opp: %ld\n", dev_name(dev), + PTR_ERR(dev_opp))) + return; + + /* Hold our list modification lock here */ + mutex_lock(&dev_opp_list_lock); + + /* Free static OPPs */ + list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) { + if (!opp->dynamic) + __dev_pm_opp_remove(dev_opp, opp); + } + + mutex_unlock(&dev_opp_list_lock); +} +EXPORT_SYMBOL_GPL(of_free_opp_table); #endif diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 0330217..cec2d45 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -21,7 +21,7 @@ struct dev_pm_opp; struct device;
enum dev_pm_opp_event { - OPP_EVENT_ADD, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE, + OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE, };
#if defined(CONFIG_PM_OPP) @@ -44,6 +44,7 @@ struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt); +void dev_pm_opp_remove(struct device *dev, unsigned long freq);
int dev_pm_opp_enable(struct device *dev, unsigned long freq);
@@ -90,6 +91,10 @@ static inline int dev_pm_opp_add(struct device *dev, unsigned long freq, return -EINVAL; }
+static inline void dev_pm_opp_remove(struct device *dev, unsigned long freq) +{ +} + static inline int dev_pm_opp_enable(struct device *dev, unsigned long freq) { return 0; @@ -109,11 +114,16 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
#if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) int of_init_opp_table(struct device *dev); +void of_free_opp_table(struct device *dev); #else static inline int of_init_opp_table(struct device *dev) { return -EINVAL; } + +static inline void of_free_opp_table(struct device *dev) +{ +} #endif
#endif /* __LINUX_OPP_H__ */
On Tue, Nov 25, 2014 at 04:04:19PM +0530, Viresh Kumar wrote:
OPPs are created statically (from DT) or dynamically. Currently we don't free OPPs that are created statically, when the module unloads. And so if the module is inserted back again, we get warning for duplicate OPPs as the same were already present.
Also, there might be a need to remove dynamic OPPs in future and so API for that is also added.
This patch adds helper APIs to remove/free existing static and dynamic OPPs.
Cc: Paul McKenney paulmck@linux.vnet.ibm.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
@Paul/Rafael: Do we need to use call_srcu() instead of kfree_rcu() in opp_set_availability()? I left it as it looked a bit different. srcu_notifier_call_chain() is getting called after deleting the node.
If the data is alway accessed under an SRCU read-side critical section, then you do need call_srcu() when freeing it -- as you pointed out, -with- the srcu_struct included. ;-)
If the data is accessed under both SRCU and RCU, then things get a bit more involved.
drivers/base/power/opp.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 12 +++++- 2 files changed, 113 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index b249b01..7ae4db5 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -79,6 +79,7 @@ struct dev_pm_opp {
however addition is possible and is secured by dev_opp_list_lock- @dev: device pointer
- @srcu_head: notifier head to notify the OPP availability changes.
- @rcu_head: RCU callback head used for deferred freeing
- @opp_list: list of opps
- This is an internal data structure maintaining the link to opps attached to
@@ -90,6 +91,7 @@ struct device_opp {
struct device *dev; struct srcu_notifier_head srcu_head;
- struct rcu_head rcu_head; struct list_head opp_list;
};
@@ -498,6 +500,76 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) } EXPORT_SYMBOL_GPL(dev_pm_opp_add);
+static void kfree_opp_rcu(struct rcu_head *head) +{
- struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head);
- kfree(opp);
+}
+static void kfree_device_rcu(struct rcu_head *head) +{
- struct device_opp *device_opp = container_of(head, struct device_opp, rcu_head);
- kfree(device_opp);
+}
+void __dev_pm_opp_remove(struct device_opp *dev_opp, struct dev_pm_opp *opp) +{
- /*
* Notify the changes in the availability of the operable* frequency/voltage list.*/- srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
- list_del_rcu(&opp->node);
- call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, kfree_opp_rcu);
I am guessing that opp->node is being removed from the list traversed by srcu_notifier_call_chain()... (Looks that way below.)
- if (list_empty(&dev_opp->opp_list)) {
Does the above want to be "if (!list_empty(&dev_opp->opp_list)) {"?
Thanx, Paul
list_del_rcu(&dev_opp->node);call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,kfree_device_rcu);- }
+}
+/**
- dev_pm_opp_remove() - Remove an OPP from OPP list
- @dev: device for which we do this operation
- @freq: OPP to remove with matching 'freq'
- This function removes an opp from the opp list.
- */
+void dev_pm_opp_remove(struct device *dev, unsigned long freq) +{
- struct dev_pm_opp *opp;
- struct device_opp *dev_opp;
- bool found = false;
- /* Hold our list modification lock here */
- mutex_lock(&dev_opp_list_lock);
- dev_opp = find_device_opp(dev);
- if (IS_ERR(dev_opp))
goto unlock;- list_for_each_entry(opp, &dev_opp->opp_list, node) {
if (opp->rate == freq) {found = true;break;}- }
- if (!found) {
dev_warn(dev, "%s: Couldn't find OPP with freq: %lu\n",__func__, freq);goto unlock;- }
- __dev_pm_opp_remove(dev_opp, opp);
+unlock:
- mutex_unlock(&dev_opp_list_lock);
+} +EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
/**
- opp_set_availability() - helper to set the availability of an opp
- @dev: device for which we do this operation
@@ -687,4 +759,34 @@ int of_init_opp_table(struct device *dev) return 0; } EXPORT_SYMBOL_GPL(of_init_opp_table);
+/**
- of_free_opp_table() - Free OPP table entries created from static DT entries
- @dev: device pointer used to lookup device OPPs.
- Free OPPs created using static entries present in DT.
- */
+void of_free_opp_table(struct device *dev) +{
- struct device_opp *dev_opp = find_device_opp(dev);
- struct dev_pm_opp *opp, *tmp;
- /* Check for existing list for 'dev' */
- dev_opp = find_device_opp(dev);
- if (WARN(IS_ERR(dev_opp), "%s: dev_opp: %ld\n", dev_name(dev),
PTR_ERR(dev_opp)))return;- /* Hold our list modification lock here */
- mutex_lock(&dev_opp_list_lock);
- /* Free static OPPs */
- list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
if (!opp->dynamic)__dev_pm_opp_remove(dev_opp, opp);- }
- mutex_unlock(&dev_opp_list_lock);
+} +EXPORT_SYMBOL_GPL(of_free_opp_table); #endif diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 0330217..cec2d45 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -21,7 +21,7 @@ struct dev_pm_opp; struct device;
enum dev_pm_opp_event {
- OPP_EVENT_ADD, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
- OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
};
#if defined(CONFIG_PM_OPP) @@ -44,6 +44,7 @@ struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt); +void dev_pm_opp_remove(struct device *dev, unsigned long freq);
int dev_pm_opp_enable(struct device *dev, unsigned long freq);
@@ -90,6 +91,10 @@ static inline int dev_pm_opp_add(struct device *dev, unsigned long freq, return -EINVAL; }
+static inline void dev_pm_opp_remove(struct device *dev, unsigned long freq) +{ +}
static inline int dev_pm_opp_enable(struct device *dev, unsigned long freq) { return 0; @@ -109,11 +114,16 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
#if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) int of_init_opp_table(struct device *dev); +void of_free_opp_table(struct device *dev); #else static inline int of_init_opp_table(struct device *dev) { return -EINVAL; }
+static inline void of_free_opp_table(struct device *dev) +{ +} #endif
#endif /* __LINUX_OPP_H__ */
2.0.3.693.g996b0fd
On 25 November 2014 at 21:54, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote:
If the data is alway accessed under an SRCU read-side critical section, then you do need call_srcu() when freeing it -- as you pointed out, -with- the srcu_struct included. ;-)
:)
If the data is accessed under both SRCU and RCU, then things get a bit more involved.
Yes, that is always the case here.
+void __dev_pm_opp_remove(struct device_opp *dev_opp, struct dev_pm_opp *opp) +{
/** Notify the changes in the availability of the operable* frequency/voltage list.*/srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);list_del_rcu(&opp->node);call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, kfree_opp_rcu);I am guessing that opp->node is being removed from the list traversed by srcu_notifier_call_chain()... (Looks that way below.)
I couldn't find any code in kernel which is registered for this notifier chain yet. And so don't know what that code will do. It may not traverse the list or it may :)
I couldn't understand this comment of yours, sorry: (Looks that way below.)
if (list_empty(&dev_opp->opp_list)) {Does the above want to be "if (!list_empty(&dev_opp->opp_list)) {"?
No. dev_opp contains list of all OPPs. When all OPPs are gone, we don't want dev_opp anymore and so freeing it.
list_del_rcu(&dev_opp->node);call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,kfree_device_rcu);}+}
So, I am still not sure what we need to do here as we have readers with both rcu and srcu critical regions.
On Tue, Nov 25, 2014 at 10:46:29PM +0530, Viresh Kumar wrote:
On 25 November 2014 at 21:54, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote:
If the data is alway accessed under an SRCU read-side critical section, then you do need call_srcu() when freeing it -- as you pointed out, -with- the srcu_struct included. ;-)
:)
If the data is accessed under both SRCU and RCU, then things get a bit more involved.
Yes, that is always the case here.
+void __dev_pm_opp_remove(struct device_opp *dev_opp, struct dev_pm_opp *opp) +{
/** Notify the changes in the availability of the operable* frequency/voltage list.*/srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);list_del_rcu(&opp->node);call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, kfree_opp_rcu);I am guessing that opp->node is being removed from the list traversed by srcu_notifier_call_chain()... (Looks that way below.)
I couldn't find any code in kernel which is registered for this notifier chain yet. And so don't know what that code will do. It may not traverse the list or it may :)
I would guess that the srcu_notifier_chain_register() in devfreq_register_opp_notifier() is adding to the call chain, but I do not claim to understand this code. The srcu_notifier_call_chain() above is traversing it.
I couldn't understand this comment of yours, sorry: (Looks that way below.)
Well, that comment might or might not be meaningful. Again, I just took a quick look at the patch -- I have not gotten my head fully around the dev-PM code.
if (list_empty(&dev_opp->opp_list)) {Does the above want to be "if (!list_empty(&dev_opp->opp_list)) {"?
No. dev_opp contains list of all OPPs. When all OPPs are gone, we don't want dev_opp anymore and so freeing it.
OK.
list_del_rcu(&dev_opp->node);call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,kfree_device_rcu);}+}
So, I am still not sure what we need to do here as we have readers with both rcu and srcu critical regions.
Well, the most straightforward approach from an RCU perspective would be to make all the readers use the same flavor of RCU. From Rafael's earlier note, I am guessing that some of the code paths absolutely require SRCU. Of course, if all the readers use SRCU, then you can simply free using SRCU.
You -can- handle situations having more than one type of reader, but this requires waiting for all corresponding flavors of grace period. This turns out to be fairly simple in this case, for example, have your kfree_opp_rcu() function invoke kfree_rcu() instead of kfree().
But I strongly recommend gaining a full understanding of the code first. You can dig yourself a really deep hole really quickly by patching code without understanding it! And apologies if you really do already fully understand the code, but your statement above leads me to believe that you do not yet fully understand it.
I couldn't find any code in kernel which is registered for this notifier chain yet. And so don't know what that code will do. It may not traverse the list or it may :)
One thing that can help internalize code (relatively) quickly is to get a large piece of paper and to draw the relationships of the data structures first and the relationship of the code later. When the drawing gets too messy, start over with a clean sheet of paper.
Thanx, Paul
On 25 November 2014 at 23:09, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote:
On Tue, Nov 25, 2014 at 10:46:29PM +0530, Viresh Kumar wrote:
I couldn't find any code in kernel which is registered for this notifier chain yet. And so don't know what that code will do. It may not traverse the list or it may :)
I would guess that the srcu_notifier_chain_register() in devfreq_register_opp_notifier() is adding to the call chain, but I do not claim to understand this code. The srcu_notifier_call_chain() above is traversing it.
Actually I searched for who is calling devfreq_register_opp_notifier() and couldn't notice any callers. But there was one from within devfreq.c and that was called from exynos code..
So yes, it is used and I was wrong. Ultimately update_devfreq() would be called from the notifier chain and that may try to change the freq and so is sleep-able.
So, I am still not sure what we need to do here as we have readers with both rcu and srcu critical regions.
Well, the most straightforward approach from an RCU perspective would be to make all the readers use the same flavor of RCU. From Rafael's earlier note, I am guessing that some of the code paths absolutely require SRCU. Of course, if all the readers use SRCU, then you can simply free using SRCU.
Yeah, so the update_devfreq() call surely requires SRCU.
You -can- handle situations having more than one type of reader, but this requires waiting for all corresponding flavors of grace period. This turns out to be fairly simple in this case, for example, have your kfree_opp_rcu() function invoke kfree_rcu() instead of kfree().
Hmm, I see.
But I strongly recommend gaining a full understanding of the code first. You can dig yourself a really deep hole really quickly by patching code without understanding it! And apologies if you really do already fully understand the code, but your statement above leads me to believe that you do not yet fully understand it.
I wouldn't claim to know every part of these frameworks (OPP, Devfreq), but the maintenance of the nodes/lists is what I understand well.
But yeah, I understand you are worried about. Pushing more bugs for solving one.
I couldn't find any code in kernel which is registered for this notifier chain yet. And so don't know what that code will do. It may not traverse the list or it may :)One thing that can help internalize code (relatively) quickly is to get a large piece of paper and to draw the relationships of the data structures first and the relationship of the code later. When the drawing gets too messy, start over with a clean sheet of paper.
Thanks for your suggestions Paul. I was able to do it without a pen and paper this time as it wasn't that complex. But yes this pen-paper things really works while working on complex stuff. I found a memory leak Bug in scheduling domains creation code earlier that way :)
Also it looks like I should use call_srcu() with kfree_rcu() for opp_set_availability() case as well to avoid any corner cases.
Thanks for your time and help. Really appreciate that :)
-- viresh
On 26 November 2014 at 11:59, Viresh Kumar viresh.kumar@linaro.org wrote:
Also it looks like I should use call_srcu() with kfree_rcu() for opp_set_availability() case as well to avoid any corner cases.
This is the diff I would be adding to this patch:
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index f9d80e6..3f728cd 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -506,7 +506,7 @@ static void kfree_opp_rcu(struct rcu_head *head) struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head);
pr_info("%s: opp:%p, dynamic:%d\n", __func__, opp, opp->dynamic); - kfree(opp); + kfree_rcu(opp); }
static void kfree_device_rcu(struct rcu_head *head) @@ -644,7 +644,7 @@ static int opp_set_availability(struct device *dev, unsigned long freq,
list_replace_rcu(&opp->node, &new_opp->node); mutex_unlock(&dev_opp_list_lock); - kfree_rcu(opp, rcu_head); + call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, kfree_opp_rcu);
/* Notify the change of the OPP availability */ if (availability_req)
On Wednesday, November 26, 2014 02:47:47 PM Viresh Kumar wrote:
On 26 November 2014 at 11:59, Viresh Kumar viresh.kumar@linaro.org wrote:
Also it looks like I should use call_srcu() with kfree_rcu() for opp_set_availability() case as well to avoid any corner cases.
This is the diff I would be adding to this patch:
That should work if I'm not mistaken, but I'm wondering if we could settle on using one RCU type for this (later, though).
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index f9d80e6..3f728cd 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -506,7 +506,7 @@ static void kfree_opp_rcu(struct rcu_head *head) struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head);
pr_info("%s: opp:%p, dynamic:%d\n", __func__, opp, opp->dynamic);
kfree(opp);
kfree_rcu(opp);}
static void kfree_device_rcu(struct rcu_head *head) @@ -644,7 +644,7 @@ static int opp_set_availability(struct device *dev, unsigned long freq,
list_replace_rcu(&opp->node, &new_opp->node); mutex_unlock(&dev_opp_list_lock);
kfree_rcu(opp, rcu_head);
call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, kfree_opp_rcu); /* Notify the change of the OPP availability */ if (availability_req)
On 27 November 2014 at 04:02, Rafael J. Wysocki rjw@rjwysocki.net wrote:
That should work if I'm not mistaken, but I'm wondering if we could settle on using one RCU type for this (later, though).
We can convert all call sites to use SRCU then. Once we are sure about it.
On Thursday, November 27, 2014 05:56:21 AM Viresh Kumar wrote:
On 27 November 2014 at 04:02, Rafael J. Wysocki rjw@rjwysocki.net wrote:
That should work if I'm not mistaken, but I'm wondering if we could settle on using one RCU type for this (later, though).
We can convert all call sites to use SRCU then. Once we are sure about it.
Yes, later.
OPPs are created statically (from DT) or dynamically. Currently we don't free OPPs that are created statically, when the module unloads. And so if the module is inserted back again, we get warning for duplicate OPPs as the same were already present.
Also, there might be a need to remove dynamic OPPs in future and so API for that is also added.
This patch adds helper APIs to remove/free existing static and dynamic OPPs.
Because the OPPs are used both under RCU and SRCU, we have to wait for grace period of both. And so are using kfree_rcu() from within call_srcu().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- V1->V2: - Use kfree_rcu() instead of kfree() as OPP is accessed under rcu locks as well and so we should wait for its grace period as well. - Another patch which fixes a potential bug, 5/8. Must be applied after this patch and before the original 5/8. Total of 9 patches now.
drivers/base/power/opp.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 12 +++++- 2 files changed, 113 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index b249b01..977474a 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -79,6 +79,7 @@ struct dev_pm_opp { * however addition is possible and is secured by dev_opp_list_lock * @dev: device pointer * @srcu_head: notifier head to notify the OPP availability changes. + * @rcu_head: RCU callback head used for deferred freeing * @opp_list: list of opps * * This is an internal data structure maintaining the link to opps attached to @@ -90,6 +91,7 @@ struct device_opp {
struct device *dev; struct srcu_notifier_head srcu_head; + struct rcu_head rcu_head; struct list_head opp_list; };
@@ -498,6 +500,76 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) } EXPORT_SYMBOL_GPL(dev_pm_opp_add);
+static void kfree_opp_rcu(struct rcu_head *head) +{ + struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head); + + kfree_rcu(opp, rcu_head); +} + +static void kfree_device_rcu(struct rcu_head *head) +{ + struct device_opp *device_opp = container_of(head, struct device_opp, rcu_head); + + kfree(device_opp); +} + +void __dev_pm_opp_remove(struct device_opp *dev_opp, struct dev_pm_opp *opp) +{ + /* + * Notify the changes in the availability of the operable + * frequency/voltage list. + */ + srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp); + list_del_rcu(&opp->node); + call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, kfree_opp_rcu); + + if (list_empty(&dev_opp->opp_list)) { + list_del_rcu(&dev_opp->node); + call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head, + kfree_device_rcu); + } +} + +/** + * dev_pm_opp_remove() - Remove an OPP from OPP list + * @dev: device for which we do this operation + * @freq: OPP to remove with matching 'freq' + * + * This function removes an opp from the opp list. + */ +void dev_pm_opp_remove(struct device *dev, unsigned long freq) +{ + struct dev_pm_opp *opp; + struct device_opp *dev_opp; + bool found = false; + + /* Hold our list modification lock here */ + mutex_lock(&dev_opp_list_lock); + + dev_opp = find_device_opp(dev); + if (IS_ERR(dev_opp)) + goto unlock; + + list_for_each_entry(opp, &dev_opp->opp_list, node) { + if (opp->rate == freq) { + found = true; + break; + } + } + + if (!found) { + dev_warn(dev, "%s: Couldn't find OPP with freq: %lu\n", + __func__, freq); + goto unlock; + } + + __dev_pm_opp_remove(dev_opp, opp); +unlock: + mutex_unlock(&dev_opp_list_lock); +} +EXPORT_SYMBOL_GPL(dev_pm_opp_remove); + /** * opp_set_availability() - helper to set the availability of an opp * @dev: device for which we do this operation @@ -687,4 +759,34 @@ int of_init_opp_table(struct device *dev) return 0; } EXPORT_SYMBOL_GPL(of_init_opp_table); + +/** + * of_free_opp_table() - Free OPP table entries created from static DT entries + * @dev: device pointer used to lookup device OPPs. + * + * Free OPPs created using static entries present in DT. + */ +void of_free_opp_table(struct device *dev) +{ + struct device_opp *dev_opp = find_device_opp(dev); + struct dev_pm_opp *opp, *tmp; + + /* Check for existing list for 'dev' */ + dev_opp = find_device_opp(dev); + if (WARN(IS_ERR(dev_opp), "%s: dev_opp: %ld\n", dev_name(dev), + PTR_ERR(dev_opp))) + return; + + /* Hold our list modification lock here */ + mutex_lock(&dev_opp_list_lock); + + /* Free static OPPs */ + list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) { + if (!opp->dynamic) + __dev_pm_opp_remove(dev_opp, opp); + } + + mutex_unlock(&dev_opp_list_lock); +} +EXPORT_SYMBOL_GPL(of_free_opp_table); #endif diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 0330217..cec2d45 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -21,7 +21,7 @@ struct dev_pm_opp; struct device;
enum dev_pm_opp_event { - OPP_EVENT_ADD, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE, + OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE, };
#if defined(CONFIG_PM_OPP) @@ -44,6 +44,7 @@ struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt); +void dev_pm_opp_remove(struct device *dev, unsigned long freq);
int dev_pm_opp_enable(struct device *dev, unsigned long freq);
@@ -90,6 +91,10 @@ static inline int dev_pm_opp_add(struct device *dev, unsigned long freq, return -EINVAL; }
+static inline void dev_pm_opp_remove(struct device *dev, unsigned long freq) +{ +} + static inline int dev_pm_opp_enable(struct device *dev, unsigned long freq) { return 0; @@ -109,11 +114,16 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
#if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) int of_init_opp_table(struct device *dev); +void of_free_opp_table(struct device *dev); #else static inline int of_init_opp_table(struct device *dev) { return -EINVAL; } + +static inline void of_free_opp_table(struct device *dev) +{ +} #endif
#endif /* __LINUX_OPP_H__ */
This existed before we introduced call_srcu() in opp layer to synchronize with srcu_notifier_call_chain() while removing OPPs. And is a potential bug which wasn't noticed earlier.
Let fix it as well by using the right API to free OPP.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/opp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 977474a..2d195f3 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -641,7 +641,7 @@ static int opp_set_availability(struct device *dev, unsigned long freq,
list_replace_rcu(&opp->node, &new_opp->node); mutex_unlock(&dev_opp_list_lock); - kfree_rcu(opp, rcu_head); + call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, kfree_opp_rcu);
/* Notify the change of the OPP availability */ if (availability_req)
On Thu, Nov 27, 2014 at 08:54:06AM +0530, Viresh Kumar wrote:
OPPs are created statically (from DT) or dynamically. Currently we don't free OPPs that are created statically, when the module unloads. And so if the module is inserted back again, we get warning for duplicate OPPs as the same were already present.
Also, there might be a need to remove dynamic OPPs in future and so API for that is also added.
This patch adds helper APIs to remove/free existing static and dynamic OPPs.
Because the OPPs are used both under RCU and SRCU, we have to wait for grace period of both. And so are using kfree_rcu() from within call_srcu().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
This looks plausible from an RCU viewpoint. In particular, the kfree_rcu() within the SRCU callback will result in freeing being deferred until after both an SRCU and an RCU grace period elapsing.
Thanx, Paul
V1->V2:
- Use kfree_rcu() instead of kfree() as OPP is accessed under rcu locks as well and so we should wait for its grace period as well.
- Another patch which fixes a potential bug, 5/8. Must be applied after this patch and before the original 5/8. Total of 9 patches now.
drivers/base/power/opp.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_opp.h | 12 +++++- 2 files changed, 113 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index b249b01..977474a 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -79,6 +79,7 @@ struct dev_pm_opp {
however addition is possible and is secured by dev_opp_list_lock- @dev: device pointer
- @srcu_head: notifier head to notify the OPP availability changes.
- @rcu_head: RCU callback head used for deferred freeing
- @opp_list: list of opps
- This is an internal data structure maintaining the link to opps attached to
@@ -90,6 +91,7 @@ struct device_opp {
struct device *dev; struct srcu_notifier_head srcu_head;
- struct rcu_head rcu_head; struct list_head opp_list;
};
@@ -498,6 +500,76 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) } EXPORT_SYMBOL_GPL(dev_pm_opp_add);
+static void kfree_opp_rcu(struct rcu_head *head) +{
- struct dev_pm_opp *opp = container_of(head, struct dev_pm_opp, rcu_head);
- kfree_rcu(opp, rcu_head);
+}
+static void kfree_device_rcu(struct rcu_head *head) +{
- struct device_opp *device_opp = container_of(head, struct device_opp, rcu_head);
- kfree(device_opp);
+}
+void __dev_pm_opp_remove(struct device_opp *dev_opp, struct dev_pm_opp *opp) +{
- /*
* Notify the changes in the availability of the operable* frequency/voltage list.*/- srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
- list_del_rcu(&opp->node);
- call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, kfree_opp_rcu);
- if (list_empty(&dev_opp->opp_list)) {
list_del_rcu(&dev_opp->node);call_srcu(&dev_opp->srcu_head.srcu, &dev_opp->rcu_head,kfree_device_rcu);- }
+}
+/**
- dev_pm_opp_remove() - Remove an OPP from OPP list
- @dev: device for which we do this operation
- @freq: OPP to remove with matching 'freq'
- This function removes an opp from the opp list.
- */
+void dev_pm_opp_remove(struct device *dev, unsigned long freq) +{
- struct dev_pm_opp *opp;
- struct device_opp *dev_opp;
- bool found = false;
- /* Hold our list modification lock here */
- mutex_lock(&dev_opp_list_lock);
- dev_opp = find_device_opp(dev);
- if (IS_ERR(dev_opp))
goto unlock;- list_for_each_entry(opp, &dev_opp->opp_list, node) {
if (opp->rate == freq) {found = true;break;}- }
- if (!found) {
dev_warn(dev, "%s: Couldn't find OPP with freq: %lu\n",__func__, freq);goto unlock;- }
- __dev_pm_opp_remove(dev_opp, opp);
+unlock:
- mutex_unlock(&dev_opp_list_lock);
+} +EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
/**
- opp_set_availability() - helper to set the availability of an opp
- @dev: device for which we do this operation
@@ -687,4 +759,34 @@ int of_init_opp_table(struct device *dev) return 0; } EXPORT_SYMBOL_GPL(of_init_opp_table);
+/**
- of_free_opp_table() - Free OPP table entries created from static DT entries
- @dev: device pointer used to lookup device OPPs.
- Free OPPs created using static entries present in DT.
- */
+void of_free_opp_table(struct device *dev) +{
- struct device_opp *dev_opp = find_device_opp(dev);
- struct dev_pm_opp *opp, *tmp;
- /* Check for existing list for 'dev' */
- dev_opp = find_device_opp(dev);
- if (WARN(IS_ERR(dev_opp), "%s: dev_opp: %ld\n", dev_name(dev),
PTR_ERR(dev_opp)))return;- /* Hold our list modification lock here */
- mutex_lock(&dev_opp_list_lock);
- /* Free static OPPs */
- list_for_each_entry_safe(opp, tmp, &dev_opp->opp_list, node) {
if (!opp->dynamic)__dev_pm_opp_remove(dev_opp, opp);- }
- mutex_unlock(&dev_opp_list_lock);
+} +EXPORT_SYMBOL_GPL(of_free_opp_table); #endif diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h index 0330217..cec2d45 100644 --- a/include/linux/pm_opp.h +++ b/include/linux/pm_opp.h @@ -21,7 +21,7 @@ struct dev_pm_opp; struct device;
enum dev_pm_opp_event {
- OPP_EVENT_ADD, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
- OPP_EVENT_ADD, OPP_EVENT_REMOVE, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
};
#if defined(CONFIG_PM_OPP) @@ -44,6 +44,7 @@ struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt); +void dev_pm_opp_remove(struct device *dev, unsigned long freq);
int dev_pm_opp_enable(struct device *dev, unsigned long freq);
@@ -90,6 +91,10 @@ static inline int dev_pm_opp_add(struct device *dev, unsigned long freq, return -EINVAL; }
+static inline void dev_pm_opp_remove(struct device *dev, unsigned long freq) +{ +}
static inline int dev_pm_opp_enable(struct device *dev, unsigned long freq) { return 0; @@ -109,11 +114,16 @@ static inline struct srcu_notifier_head *dev_pm_opp_get_notifier(
#if defined(CONFIG_PM_OPP) && defined(CONFIG_OF) int of_init_opp_table(struct device *dev); +void of_free_opp_table(struct device *dev); #else static inline int of_init_opp_table(struct device *dev) { return -EINVAL; }
+static inline void of_free_opp_table(struct device *dev) +{ +} #endif
#endif /* __LINUX_OPP_H__ */
2.0.3.693.g996b0fd
OPP layer now supports freeing of OPPs and we should free them once they aren't useful anymore.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/arm_big_little.c | 7 ++++++- drivers/cpufreq/arm_big_little.h | 5 ++++- drivers/cpufreq/arm_big_little_dt.c | 1 + 3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c index a46c223..e1a6ba6 100644 --- a/drivers/cpufreq/arm_big_little.c +++ b/drivers/cpufreq/arm_big_little.c @@ -289,6 +289,8 @@ static void _put_cluster_clk_and_freq_table(struct device *cpu_dev)
clk_put(clk[cluster]); dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table[cluster]); + if (arm_bL_ops->free_opp_table) + arm_bL_ops->free_opp_table(cpu_dev); dev_dbg(cpu_dev, "%s: cluster: %d\n", __func__, cluster); }
@@ -337,7 +339,7 @@ static int _get_cluster_clk_and_freq_table(struct device *cpu_dev) if (ret) { dev_err(cpu_dev, "%s: failed to init cpufreq table, cpu: %d, err: %d\n", __func__, cpu_dev->id, ret); - goto out; + goto free_opp_table; }
name[12] = cluster + '0'; @@ -354,6 +356,9 @@ static int _get_cluster_clk_and_freq_table(struct device *cpu_dev) ret = PTR_ERR(clk[cluster]); dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table[cluster]);
+free_opp_table: + if (arm_bL_ops->free_opp_table) + arm_bL_ops->free_opp_table(cpu_dev); out: dev_err(cpu_dev, "%s: Failed to get data for cluster: %d\n", __func__, cluster); diff --git a/drivers/cpufreq/arm_big_little.h b/drivers/cpufreq/arm_big_little.h index 70f18fc..a211f7d 100644 --- a/drivers/cpufreq/arm_big_little.h +++ b/drivers/cpufreq/arm_big_little.h @@ -25,13 +25,16 @@
struct cpufreq_arm_bL_ops { char name[CPUFREQ_NAME_LEN]; - int (*get_transition_latency)(struct device *cpu_dev);
/* * This must set opp table for cpu_dev in a similar way as done by * of_init_opp_table(). */ int (*init_opp_table)(struct device *cpu_dev); + + /* Optional */ + int (*get_transition_latency)(struct device *cpu_dev); + void (*free_opp_table)(struct device *cpu_dev); };
int bL_cpufreq_register(struct cpufreq_arm_bL_ops *ops); diff --git a/drivers/cpufreq/arm_big_little_dt.c b/drivers/cpufreq/arm_big_little_dt.c index 4550f69..ef0b3f1 100644 --- a/drivers/cpufreq/arm_big_little_dt.c +++ b/drivers/cpufreq/arm_big_little_dt.c @@ -82,6 +82,7 @@ static struct cpufreq_arm_bL_ops dt_bL_ops = { .name = "dt-bl", .get_transition_latency = dt_get_transition_latency, .init_opp_table = dt_init_opp_table, + .free_opp_table = of_free_opp_table, };
static int generic_bL_probe(struct platform_device *pdev)
Hi Rafael,
On 25 November 2014 at 16:04, Viresh Kumar viresh.kumar@linaro.org wrote:
OPP layer now supports freeing of OPPs and we should free them once they aren't useful anymore.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/arm_big_little.c | 7 ++++++- drivers/cpufreq/arm_big_little.h | 5 ++++- drivers/cpufreq/arm_big_little_dt.c | 1 + 3 files changed, 11 insertions(+), 2 deletions(-)
Looks like you missed applying this one from this series ?
On Monday, December 01, 2014 12:41:59 PM Viresh Kumar wrote:
Hi Rafael,
On 25 November 2014 at 16:04, Viresh Kumar viresh.kumar@linaro.org wrote:
OPP layer now supports freeing of OPPs and we should free them once they aren't useful anymore.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/arm_big_little.c | 7 ++++++- drivers/cpufreq/arm_big_little.h | 5 ++++- drivers/cpufreq/arm_big_little_dt.c | 1 + 3 files changed, 11 insertions(+), 2 deletions(-)
Looks like you missed applying this one from this series ?
I overlooked this one due to a numbering conflict. Applied now.
On 2 December 2014 at 04:07, Rafael J. Wysocki rjw@rjwysocki.net wrote:
I overlooked this one due to a numbering conflict. Applied now.
I guessed that :).. Thanks.
OPP layer now supports freeing of OPPs and we should free them once they aren't useful anymore.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq-dt.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index 8cba13d..f44419c 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -215,7 +215,7 @@ static int cpufreq_init(struct cpufreq_policy *policy) priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (!priv) { ret = -ENOMEM; - goto out_put_node; + goto out_free_opp; }
of_property_read_u32(np, "voltage-tolerance", &priv->voltage_tolerance); @@ -310,7 +310,8 @@ static int cpufreq_init(struct cpufreq_policy *policy) dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); out_free_priv: kfree(priv); -out_put_node: +out_free_opp: + of_free_opp_table(cpu_dev); of_node_put(np); out_put_reg_clk: clk_put(cpu_clk); @@ -326,6 +327,7 @@ static int cpufreq_exit(struct cpufreq_policy *policy)
cpufreq_cooling_unregister(priv->cdev); dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); + of_free_opp_table(priv->cpu_dev); clk_put(policy->clk); if (!IS_ERR(priv->cpu_reg)) regulator_put(priv->cpu_reg);
Hi Viresh,
Viresh Kumar viresh.kumar@linaro.org hat am 25. November 2014 um 11:34 geschrieben:
OPP layer now supports freeing of OPPs and we should free them once they aren't useful anymore.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
i applied the complete patch series to my repository and tested successfully the cpufreq-dt with my i.MX28 board.
Tested-by: Stefan Wahren stefan.wahren@i2se.com
Thanks
Stefan
OPP layer now supports freeing of OPPs and we should free them once they aren't useful anymore.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/exynos5440-cpufreq.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c index f33f25b..27a57ed 100644 --- a/drivers/cpufreq/exynos5440-cpufreq.c +++ b/drivers/cpufreq/exynos5440-cpufreq.c @@ -371,7 +371,7 @@ static int exynos_cpufreq_probe(struct platform_device *pdev) if (ret) { dev_err(dvfs_info->dev, "failed to init cpufreq table: %d\n", ret); - goto err_put_node; + goto err_free_opp; } dvfs_info->freq_count = dev_pm_opp_get_opp_count(dvfs_info->dev); exynos_sort_descend_freq_table(); @@ -423,6 +423,8 @@ static int exynos_cpufreq_probe(struct platform_device *pdev)
err_free_table: dev_pm_opp_free_cpufreq_table(dvfs_info->dev, &dvfs_info->freq_table); +err_free_opp: + of_free_opp_table(dvfs_info->dev); err_put_node: of_node_put(np); dev_err(&pdev->dev, "%s: failed initialization\n", __func__); @@ -433,6 +435,7 @@ static int exynos_cpufreq_remove(struct platform_device *pdev) { cpufreq_unregister_driver(&exynos_driver); dev_pm_opp_free_cpufreq_table(dvfs_info->dev, &dvfs_info->freq_table); + of_free_opp_table(dvfs_info->dev); return 0; }
OPP layer now supports freeing of OPPs and we should free them once they aren't useful anymore.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/imx6q-cpufreq.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c index c2d3076..5da1d13 100644 --- a/drivers/cpufreq/imx6q-cpufreq.c +++ b/drivers/cpufreq/imx6q-cpufreq.c @@ -31,6 +31,7 @@ static struct clk *step_clk; static struct clk *pll2_pfd2_396m_clk;
static struct device *cpu_dev; +static bool free_opp; static struct cpufreq_frequency_table *freq_table; static unsigned int transition_latency;
@@ -207,11 +208,14 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev) goto put_reg; }
+ /* Because we have added the OPPs here, we must free them */ + free_opp = true; + num = dev_pm_opp_get_opp_count(cpu_dev); if (num < 0) { ret = num; dev_err(cpu_dev, "no OPP table is found: %d\n", ret); - goto put_reg; + goto out_free_opp; } }
@@ -306,6 +310,9 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
free_freq_table: dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); +out_free_opp: + if (free_opp) + of_free_opp_table(cpu_dev); put_reg: if (!IS_ERR(arm_reg)) regulator_put(arm_reg); @@ -332,6 +339,8 @@ static int imx6q_cpufreq_remove(struct platform_device *pdev) { cpufreq_unregister_driver(&imx6q_cpufreq_driver); dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); + if (free_opp) + of_free_opp_table(cpu_dev); regulator_put(arm_reg); if (!IS_ERR(pu_reg)) regulator_put(pu_reg);
linaro-kernel@lists.linaro.org
-
Paul E. McKenney -
Rafael J. Wysocki -
Stefan Wahren -
Viresh Kumar