[PATCH v7 0/6] PM / devfreq: Add dev_pm_qos support

[PATCH v7 0/6] 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 v6:
* Don't return errno from devfreq_qos_notifier_call, return NOTIFY_DONE
and print the error. Message mirrors devfreq_monitor.
* More spelling and punctuation nits
Link to v6: https://patchwork.kernel.org/cover/11157201/

Changes since v5 are mostly cosmetic:
* Drop patches which are not strictly related to PM QoS.
* Add a comment explaining why devfreq_add_device needs two cleanup paths.
* Remove {} for single line.
* Rename {min,max}_freq_req to user_{min,max}_freq_req
* Collect reviews
Link to v5: https://patchwork.kernel.org/cover/11149497/

Sorry for forgetting to properly label v5. I know this is inside the
merge window but review would still be appreciated.

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 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)
Like 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 (6):
  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 PM QoS support
  PM / devfreq: Use PM QoS for sysfs min/max_freq

 drivers/devfreq/devfreq.c | 269 ++++++++++++++++++++++++++------------
 include/linux/devfreq.h   |  14 +-
 2 files changed, 193 insertions(+), 90 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 v7 1/6] 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>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 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 e9e6c8f1dc4b..f5bea7350f72 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 v7 2/6] 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 makes it possible to avoid relying on locking a partially
initialized object.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/devfreq/devfreq.c | 43 +++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index f5bea7350f72..8bbcd4efa09f 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -588,10 +588,12 @@ static void devfreq_dev_release(struct device *dev)
 	mutex_unlock(&devfreq_list_lock);
 
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
+	kfree(devfreq->time_in_state);
+	kfree(devfreq->trans_table);
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
 }
 
 /**
@@ -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,20 @@ 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:
+	/*
+	 * Cleanup path for errors that happen before registration.
+	 * Otherwise we rely on devfreq_dev_release.
+	 */
+	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 v7 3/6] 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>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/devfreq/devfreq.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 8bbcd4efa09f..1cec816d3d00 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 v7 4/6] 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>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 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 1cec816d3d00..7f152a582e78 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) {
@@ -1278,40 +1312,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)
 {
@@ -1323,40 +1345,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 v7 5/6] PM / devfreq: Add PM QoS support

[Leonard Crestez]

Register notifiers with the PM QoS framework in order to respond to
requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.

No notifiers are added by this patch but PM QoS constraints can be
imposed externally (for example from other devices).

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 7f152a582e78..9887408f23bb 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 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,53 @@ 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 constraints.
+ * @devfreq:    the devfreq instance.
+ */
+static int devfreq_qos_notifier_call(struct devfreq *devfreq)
+{
+	int err;
+
+	mutex_lock(&devfreq->lock);
+	err = update_devfreq(devfreq);
+	mutex_unlock(&devfreq->lock);
+	if (err)
+		dev_err(&devfreq->dev, "dvfs for QoS constraints"
+				" failed with (%d) error\n", err);
+
+	/* QoS is best effort - let all notifiers run on error */
+	return NOTIFY_OK;
+}
+
+/**
+ * devfreq_qos_min_notifier_call() - Callback for QoS min_freq changes.
+ * @nb:		Should 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 max_freq changes.
+ * @nb:		Should 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 +671,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);
 
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
@@ -732,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	if (err) {
 		put_device(&devfreq->dev);
 		goto err_out;
 	}
 
+	/*
+	 * Register notifiers for updates to min/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 +834,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:
 	/*
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 2bae9ed3c783..8b92ccbd1962 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 v7 6/6] PM / devfreq: Use 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_request interface.

Since 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 compatibility by rounding min values down and rounding
max values up.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++--------------
 include/linux/devfreq.h   |  9 +++----
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 9887408f23bb..a00737e34d36 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);
@@ -679,10 +675,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->user_max_freq_req);
+	dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
 }
@@ -747,18 +745,26 @@ 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;
+
+	/* PM QoS requests for min/max freq from sysfs */
+	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (err < 0)
+		goto err_dev;
+	err = dev_pm_qos_add_request(dev, &devfreq->user_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(
@@ -843,10 +849,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
 err_dev:
 	/*
 	 * Cleanup path for errors that happen before registration.
 	 * Otherwise we rely on devfreq_dev_release.
 	 */
+	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
+		dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
+	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
+		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
@@ -1387,14 +1397,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->user_min_freq_req, value);
+	if (ret < 0)
+		return ret;
 
 	return count;
 }
 
 static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
@@ -1419,19 +1432,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->user_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 8b92ccbd1962..3162eb9b0954 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)
+ * @user_min_freq_req:	PM QoS min frequency request from user (via sysfs)
+ * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)
  * @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 user_min_freq_req;
+	struct dev_pm_qos_request user_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

Re: [PATCH v7 5/6] PM / devfreq: Add PM QoS support

[Matthias Kaehlcke]

On Tue, Sep 24, 2019 at 12:10:33AM +0300, Leonard Crestez wrote:
> Register notifiers with the PM QoS framework in order to respond to
> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.
> 
> No notifiers are added by this patch but PM QoS constraints can be
> imposed externally (for example from other devices).
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 76 +++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq.h   |  5 +++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 7f152a582e78..9887408f23bb 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 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,53 @@ 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 constraints.
> + * @devfreq:    the devfreq instance.
> + */
> +static int devfreq_qos_notifier_call(struct devfreq *devfreq)
> +{
> +	int err;
> +
> +	mutex_lock(&devfreq->lock);
> +	err = update_devfreq(devfreq);
> +	mutex_unlock(&devfreq->lock);
> +	if (err)
> +		dev_err(&devfreq->dev, "dvfs for QoS constraints"
> +				" failed with (%d) error\n", err);

nit: DVFS. devfreq_monitor() also uses the lower-case acronym though, so
you can claim this is consistent :)

I'd prefer to spare you another trivial re-spin, but unfortunately
breaking the log message into multiple lines is a coding style
violation:

Documentation/process/coding-style.rst
  2) Breaking long lines and strings

  However, never break user-visible strings such as printk messages,
  because that breaks the ability to grep for them.


With that fixed:

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 v7 1/6] PM / devfreq: Don't fail devfreq_dev_release if not in list

[Chanwoo Choi]

Hi,

On 19. 9. 24. 오전 6:10, 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>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  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 e9e6c8f1dc4b..f5bea7350f72 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;
> 

It's reasonable. Thanks.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
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 v7 2/6] PM / devfreq: Move more initialization before registration

[Chanwoo Choi]

Hi,

I'm not sure that it is necessary. Because, mainline kernel
provides the resource-managed feature with 'devm_*'
in order to reduce the memory leak by user.

It's like going back to a time when 'devm_*' feature wasn't there.
I think just it depends on the coding style of author.

I leave the final decision of this patch to Myungjoo.

On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
> In general it is a better to initialize an object before making it
> accessible externally (through device_register).
> 
> This makes it possible to avoid relying on locking a partially
> initialized object.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/devfreq/devfreq.c | 43 +++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index f5bea7350f72..8bbcd4efa09f 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -588,10 +588,12 @@ static void devfreq_dev_release(struct device *dev)
>  	mutex_unlock(&devfreq_list_lock);
>  
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
> +	kfree(devfreq->time_in_state);
> +	kfree(devfreq->trans_table);
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
>  }
>  
>  /**
> @@ -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,20 @@ 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:
> +	/*
> +	 * Cleanup path for errors that happen before registration.
> +	 * Otherwise we rely on devfreq_dev_release.
> +	 */
> +	kfree(devfreq->time_in_state);
> +	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL(devfreq_add_device);
> 


-- 
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 v7 3/6] PM / devfreq: Don't take lock in devfreq_add_device

[Chanwoo Choi]

Hi,

On 19. 9. 24. 오전 6:10, 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>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/devfreq/devfreq.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 8bbcd4efa09f..1cec816d3d00 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: Chanwoo Choi <cw00.choi@samsung.com>

-- 
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 v7 4/6] PM / devfreq: Introduce devfreq_get_freq_range

[Chanwoo Choi]

Hi,

On 19. 9. 24. 오전 6:10, 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>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  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 1cec816d3d00..7f152a582e78 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

It is not used on this patch. If it is used on later patch,
move it to other patch.

> +
>  #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,

This function is used in the only devfreq core.
I think that the internal function doesn't need to add 'devfreq' prefix.
Also, when I developed the patches, I didn't use 'devfreq' prefix
for internal function.

> +				   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 */

I think that the comments in the devfreq_get_freq_range(),
it is not necessary. But if you think that it is necessary,
please add more detailed description like as following:

	/*
	 * The devfreq recommend that freq_table must be generated
	 * in ascending order but, some devfreq driver used the descending order
	 * for freq_table. In order to support all cases, check the order
	 * of freq_table and then initialize the 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 */

ditto. Need to add more detailed comment by keeping the comment style of devfreq.


> +	*min_freq = max(*min_freq, devfreq->min_freq);
> +	*max_freq = min(*max_freq, devfreq->max_freq);
> +
> +	/* constraints from OPP interface */

ditto.

> +	*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 */

It is not necessary. We can know that min_freq have to be
under max_freq without any comment.

> +	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 */

Please remove it. 

>  	if (freq < min_freq) {
>  		freq = min_freq;
>  		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>  	}
>  	if (freq > max_freq) {
> @@ -1278,40 +1312,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)
>  {
> @@ -1323,40 +1345,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" */

Please remove this comment.

> +	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,
> 

-- 
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 v7 5/6] PM / devfreq: Add PM QoS support

[Chanwoo Choi]

Hi,

On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
> Register notifiers with the PM QoS framework in order to respond to
> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.
> 
> No notifiers are added by this patch but PM QoS constraints can be
> imposed externally (for example from other devices).
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 76 +++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq.h   |  5 +++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 7f152a582e78..9887408f23bb 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

You have to remove it on patch4.

>  
>  #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 PM QoS */

I think that it is not necessary. But, if you think it is required,
Please add the detailed comment with consistent comment style.

> +	*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));

If you use the separate variable for getting the value from dev_pm_qos_read_value(),
you can add this line under 80 char. If there are any special reason,
I prefer to keep the line under 80 char.


> +
>  	/* 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,53 @@ 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 constraints.
> + * @devfreq:    the devfreq instance.
> + */
> +static int devfreq_qos_notifier_call(struct devfreq *devfreq)

Also, as I commented on patch4, we better to remove 'devfreq' prefix
for internal function as following:
	devfreq_qos_notifier_call -> qos_notifier_call

> +{
> +	int err;
> +
> +	mutex_lock(&devfreq->lock);
> +	err = update_devfreq(devfreq);
> +	mutex_unlock(&devfreq->lock);
> +	if (err)
> +		dev_err(&devfreq->dev, "dvfs for QoS constraints"
> +				" failed with (%d) error\n", err);

'dvfs' is not full name. Also, the capital letter is more correct.
But, the devfreq used 'failed to ...' comment style on some points.

I suggest the comment as following:

--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -628,8 +628,8 @@ static int devfreq_qos_notifier_call(struct devfreq *devfreq)
        err = update_devfreq(devfreq);
        mutex_unlock(&devfreq->lock);
        if (err)
-               dev_err(&devfreq->dev, "dvfs for QoS constraints"
-                               " failed with (%d) error\n", err);
+               dev_err(&devfreq->dev,
+                       "failed to update frequency with PMQoS (%d)\n", err);
 


> +
> +	/* QoS is best effort - let all notifiers run on error */

Please remove it.

> +	return NOTIFY_OK;
> +}
> +
> +/**
> + * devfreq_qos_min_notifier_call() - Callback for QoS min_freq changes.
> + * @nb:		Should be devfreq->nb_min
> + */
> +static int devfreq_qos_min_notifier_call(struct notifier_block *nb,

ditto.
	devfreq_qos_min_notifier_call -> qos_min_notifier_call

> +					 unsigned long val, void *ptr)
> +{
> +	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min);
> +

nitpick. You can remove this line.

> +	return devfreq_qos_notifier_call(devfreq);
> +}
> +
> +/**
> + * devfreq_qos_max_notifier_call() - Callback for QoS max_freq changes.
> + * @nb:		Should be devfreq->nb_max
> + */
> +static int devfreq_qos_max_notifier_call(struct notifier_block *nb,

ditto.
	devfreq_qos_max_notifier_call -> qos_max_notifier_call


> +					 unsigned long val, void *ptr)
> +{
> +	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_max);
> +

nitpick. You can remove this line.

> +	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 +671,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);

Even if devfreq_dev_release() is called at the end of device driver,
dev_pm_remove_notifier() have returned the value. Need to check
the return value for checking the error state.

> +
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
> @@ -732,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	if (err) {
>  		put_device(&devfreq->dev);
>  		goto err_out;
>  	}
>  
> +	/*
> +	 * Register notifiers for updates to min/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).> +	 */

In the devfreq_add_device(), each step has not contained
the detailed comment. If possible, in order to keep the existing style,
please remove it. 

> +	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 +834,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:
>  	/*
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 2bae9ed3c783..8b92ccbd1962 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;
> 


-- 
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 v7 6/6] PM / devfreq: Use PM QoS for sysfs min/max_freq

[Chanwoo Choi]

Hi,

On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
> Switch the handling of min_freq and max_freq from sysfs to use the
> dev_pm_qos_request interface.
> 
> Since 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 compatibility by rounding min values down and rounding
> max values up.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++--------------
>  include/linux/devfreq.h   |  9 +++----
>  2 files changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 9887408f23bb..a00737e34d36 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);
> @@ -679,10 +675,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->user_max_freq_req);
> +	dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
>  }
> @@ -747,18 +745,26 @@ 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;
> +
> +	/* PM QoS requests for min/max freq from sysfs */

The comment is important. But the devfreq_add_device() has usually
not added the comment for each step. I'm not sure to add the some
comments for only this. How about removing it?

> +	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +	if (err < 0)
> +		goto err_dev;
> +	err = dev_pm_qos_add_request(dev, &devfreq->user_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(
> @@ -843,10 +849,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  err_dev:
>  	/*
>  	 * Cleanup path for errors that happen before registration.
>  	 * Otherwise we rely on devfreq_dev_release.
>  	 */
> +	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
> @@ -1387,14 +1397,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 */

Please remove it.

> +	if (value)
> +		value = value / HZ_PER_KHZ;

It doesn't be necessary.

> +
> +	ret = dev_pm_qos_update_request(&df->user_min_freq_req, value);

Change it as following:
	ret = dev_pm_qos_update_request(&df->user_min_freq_req, (value / HZ_PER_KHZ));

> +	if (ret < 0)
> +		return ret;
>  
>  	return count;
>  }
>  
>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
> @@ -1419,19 +1432,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" */

Please remove it.

> +	if (value)
> +		value = DIV_ROUND_UP(value, HZ_PER_KHZ);

Why do you use 'DIV_ROUND_UP(value, HZ_PER_KHZ)'
instead of 'value / HZ_PER_KHZ' in min_freq_store()?

> +	else
> +		value = S32_MAX;> > -	df->max_freq = value;
> -	update_devfreq(df);
> -	mutex_unlock(&df->lock);
> +	ret = dev_pm_qos_update_request(&df->user_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 8b92ccbd1962..3162eb9b0954 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)
> + * @user_min_freq_req:	PM QoS min frequency request from user (via sysfs)
> + * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)

I think that 'user' prefix is not needed. You better to change it as following
	@min_freq_req:	PM QoS minimum frequency request by user via sysfs
	@max_freq_req:	PM QoS maximum frequency request by user via sysfs

>   * @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 user_min_freq_req;
> +	struct dev_pm_qos_request user_max_freq_req;

ditto.

>  	unsigned long scaling_min_freq;
>  	unsigned long scaling_max_freq;
>  	bool stop_polling;
>  
>  	unsigned long suspend_freq;
> 


-- 
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 v7 2/6] PM / devfreq: Move more initialization before registration

[Leonard Crestez]

On 2019-09-24 5:52 AM, Chanwoo Choi wrote:
> Hi,
> 
> I'm not sure that it is necessary. Because, mainline kernel
> provides the resource-managed feature with 'devm_*'
> in order to reduce the memory leak by user.
> 
> It's like going back to a time when 'devm_*' feature wasn't there.
> I think just it depends on the coding style of author.
> 
> I leave the final decision of this patch to Myungjoo.

Initializing before registration is a dependency of the next patch which 
relaxes locking on init path.

Dropping devm is indeed odd but manual memory management in a subsystem 
core seems reasonable. I think devm could be kept around by spliting 
device_register into separate device_initialize and device_add steps.

> On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
>> In general it is a better to initialize an object before making it
>> accessible externally (through device_register).
>>
>> This makes it possible to avoid relying on locking a partially
>> initialized object.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>   drivers/devfreq/devfreq.c | 43 +++++++++++++++++++++++----------------
>>   1 file changed, 25 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index f5bea7350f72..8bbcd4efa09f 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -588,10 +588,12 @@ static void devfreq_dev_release(struct device *dev)
>>   	mutex_unlock(&devfreq_list_lock);
>>   
>>   	if (devfreq->profile->exit)
>>   		devfreq->profile->exit(devfreq->dev.parent);
>>   
>> +	kfree(devfreq->time_in_state);
>> +	kfree(devfreq->trans_table);
>>   	mutex_destroy(&devfreq->lock);
>>   	kfree(devfreq);
>>   }
>>   
>>   /**
>> @@ -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,20 @@ 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:
>> +	/*
>> +	 * Cleanup path for errors that happen before registration.
>> +	 * Otherwise we rely on devfreq_dev_release.
>> +	 */
>> +	kfree(devfreq->time_in_state);
>> +	kfree(devfreq->trans_table);
>>   	kfree(devfreq);
>>   err_out:
>>   	return ERR_PTR(err);
>>   }
>>   EXPORT_SYMBOL(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 v7 4/6] PM / devfreq: Introduce devfreq_get_freq_range

[Leonard Crestez]

On 2019-09-24 8:09 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 9. 24. 오전 6:10, 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>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>   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 1cec816d3d00..7f152a582e78 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
> 
> It is not used on this patch. If it is used on later patch,
> move it to other patch.

Good catch.

>>   #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,
> 
> This function is used in the only devfreq core.
> I think that the internal function doesn't need to add 'devfreq' prefix.
> Also, when I developed the patches, I didn't use 'devfreq' prefix
> for internal function.

This does not appear to a clear convention, there are other internal 
functions with a devfreq prefix. I usually keep prefixes always but it 
does result in extra verbosity.
>> +				   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 */
> 
> I think that the comments in the devfreq_get_freq_range(),
> it is not necessary. But if you think that it is necessary,
> please add more detailed description like as following:
> 
> 	/*
> 	 * The devfreq recommend that freq_table must be generated
> 	 * in ascending order but, some devfreq driver used the descending order
> 	 * for freq_table. In order to support all cases, check the order
> 	 * of freq_table and then initialize the min/max frequency from freq_table.
> 	 */

OK. Not strictly related to this series: wouldn't it make sense to just 
fix the drivers? In linux kernel API compatibility hacks are usually 
avoided.

>> +	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 */
> 
> ditto. Need to add more detailed comment by keeping the comment style of devfreq.

What comment style, capitalization and punctuation? This is similar to 
the rest of the file.

>> +	*min_freq = max(*min_freq, devfreq->min_freq);
>> +	*max_freq = min(*max_freq, devfreq->max_freq);
>> +
>> +	/* constraints from OPP interface */
> 
> ditto.
> 
>> +	*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 */
> 
> It is not necessary. We can know that min_freq have to be
> under max_freq without any comment.
> 
>> +	if (*min_freq > *max_freq)
>> +		*min_freq = *max_freq;

This is a deliberate change introduced by this patch: instead of 
ensuring min_freq <= max_freq at sysfs write time we make min_freq take 
precedence at evaluation time.

With PM QoS there is no way to ensure that the value coming through 
DEV_PM_QOS_MIN_FREQ is less than through DEV_PM_QOS_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 */
> 
> Please remove it.

OK

>>   	if (freq < min_freq) {
>>   		freq = min_freq;
>>   		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>>   	}
>>   	if (freq > max_freq) {
>> @@ -1278,40 +1312,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)
>>   {
>> @@ -1323,40 +1345,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" */
> 
> Please remove this comment.

This is documented in Documentation/ABI/testing/sysfs-class-devfreq but 
not immediately obvious. Setting max to "zero" could have meant "min 
possible" instead of "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,
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 5/6] PM / devfreq: Add PM QoS support

[Leonard Crestez]

On 2019-09-24 9:47 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
>> Register notifiers with the PM QoS framework in order to respond to
>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.
>>
>> No notifiers are added by this patch but PM QoS constraints can be
>> imposed externally (for example from other devices).
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/devfreq/devfreq.c | 76 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/devfreq.h   |  5 +++
>>   2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 7f152a582e78..9887408f23bb 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
> 
> You have to remove it on patch4.

Done. This was added twice, probably messed up during rebasing.

>>   /*
>>    * 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 PM QoS */
> 
> I think that it is not necessary. But, if you think it is required,
> Please add the detailed comment with consistent comment style.

Not sure what comment style you mean, it seems like a reasonable 
single-line comment.
>> +	*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));
> 
> If you use the separate variable for getting the value from dev_pm_qos_read_value(),
> you can add this line under 80 char. If there are any special reason,
> I prefer to keep the line under 80 char.

OK

>>   	/* 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,53 @@ 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 constraints.
>> + * @devfreq:    the devfreq instance.
>> + */
>> +static int devfreq_qos_notifier_call(struct devfreq *devfreq)
> 
> Also, as I commented on patch4, we better to remove 'devfreq' prefix
> for internal function as following:
> 	devfreq_qos_notifier_call -> qos_notifier_call

OK

>> +{
>> +	int err;
>> +
>> +	mutex_lock(&devfreq->lock);
>> +	err = update_devfreq(devfreq);
>> +	mutex_unlock(&devfreq->lock);
>> +	if (err)
>> +		dev_err(&devfreq->dev, "dvfs for QoS constraints"
>> +				" failed with (%d) error\n", err);
> 
> 'dvfs' is not full name. Also, the capital letter is more correct.
> But, the devfreq used 'failed to ...' comment style on some points.
> 
> I suggest the comment as following:
> 
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -628,8 +628,8 @@ static int devfreq_qos_notifier_call(struct devfreq *devfreq)
>          err = update_devfreq(devfreq);
>          mutex_unlock(&devfreq->lock);
>          if (err)
> -               dev_err(&devfreq->dev, "dvfs for QoS constraints"
> -                               " failed with (%d) error\n", err);
> +               dev_err(&devfreq->dev,
> +                       "failed to update frequency with PMQoS (%d)\n", err);

I tried to mirror comment in monitor_devfreq, I'll adjust. The dev_name 
of devfreq->dev is usually something unhelpful like "devfreq1" so maybe 
&devfreq->dev.parent should be passed instead.
> 
>> +
>> +	/* QoS is best effort - let all notifiers run on error */
> 
> Please remove it.

OK but an explanation as to why errors are not propagated seems worth 
keeping.

>> +	return NOTIFY_OK;
>> +}
>> +
>> +/**
>> + * devfreq_qos_min_notifier_call() - Callback for QoS min_freq changes.
>> + * @nb:		Should be devfreq->nb_min
>> + */
>> +static int devfreq_qos_min_notifier_call(struct notifier_block *nb,
> 
> ditto.
> 	devfreq_qos_min_notifier_call -> qos_min_notifier_call
> 
>> +					 unsigned long val, void *ptr)
>> +{
>> +	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min);
>> +
> 
> nitpick. You can remove this line.

OK, if the devfreq_ prefix is removed this can be under 80 chars.

>> @@ -619,10 +671,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);
> 
> Even if devfreq_dev_release() is called at the end of device driver,
> dev_pm_remove_notifier() have returned the value. Need to check
> the return value for checking the error state.

And if dev_pm_qos_remove_notifier returns an error what next?

In cleanup functions there is not much you can do on error other than to 
continue cleaning up. This is why they usually return void.

>>   	if (devfreq->profile->exit)
>>   		devfreq->profile->exit(devfreq->dev.parent);
>>   
>>   	kfree(devfreq->time_in_state);
>>   	kfree(devfreq->trans_table);
>> @@ -732,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   	if (err) {
>>   		put_device(&devfreq->dev);
>>   		goto err_out;
>>   	}
>>   
>> +	/*
>> +	 * Register notifiers for updates to min/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).> +	 */
> 
> In the devfreq_add_device(), each step has not contained
> the detailed comment. If possible, in order to keep the existing style,
> please remove it.

I can remove it but the comment explains why this is the right step at 
which notifiers should be registered so it seems useful.

>> +	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 +834,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:
>>   	/*
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 2bae9ed3c783..8b92ccbd1962 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;
>>
> 
> 

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

Re: [PATCH v7 6/6] PM / devfreq: Use PM QoS for sysfs min/max_freq

[Leonard Crestez]

On 2019-09-24 10:22 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
>> Switch the handling of min_freq and max_freq from sysfs to use the
>> dev_pm_qos_request interface.
>>
>> Since 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 compatibility by rounding min values down and rounding
>> max values up.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>   drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++--------------
>>   include/linux/devfreq.h   |  9 +++----
>>   2 files changed, 36 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 9887408f23bb..a00737e34d36 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);
>> @@ -679,10 +675,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->user_max_freq_req);
>> +	dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>>   	kfree(devfreq->time_in_state);
>>   	kfree(devfreq->trans_table);
>>   	mutex_destroy(&devfreq->lock);
>>   	kfree(devfreq);
>>   }
>> @@ -747,18 +745,26 @@ 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;
>> +
>> +	/* PM QoS requests for min/max freq from sysfs */
> 
> The comment is important. But the devfreq_add_device() has usually
> not added the comment for each step. I'm not sure to add the some
> comments for only this. How about removing it?

OK.

>> +	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
>> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
>> +	if (err < 0)
>> +		goto err_dev;
>> +	err = dev_pm_qos_add_request(dev, &devfreq->user_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(
>> @@ -843,10 +849,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   err_dev:
>>   	/*
>>   	 * Cleanup path for errors that happen before registration.
>>   	 * Otherwise we rely on devfreq_dev_release.
>>   	 */
>> +	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
>> +		dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
>> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
>> +		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>>   	kfree(devfreq->time_in_state);
>>   	kfree(devfreq->trans_table);
>>   	kfree(devfreq);
>>   err_out:
>>   	return ERR_PTR(err);
>> @@ -1387,14 +1397,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 */
> 
> Please remove it.
> 
>> +	if (value)
>> +		value = value / HZ_PER_KHZ;
> 
> It doesn't be necessary.
> 
>> +
>> +	ret = dev_pm_qos_update_request(&df->user_min_freq_req, value);
> 
> Change it as following:
> 	ret = dev_pm_qos_update_request(&df->user_min_freq_req, (value / HZ_PER_KHZ));

OK

>> +	if (ret < 0)
>> +		return ret;
>>   
>>   	return count;
>>   }
>>   
>>   static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>> @@ -1419,19 +1432,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" */
> 
> Please remove it.
> 
>> +	if (value)
>> +		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
> 
> Why do you use 'DIV_ROUND_UP(value, HZ_PER_KHZ)'
> instead of 'value / HZ_PER_KHZ' in min_freq_store()?

If an user request of max=666666666 Hz is converted to max=666666 kHz 
and then back to 666666000 Hz then the OPP with freq=2/3 gHz might be 
cut out. I deliberately rounded down for min and up for max to make the 
constraint interval "inclusive".

Does this make sense? It seems like the path least likely to cause issues.
>> +	else
>> +		value = S32_MAX; >> -	df->max_freq = value;
>> -	update_devfreq(df);
>> -	mutex_unlock(&df->lock);
>> +	ret = dev_pm_qos_update_request(&df->user_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 8b92ccbd1962..3162eb9b0954 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)
>> + * @user_min_freq_req:	PM QoS min frequency request from user (via sysfs)
>> + * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)
> 
> I think that 'user' prefix is not needed. You better to change it as following

Was requested here:
     https://patchwork.kernel.org/patch/11149505/#22891887

It makes sense to mention ths request is from userspace via sysfs. Other 
drivers can also make PM QoS frequency requests for their own reasons.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel