[PATCH 0/8] PM / devfreq: Add dev_pm_qos support

[PATCH 0/8] PM / devfreq: Add dev_pm_qos support

[Leonard Crestez]

Add dev_pm_qos notifiers to devfreq core in order to support frequency
limits via dev_pm_qos_add_request.

Unlike the rest of devfreq the dev_pm_qos frequency is measured in Khz,
this is consistent with current dev_pm_qos usage for cpufreq and
allows frequencies above 2Ghz (pm_qos expresses limits as s32).

Like with cpufreq the handling of min_freq/max_freq is moved to the
dev_pm_qos mechanism. Constraints from userspace are no longer clamped on
store, instead all values can be written and we only check against OPPs in a
new devfreq_get_freq_range function. This is consistent with the design of
dev_pm_qos.

Notifiers from pm_qos are executed under a single global dev_pm_qos_mtx and
need to take devfreq->lock. Notifier registration takes the same dev_pm_qos_mtx
so in order to prevent lockdep warnings it must be done outside devfreq->lock.
Current devfreq_add_device does *all* initialization under devfreq->lock
and that needs to be relaxed.

---

Changes since v4:
* Move more devfreq_add_device init ahead of device_register.
* Make devfreq_dev_release cleanup devices not yet in devfreq_list. This is
simpler than previous attempt to add to devfreq_list sonner.
* Take devfreq->lock in trans_stat_show
* Register dev_pm_opp notifier on devfreq parent dev (which has OPPs)
Link to v4: https://patchwork.kernel.org/cover/11114657/

Changes since v3:
* Cleanup locking and error-handling in devfreq_add_device
* Register notifiers after device registration but before governor start
* Keep the initialization of min_req/max_req ahead of device_register
because it's used for sysfs handling
* Use HZ_PER_KHZ instead of 1000
* Add kernel-doc comments
* Move OPP notifier to core
Link to v3: https://patchwork.kernel.org/cover/11104061/

Changes since v2:
* Handle sysfs via dev_pm_qos (in separate patch)
* Add locking to {min,max}_freq_show
* Fix checkpatch issues (long lines etc)
Link to v2: https://patchwork.kernel.org/patch/11084279/

Changes since v1:
* Add doxygen comments for min_nb/max_nb
* Remove notifiers on error/cleanup paths. Keep gotos simple by relying on
dev_pm_qos_remove_notifier ignoring notifiers which were not added.
Link to v1: https://patchwork.kernel.org/patch/11078475/

Leonard Crestez (8):
  PM / devfreq: Lock devfreq in trans_stat_show
  PM / devfreq: Don't fail devfreq_dev_release if not in list
  PM / devfreq: Move more initialization before registration
  PM / devfreq: Don't take lock in devfreq_add_device
  PM / devfreq: Introduce devfreq_get_freq_range
  PM / devfreq: Add dev_pm_qos support
  PM / devfreq: Use dev_pm_qos for sysfs min/max_freq
  PM / devfreq: Move opp notifier registration to core

 drivers/devfreq/devfreq.c    | 362 +++++++++++++++++++----------------
 drivers/devfreq/exynos-bus.c |   7 -
 drivers/devfreq/rk3399_dmc.c |   6 -
 include/linux/devfreq.h      |  22 +--
 4 files changed, 204 insertions(+), 193 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/8] PM / devfreq: Lock devfreq in trans_stat_show

[Leonard Crestez]

There is no locking in this sysfs show function so stats printing can
race with a devfreq_update_status called as part of freq switching or
with initialization.

Also add an assert in devfreq_update_status to make it clear that lock
must be held by caller.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2494ee16f502..665575228c4f 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -159,10 +159,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
 {
 	int lev, prev_lev, ret = 0;
 	unsigned long cur_time;
 
 	cur_time = jiffies;
+	lockdep_assert_held(&devfreq->lock);
 
 	/* Immediately exit if previous_freq is not initialized yet. */
 	if (!devfreq->previous_freq)
 		goto out;
 
@@ -1415,15 +1416,20 @@ static ssize_t trans_stat_show(struct device *dev,
 	struct devfreq *devfreq = to_devfreq(dev);
 	ssize_t len;
 	int i, j;
 	unsigned int max_state = devfreq->profile->max_state;
 
+	mutex_lock(&devfreq->lock);
 	if (!devfreq->stop_polling &&
-			devfreq_update_status(devfreq, devfreq->previous_freq))
-		return 0;
-	if (max_state == 0)
-		return sprintf(buf, "Not Supported.\n");
+			devfreq_update_status(devfreq, devfreq->previous_freq)) {
+		len = 0;
+		goto out;
+	}
+	if (max_state == 0) {
+		len = sprintf(buf, "Not Supported.\n");
+		goto out;
+	}
 
 	len = sprintf(buf, "     From  :   To\n");
 	len += sprintf(buf + len, "           :");
 	for (i = 0; i < max_state; i++)
 		len += sprintf(buf + len, "%10lu",
@@ -1447,10 +1453,13 @@ static ssize_t trans_stat_show(struct device *dev,
 			jiffies_to_msecs(devfreq->time_in_state[i]));
 	}
 
 	len += sprintf(buf + len, "Total transition : %u\n",
 					devfreq->total_trans);
+
+out:
+	mutex_unlock(&devfreq->lock);
 	return len;
 }
 static DEVICE_ATTR_RO(trans_stat);
 
 static struct attribute *devfreq_attrs[] = {
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/8] PM / devfreq: Don't fail devfreq_dev_release if not in list

[Leonard Crestez]

Right now devfreq_dev_release will print a warning and abort the rest of
the cleanup if the devfreq instance is not part of the global
devfreq_list. But this is a valid scenario, for example it can happen if
the governor can't be found or on any other init error that happens
after device_register.

Initialize devfreq->node to an empty list head in devfreq_add_device so
that list_del becomes a safe noop inside devfreq_dev_release and we can
continue the rest of the cleanup.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 665575228c4f..a715f27f35fd 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -582,15 +582,10 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
 static void devfreq_dev_release(struct device *dev)
 {
 	struct devfreq *devfreq = to_devfreq(dev);
 
 	mutex_lock(&devfreq_list_lock);
-	if (IS_ERR(find_device_devfreq(devfreq->dev.parent))) {
-		mutex_unlock(&devfreq_list_lock);
-		dev_warn(&devfreq->dev, "releasing devfreq which doesn't exist\n");
-		return;
-	}
 	list_del(&devfreq->node);
 	mutex_unlock(&devfreq_list_lock);
 
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
@@ -641,10 +636,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	mutex_init(&devfreq->lock);
 	mutex_lock(&devfreq->lock);
 	devfreq->dev.parent = dev;
 	devfreq->dev.class = devfreq_class;
 	devfreq->dev.release = devfreq_dev_release;
+	INIT_LIST_HEAD(&devfreq->node);
 	devfreq->profile = profile;
 	strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
 	devfreq->previous_freq = profile->initial_freq;
 	devfreq->last_status.current_frequency = profile->initial_freq;
 	devfreq->data = data;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/8] PM / devfreq: Move more initialization before registration

[Leonard Crestez]

In general it is a better to initialize an object before making it
accessible externally (through device_register).

This make it possible to avoid relying on locking a partially
initialized object.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index a715f27f35fd..57a217fc92de 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev)
 
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
 	mutex_destroy(&devfreq->lock);
+	kfree(devfreq->time_in_state);
+	kfree(devfreq->trans_table);
 	kfree(devfreq);
 }
 
 /**
  * devfreq_add_device() - Add devfreq feature to the device
@@ -671,44 +673,43 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->max_freq = devfreq->scaling_max_freq;
 
 	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
 	atomic_set(&devfreq->suspend_count, 0);
 
-	dev_set_name(&devfreq->dev, "devfreq%d",
-				atomic_inc_return(&devfreq_no));
-	err = device_register(&devfreq->dev);
-	if (err) {
-		mutex_unlock(&devfreq->lock);
-		put_device(&devfreq->dev);
-		goto err_out;
-	}
-
-	devfreq->trans_table = devm_kzalloc(&devfreq->dev,
+	devfreq->trans_table = kzalloc(
 			array3_size(sizeof(unsigned int),
 				    devfreq->profile->max_state,
 				    devfreq->profile->max_state),
 			GFP_KERNEL);
 	if (!devfreq->trans_table) {
 		mutex_unlock(&devfreq->lock);
 		err = -ENOMEM;
-		goto err_devfreq;
+		goto err_dev;
 	}
 
-	devfreq->time_in_state = devm_kcalloc(&devfreq->dev,
-			devfreq->profile->max_state,
-			sizeof(unsigned long),
-			GFP_KERNEL);
+	devfreq->time_in_state = kcalloc(devfreq->profile->max_state,
+					 sizeof(unsigned long),
+					 GFP_KERNEL);
 	if (!devfreq->time_in_state) {
 		mutex_unlock(&devfreq->lock);
 		err = -ENOMEM;
-		goto err_devfreq;
+		goto err_dev;
 	}
 
 	devfreq->last_stat_updated = jiffies;
 
 	srcu_init_notifier_head(&devfreq->transition_notifier_list);
 
+	dev_set_name(&devfreq->dev, "devfreq%d",
+				atomic_inc_return(&devfreq_no));
+	err = device_register(&devfreq->dev);
+	if (err) {
+		mutex_unlock(&devfreq->lock);
+		put_device(&devfreq->dev);
+		goto err_out;
+	}
+
 	mutex_unlock(&devfreq->lock);
 
 	mutex_lock(&devfreq_list_lock);
 
 	governor = try_then_request_governor(devfreq->governor_name);
@@ -734,14 +735,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
 
 	return devfreq;
 
 err_init:
 	mutex_unlock(&devfreq_list_lock);
-err_devfreq:
 	devfreq_remove_device(devfreq);
-	devfreq = NULL;
+	return ERR_PTR(err);
 err_dev:
+	kfree(devfreq->time_in_state);
+	kfree(devfreq->trans_table);
 	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL(devfreq_add_device);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/8] PM / devfreq: Don't take lock in devfreq_add_device

[Leonard Crestez]

A device usually doesn't need to lock itself during initialization
because it is not yet reachable from other threads.

This simplifies the code and helps avoid recursive lock warnings.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 57a217fc92de..860cbbab476c 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -634,11 +634,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
 		err = -ENOMEM;
 		goto err_out;
 	}
 
 	mutex_init(&devfreq->lock);
-	mutex_lock(&devfreq->lock);
 	devfreq->dev.parent = dev;
 	devfreq->dev.class = devfreq_class;
 	devfreq->dev.release = devfreq_dev_release;
 	INIT_LIST_HEAD(&devfreq->node);
 	devfreq->profile = profile;
@@ -647,28 +646,24 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->last_status.current_frequency = profile->initial_freq;
 	devfreq->data = data;
 	devfreq->nb.notifier_call = devfreq_notifier_call;
 
 	if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
-		mutex_unlock(&devfreq->lock);
 		err = set_freq_table(devfreq);
 		if (err < 0)
 			goto err_dev;
-		mutex_lock(&devfreq->lock);
 	}
 
 	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
 	if (!devfreq->scaling_min_freq) {
-		mutex_unlock(&devfreq->lock);
 		err = -EINVAL;
 		goto err_dev;
 	}
 	devfreq->min_freq = devfreq->scaling_min_freq;
 
 	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
 	if (!devfreq->scaling_max_freq) {
-		mutex_unlock(&devfreq->lock);
 		err = -EINVAL;
 		goto err_dev;
 	}
 	devfreq->max_freq = devfreq->scaling_max_freq;
 
@@ -679,20 +674,18 @@ struct devfreq *devfreq_add_device(struct device *dev,
 			array3_size(sizeof(unsigned int),
 				    devfreq->profile->max_state,
 				    devfreq->profile->max_state),
 			GFP_KERNEL);
 	if (!devfreq->trans_table) {
-		mutex_unlock(&devfreq->lock);
 		err = -ENOMEM;
 		goto err_dev;
 	}
 
 	devfreq->time_in_state = kcalloc(devfreq->profile->max_state,
 					 sizeof(unsigned long),
 					 GFP_KERNEL);
 	if (!devfreq->time_in_state) {
-		mutex_unlock(&devfreq->lock);
 		err = -ENOMEM;
 		goto err_dev;
 	}
 
 	devfreq->last_stat_updated = jiffies;
@@ -701,17 +694,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
 
 	dev_set_name(&devfreq->dev, "devfreq%d",
 				atomic_inc_return(&devfreq_no));
 	err = device_register(&devfreq->dev);
 	if (err) {
-		mutex_unlock(&devfreq->lock);
 		put_device(&devfreq->dev);
 		goto err_out;
 	}
 
-	mutex_unlock(&devfreq->lock);
-
 	mutex_lock(&devfreq_list_lock);
 
 	governor = try_then_request_governor(devfreq->governor_name);
 	if (IS_ERR(governor)) {
 		dev_err(dev, "%s: Unable to find governor for the device\n",
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/8] PM / devfreq: Introduce devfreq_get_freq_range

[Leonard Crestez]

Moving handling of min/max freq to a single function and call it from
update_devfreq and for printing min/max freq values in sysfs.

This changes the behavior of out-of-range min_freq/max_freq: clamping
is now done at evaluation time. This means that if an out-of-range
constraint is imposed by sysfs and it later becomes valid then it will
be enforced.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 111 +++++++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 48 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 860cbbab476c..51a4179e2c69 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -24,10 +24,12 @@
 #include <linux/printk.h>
 #include <linux/hrtimer.h>
 #include <linux/of.h>
 #include "governor.h"
 
+#define HZ_PER_KHZ 1000
+
 #define CREATE_TRACE_POINTS
 #include <trace/events/devfreq.h>
 
 static struct class *devfreq_class;
 
@@ -96,10 +98,50 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
 		dev_pm_opp_put(opp);
 
 	return max_freq;
 }
 
+/**
+ * devfreq_get_freq_range() - Get the current freq range
+ * @devfreq:	the devfreq instance
+ * @min_freq:	the min frequency
+ * @max_freq:	the max frequency
+ *
+ * This takes into consideration all constraints.
+ */
+static void devfreq_get_freq_range(struct devfreq *devfreq,
+				   unsigned long *min_freq,
+				   unsigned long *max_freq)
+{
+	unsigned long *freq_table = devfreq->profile->freq_table;
+
+	lockdep_assert_held(&devfreq->lock);
+
+	/* Init min/max frequency from freq table */
+	if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
+		*min_freq = freq_table[0];
+		*max_freq = freq_table[devfreq->profile->max_state - 1];
+	} else {
+		*min_freq = freq_table[devfreq->profile->max_state - 1];
+		*max_freq = freq_table[0];
+	}
+
+	/* constraints from sysfs: */
+	*min_freq = max(*min_freq, devfreq->min_freq);
+	*max_freq = min(*max_freq, devfreq->max_freq);
+
+	/* constraints from opp interface: */
+	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
+	/* scaling_max_freq can be zero on error */
+	if (devfreq->scaling_max_freq)
+		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
+
+	/* max_freq takes precedence over min_freq */
+	if (*min_freq > *max_freq)
+		*min_freq = *max_freq;
+}
+
 /**
  * devfreq_get_freq_level() - Lookup freq_table for the frequency
  * @devfreq:	the devfreq instance
  * @freq:	the target frequency
  */
@@ -349,21 +391,13 @@ int update_devfreq(struct devfreq *devfreq)
 
 	/* Reevaluate the proper frequency */
 	err = devfreq->governor->get_target_freq(devfreq, &freq);
 	if (err)
 		return err;
+	devfreq_get_freq_range(devfreq, &min_freq, &max_freq);
 
-	/*
-	 * Adjust the frequency with user freq, QoS and available freq.
-	 *
-	 * List from the highest priority
-	 * max_freq
-	 * min_freq
-	 */
-	max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq);
-	min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq);
-
+	/* max freq takes priority over min freq */
 	if (freq < min_freq) {
 		freq = min_freq;
 		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
 	}
 	if (freq > max_freq) {
@@ -1293,40 +1327,28 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
 		return -EINVAL;
 
 	mutex_lock(&df->lock);
-
-	if (value) {
-		if (value > df->max_freq) {
-			ret = -EINVAL;
-			goto unlock;
-		}
-	} else {
-		unsigned long *freq_table = df->profile->freq_table;
-
-		/* Get minimum frequency according to sorting order */
-		if (freq_table[0] < freq_table[df->profile->max_state - 1])
-			value = freq_table[0];
-		else
-			value = freq_table[df->profile->max_state - 1];
-	}
-
 	df->min_freq = value;
 	update_devfreq(df);
-	ret = count;
-unlock:
 	mutex_unlock(&df->lock);
-	return ret;
+
+	return count;
 }
 
 static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
 	struct devfreq *df = to_devfreq(dev);
+	unsigned long min_freq, max_freq;
 
-	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
+	mutex_lock(&df->lock);
+	devfreq_get_freq_range(df, &min_freq, &max_freq);
+	mutex_unlock(&df->lock);
+
+	return sprintf(buf, "%lu\n", min_freq);
 }
 
 static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
@@ -1338,40 +1360,33 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 	if (ret != 1)
 		return -EINVAL;
 
 	mutex_lock(&df->lock);
 
-	if (value) {
-		if (value < df->min_freq) {
-			ret = -EINVAL;
-			goto unlock;
-		}
-	} else {
-		unsigned long *freq_table = df->profile->freq_table;
-
-		/* Get maximum frequency according to sorting order */
-		if (freq_table[0] < freq_table[df->profile->max_state - 1])
-			value = freq_table[df->profile->max_state - 1];
-		else
-			value = freq_table[0];
-	}
+	/* Interpret zero as "don't care" */
+	if (!value)
+		value = ULONG_MAX;
 
 	df->max_freq = value;
 	update_devfreq(df);
-	ret = count;
-unlock:
 	mutex_unlock(&df->lock);
-	return ret;
+
+	return count;
 }
 static DEVICE_ATTR_RW(min_freq);
 
 static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
 	struct devfreq *df = to_devfreq(dev);
+	unsigned long min_freq, max_freq;
+
+	mutex_lock(&df->lock);
+	devfreq_get_freq_range(df, &min_freq, &max_freq);
+	mutex_unlock(&df->lock);
 
-	return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
+	return sprintf(buf, "%lu\n", max_freq);
 }
 static DEVICE_ATTR_RW(max_freq);
 
 static ssize_t available_frequencies_show(struct device *d,
 					  struct device_attribute *attr,
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/8] PM / devfreq: Add dev_pm_qos support

[Leonard Crestez]

Register notifiers with the pm_qos framework in order to respond to
requests for MIN_FREQUENCY and MAX_FREQUENCY.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 71 +++++++++++++++++++++++++++++++++++++++
 include/linux/devfreq.h   |  5 +++
 2 files changed, 76 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 51a4179e2c69..d8d57318b12c 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -22,17 +22,20 @@
 #include <linux/platform_device.h>
 #include <linux/list.h>
 #include <linux/printk.h>
 #include <linux/hrtimer.h>
 #include <linux/of.h>
+#include <linux/pm_qos.h>
 #include "governor.h"
 
 #define HZ_PER_KHZ 1000
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/devfreq.h>
 
+#define HZ_PER_KHZ	1000
+
 static struct class *devfreq_class;
 
 /*
  * devfreq core provides delayed work based load monitoring helper
  * functions. Governors can use these or can implement their own
@@ -123,10 +126,16 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
 	} else {
 		*min_freq = freq_table[devfreq->profile->max_state - 1];
 		*max_freq = freq_table[0];
 	}
 
+	/* constraints from dev_pm_qos: */
+	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
+				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
+	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
+				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
+
 	/* constraints from sysfs: */
 	*min_freq = max(*min_freq, devfreq->min_freq);
 	*max_freq = min(*max_freq, devfreq->max_freq);
 
 	/* constraints from opp interface: */
@@ -605,10 +614,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
 	mutex_unlock(&devfreq->lock);
 
 	return ret;
 }
 
+/**
+ * devfreq_qos_notifier_call() - Common handler for qos freq changes.
+ * @devfreq:    the devfreq instance.
+ */
+static int devfreq_qos_notifier_call(struct devfreq *devfreq)
+{
+	int ret;
+
+	mutex_lock(&devfreq->lock);
+	ret = update_devfreq(devfreq);
+	mutex_unlock(&devfreq->lock);
+
+	return ret;
+}
+
+/**
+ * devfreq_qos_min_notifier_call() - Callback for qos min_freq changes.
+ * @nb:		Should to be devfreq->nb_min
+ */
+static int devfreq_qos_min_notifier_call(struct notifier_block *nb,
+					 unsigned long val, void *ptr)
+{
+	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min);
+
+	return devfreq_qos_notifier_call(devfreq);
+}
+
+/**
+ * devfreq_qos_max_notifier_call() - Callback for qos min_freq changes.
+ * @nb:		Should to be devfreq->nb_max
+ */
+static int devfreq_qos_max_notifier_call(struct notifier_block *nb,
+					 unsigned long val, void *ptr)
+{
+	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_max);
+
+	return devfreq_qos_notifier_call(devfreq);
+}
+
 /**
  * devfreq_dev_release() - Callback for struct device to release the device.
  * @dev:	the devfreq device
  *
  * Remove devfreq from the list and release its resources.
@@ -619,10 +667,15 @@ static void devfreq_dev_release(struct device *dev)
 
 	mutex_lock(&devfreq_list_lock);
 	list_del(&devfreq->node);
 	mutex_unlock(&devfreq_list_lock);
 
+	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
+			DEV_PM_QOS_MAX_FREQUENCY);
+	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
+			DEV_PM_QOS_MIN_FREQUENCY);
+
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq->time_in_state);
@@ -732,10 +785,27 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	if (err) {
 		put_device(&devfreq->dev);
 		goto err_out;
 	}
 
+	/*
+	 * Register notifiers for updates to min_freq/max_freq after device is
+	 * initialized (and we can handle notifications) but before the governor
+	 * is started (which should do an initial enforcement of constraints)
+	 */
+	devfreq->nb_min.notifier_call = devfreq_qos_min_notifier_call;
+	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
+				      DEV_PM_QOS_MIN_FREQUENCY);
+	if (err)
+		goto err_devfreq;
+
+	devfreq->nb_max.notifier_call = devfreq_qos_max_notifier_call;
+	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
+				      DEV_PM_QOS_MAX_FREQUENCY);
+	if (err)
+		goto err_devfreq;
+
 	mutex_lock(&devfreq_list_lock);
 
 	governor = try_then_request_governor(devfreq->governor_name);
 	if (IS_ERR(governor)) {
 		dev_err(dev, "%s: Unable to find governor for the device\n",
@@ -759,10 +829,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
 
 	return devfreq;
 
 err_init:
 	mutex_unlock(&devfreq_list_lock);
+err_devfreq:
 	devfreq_remove_device(devfreq);
 	return ERR_PTR(err);
 err_dev:
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index c3cbc15fdf08..dac0dffeabb4 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -134,10 +134,12 @@ struct devfreq_dev_profile {
  * @total_trans:	Number of devfreq transitions
  * @trans_table:	Statistics of devfreq transitions
  * @time_in_state:	Statistics of devfreq states
  * @last_stat_updated:	The last time stat updated
  * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
+ * @nb_min:		Notifier block for DEV_PM_QOS_MIN_FREQUENCY
+ * @nb_max:		Notifier block for DEV_PM_QOS_MAX_FREQUENCY
  *
  * This structure stores the devfreq information for a give device.
  *
  * Note that when a governor accesses entries in struct devfreq in its
  * functions except for the context of callbacks defined in struct
@@ -176,10 +178,13 @@ struct devfreq {
 	unsigned int *trans_table;
 	unsigned long *time_in_state;
 	unsigned long last_stat_updated;
 
 	struct srcu_notifier_head transition_notifier_list;
+
+	struct notifier_block nb_min;
+	struct notifier_block nb_max;
 };
 
 struct devfreq_freqs {
 	unsigned long old;
 	unsigned long new;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/8] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Leonard Crestez]

Switch the handling of min_freq and max_freq from sysfs to use the
dev_pm_qos interface.

Since dev_pm_qos handles frequencies as kHz this change reduces the
precision of min_freq and max_freq. This shouldn't introduce problems
because frequencies which are not an integer number of kHz are likely
not an integer number of Hz either.

Try to ensure compatibilitity by rounding min values down and rounding
max values up.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 51 +++++++++++++++++++++++++--------------
 include/linux/devfreq.h   |  9 ++++---
 2 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index d8d57318b12c..7977bad93949 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
 	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
 				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
 	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
 				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
 
-	/* constraints from sysfs: */
-	*min_freq = max(*min_freq, devfreq->min_freq);
-	*max_freq = min(*max_freq, devfreq->max_freq);
-
 	/* constraints from opp interface: */
 	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
 	/* scaling_max_freq can be zero on error */
 	if (devfreq->scaling_max_freq)
 		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
@@ -675,10 +671,12 @@ static void devfreq_dev_release(struct device *dev)
 			DEV_PM_QOS_MIN_FREQUENCY);
 
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
+	dev_pm_qos_remove_request(&devfreq->max_freq_req);
+	dev_pm_qos_remove_request(&devfreq->min_freq_req);
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	kfree(devfreq);
 }
@@ -743,18 +741,28 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
 	if (!devfreq->scaling_min_freq) {
 		err = -EINVAL;
 		goto err_dev;
 	}
-	devfreq->min_freq = devfreq->scaling_min_freq;
 
 	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
 	if (!devfreq->scaling_max_freq) {
 		err = -EINVAL;
 		goto err_dev;
 	}
-	devfreq->max_freq = devfreq->scaling_max_freq;
+
+	/* dev_pm_qos requests for min/max freq from sysfs */
+	err = dev_pm_qos_add_request(dev, &devfreq->min_freq_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (err < 0) {
+		goto err_dev;
+	}
+	err = dev_pm_qos_add_request(dev, &devfreq->max_freq_req,
+				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
+	if (err < 0) {
+		goto err_dev;
+	}
 
 	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
 	atomic_set(&devfreq->suspend_count, 0);
 
 	devfreq->trans_table = kzalloc(
@@ -833,10 +841,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	mutex_unlock(&devfreq_list_lock);
 err_devfreq:
 	devfreq_remove_device(devfreq);
 	return ERR_PTR(err);
 err_dev:
+	if (dev_pm_qos_request_active(&devfreq->max_freq_req))
+		dev_pm_qos_remove_request(&devfreq->max_freq_req);
+	if (dev_pm_qos_request_active(&devfreq->min_freq_req))
+		dev_pm_qos_remove_request(&devfreq->min_freq_req);
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
@@ -1397,14 +1409,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
 		return -EINVAL;
 
-	mutex_lock(&df->lock);
-	df->min_freq = value;
-	update_devfreq(df);
-	mutex_unlock(&df->lock);
+	/* round down to kHz for dev_pm_qos */
+	if (value)
+		value = value / HZ_PER_KHZ;
+
+	ret = dev_pm_qos_update_request(&df->min_freq_req, value);
+	if (ret < 0)
+		return ret;
 
 	return count;
 }
 
 static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
@@ -1429,19 +1444,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
 		return -EINVAL;
 
-	mutex_lock(&df->lock);
-
-	/* Interpret zero as "don't care" */
-	if (!value)
-		value = ULONG_MAX;
+	/* round up to kHz for dev_pm_qos and interpret zero as "don't care" */
+	if (value)
+		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
+	else
+		value = S32_MAX;
 
-	df->max_freq = value;
-	update_devfreq(df);
-	mutex_unlock(&df->lock);
+	ret = dev_pm_qos_update_request(&df->max_freq_req, value);
+	if (ret < 0)
+		return ret;
 
 	return count;
 }
 static DEVICE_ATTR_RW(min_freq);
 
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index dac0dffeabb4..4b5cc80abbe3 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -11,10 +11,11 @@
 #define __LINUX_DEVFREQ_H__
 
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/pm_opp.h>
+#include <linux/pm_qos.h>
 
 #define DEVFREQ_NAME_LEN 16
 
 /* DEVFREQ governor name */
 #define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
@@ -121,12 +122,12 @@ struct devfreq_dev_profile {
  *		devfreq.nb to the corresponding register notifier call chain.
  * @work:	delayed work for load monitoring.
  * @previous_freq:	previously configured frequency value.
  * @data:	Private data of the governor. The devfreq framework does not
  *		touch this.
- * @min_freq:	Limit minimum frequency requested by user (0: none)
- * @max_freq:	Limit maximum frequency requested by user (0: none)
+ * @min_freq_req:	Limit minimum frequency requested by user (0: none)
+ * @max_freq_req:	Limit maximum frequency requested by user (0: none)
  * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
  * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
  * @stop_polling:	 devfreq polling status of a device.
  * @suspend_freq:	 frequency of a device set during suspend phase.
  * @resume_freq:	 frequency of a device set in resume phase.
@@ -161,12 +162,12 @@ struct devfreq {
 	unsigned long previous_freq;
 	struct devfreq_dev_status last_status;
 
 	void *data; /* private data for governors */
 
-	unsigned long min_freq;
-	unsigned long max_freq;
+	struct dev_pm_qos_request min_freq_req;
+	struct dev_pm_qos_request max_freq_req;
 	unsigned long scaling_min_freq;
 	unsigned long scaling_max_freq;
 	bool stop_polling;
 
 	unsigned long suspend_freq;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 8/8] PM / devfreq: Move opp notifier registration to core

[Leonard Crestez]

An opp notifier can be registered by devfreq in order to respond to OPPs
being enabled or disabled at runtime (for example by thermal). This is
currently handled explicitly by drivers.

Move notifier handling to devfreq_add_device because this shouldn't be
hardware-specific.

Handling this inside the core also takes less code.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c    | 84 +++---------------------------------
 drivers/devfreq/exynos-bus.c |  7 ---
 drivers/devfreq/rk3399_dmc.c |  6 ---
 include/linux/devfreq.h      |  8 ----
 4 files changed, 6 insertions(+), 99 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 7977bad93949..b9177430ae8f 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -663,10 +663,11 @@ static void devfreq_dev_release(struct device *dev)
 
 	mutex_lock(&devfreq_list_lock);
 	list_del(&devfreq->node);
 	mutex_unlock(&devfreq_list_lock);
 
+	dev_pm_opp_unregister_notifier(devfreq->dev.parent, &devfreq->nb);
 	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
 			DEV_PM_QOS_MAX_FREQUENCY);
 	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
 			DEV_PM_QOS_MIN_FREQUENCY);
 
@@ -728,11 +729,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->profile = profile;
 	strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
 	devfreq->previous_freq = profile->initial_freq;
 	devfreq->last_status.current_frequency = profile->initial_freq;
 	devfreq->data = data;
-	devfreq->nb.notifier_call = devfreq_notifier_call;
 
 	if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
 		err = set_freq_table(devfreq);
 		if (err < 0)
 			goto err_dev;
@@ -810,10 +810,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
 				      DEV_PM_QOS_MAX_FREQUENCY);
 	if (err)
 		goto err_devfreq;
 
+	devfreq->nb.notifier_call = devfreq_notifier_call;
+	err = dev_pm_opp_register_notifier(devfreq->dev.parent, &devfreq->nb);
+	if (err)
+		goto err_devfreq;
+
 	mutex_lock(&devfreq_list_lock);
 
 	governor = try_then_request_governor(devfreq->governor_name);
 	if (IS_ERR(governor)) {
 		dev_err(dev, "%s: Unable to find governor for the device\n",
@@ -1624,87 +1629,10 @@ struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
 
 	return opp;
 }
 EXPORT_SYMBOL(devfreq_recommended_opp);
 
-/**
- * devfreq_register_opp_notifier() - Helper function to get devfreq notified
- *				     for any changes in the OPP availability
- *				     changes
- * @dev:	The devfreq user device. (parent of devfreq)
- * @devfreq:	The devfreq object.
- */
-int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
-{
-	return dev_pm_opp_register_notifier(dev, &devfreq->nb);
-}
-EXPORT_SYMBOL(devfreq_register_opp_notifier);
-
-/**
- * devfreq_unregister_opp_notifier() - Helper function to stop getting devfreq
- *				       notified for any changes in the OPP
- *				       availability changes anymore.
- * @dev:	The devfreq user device. (parent of devfreq)
- * @devfreq:	The devfreq object.
- *
- * At exit() callback of devfreq_dev_profile, this must be included if
- * devfreq_recommended_opp is used.
- */
-int devfreq_unregister_opp_notifier(struct device *dev, struct devfreq *devfreq)
-{
-	return dev_pm_opp_unregister_notifier(dev, &devfreq->nb);
-}
-EXPORT_SYMBOL(devfreq_unregister_opp_notifier);
-
-static void devm_devfreq_opp_release(struct device *dev, void *res)
-{
-	devfreq_unregister_opp_notifier(dev, *(struct devfreq **)res);
-}
-
-/**
- * devm_devfreq_register_opp_notifier() - Resource-managed
- *					  devfreq_register_opp_notifier()
- * @dev:	The devfreq user device. (parent of devfreq)
- * @devfreq:	The devfreq object.
- */
-int devm_devfreq_register_opp_notifier(struct device *dev,
-				       struct devfreq *devfreq)
-{
-	struct devfreq **ptr;
-	int ret;
-
-	ptr = devres_alloc(devm_devfreq_opp_release, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return -ENOMEM;
-
-	ret = devfreq_register_opp_notifier(dev, devfreq);
-	if (ret) {
-		devres_free(ptr);
-		return ret;
-	}
-
-	*ptr = devfreq;
-	devres_add(dev, ptr);
-
-	return 0;
-}
-EXPORT_SYMBOL(devm_devfreq_register_opp_notifier);
-
-/**
- * devm_devfreq_unregister_opp_notifier() - Resource-managed
- *					    devfreq_unregister_opp_notifier()
- * @dev:	The devfreq user device. (parent of devfreq)
- * @devfreq:	The devfreq object.
- */
-void devm_devfreq_unregister_opp_notifier(struct device *dev,
-					 struct devfreq *devfreq)
-{
-	WARN_ON(devres_release(dev, devm_devfreq_opp_release,
-			       devm_devfreq_dev_match, devfreq));
-}
-EXPORT_SYMBOL(devm_devfreq_unregister_opp_notifier);
-
 /**
  * devfreq_register_notifier() - Register a driver with devfreq
  * @devfreq:	The devfreq object.
  * @nb:		The notifier block to register.
  * @list:	DEVFREQ_TRANSITION_NOTIFIER.
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index c832673273a2..29f422469960 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -356,17 +356,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
 		dev_err(dev, "failed to add devfreq device\n");
 		ret = PTR_ERR(bus->devfreq);
 		goto err;
 	}
 
-	/* Register opp_notifier to catch the change of OPP  */
-	ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
-	if (ret < 0) {
-		dev_err(dev, "failed to register opp notifier\n");
-		goto err;
-	}
-
 	/*
 	 * Enable devfreq-event to get raw data which is used to determine
 	 * current bus load.
 	 */
 	ret = exynos_bus_enable_edev(bus);
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index 2e65d7279d79..f660d2031e8a 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -454,12 +454,10 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 	if (IS_ERR(data->devfreq)) {
 		ret = PTR_ERR(data->devfreq);
 		goto err_free_opp;
 	}
 
-	devm_devfreq_register_opp_notifier(dev, data->devfreq);
-
 	data->dev = dev;
 	platform_set_drvdata(pdev, data);
 
 	return 0;
 
@@ -470,14 +468,10 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 
 static int rk3399_dmcfreq_remove(struct platform_device *pdev)
 {
 	struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(&pdev->dev);
 
-	/*
-	 * Before remove the opp table we need to unregister the opp notifier.
-	 */
-	devm_devfreq_unregister_opp_notifier(dmcfreq->dev, dmcfreq->devfreq);
 	dev_pm_opp_of_remove_table(dmcfreq->dev);
 
 	return 0;
 }
 
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 4b5cc80abbe3..bf6ebfbc1e8a 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -220,18 +220,10 @@ extern void devfreq_resume(void);
 extern int update_devfreq(struct devfreq *devfreq);
 
 /* Helper functions for devfreq user device driver with OPP. */
 extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
 					   unsigned long *freq, u32 flags);
-extern int devfreq_register_opp_notifier(struct device *dev,
-					 struct devfreq *devfreq);
-extern int devfreq_unregister_opp_notifier(struct device *dev,
-					   struct devfreq *devfreq);
-extern int devm_devfreq_register_opp_notifier(struct device *dev,
-					      struct devfreq *devfreq);
-extern void devm_devfreq_unregister_opp_notifier(struct device *dev,
-						struct devfreq *devfreq);
 extern int devfreq_register_notifier(struct devfreq *devfreq,
 					struct notifier_block *nb,
 					unsigned int list);
 extern int devfreq_unregister_notifier(struct devfreq *devfreq,
 					struct notifier_block *nb,
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] PM / devfreq: Lock devfreq in trans_stat_show

[Matthias Kaehlcke]

Hi Leonard,

this series doesn't indicate the version, from the change history in
the cover letter I suppose it is v5.

On Wed, Sep 18, 2019 at 03:18:20AM +0300, Leonard Crestez wrote:
> There is no locking in this sysfs show function so stats printing can
> race with a devfreq_update_status called as part of freq switching or
> with initialization.
> 
> Also add an assert in devfreq_update_status to make it clear that lock
> must be held by caller.

This and some other patches look like generic improvements and not
directly related to the series "PM / devfreq: Add dev_pm_qos
support". If there are no dependencies I think it is usually better to
send the improvements separately, it keeps the series more focussed
and might reduce version churn. Just my POV though ;-)

> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2494ee16f502..665575228c4f 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -159,10 +159,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>  {
>  	int lev, prev_lev, ret = 0;
>  	unsigned long cur_time;
>  
>  	cur_time = jiffies;
> +	lockdep_assert_held(&devfreq->lock);
>  
>  	/* Immediately exit if previous_freq is not initialized yet. */
>  	if (!devfreq->previous_freq)
>  		goto out;
>  
> @@ -1415,15 +1416,20 @@ static ssize_t trans_stat_show(struct device *dev,
>  	struct devfreq *devfreq = to_devfreq(dev);
>  	ssize_t len;
>  	int i, j;
>  	unsigned int max_state = devfreq->profile->max_state;
>  
> +	mutex_lock(&devfreq->lock);
>  	if (!devfreq->stop_polling &&
> -			devfreq_update_status(devfreq, devfreq->previous_freq))
> -		return 0;
> -	if (max_state == 0)
> -		return sprintf(buf, "Not Supported.\n");
> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
> +		len = 0;

you could assign 'len' in the declaration instead, but it's just
another option, it'ss fine as is.

> +		goto out;
> +	}
> +	if (max_state == 0) {
> +		len = sprintf(buf, "Not Supported.\n");
> +		goto out;
> +	}

This leaves the general structure of the code as is, which is great,
but since you are already touching this part you can consider to
improve it: 'max_state' is constant after device creation, hence the
check could be done at the beginning, which IMO would be clearer, it
could also save an unnecessary devfreq_update_status() call and it
wouldn't be necessary to hold the lock (one goto less).

>  	len = sprintf(buf, "     From  :   To\n");
>  	len += sprintf(buf + len, "           :");
>  	for (i = 0; i < max_state; i++)
>  		len += sprintf(buf + len, "%10lu",
> @@ -1447,10 +1453,13 @@ static ssize_t trans_stat_show(struct device *dev,
>  			jiffies_to_msecs(devfreq->time_in_state[i]));
>  	}
>  
>  	len += sprintf(buf + len, "Total transition : %u\n",
>  					devfreq->total_trans);
> +
> +out:
> +	mutex_unlock(&devfreq->lock);
>  	return len;
>  }
>  static DEVICE_ATTR_RO(trans_stat);
>  
>  static struct attribute *devfreq_attrs[] = {

My only comments are possible improvements, but the change also looks
good as is, so:

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] PM / devfreq: Don't fail devfreq_dev_release if not in list

[Matthias Kaehlcke]

On Wed, Sep 18, 2019 at 03:18:21AM +0300, Leonard Crestez wrote:
> Right now devfreq_dev_release will print a warning and abort the rest of
> the cleanup if the devfreq instance is not part of the global
> devfreq_list. But this is a valid scenario, for example it can happen if
> the governor can't be found or on any other init error that happens
> after device_register.
> 
> Initialize devfreq->node to an empty list head in devfreq_add_device so
> that list_del becomes a safe noop inside devfreq_dev_release and we can
> continue the rest of the cleanup.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 665575228c4f..a715f27f35fd 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -582,15 +582,10 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  static void devfreq_dev_release(struct device *dev)
>  {
>  	struct devfreq *devfreq = to_devfreq(dev);
>  
>  	mutex_lock(&devfreq_list_lock);
> -	if (IS_ERR(find_device_devfreq(devfreq->dev.parent))) {
> -		mutex_unlock(&devfreq_list_lock);
> -		dev_warn(&devfreq->dev, "releasing devfreq which doesn't exist\n");
> -		return;
> -	}
>  	list_del(&devfreq->node);
>  	mutex_unlock(&devfreq_list_lock);
>  
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
> @@ -641,10 +636,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	mutex_init(&devfreq->lock);
>  	mutex_lock(&devfreq->lock);
>  	devfreq->dev.parent = dev;
>  	devfreq->dev.class = devfreq_class;
>  	devfreq->dev.release = devfreq_dev_release;
> +	INIT_LIST_HEAD(&devfreq->node);
>  	devfreq->profile = profile;
>  	strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
>  	devfreq->previous_freq = profile->initial_freq;
>  	devfreq->last_status.current_frequency = profile->initial_freq;
>  	devfreq->data = data;

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] PM / devfreq: Move more initialization before registration

[Matthias Kaehlcke]

Hi Leonard,

On Wed, Sep 18, 2019 at 03:18:22AM +0300, Leonard Crestez wrote:
> In general it is a better to initialize an object before making it
> accessible externally (through device_register).
> 
> This make it possible to avoid relying on locking a partially
> initialized object.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index a715f27f35fd..57a217fc92de 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev)
>  
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
>  	mutex_destroy(&devfreq->lock);
> +	kfree(devfreq->time_in_state);
> +	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  }
>  
>  /**
>   * devfreq_add_device() - Add devfreq feature to the device
> @@ -671,44 +673,43 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->max_freq = devfreq->scaling_max_freq;
>  
>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>  	atomic_set(&devfreq->suspend_count, 0);
>  
> -	dev_set_name(&devfreq->dev, "devfreq%d",
> -				atomic_inc_return(&devfreq_no));
> -	err = device_register(&devfreq->dev);
> -	if (err) {
> -		mutex_unlock(&devfreq->lock);
> -		put_device(&devfreq->dev);
> -		goto err_out;
> -	}
> -
> -	devfreq->trans_table = devm_kzalloc(&devfreq->dev,
> +	devfreq->trans_table = kzalloc(
>  			array3_size(sizeof(unsigned int),
>  				    devfreq->profile->max_state,
>  				    devfreq->profile->max_state),
>  			GFP_KERNEL);
>  	if (!devfreq->trans_table) {
>  		mutex_unlock(&devfreq->lock);
>  		err = -ENOMEM;
> -		goto err_devfreq;
> +		goto err_dev;
>  	}
>  
> -	devfreq->time_in_state = devm_kcalloc(&devfreq->dev,
> -			devfreq->profile->max_state,
> -			sizeof(unsigned long),
> -			GFP_KERNEL);
> +	devfreq->time_in_state = kcalloc(devfreq->profile->max_state,
> +					 sizeof(unsigned long),
> +					 GFP_KERNEL);
>  	if (!devfreq->time_in_state) {
>  		mutex_unlock(&devfreq->lock);
>  		err = -ENOMEM;
> -		goto err_devfreq;
> +		goto err_dev;
>  	}
>  
>  	devfreq->last_stat_updated = jiffies;
>  
>  	srcu_init_notifier_head(&devfreq->transition_notifier_list);
>  
> +	dev_set_name(&devfreq->dev, "devfreq%d",
> +				atomic_inc_return(&devfreq_no));
> +	err = device_register(&devfreq->dev);
> +	if (err) {
> +		mutex_unlock(&devfreq->lock);
> +		put_device(&devfreq->dev);
> +		goto err_out;

  		goto err_dev;

> +	}
> +
>  	mutex_unlock(&devfreq->lock);
>  
>  	mutex_lock(&devfreq_list_lock);
>  
>  	governor = try_then_request_governor(devfreq->governor_name);
> @@ -734,14 +735,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  
>  	return devfreq;
>  
>  err_init:
>  	mutex_unlock(&devfreq_list_lock);
> -err_devfreq:
>  	devfreq_remove_device(devfreq);
> -	devfreq = NULL;
> +	return ERR_PTR(err);

The two return paths in the unwind part are unorthodox, but I
see why they are needed. Maybe add an empty line between the two paths
to make it a bit more evident that they are separate.

>  err_dev:

This code path should include

	mutex_destroy(&devfreq->lock);

That was already missing in the original code though.

Actually with the later device registration the mutex could be
initialized later and doesn't need to be held. This would
obsolete the mutex_unlock() calls in the error paths.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] PM / devfreq: Move more initialization before registration

[Matthias Kaehlcke]

On Wed, Sep 18, 2019 at 04:29:04PM -0700, Matthias Kaehlcke wrote:
> Hi Leonard,
> 
> On Wed, Sep 18, 2019 at 03:18:22AM +0300, Leonard Crestez wrote:
> > In general it is a better to initialize an object before making it
> > accessible externally (through device_register).
> > 
> > This make it possible to avoid relying on locking a partially
> > initialized object.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > ---
> >  drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------
> >  1 file changed, 20 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index a715f27f35fd..57a217fc92de 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev)
> >  
> >  	if (devfreq->profile->exit)
> >  		devfreq->profile->exit(devfreq->dev.parent);
> >  
> >  	mutex_destroy(&devfreq->lock);
> > +	kfree(devfreq->time_in_state);
> > +	kfree(devfreq->trans_table);
> >  	kfree(devfreq);
> >  }
> >  
> >  /**
> >   * devfreq_add_device() - Add devfreq feature to the device
> > @@ -671,44 +673,43 @@ struct devfreq *devfreq_add_device(struct device *dev,
> >  	devfreq->max_freq = devfreq->scaling_max_freq;
> >  
> >  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> >  	atomic_set(&devfreq->suspend_count, 0);
> >  
> > -	dev_set_name(&devfreq->dev, "devfreq%d",
> > -				atomic_inc_return(&devfreq_no));
> > -	err = device_register(&devfreq->dev);
> > -	if (err) {
> > -		mutex_unlock(&devfreq->lock);
> > -		put_device(&devfreq->dev);
> > -		goto err_out;
> > -	}
> > -
> > -	devfreq->trans_table = devm_kzalloc(&devfreq->dev,
> > +	devfreq->trans_table = kzalloc(
> >  			array3_size(sizeof(unsigned int),
> >  				    devfreq->profile->max_state,
> >  				    devfreq->profile->max_state),
> >  			GFP_KERNEL);
> >  	if (!devfreq->trans_table) {
> >  		mutex_unlock(&devfreq->lock);
> >  		err = -ENOMEM;
> > -		goto err_devfreq;
> > +		goto err_dev;
> >  	}
> >  
> > -	devfreq->time_in_state = devm_kcalloc(&devfreq->dev,
> > -			devfreq->profile->max_state,
> > -			sizeof(unsigned long),
> > -			GFP_KERNEL);
> > +	devfreq->time_in_state = kcalloc(devfreq->profile->max_state,
> > +					 sizeof(unsigned long),
> > +					 GFP_KERNEL);
> >  	if (!devfreq->time_in_state) {
> >  		mutex_unlock(&devfreq->lock);
> >  		err = -ENOMEM;
> > -		goto err_devfreq;
> > +		goto err_dev;
> >  	}
> >  
> >  	devfreq->last_stat_updated = jiffies;
> >  
> >  	srcu_init_notifier_head(&devfreq->transition_notifier_list);
> >  
> > +	dev_set_name(&devfreq->dev, "devfreq%d",
> > +				atomic_inc_return(&devfreq_no));
> > +	err = device_register(&devfreq->dev);
> > +	if (err) {
> > +		mutex_unlock(&devfreq->lock);
> > +		put_device(&devfreq->dev);
> > +		goto err_out;
> 
>   		goto err_dev;
> 
> > +	}
> > +
> >  	mutex_unlock(&devfreq->lock);
> >  
> >  	mutex_lock(&devfreq_list_lock);
> >  
> >  	governor = try_then_request_governor(devfreq->governor_name);
> > @@ -734,14 +735,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
> >  
> >  	return devfreq;
> >  
> >  err_init:
> >  	mutex_unlock(&devfreq_list_lock);
> > -err_devfreq:
> >  	devfreq_remove_device(devfreq);
> > -	devfreq = NULL;
> > +	return ERR_PTR(err);
> 
> The two return paths in the unwind part are unorthodox, but I
> see why they are needed. Maybe add an empty line between the two paths
> to make it a bit more evident that they are separate.
> 
> >  err_dev:
> 
> This code path should include
> 
> 	mutex_destroy(&devfreq->lock);
> 
> That was already missing in the original code though.
> 
> Actually with the later device registration the mutex could be
> initialized later and doesn't need to be held. This would
> obsolete the mutex_unlock() calls in the error paths.

Just saw that you are already doing this in "[4/8] PM / devfreq:
Don't take lock in devfreq_add_device" :)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/8] PM / devfreq: Don't take lock in devfreq_add_device

[Matthias Kaehlcke]

On Wed, Sep 18, 2019 at 03:18:23AM +0300, Leonard Crestez wrote:
> A device usually doesn't need to lock itself during initialization
> because it is not yet reachable from other threads.
> 
> This simplifies the code and helps avoid recursive lock warnings.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 57a217fc92de..860cbbab476c 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -634,11 +634,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  		err = -ENOMEM;
>  		goto err_out;
>  	}
>  
>  	mutex_init(&devfreq->lock);
> -	mutex_lock(&devfreq->lock);
>  	devfreq->dev.parent = dev;
>  	devfreq->dev.class = devfreq_class;
>  	devfreq->dev.release = devfreq_dev_release;
>  	INIT_LIST_HEAD(&devfreq->node);
>  	devfreq->profile = profile;
> @@ -647,28 +646,24 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->last_status.current_frequency = profile->initial_freq;
>  	devfreq->data = data;
>  	devfreq->nb.notifier_call = devfreq_notifier_call;
>  
>  	if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
> -		mutex_unlock(&devfreq->lock);
>  		err = set_freq_table(devfreq);
>  		if (err < 0)
>  			goto err_dev;
> -		mutex_lock(&devfreq->lock);
>  	}
>  
>  	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>  	if (!devfreq->scaling_min_freq) {
> -		mutex_unlock(&devfreq->lock);
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
>  	devfreq->min_freq = devfreq->scaling_min_freq;
>  
>  	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>  	if (!devfreq->scaling_max_freq) {
> -		mutex_unlock(&devfreq->lock);
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
>  	devfreq->max_freq = devfreq->scaling_max_freq;
>  
> @@ -679,20 +674,18 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  			array3_size(sizeof(unsigned int),
>  				    devfreq->profile->max_state,
>  				    devfreq->profile->max_state),
>  			GFP_KERNEL);
>  	if (!devfreq->trans_table) {
> -		mutex_unlock(&devfreq->lock);
>  		err = -ENOMEM;
>  		goto err_dev;
>  	}
>  
>  	devfreq->time_in_state = kcalloc(devfreq->profile->max_state,
>  					 sizeof(unsigned long),
>  					 GFP_KERNEL);
>  	if (!devfreq->time_in_state) {
> -		mutex_unlock(&devfreq->lock);
>  		err = -ENOMEM;
>  		goto err_dev;
>  	}
>  
>  	devfreq->last_stat_updated = jiffies;
> @@ -701,17 +694,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  
>  	dev_set_name(&devfreq->dev, "devfreq%d",
>  				atomic_inc_return(&devfreq_no));
>  	err = device_register(&devfreq->dev);
>  	if (err) {
> -		mutex_unlock(&devfreq->lock);
>  		put_device(&devfreq->dev);
>  		goto err_out;
>  	}
>  
> -	mutex_unlock(&devfreq->lock);
> -
>  	mutex_lock(&devfreq_list_lock);
>  
>  	governor = try_then_request_governor(devfreq->governor_name);
>  	if (IS_ERR(governor)) {
>  		dev_err(dev, "%s: Unable to find governor for the device\n",

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/8] PM / devfreq: Introduce devfreq_get_freq_range

[Matthias Kaehlcke]

Hi Leonard,

On Wed, Sep 18, 2019 at 03:18:24AM +0300, Leonard Crestez wrote:
> Moving handling of min/max freq to a single function and call it from
> update_devfreq and for printing min/max freq values in sysfs.
> 
> This changes the behavior of out-of-range min_freq/max_freq: clamping
> is now done at evaluation time. This means that if an out-of-range
> constraint is imposed by sysfs and it later becomes valid then it will
> be enforced.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 111 +++++++++++++++++++++-----------------
>  1 file changed, 63 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 860cbbab476c..51a4179e2c69 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -24,10 +24,12 @@
>  #include <linux/printk.h>
>  #include <linux/hrtimer.h>
>  #include <linux/of.h>
>  #include "governor.h"
>  
> +#define HZ_PER_KHZ 1000
> +
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/devfreq.h>
>  
>  static struct class *devfreq_class;
>  
> @@ -96,10 +98,50 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>  		dev_pm_opp_put(opp);
>  
>  	return max_freq;
>  }
>  
> +/**
> + * devfreq_get_freq_range() - Get the current freq range
> + * @devfreq:	the devfreq instance
> + * @min_freq:	the min frequency
> + * @max_freq:	the max frequency
> + *
> + * This takes into consideration all constraints.
> + */
> +static void devfreq_get_freq_range(struct devfreq *devfreq,
> +				   unsigned long *min_freq,
> +				   unsigned long *max_freq)
> +{
> +	unsigned long *freq_table = devfreq->profile->freq_table;
> +
> +	lockdep_assert_held(&devfreq->lock);
> +
> +	/* Init min/max frequency from freq table */
> +	if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
> +		*min_freq = freq_table[0];
> +		*max_freq = freq_table[devfreq->profile->max_state - 1];
> +	} else {
> +		*min_freq = freq_table[devfreq->profile->max_state - 1];
> +		*max_freq = freq_table[0];
> +	}
> +
> +	/* constraints from sysfs: */
> +	*min_freq = max(*min_freq, devfreq->min_freq);
> +	*max_freq = min(*max_freq, devfreq->max_freq);
> +
> +	/* constraints from opp interface: */

nit: OPP

> +	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
> +	/* scaling_max_freq can be zero on error */
> +	if (devfreq->scaling_max_freq)
> +		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
> +
> +	/* max_freq takes precedence over min_freq */
> +	if (*min_freq > *max_freq)
> +		*min_freq = *max_freq;
> +}
> +
>  /**
>   * devfreq_get_freq_level() - Lookup freq_table for the frequency
>   * @devfreq:	the devfreq instance
>   * @freq:	the target frequency
>   */
> @@ -349,21 +391,13 @@ int update_devfreq(struct devfreq *devfreq)
>  
>  	/* Reevaluate the proper frequency */
>  	err = devfreq->governor->get_target_freq(devfreq, &freq);
>  	if (err)
>  		return err;
> +	devfreq_get_freq_range(devfreq, &min_freq, &max_freq);
>  
> -	/*
> -	 * Adjust the frequency with user freq, QoS and available freq.
> -	 *
> -	 * List from the highest priority
> -	 * max_freq
> -	 * min_freq
> -	 */
> -	max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq);
> -	min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq);
> -
> +	/* max freq takes priority over min freq */
>  	if (freq < min_freq) {
>  		freq = min_freq;
>  		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>  	}
>  	if (freq > max_freq) {
> @@ -1293,40 +1327,28 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>  	ret = sscanf(buf, "%lu", &value);
>  	if (ret != 1)
>  		return -EINVAL;
>  
>  	mutex_lock(&df->lock);
> -
> -	if (value) {
> -		if (value > df->max_freq) {
> -			ret = -EINVAL;
> -			goto unlock;
> -		}
> -	} else {
> -		unsigned long *freq_table = df->profile->freq_table;
> -
> -		/* Get minimum frequency according to sorting order */
> -		if (freq_table[0] < freq_table[df->profile->max_state - 1])
> -			value = freq_table[0];
> -		else
> -			value = freq_table[df->profile->max_state - 1];
> -	}
> -
>  	df->min_freq = value;
>  	update_devfreq(df);
> -	ret = count;
> -unlock:
>  	mutex_unlock(&df->lock);
> -	return ret;
> +
> +	return count;
>  }
>  
>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
>  	struct devfreq *df = to_devfreq(dev);
> +	unsigned long min_freq, max_freq;
>  
> -	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
> +	mutex_lock(&df->lock);
> +	devfreq_get_freq_range(df, &min_freq, &max_freq);
> +	mutex_unlock(&df->lock);
> +
> +	return sprintf(buf, "%lu\n", min_freq);
>  }
>  
>  static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  			      const char *buf, size_t count)
>  {
> @@ -1338,40 +1360,33 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  	if (ret != 1)
>  		return -EINVAL;
>  
>  	mutex_lock(&df->lock);
>  
> -	if (value) {
> -		if (value < df->min_freq) {
> -			ret = -EINVAL;
> -			goto unlock;
> -		}
> -	} else {
> -		unsigned long *freq_table = df->profile->freq_table;
> -
> -		/* Get maximum frequency according to sorting order */
> -		if (freq_table[0] < freq_table[df->profile->max_state - 1])
> -			value = freq_table[df->profile->max_state - 1];
> -		else
> -			value = freq_table[0];
> -	}
> +	/* Interpret zero as "don't care" */
> +	if (!value)
> +		value = ULONG_MAX;
>  
>  	df->max_freq = value;
>  	update_devfreq(df);
> -	ret = count;
> -unlock:
>  	mutex_unlock(&df->lock);
> -	return ret;
> +
> +	return count;
>  }
>  static DEVICE_ATTR_RW(min_freq);
>  
>  static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
>  	struct devfreq *df = to_devfreq(dev);
> +	unsigned long min_freq, max_freq;
> +
> +	mutex_lock(&df->lock);
> +	devfreq_get_freq_range(df, &min_freq, &max_freq);
> +	mutex_unlock(&df->lock);
>  
> -	return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
> +	return sprintf(buf, "%lu\n", max_freq);
>  }
>  static DEVICE_ATTR_RW(max_freq);
>  
>  static ssize_t available_frequencies_show(struct device *d,
>  					  struct device_attribute *attr,

Nice, having the constraint evaluation in a single function makes it
easier to follow the code. Clamping userspace constraints at runtime
makes sense.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

I plan to take a look at the rest of the series, but probably won't
find time for all of it before next week.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] PM / devfreq: Lock devfreq in trans_stat_show

[Leonard Crestez]

On 19.09.2019 00:28, Matthias Kaehlcke wrote:
> Hi Leonard,
> 
> this series doesn't indicate the version, from the change history in
> the cover letter I suppose it is v5.

Sorry about that, I forgot --subject-prefix. It is indeed v5

> On Wed, Sep 18, 2019 at 03:18:20AM +0300, Leonard Crestez wrote:
>> There is no locking in this sysfs show function so stats printing can
>> race with a devfreq_update_status called as part of freq switching or
>> with initialization.
>>
>> Also add an assert in devfreq_update_status to make it clear that lock
>> must be held by caller.
> 
> This and some other patches look like generic improvements and not
> directly related to the series "PM / devfreq: Add dev_pm_qos
> support". If there are no dependencies I think it is usually better to
> send the improvements separately, it keeps the series more focussed
> and might reduce version churn. Just my POV though ;-)

The locking cleanups are required in order to initialize pm_qos request 
and notifiers without introducing lockdep warnings.

pm_qos calls notifiers under dev_pm_qos_mtx and those notifiers needs to 
take &devfreq->lock. This means initializing pm_qos notifiers and 
requests must be done outside &devfreq->lock which needs some cleanups 
in devfreq_add_device.

This particular patch is a more loosely related bugfix. Devfreq 
maintainers: would it help to post it separately?

>> @@ -1415,15 +1416,20 @@ static ssize_t trans_stat_show(struct device *dev,
>>   	struct devfreq *devfreq = to_devfreq(dev);
>>   	ssize_t len;
>>   	int i, j;
>>   	unsigned int max_state = devfreq->profile->max_state;
>>   
>> +	mutex_lock(&devfreq->lock);
>>   	if (!devfreq->stop_polling &&
>> -			devfreq_update_status(devfreq, devfreq->previous_freq))
>> -		return 0;
>> -	if (max_state == 0)
>> -		return sprintf(buf, "Not Supported.\n");
>> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
>> +		len = 0;
> 
> you could assign 'len' in the declaration instead, but it's just
> another option, it'ss fine as is
>> +		goto out;
>> +	}
>> +	if (max_state == 0) {
>> +		len = sprintf(buf, "Not Supported.\n");
>> +		goto out;
>> +	}
> 
> This leaves the general structure of the code as is, which is great,
> but since you are already touching this part you can consider to
> improve it: 'max_state' is constant after device creation, hence the
> check could be done at the beginning, which IMO would be clearer, it
> could also save an unnecessary devfreq_update_status() call and it
> wouldn't be necessary to hold the lock (one goto less).

Now that I look at this more closely &devfreq->lock only really needs to 
be held during the stats update, it can be released during sprintf.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] PM / devfreq: Move more initialization before registration

[Leonard Crestez]

On 19.09.2019 02:29, Matthias Kaehlcke wrote:
> Hi Leonard,
> 
> On Wed, Sep 18, 2019 at 03:18:22AM +0300, Leonard Crestez wrote:
>> In general it is a better to initialize an object before making it
>> accessible externally (through device_register).
>>
>> This make it possible to avoid relying on locking a partially
>> initialized object.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------
>>   1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index a715f27f35fd..57a217fc92de 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev)
>>   
>>   	if (devfreq->profile->exit)
>>   		devfreq->profile->exit(devfreq->dev.parent);
>>   
>>   	mutex_destroy(&devfreq->lock);
>> +	kfree(devfreq->time_in_state);
>> +	kfree(devfreq->trans_table);
>>   	kfree(devfreq);
>>   }
>>   
>>   /**
>>    * devfreq_add_device() - Add devfreq feature to the device
>> @@ -671,44 +673,43 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   	devfreq->max_freq = devfreq->scaling_max_freq;
>>   
>>   	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>>   	atomic_set(&devfreq->suspend_count, 0);
>>   
>> -	dev_set_name(&devfreq->dev, "devfreq%d",
>> -				atomic_inc_return(&devfreq_no));
>> -	err = device_register(&devfreq->dev);
>> -	if (err) {
>> -		mutex_unlock(&devfreq->lock);
>> -		put_device(&devfreq->dev);
>> -		goto err_out;
>> -	}
>> -
>> -	devfreq->trans_table = devm_kzalloc(&devfreq->dev,
>> +	devfreq->trans_table = kzalloc(
>>   			array3_size(sizeof(unsigned int),
>>   				    devfreq->profile->max_state,
>>   				    devfreq->profile->max_state),
>>   			GFP_KERNEL);
>>   	if (!devfreq->trans_table) {
>>   		mutex_unlock(&devfreq->lock);
>>   		err = -ENOMEM;
>> -		goto err_devfreq;
>> +		goto err_dev;
>>   	}
>>   
>> -	devfreq->time_in_state = devm_kcalloc(&devfreq->dev,
>> -			devfreq->profile->max_state,
>> -			sizeof(unsigned long),
>> -			GFP_KERNEL);
>> +	devfreq->time_in_state = kcalloc(devfreq->profile->max_state,
>> +					 sizeof(unsigned long),
>> +					 GFP_KERNEL);
>>   	if (!devfreq->time_in_state) {
>>   		mutex_unlock(&devfreq->lock);
>>   		err = -ENOMEM;
>> -		goto err_devfreq;
>> +		goto err_dev;
>>   	}
>>   
>>   	devfreq->last_stat_updated = jiffies;
>>   
>>   	srcu_init_notifier_head(&devfreq->transition_notifier_list);
>>   
>> +	dev_set_name(&devfreq->dev, "devfreq%d",
>> +				atomic_inc_return(&devfreq_no));
>> +	err = device_register(&devfreq->dev);
>> +	if (err) {
>> +		mutex_unlock(&devfreq->lock);
>> +		put_device(&devfreq->dev);
>> +		goto err_out;
> 
>    		goto err_dev;
> 
>> +	}
>> +
>>   	mutex_unlock(&devfreq->lock);
>>   
>>   	mutex_lock(&devfreq_list_lock);
>>   
>>   	governor = try_then_request_governor(devfreq->governor_name);
>> @@ -734,14 +735,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   
>>   	return devfreq;
>>   
>>   err_init:
>>   	mutex_unlock(&devfreq_list_lock);
>> -err_devfreq:
>>   	devfreq_remove_device(devfreq);
>> -	devfreq = NULL;
>> +	return ERR_PTR(err);
> 
> The two return paths in the unwind part are unorthodox, but I
> see why they are needed. Maybe add an empty line between the two paths
> to make it a bit more evident that they are separate.

Old code did "devfreq = NULL" just so that the later kfree did nothing. 
There were already two unwind paths, it's just that the second one was 
less obvious. I will add a comment.

>>   err_dev:
> 
> This code path should include
> 
> 	mutex_destroy(&devfreq->lock);
> 
> That was already missing in the original code though.

Yes, that would be a separate patch.

> Actually with the later device registration the mutex could be
> initialized later and doesn't need to be held. This would
> obsolete the mutex_unlock() calls in the error paths
Next patch already removes mutex_lock before device_register (that's the 
purpose of the cleanup). If you're suggesting to move mutex_init around 
it's not clear what would be gained?

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/8] PM / devfreq: Add dev_pm_qos support

[Matthias Kaehlcke]

On Wed, Sep 18, 2019 at 03:18:25AM +0300, Leonard Crestez wrote:
> Register notifiers with the pm_qos framework in order to respond to
> requests for MIN_FREQUENCY and MAX_FREQUENCY.

To make it clear that this change on it's own is a NOP maybe add
something like "No constraints are added for now though.", as in
67d874c3b2c6 ("cpufreq: Register notifiers with the PM QoS framework")

> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 71 +++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq.h   |  5 +++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 51a4179e2c69..d8d57318b12c 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -22,17 +22,20 @@
>  #include <linux/platform_device.h>
>  #include <linux/list.h>
>  #include <linux/printk.h>
>  #include <linux/hrtimer.h>
>  #include <linux/of.h>
> +#include <linux/pm_qos.h>
>  #include "governor.h"
>  
>  #define HZ_PER_KHZ 1000
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/devfreq.h>
>  
> +#define HZ_PER_KHZ	1000
> +
>  static struct class *devfreq_class;
>  
>  /*
>   * devfreq core provides delayed work based load monitoring helper
>   * functions. Governors can use these or can implement their own
> @@ -123,10 +126,16 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
>  	} else {
>  		*min_freq = freq_table[devfreq->profile->max_state - 1];
>  		*max_freq = freq_table[0];
>  	}
>  
> +	/* constraints from dev_pm_qos: */

nit: QoS constraints?

> +	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
> +				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
> +	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
> +				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
> +
>  	/* constraints from sysfs: */
>  	*min_freq = max(*min_freq, devfreq->min_freq);
>  	*max_freq = min(*max_freq, devfreq->max_freq);
>  
>  	/* constraints from opp interface: */
> @@ -605,10 +614,49 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  	mutex_unlock(&devfreq->lock);
>  
>  	return ret;
>  }
>  
> +/**
> + * devfreq_qos_notifier_call() - Common handler for qos freq changes.

nit: QoS

s/freq/constraint/ ?

> + * @devfreq:    the devfreq instance.
> + */
> +static int devfreq_qos_notifier_call(struct devfreq *devfreq)
> +{
> +	int ret;
> +
> +	mutex_lock(&devfreq->lock);
> +	ret = update_devfreq(devfreq);
> +	mutex_unlock(&devfreq->lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * devfreq_qos_min_notifier_call() - Callback for qos min_freq changes.

nit: QoS

> + * @nb:		Should to be devfreq->nb_min

s/to//

> + */
> +static int devfreq_qos_min_notifier_call(struct notifier_block *nb,
> +					 unsigned long val, void *ptr)
> +{
> +	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min);
> +
> +	return devfreq_qos_notifier_call(devfreq);
> +}
> +
> +/**
> + * devfreq_qos_max_notifier_call() - Callback for qos min_freq changes.

nit: QoS

s/min/max/

> + * @nb:		Should to be devfreq->nb_max

s/to//

> + */
> +static int devfreq_qos_max_notifier_call(struct notifier_block *nb,
> +					 unsigned long val, void *ptr)
> +{
> +	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_max);
> +
> +	return devfreq_qos_notifier_call(devfreq);
> +}
> +
>  /**
>   * devfreq_dev_release() - Callback for struct device to release the device.
>   * @dev:	the devfreq device
>   *
>   * Remove devfreq from the list and release its resources.
> @@ -619,10 +667,15 @@ static void devfreq_dev_release(struct device *dev)
>  
>  	mutex_lock(&devfreq_list_lock);
>  	list_del(&devfreq->node);
>  	mutex_unlock(&devfreq_list_lock);
>  
> +	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
> +			DEV_PM_QOS_MAX_FREQUENCY);
> +	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
> +			DEV_PM_QOS_MIN_FREQUENCY);
> +

mega-nit: removing 'max' then 'min' does things in reverse order as
during initialization, which is common practice. But since the order
here doesn't really matter I'd stick to the common min/max order,
might readers save a few milli-seconds wondering why 'max' comes first.

>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq->time_in_state);
> @@ -732,10 +785,27 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	if (err) {
>  		put_device(&devfreq->dev);
>  		goto err_out;
>  	}
>  
> +	/*
> +	 * Register notifiers for updates to min_freq/max_freq after device is

nit: min/max_freq?

> +	 * initialized (and we can handle notifications) but before the governor
> +	 * is started (which should do an initial enforcement of constraints)
> +	 */
> +	devfreq->nb_min.notifier_call = devfreq_qos_min_notifier_call;
> +	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
> +				      DEV_PM_QOS_MIN_FREQUENCY);
> +	if (err)
> +		goto err_devfreq;

IIUC you rely on the notifiers being removed by
devfreq_dev_release(). Does dev_pm_qos_remove_notifier() behave
gracefully if the notifier is not initialized/added or do we need
to use BLOCKING_NOTIFIER_INIT() or similar?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] PM / devfreq: Move more initialization before registration

[Matthias Kaehlcke]

On Thu, Sep 19, 2019 at 06:52:07PM +0000, Leonard Crestez wrote:
> On 19.09.2019 02:29, Matthias Kaehlcke wrote:
> > Hi Leonard,
> > 
> > On Wed, Sep 18, 2019 at 03:18:22AM +0300, Leonard Crestez wrote:
> >> In general it is a better to initialize an object before making it
> >> accessible externally (through device_register).
> >>
> >> This make it possible to avoid relying on locking a partially
> >> initialized object.
> >>
> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >> ---
> >>   drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------
> >>   1 file changed, 20 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >> index a715f27f35fd..57a217fc92de 100644
> >> --- a/drivers/devfreq/devfreq.c
> >> +++ b/drivers/devfreq/devfreq.c
> >> @@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev)
> >>   
> >>   	if (devfreq->profile->exit)
> >>   		devfreq->profile->exit(devfreq->dev.parent);
> >>   
> >>   	mutex_destroy(&devfreq->lock);
> >> +	kfree(devfreq->time_in_state);
> >> +	kfree(devfreq->trans_table);
> >>   	kfree(devfreq);
> >>   }
> >>   
> >>   /**
> >>    * devfreq_add_device() - Add devfreq feature to the device
> >> @@ -671,44 +673,43 @@ struct devfreq *devfreq_add_device(struct device *dev,
> >>   	devfreq->max_freq = devfreq->scaling_max_freq;
> >>   
> >>   	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> >>   	atomic_set(&devfreq->suspend_count, 0);
> >>   
> >> -	dev_set_name(&devfreq->dev, "devfreq%d",
> >> -				atomic_inc_return(&devfreq_no));
> >> -	err = device_register(&devfreq->dev);
> >> -	if (err) {
> >> -		mutex_unlock(&devfreq->lock);
> >> -		put_device(&devfreq->dev);
> >> -		goto err_out;
> >> -	}
> >> -
> >> -	devfreq->trans_table = devm_kzalloc(&devfreq->dev,
> >> +	devfreq->trans_table = kzalloc(
> >>   			array3_size(sizeof(unsigned int),
> >>   				    devfreq->profile->max_state,
> >>   				    devfreq->profile->max_state),
> >>   			GFP_KERNEL);
> >>   	if (!devfreq->trans_table) {
> >>   		mutex_unlock(&devfreq->lock);
> >>   		err = -ENOMEM;
> >> -		goto err_devfreq;
> >> +		goto err_dev;
> >>   	}
> >>   
> >> -	devfreq->time_in_state = devm_kcalloc(&devfreq->dev,
> >> -			devfreq->profile->max_state,
> >> -			sizeof(unsigned long),
> >> -			GFP_KERNEL);
> >> +	devfreq->time_in_state = kcalloc(devfreq->profile->max_state,
> >> +					 sizeof(unsigned long),
> >> +					 GFP_KERNEL);
> >>   	if (!devfreq->time_in_state) {
> >>   		mutex_unlock(&devfreq->lock);
> >>   		err = -ENOMEM;
> >> -		goto err_devfreq;
> >> +		goto err_dev;
> >>   	}
> >>   
> >>   	devfreq->last_stat_updated = jiffies;
> >>   
> >>   	srcu_init_notifier_head(&devfreq->transition_notifier_list);
> >>   
> >> +	dev_set_name(&devfreq->dev, "devfreq%d",
> >> +				atomic_inc_return(&devfreq_no));
> >> +	err = device_register(&devfreq->dev);
> >> +	if (err) {
> >> +		mutex_unlock(&devfreq->lock);
> >> +		put_device(&devfreq->dev);
> >> +		goto err_out;
> > 
> >    		goto err_dev;
> > 
> >> +	}
> >> +
> >>   	mutex_unlock(&devfreq->lock);
> >>   
> >>   	mutex_lock(&devfreq_list_lock);
> >>   
> >>   	governor = try_then_request_governor(devfreq->governor_name);
> >> @@ -734,14 +735,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
> >>   
> >>   	return devfreq;
> >>   
> >>   err_init:
> >>   	mutex_unlock(&devfreq_list_lock);
> >> -err_devfreq:
> >>   	devfreq_remove_device(devfreq);
> >> -	devfreq = NULL;
> >> +	return ERR_PTR(err);
> > 
> > The two return paths in the unwind part are unorthodox, but I
> > see why they are needed. Maybe add an empty line between the two paths
> > to make it a bit more evident that they are separate.
> 
> Old code did "devfreq = NULL" just so that the later kfree did nothing. 
> There were already two unwind paths, it's just that the second one was 
> less obvious. I will add a comment.
> 
> >>   err_dev:
> > 
> > This code path should include
> > 
> > 	mutex_destroy(&devfreq->lock);
> > 
> > That was already missing in the original code though.
> 
> Yes, that would be a separate patch.
> 
> > Actually with the later device registration the mutex could be
> > initialized later and doesn't need to be held. This would
> > obsolete the mutex_unlock() calls in the error paths
> Next patch already removes mutex_lock before device_register (that's the 
> purpose of the cleanup). If you're suggesting to move mutex_init around 
> it's not clear what would be gained?

As per my earlier reply to self: I didn't look at the next patch
before writing this, it's all good, nothing to do here :)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] PM / devfreq: Lock devfreq in trans_stat_show

[Matthias Kaehlcke]

On Thu, Sep 19, 2019 at 06:42:22PM +0000, Leonard Crestez wrote:
> On 19.09.2019 00:28, Matthias Kaehlcke wrote:
> > Hi Leonard,
> > 
> > this series doesn't indicate the version, from the change history in
> > the cover letter I suppose it is v5.
> 
> Sorry about that, I forgot --subject-prefix. It is indeed v5
> 
> > On Wed, Sep 18, 2019 at 03:18:20AM +0300, Leonard Crestez wrote:
> >> There is no locking in this sysfs show function so stats printing can
> >> race with a devfreq_update_status called as part of freq switching or
> >> with initialization.
> >>
> >> Also add an assert in devfreq_update_status to make it clear that lock
> >> must be held by caller.
> > 
> > This and some other patches look like generic improvements and not
> > directly related to the series "PM / devfreq: Add dev_pm_qos
> > support". If there are no dependencies I think it is usually better to
> > send the improvements separately, it keeps the series more focussed
> > and might reduce version churn. Just my POV though ;-)
> 
> The locking cleanups are required in order to initialize pm_qos request 
> and notifiers without introducing lockdep warnings.
> 
> pm_qos calls notifiers under dev_pm_qos_mtx and those notifiers needs to 
> take &devfreq->lock. This means initializing pm_qos notifiers and 
> requests must be done outside &devfreq->lock which needs some cleanups 
> in devfreq_add_device.

Thanks for the clarification!

> This particular patch is a more loosely related bugfix. Devfreq 
> maintainers: would it help to post it separately?

If it's just this single patch it probably isn't a problem, I'd be
more concerned about multiple unrelated patches or if the changes are
complex.

> >> @@ -1415,15 +1416,20 @@ static ssize_t trans_stat_show(struct device *dev,
> >>   	struct devfreq *devfreq = to_devfreq(dev);
> >>   	ssize_t len;
> >>   	int i, j;
> >>   	unsigned int max_state = devfreq->profile->max_state;
> >>   
> >> +	mutex_lock(&devfreq->lock);
> >>   	if (!devfreq->stop_polling &&
> >> -			devfreq_update_status(devfreq, devfreq->previous_freq))
> >> -		return 0;
> >> -	if (max_state == 0)
> >> -		return sprintf(buf, "Not Supported.\n");
> >> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
> >> +		len = 0;
> > 
> > you could assign 'len' in the declaration instead, but it's just
> > another option, it'ss fine as is
> >> +		goto out;
> >> +	}
> >> +	if (max_state == 0) {
> >> +		len = sprintf(buf, "Not Supported.\n");
> >> +		goto out;
> >> +	}
> > 
> > This leaves the general structure of the code as is, which is great,
> > but since you are already touching this part you can consider to
> > improve it: 'max_state' is constant after device creation, hence the
> > check could be done at the beginning, which IMO would be clearer, it
> > could also save an unnecessary devfreq_update_status() call and it
> > wouldn't be necessary to hold the lock (one goto less).
> 
> Now that I look at this more closely &devfreq->lock only really needs to 
> be held during the stats update, it can be released during sprintf.

right, another simplification :)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Matthias Kaehlcke]

On Wed, Sep 18, 2019 at 03:18:26AM +0300, Leonard Crestez wrote:
> Switch the handling of min_freq and max_freq from sysfs to use the
> dev_pm_qos interface.

nit: PM QoS?

if you agree please change all instances in comments.

> Since dev_pm_qos handles frequencies as kHz this change reduces the
> precision of min_freq and max_freq. This shouldn't introduce problems
> because frequencies which are not an integer number of kHz are likely
> not an integer number of Hz either.
> 
> Try to ensure compatibilitity by rounding min values down and rounding
> max values up.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 51 +++++++++++++++++++++++++--------------
>  include/linux/devfreq.h   |  9 ++++---
>  2 files changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index d8d57318b12c..7977bad93949 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
>  	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>  				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
>  	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>  				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
>  
> -	/* constraints from sysfs: */
> -	*min_freq = max(*min_freq, devfreq->min_freq);
> -	*max_freq = min(*max_freq, devfreq->max_freq);
> -
>  	/* constraints from opp interface: */
>  	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
>  	/* scaling_max_freq can be zero on error */
>  	if (devfreq->scaling_max_freq)
>  		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
> @@ -675,10 +671,12 @@ static void devfreq_dev_release(struct device *dev)
>  			DEV_PM_QOS_MIN_FREQUENCY);
>  
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
> +	dev_pm_qos_remove_request(&devfreq->max_freq_req);
> +	dev_pm_qos_remove_request(&devfreq->min_freq_req);

mega-nit: keep common mix/max order since it doesn't really matter here?

>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  }
> @@ -743,18 +741,28 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>  	if (!devfreq->scaling_min_freq) {
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
> -	devfreq->min_freq = devfreq->scaling_min_freq;
>  
>  	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>  	if (!devfreq->scaling_max_freq) {
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
> -	devfreq->max_freq = devfreq->scaling_max_freq;
> +
> +	/* dev_pm_qos requests for min/max freq from sysfs */
> +	err = dev_pm_qos_add_request(dev, &devfreq->min_freq_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +	if (err < 0) {
> +		goto err_dev;
> +	}

no curly braces needed for single line.

> +	err = dev_pm_qos_add_request(dev, &devfreq->max_freq_req,
> +				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
> +	if (err < 0) {
> +		goto err_dev;
> +	}

ditto

>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>  	atomic_set(&devfreq->suspend_count, 0);
>  
>  	devfreq->trans_table = kzalloc(
> @@ -833,10 +841,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	mutex_unlock(&devfreq_list_lock);
>  err_devfreq:
>  	devfreq_remove_device(devfreq);
>  	return ERR_PTR(err);
>  err_dev:
> +	if (dev_pm_qos_request_active(&devfreq->max_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->max_freq_req);
> +	if (dev_pm_qos_request_active(&devfreq->min_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->min_freq_req);
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
> @@ -1397,14 +1409,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>  
>  	ret = sscanf(buf, "%lu", &value);
>  	if (ret != 1)
>  		return -EINVAL;
>  
> -	mutex_lock(&df->lock);
> -	df->min_freq = value;
> -	update_devfreq(df);
> -	mutex_unlock(&df->lock);
> +	/* round down to kHz for dev_pm_qos */
> +	if (value)
> +		value = value / HZ_PER_KHZ;
> +
> +	ret = dev_pm_qos_update_request(&df->min_freq_req, value);
> +	if (ret < 0)
> +		return ret;
>  
>  	return count;
>  }
>  
>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
> @@ -1429,19 +1444,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  
>  	ret = sscanf(buf, "%lu", &value);
>  	if (ret != 1)
>  		return -EINVAL;
>  
> -	mutex_lock(&df->lock);
> -
> -	/* Interpret zero as "don't care" */
> -	if (!value)
> -		value = ULONG_MAX;
> +	/* round up to kHz for dev_pm_qos and interpret zero as "don't care" */
> +	if (value)
> +		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
> +	else
> +		value = S32_MAX;
>  
> -	df->max_freq = value;
> -	update_devfreq(df);
> -	mutex_unlock(&df->lock);
> +	ret = dev_pm_qos_update_request(&df->max_freq_req, value);
> +	if (ret < 0)
> +		return ret;
>  
>  	return count;
>  }
>  static DEVICE_ATTR_RW(min_freq);
>  
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index dac0dffeabb4..4b5cc80abbe3 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -11,10 +11,11 @@
>  #define __LINUX_DEVFREQ_H__
>  
>  #include <linux/device.h>
>  #include <linux/notifier.h>
>  #include <linux/pm_opp.h>
> +#include <linux/pm_qos.h>
>  
>  #define DEVFREQ_NAME_LEN 16
>  
>  /* DEVFREQ governor name */
>  #define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
> @@ -121,12 +122,12 @@ struct devfreq_dev_profile {
>   *		devfreq.nb to the corresponding register notifier call chain.
>   * @work:	delayed work for load monitoring.
>   * @previous_freq:	previously configured frequency value.
>   * @data:	Private data of the governor. The devfreq framework does not
>   *		touch this.
> - * @min_freq:	Limit minimum frequency requested by user (0: none)
> - * @max_freq:	Limit maximum frequency requested by user (0: none)
> + * @min_freq_req:	Limit minimum frequency requested by user (0: none)

'(0: none)' is not correct anymore.

Maybe also say that it's a PM QoS request?

Since you are already changing the variable name it could be a good
opportunity to make it more specific, i.e. make clear that it's the
userspace constraint.

e.g.

min_freq_req_user
user_min_freq_req
min_freq_user_req

or

struct {
       struct {
       	      min;
	      max;
       } user;

       struct {
       	      min;
       	      max;
       } scaling; // not a great name, but that's what it is currently ...
} freq_constraints;

> + * @max_freq_req:	Limit maximum frequency requested by user (0: none)
>   * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
>   * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
>   * @stop_polling:	 devfreq polling status of a device.
>   * @suspend_freq:	 frequency of a device set during suspend phase.
>   * @resume_freq:	 frequency of a device set in resume phase.
> @@ -161,12 +162,12 @@ struct devfreq {
>  	unsigned long previous_freq;
>  	struct devfreq_dev_status last_status;
>  
>  	void *data; /* private data for governors */
>  
> -	unsigned long min_freq;
> -	unsigned long max_freq;
> +	struct dev_pm_qos_request min_freq_req;
> +	struct dev_pm_qos_request max_freq_req;
>  	unsigned long scaling_min_freq;
>  	unsigned long scaling_max_freq;
>  	bool stop_polling;
>  
>  	unsigned long suspend_freq;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Leonard Crestez]

On 2019-09-19 10:59 PM, Matthias Kaehlcke wrote:
> On Wed, Sep 18, 2019 at 03:18:26AM +0300, Leonard Crestez wrote:
>> Switch the handling of min_freq and max_freq from sysfs to use the
>> dev_pm_qos interface.
> 
> nit: PM QoS?

This series is specifically supporting dev_pm_qos_* interfaces. There is 
different but related set of pm_qos_* interfaces which is global (not 
targeted at specific devices) and as far as I can tell it is mostly used 
to prevent entering high-latency cpuidle states.

> if you agree please change all instances in comments.

I will change instances which don't mention specific APIs

>> @@ -675,10 +671,12 @@ static void devfreq_dev_release(struct device *dev)
>>   			DEV_PM_QOS_MIN_FREQUENCY);
>>   
>>   	if (devfreq->profile->exit)
>>   		devfreq->profile->exit(devfreq->dev.parent);
>>   
>> +	dev_pm_qos_remove_request(&devfreq->max_freq_req);
>> +	dev_pm_qos_remove_request(&devfreq->min_freq_req);
> 
> mega-nit: keep common mix/max order since it doesn't really matter here?

This is deliberately cleaning up in reverse order of initialization.

>> +	/* dev_pm_qos requests for min/max freq from sysfs */
>> +	err = dev_pm_qos_add_request(dev, &devfreq->min_freq_req,
>> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
>> +	if (err < 0) {
>> +		goto err_dev;
>> +	}
> 
> no curly braces needed for single line.

OK

>> @@ -121,12 +122,12 @@ struct devfreq_dev_profile {
>>    *		devfreq.nb to the corresponding register notifier call chain.
>>    * @work:	delayed work for load monitoring.
>>    * @previous_freq:	previously configured frequency value.
>>    * @data:	Private data of the governor. The devfreq framework does not
>>    *		touch this.
>> - * @min_freq:	Limit minimum frequency requested by user (0: none)
>> - * @max_freq:	Limit maximum frequency requested by user (0: none)
>> + * @min_freq_req:	Limit minimum frequency requested by user (0: none)
> 
> '(0: none)' is not correct anymore.

OK

> Maybe also say that it's a PM QoS request?
> 
> Since you are already changing the variable name it could be a good
> opportunity to make it more specific, i.e. make clear that it's the
> userspace constraint.
> 
> e.g.
> 
> min_freq_req_user
> user_min_freq_req
> min_freq_user_req

user_min_freq_req make sense

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] PM / devfreq: Move opp notifier registration to core

[Chanwoo Choi]

On 19. 9. 18. 오전 9:18, Leonard Crestez wrote:
> An opp notifier can be registered by devfreq in order to respond to OPPs
> being enabled or disabled at runtime (for example by thermal). This is
> currently handled explicitly by drivers.
> 
> Move notifier handling to devfreq_add_device because this shouldn't be
> hardware-specific.
> 
> Handling this inside the core also takes less code.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c    | 84 +++---------------------------------
>  drivers/devfreq/exynos-bus.c |  7 ---
>  drivers/devfreq/rk3399_dmc.c |  6 ---
>  include/linux/devfreq.h      |  8 ----
>  4 files changed, 6 insertions(+), 99 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 7977bad93949..b9177430ae8f 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -663,10 +663,11 @@ static void devfreq_dev_release(struct device *dev)
>  
>  	mutex_lock(&devfreq_list_lock);
>  	list_del(&devfreq->node);
>  	mutex_unlock(&devfreq_list_lock);
>  
> +	dev_pm_opp_unregister_notifier(devfreq->dev.parent, &devfreq->nb);
>  	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
>  			DEV_PM_QOS_MAX_FREQUENCY);
>  	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
>  			DEV_PM_QOS_MIN_FREQUENCY);
>  
> @@ -728,11 +729,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->profile = profile;
>  	strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
>  	devfreq->previous_freq = profile->initial_freq;
>  	devfreq->last_status.current_frequency = profile->initial_freq;
>  	devfreq->data = data;
> -	devfreq->nb.notifier_call = devfreq_notifier_call;
>  
>  	if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
>  		err = set_freq_table(devfreq);
>  		if (err < 0)
>  			goto err_dev;
> @@ -810,10 +810,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
>  				      DEV_PM_QOS_MAX_FREQUENCY);
>  	if (err)
>  		goto err_devfreq;
>  
> +	devfreq->nb.notifier_call = devfreq_notifier_call;
> +	err = dev_pm_opp_register_notifier(devfreq->dev.parent, &devfreq->nb);
> +	if (err)
> +		goto err_devfreq;
> +
>  	mutex_lock(&devfreq_list_lock);
>  
>  	governor = try_then_request_governor(devfreq->governor_name);
>  	if (IS_ERR(governor)) {
>  		dev_err(dev, "%s: Unable to find governor for the device\n",
> @@ -1624,87 +1629,10 @@ struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>  
>  	return opp;
>  }
>  EXPORT_SYMBOL(devfreq_recommended_opp);
>  
> -/**
> - * devfreq_register_opp_notifier() - Helper function to get devfreq notified
> - *				     for any changes in the OPP availability
> - *				     changes
> - * @dev:	The devfreq user device. (parent of devfreq)
> - * @devfreq:	The devfreq object.
> - */
> -int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
> -{
> -	return dev_pm_opp_register_notifier(dev, &devfreq->nb);
> -}
> -EXPORT_SYMBOL(devfreq_register_opp_notifier);
> -
> -/**
> - * devfreq_unregister_opp_notifier() - Helper function to stop getting devfreq
> - *				       notified for any changes in the OPP
> - *				       availability changes anymore.
> - * @dev:	The devfreq user device. (parent of devfreq)
> - * @devfreq:	The devfreq object.
> - *
> - * At exit() callback of devfreq_dev_profile, this must be included if
> - * devfreq_recommended_opp is used.
> - */
> -int devfreq_unregister_opp_notifier(struct device *dev, struct devfreq *devfreq)
> -{
> -	return dev_pm_opp_unregister_notifier(dev, &devfreq->nb);
> -}
> -EXPORT_SYMBOL(devfreq_unregister_opp_notifier);
> -
> -static void devm_devfreq_opp_release(struct device *dev, void *res)
> -{
> -	devfreq_unregister_opp_notifier(dev, *(struct devfreq **)res);
> -}
> -
> -/**
> - * devm_devfreq_register_opp_notifier() - Resource-managed
> - *					  devfreq_register_opp_notifier()
> - * @dev:	The devfreq user device. (parent of devfreq)
> - * @devfreq:	The devfreq object.
> - */
> -int devm_devfreq_register_opp_notifier(struct device *dev,
> -				       struct devfreq *devfreq)
> -{
> -	struct devfreq **ptr;
> -	int ret;
> -
> -	ptr = devres_alloc(devm_devfreq_opp_release, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return -ENOMEM;
> -
> -	ret = devfreq_register_opp_notifier(dev, devfreq);
> -	if (ret) {
> -		devres_free(ptr);
> -		return ret;
> -	}
> -
> -	*ptr = devfreq;
> -	devres_add(dev, ptr);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(devm_devfreq_register_opp_notifier);
> -
> -/**
> - * devm_devfreq_unregister_opp_notifier() - Resource-managed
> - *					    devfreq_unregister_opp_notifier()
> - * @dev:	The devfreq user device. (parent of devfreq)
> - * @devfreq:	The devfreq object.
> - */
> -void devm_devfreq_unregister_opp_notifier(struct device *dev,
> -					 struct devfreq *devfreq)
> -{
> -	WARN_ON(devres_release(dev, devm_devfreq_opp_release,
> -			       devm_devfreq_dev_match, devfreq));
> -}
> -EXPORT_SYMBOL(devm_devfreq_unregister_opp_notifier);
> -
>  /**
>   * devfreq_register_notifier() - Register a driver with devfreq
>   * @devfreq:	The devfreq object.
>   * @nb:		The notifier block to register.
>   * @list:	DEVFREQ_TRANSITION_NOTIFIER.
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index c832673273a2..29f422469960 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -356,17 +356,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>  		dev_err(dev, "failed to add devfreq device\n");
>  		ret = PTR_ERR(bus->devfreq);
>  		goto err;
>  	}
>  
> -	/* Register opp_notifier to catch the change of OPP  */
> -	ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to register opp notifier\n");
> -		goto err;
> -	}
> -
>  	/*
>  	 * Enable devfreq-event to get raw data which is used to determine
>  	 * current bus load.
>  	 */
>  	ret = exynos_bus_enable_edev(bus);
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index 2e65d7279d79..f660d2031e8a 100644
> --- a/drivers/devfreq/rk3399_dmc.c
> +++ b/drivers/devfreq/rk3399_dmc.c
> @@ -454,12 +454,10 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>  	if (IS_ERR(data->devfreq)) {
>  		ret = PTR_ERR(data->devfreq);
>  		goto err_free_opp;
>  	}
>  
> -	devm_devfreq_register_opp_notifier(dev, data->devfreq);
> -
>  	data->dev = dev;
>  	platform_set_drvdata(pdev, data);
>  
>  	return 0;
>  
> @@ -470,14 +468,10 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>  
>  static int rk3399_dmcfreq_remove(struct platform_device *pdev)
>  {
>  	struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(&pdev->dev);
>  
> -	/*
> -	 * Before remove the opp table we need to unregister the opp notifier.
> -	 */
> -	devm_devfreq_unregister_opp_notifier(dmcfreq->dev, dmcfreq->devfreq);
>  	dev_pm_opp_of_remove_table(dmcfreq->dev);
>  
>  	return 0;
>  }
>  
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 4b5cc80abbe3..bf6ebfbc1e8a 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -220,18 +220,10 @@ extern void devfreq_resume(void);
>  extern int update_devfreq(struct devfreq *devfreq);
>  
>  /* Helper functions for devfreq user device driver with OPP. */
>  extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>  					   unsigned long *freq, u32 flags);
> -extern int devfreq_register_opp_notifier(struct device *dev,
> -					 struct devfreq *devfreq);
> -extern int devfreq_unregister_opp_notifier(struct device *dev,
> -					   struct devfreq *devfreq);
> -extern int devm_devfreq_register_opp_notifier(struct device *dev,
> -					      struct devfreq *devfreq);
> -extern void devm_devfreq_unregister_opp_notifier(struct device *dev,
> -						struct devfreq *devfreq);
>  extern int devfreq_register_notifier(struct devfreq *devfreq,
>  					struct notifier_block *nb,
>  					unsigned int list);
>  extern int devfreq_unregister_notifier(struct devfreq *devfreq,
>  					struct notifier_block *nb,
> 

It looks good to me.
But, this patch don't remove the inline functions in devfreq.h.
- devfreq_register_opp_notifier
- devfreq_unregister_opp_notifier
- devm_devfreq_register_opp_notifier
- devm_devfreq_unregister_opp_notifier

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] PM / devfreq: Move opp notifier registration to core

[Leonard Crestez]

On 01.10.2019 00:44, Chanwoo Choi wrote:
> On 19. 9. 18. 오전 9:18, Leonard Crestez wrote:
>> An opp notifier can be registered by devfreq in order to respond to OPPs
>> being enabled or disabled at runtime (for example by thermal). This is
>> currently handled explicitly by drivers.
>>
>> Move notifier handling to devfreq_add_device because this shouldn't be
>> hardware-specific.
>>
>> Handling this inside the core also takes less code.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/devfreq/devfreq.c    | 84 +++---------------------------------
>>   drivers/devfreq/exynos-bus.c |  7 ---
>>   drivers/devfreq/rk3399_dmc.c |  6 ---
>>   include/linux/devfreq.h      |  8 ----
>>   4 files changed, 6 insertions(+), 99 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 7977bad93949..b9177430ae8f 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -663,10 +663,11 @@ static void devfreq_dev_release(struct device *dev)
>>   
>>   	mutex_lock(&devfreq_list_lock);
>>   	list_del(&devfreq->node);
>>   	mutex_unlock(&devfreq_list_lock);
>>   
>> +	dev_pm_opp_unregister_notifier(devfreq->dev.parent, &devfreq->nb);
>>   	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
>>   			DEV_PM_QOS_MAX_FREQUENCY);
>>   	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
>>   			DEV_PM_QOS_MIN_FREQUENCY);
>>   
>> @@ -728,11 +729,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   	devfreq->profile = profile;
>>   	strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
>>   	devfreq->previous_freq = profile->initial_freq;
>>   	devfreq->last_status.current_frequency = profile->initial_freq;
>>   	devfreq->data = data;
>> -	devfreq->nb.notifier_call = devfreq_notifier_call;
>>   
>>   	if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
>>   		err = set_freq_table(devfreq);
>>   		if (err < 0)
>>   			goto err_dev;
>> @@ -810,10 +810,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
>>   				      DEV_PM_QOS_MAX_FREQUENCY);
>>   	if (err)
>>   		goto err_devfreq;
>>   
>> +	devfreq->nb.notifier_call = devfreq_notifier_call;
>> +	err = dev_pm_opp_register_notifier(devfreq->dev.parent, &devfreq->nb);
>> +	if (err)
>> +		goto err_devfreq;
>> +
>>   	mutex_lock(&devfreq_list_lock);
>>   
>>   	governor = try_then_request_governor(devfreq->governor_name);
>>   	if (IS_ERR(governor)) {
>>   		dev_err(dev, "%s: Unable to find governor for the device\n",
>> @@ -1624,87 +1629,10 @@ struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>>   
>>   	return opp;
>>   }
>>   EXPORT_SYMBOL(devfreq_recommended_opp);
>>   
>> -/**
>> - * devfreq_register_opp_notifier() - Helper function to get devfreq notified
>> - *				     for any changes in the OPP availability
>> - *				     changes
>> - * @dev:	The devfreq user device. (parent of devfreq)
>> - * @devfreq:	The devfreq object.
>> - */
>> -int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
>> -{
>> -	return dev_pm_opp_register_notifier(dev, &devfreq->nb);
>> -}
>> -EXPORT_SYMBOL(devfreq_register_opp_notifier);
>> -
>> -/**
>> - * devfreq_unregister_opp_notifier() - Helper function to stop getting devfreq
>> - *				       notified for any changes in the OPP
>> - *				       availability changes anymore.
>> - * @dev:	The devfreq user device. (parent of devfreq)
>> - * @devfreq:	The devfreq object.
>> - *
>> - * At exit() callback of devfreq_dev_profile, this must be included if
>> - * devfreq_recommended_opp is used.
>> - */
>> -int devfreq_unregister_opp_notifier(struct device *dev, struct devfreq *devfreq)
>> -{
>> -	return dev_pm_opp_unregister_notifier(dev, &devfreq->nb);
>> -}
>> -EXPORT_SYMBOL(devfreq_unregister_opp_notifier);
>> -
>> -static void devm_devfreq_opp_release(struct device *dev, void *res)
>> -{
>> -	devfreq_unregister_opp_notifier(dev, *(struct devfreq **)res);
>> -}
>> -
>> -/**
>> - * devm_devfreq_register_opp_notifier() - Resource-managed
>> - *					  devfreq_register_opp_notifier()
>> - * @dev:	The devfreq user device. (parent of devfreq)
>> - * @devfreq:	The devfreq object.
>> - */
>> -int devm_devfreq_register_opp_notifier(struct device *dev,
>> -				       struct devfreq *devfreq)
>> -{
>> -	struct devfreq **ptr;
>> -	int ret;
>> -
>> -	ptr = devres_alloc(devm_devfreq_opp_release, sizeof(*ptr), GFP_KERNEL);
>> -	if (!ptr)
>> -		return -ENOMEM;
>> -
>> -	ret = devfreq_register_opp_notifier(dev, devfreq);
>> -	if (ret) {
>> -		devres_free(ptr);
>> -		return ret;
>> -	}
>> -
>> -	*ptr = devfreq;
>> -	devres_add(dev, ptr);
>> -
>> -	return 0;
>> -}
>> -EXPORT_SYMBOL(devm_devfreq_register_opp_notifier);
>> -
>> -/**
>> - * devm_devfreq_unregister_opp_notifier() - Resource-managed
>> - *					    devfreq_unregister_opp_notifier()
>> - * @dev:	The devfreq user device. (parent of devfreq)
>> - * @devfreq:	The devfreq object.
>> - */
>> -void devm_devfreq_unregister_opp_notifier(struct device *dev,
>> -					 struct devfreq *devfreq)
>> -{
>> -	WARN_ON(devres_release(dev, devm_devfreq_opp_release,
>> -			       devm_devfreq_dev_match, devfreq));
>> -}
>> -EXPORT_SYMBOL(devm_devfreq_unregister_opp_notifier);
>> -
>>   /**
>>    * devfreq_register_notifier() - Register a driver with devfreq
>>    * @devfreq:	The devfreq object.
>>    * @nb:		The notifier block to register.
>>    * @list:	DEVFREQ_TRANSITION_NOTIFIER.
>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>> index c832673273a2..29f422469960 100644
>> --- a/drivers/devfreq/exynos-bus.c
>> +++ b/drivers/devfreq/exynos-bus.c
>> @@ -356,17 +356,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>   		dev_err(dev, "failed to add devfreq device\n");
>>   		ret = PTR_ERR(bus->devfreq);
>>   		goto err;
>>   	}
>>   
>> -	/* Register opp_notifier to catch the change of OPP  */
>> -	ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
>> -	if (ret < 0) {
>> -		dev_err(dev, "failed to register opp notifier\n");
>> -		goto err;
>> -	}
>> -
>>   	/*
>>   	 * Enable devfreq-event to get raw data which is used to determine
>>   	 * current bus load.
>>   	 */
>>   	ret = exynos_bus_enable_edev(bus);
>> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
>> index 2e65d7279d79..f660d2031e8a 100644
>> --- a/drivers/devfreq/rk3399_dmc.c
>> +++ b/drivers/devfreq/rk3399_dmc.c
>> @@ -454,12 +454,10 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>>   	if (IS_ERR(data->devfreq)) {
>>   		ret = PTR_ERR(data->devfreq);
>>   		goto err_free_opp;
>>   	}
>>   
>> -	devm_devfreq_register_opp_notifier(dev, data->devfreq);
>> -
>>   	data->dev = dev;
>>   	platform_set_drvdata(pdev, data);
>>   
>>   	return 0;
>>   
>> @@ -470,14 +468,10 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>>   
>>   static int rk3399_dmcfreq_remove(struct platform_device *pdev)
>>   {
>>   	struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(&pdev->dev);
>>   
>> -	/*
>> -	 * Before remove the opp table we need to unregister the opp notifier.
>> -	 */
>> -	devm_devfreq_unregister_opp_notifier(dmcfreq->dev, dmcfreq->devfreq);
>>   	dev_pm_opp_of_remove_table(dmcfreq->dev);
>>   
>>   	return 0;
>>   }
>>   
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 4b5cc80abbe3..bf6ebfbc1e8a 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -220,18 +220,10 @@ extern void devfreq_resume(void);
>>   extern int update_devfreq(struct devfreq *devfreq);
>>   
>>   /* Helper functions for devfreq user device driver with OPP. */
>>   extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>>   					   unsigned long *freq, u32 flags);
>> -extern int devfreq_register_opp_notifier(struct device *dev,
>> -					 struct devfreq *devfreq);
>> -extern int devfreq_unregister_opp_notifier(struct device *dev,
>> -					   struct devfreq *devfreq);
>> -extern int devm_devfreq_register_opp_notifier(struct device *dev,
>> -					      struct devfreq *devfreq);
>> -extern void devm_devfreq_unregister_opp_notifier(struct device *dev,
>> -						struct devfreq *devfreq);
>>   extern int devfreq_register_notifier(struct devfreq *devfreq,
>>   					struct notifier_block *nb,
>>   					unsigned int list);
>>   extern int devfreq_unregister_notifier(struct devfreq *devfreq,
>>   					struct notifier_block *nb,
>>
> 
> It looks good to me.
> But, this patch don't remove the inline functions in devfreq.h.
> - devfreq_register_opp_notifier
> - devfreq_unregister_opp_notifier
> - devm_devfreq_register_opp_notifier
> - devm_devfreq_unregister_opp_notifier

Will fix.

I dropped this patch from later versions of the 'PM QoS' series because 
it's not required for PM QoS. It does however touch devfreq_add_device 
in a way that can cause a conflict.

--
Regards,
Leonard
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel