[PATCH 0/8] OPP: Remove OPPs when not in use

[PATCH 0/8] OPP: Remove OPPs when not in use

[Viresh Kumar]

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(-)

-- 
2.0.3.693.g996b0fd

[PATCH 1/8] opp: rename 'head' as 'rcu_head' or 'srcu_head' based on its type

[Viresh Kumar]

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
-- 
2.0.3.693.g996b0fd

[PATCH 2/8] opp: don't match for existing OPPs when list is empty

[Viresh Kumar]

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);
 
-- 
2.0.3.693.g996b0fd

[PATCH 3/8] opp: mark OPPs as 'static' or 'dynamic'

[Viresh Kumar]

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;
-- 
2.0.3.693.g996b0fd

[PATCH 4/8] opp: Introduce APIs to remove OPPs

[Viresh Kumar]

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__ */
-- 
2.0.3.693.g996b0fd

Re: [PATCH 4/8] opp: Introduce APIs to remove OPPs

[Paul E. McKenney]

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
> 

Re: [PATCH 4/8] opp: Introduce APIs to remove OPPs

[Viresh Kumar]

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.

Re: [PATCH 4/8] opp: Introduce APIs to remove OPPs

[Paul E. McKenney]

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

Re: [PATCH 4/8] opp: Introduce APIs to remove OPPs

[Viresh Kumar]

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

Re: [PATCH 4/8] opp: Introduce APIs to remove OPPs

[Viresh Kumar]

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)

Re: [PATCH 4/8] opp: Introduce APIs to remove OPPs

[Rafael J. Wysocki]

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)

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 4/8] opp: Introduce APIs to remove OPPs

[Viresh Kumar]

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.

Re: [PATCH 4/8] opp: Introduce APIs to remove OPPs

[Rafael J. Wysocki]

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.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH V2 4/8] opp: Introduce APIs to remove OPPs

[Viresh Kumar]

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__ */
-- 
2.0.3.693.g996b0fd

Re: [PATCH V2 4/8] opp: Introduce APIs to remove OPPs

[Paul E. McKenney]

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
> 

[PATCH V1 5/8] opp: replace kfree_rcu() with call_srcu() in opp_set_availability()

[Viresh Kumar]

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)
-- 
2.0.3.693.g996b0fd

[PATCH 5/8] arm_big_little: free OPP table created during ->init()

[Viresh Kumar]

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)
-- 
2.0.3.693.g996b0fd

Re: [PATCH 5/8] arm_big_little: free OPP table created during ->init()

[Viresh Kumar]

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 ?

Re: [PATCH 5/8] arm_big_little: free OPP table created during ->init()

[Rafael J. Wysocki]

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.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 5/8] arm_big_little: free OPP table created during ->init()

[Viresh Kumar]

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.

[PATCH 6/8] cpufreq-dt: free OPP table created during ->init()

[Viresh Kumar]

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);
-- 
2.0.3.693.g996b0fd

Re: [PATCH 6/8] cpufreq-dt: free OPP table created during ->init()

[Stefan Wahren]

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

[PATCH 7/8] exynos5440: free OPP table created during ->init()

[Viresh Kumar]

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;
 }
 
-- 
2.0.3.693.g996b0fd

[PATCH 8/8] imx6q: free OPP table created during ->init()

[Viresh Kumar]

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);
-- 
2.0.3.693.g996b0fd