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

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

[Leonard Crestez]

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

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

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

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

Some of first patches in the series are bugfixes and cleanups, they could be
applied separately.

---
Changes since v8:
* Fix incorrectly reading qos max twice in get_freq_range.
* Make devfreq_notifier_call set scaling_max_freq to ULONG_MAX instead of 0 on
error. This avoids special-casing this in get_freq_range.
* Add a fix for devfreq_notifier_call returning -errno.
* Replace S32_MAX with PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE. This seems cleaner
and avoids overflow when multiplying S32_MAX with 1000 on 32bit platforms. It
overflowed to 0xfffffc18 hz so it was mostly safe anyway.
* Warn when encountering errors on cleanup (but continue).
* Incorporate other smaller comment and printk adjustments from review.
Link to v8:
https://patchwork.kernel.org/cover/11158383/

Changes since v7:
* Only #define HZ_PER_KHZ in patch where it's used.
* Drop devfreq_ prefix for some internal functions.
* Improve qos update error message.
* Remove some unnecessary comments.
* Collect reviews
Link to v7: https://patchwork.kernel.org/cover/11157649/

Changes since v6:
* Don't return errno from devfreq_qos_notifier_call, return NOTIFY_DONE
and print the error.
* More spelling and punctuation nits
Link to v6: https://patchwork.kernel.org/cover/11157201/

Changes since v5:
* 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/

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

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

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

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

Leonard Crestez (8):
  PM / devfreq: Don't fail devfreq_dev_release if not in list
  PM / devfreq: Fix devfreq_notifier_call returning errno
  PM / devfreq: Set scaling_max_freq to max on OPP notifier error
  PM / devfreq: Move more initialization before registration
  PM / devfreq: Don't take lock in devfreq_add_device
  PM / devfreq: Introduce get_freq_range helper
  PM / devfreq: Add PM QoS support
  PM / devfreq: Use PM QoS for sysfs min/max_freq

 drivers/devfreq/devfreq.c | 310 ++++++++++++++++++++++++++------------
 include/linux/devfreq.h   |  14 +-
 2 files changed, 224 insertions(+), 100 deletions(-)

-- 
2.17.1

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

[Leonard Crestez]

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

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

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index b905963cea7d..7dc899da1172 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -581,15 +581,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);
@@ -640,10 +635,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

[PATCH v9 2/8] PM / devfreq: Fix devfreq_notifier_call returning errno

[Leonard Crestez]

Notifier callbacks shouldn't return negative errno but one of the
NOTIFY_OK/DONE/BAD values.

The OPP core will ignore return values from notifiers but returning a
value that matches NOTIFY_STOP_MASK will stop the notification chain.

Fix by always returning NOTIFY_OK.

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 7dc899da1172..32bbf6e80380 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -548,30 +548,32 @@ EXPORT_SYMBOL(devfreq_interval_update);
  */
 static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
 				 void *devp)
 {
 	struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
-	int ret;
+	int err = -EINVAL;
 
 	mutex_lock(&devfreq->lock);
 
 	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
-	if (!devfreq->scaling_min_freq) {
-		mutex_unlock(&devfreq->lock);
-		return -EINVAL;
-	}
+	if (!devfreq->scaling_min_freq)
+		goto out;
 
 	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
-	if (!devfreq->scaling_max_freq) {
-		mutex_unlock(&devfreq->lock);
-		return -EINVAL;
-	}
+	if (!devfreq->scaling_max_freq)
+		goto out;
+
+	err = update_devfreq(devfreq);
 
-	ret = update_devfreq(devfreq);
+out:
 	mutex_unlock(&devfreq->lock);
+	if (err)
+		dev_err(devfreq->dev.parent,
+			"failed to update frequency from OPP notifier (%d)\n",
+			err);
 
-	return ret;
+	return NOTIFY_OK;
 }
 
 /**
  * devfreq_dev_release() - Callback for struct device to release the device.
  * @dev:	the devfreq device
-- 
2.17.1

[PATCH v9 3/8] PM / devfreq: Set scaling_max_freq to max on OPP notifier error

[Leonard Crestez]

The devfreq_notifier_call functions will update scaling_min_freq and
scaling_max_freq when the OPP table is updated.

If fetching the maximum frequency fails then scaling_max_freq remains
set to zero which is confusing. Set to ULONG_MAX instead so we don't
need special handling for this case in other places.

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 32bbf6e80380..3e0e936185a3 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -557,12 +557,14 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
 	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
 	if (!devfreq->scaling_min_freq)
 		goto out;
 
 	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
-	if (!devfreq->scaling_max_freq)
+	if (!devfreq->scaling_max_freq) {
+		devfreq->scaling_max_freq = ULONG_MAX;
 		goto out;
+	}
 
 	err = update_devfreq(devfreq);
 
 out:
 	mutex_unlock(&devfreq->lock);
-- 
2.17.1

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

[Leonard Crestez]

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

This makes it possible to avoid remove locking the partially initialized
object and simplifies the code. However devm is not available before
device_register (only after the device_initialize step) so the two
allocations need to be managed manually.

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 3e0e936185a3..0b40f40ee7aa 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -591,10 +591,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);
 }
 
 /**
@@ -674,44 +676,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);
@@ -737,14 +738,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

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

[Leonard Crestez]

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

This simplifies the code and helps avoid recursive lock warnings.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 0b40f40ee7aa..87eff789ce24 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -637,11 +637,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;
@@ -650,28 +649,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;
 
@@ -682,20 +677,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;
@@ -704,17 +697,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

[PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper

[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 | 110 +++++++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 48 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 87eff789ce24..2d63692903ff 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -96,10 +96,53 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
 		dev_pm_opp_put(opp);
 
 	return max_freq;
 }
 
+/**
+ * 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 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);
+
+	/*
+	 * Initialize minimum/maximum frequency from freq table.
+	 * The devfreq drivers can initialize this in either ascending or
+	 * descending order and devfreq core supports both.
+	 */
+	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];
+	}
+
+	/* Apply constraints from sysfs */
+	*min_freq = max(*min_freq, devfreq->min_freq);
+	*max_freq = min(*max_freq, devfreq->max_freq);
+
+	/* Apply 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);
+
+	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
  */
@@ -348,20 +391,11 @@ int update_devfreq(struct devfreq *devfreq)
 
 	/* Reevaluate the proper frequency */
 	err = devfreq->governor->get_target_freq(devfreq, &freq);
 	if (err)
 		return err;
-
-	/*
-	 * 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);
+	get_freq_range(devfreq, &min_freq, &max_freq);
 
 	if (freq < min_freq) {
 		freq = min_freq;
 		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
 	}
@@ -1281,40 +1315,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);
+	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)
 {
@@ -1326,40 +1348,32 @@ 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];
-	}
+	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);
+	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

[PATCH v9 7/8] 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 | 78 +++++++++++++++++++++++++++++++++++++++
 include/linux/devfreq.h   |  5 +++
 2 files changed, 83 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2d63692903ff..46f699b84a22 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -22,15 +22,18 @@
 #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 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
@@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
 static void get_freq_range(struct devfreq *devfreq,
 			   unsigned long *min_freq,
 			   unsigned long *max_freq)
 {
 	unsigned long *freq_table = devfreq->profile->freq_table;
+	s32 qos_min_freq, qos_max_freq;
 
 	lockdep_assert_held(&devfreq->lock);
 
 	/*
 	 * Initialize minimum/maximum frequency from freq table.
@@ -125,10 +129,20 @@ static void get_freq_range(struct devfreq *devfreq,
 	} else {
 		*min_freq = freq_table[devfreq->profile->max_state - 1];
 		*max_freq = freq_table[0];
 	}
 
+	/* Apply constraints from PM QoS */
+	qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent,
+					     DEV_PM_QOS_MIN_FREQUENCY);
+	qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
+					     DEV_PM_QOS_MAX_FREQUENCY);
+	*min_freq = max(*min_freq, (unsigned long)HZ_PER_KHZ * qos_min_freq);
+	if (qos_max_freq != PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE)
+		*max_freq = min(*max_freq,
+				(unsigned long)HZ_PER_KHZ * qos_max_freq);
+
 	/* Apply constraints from sysfs */
 	*min_freq = max(*min_freq, devfreq->min_freq);
 	*max_freq = min(*max_freq, devfreq->max_freq);
 
 	/* Apply constraints from OPP interface */
@@ -608,24 +622,75 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
 			err);
 
 	return NOTIFY_OK;
 }
 
+/**
+ * qos_notifier_call() - Common handler for QoS constraints.
+ * @devfreq:    the devfreq instance.
+ */
+static int 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.parent,
+			"failed to update frequency from PM QoS (%d)\n",
+			err);
+
+	return NOTIFY_OK;
+}
+
+/**
+ * qos_min_notifier_call() - Callback for QoS min_freq changes.
+ * @nb:		Should be devfreq->nb_min
+ */
+static int qos_min_notifier_call(struct notifier_block *nb,
+					 unsigned long val, void *ptr)
+{
+	return qos_notifier_call(container_of(nb, struct devfreq, nb_min));
+}
+
+/**
+ * qos_max_notifier_call() - Callback for QoS max_freq changes.
+ * @nb:		Should be devfreq->nb_max
+ */
+static int qos_max_notifier_call(struct notifier_block *nb,
+					 unsigned long val, void *ptr)
+{
+	return qos_notifier_call(container_of(nb, struct devfreq, nb_max));
+}
+
 /**
  * devfreq_dev_release() - Callback for struct device to release the device.
  * @dev:	the devfreq device
  *
  * Remove devfreq from the list and release its resources.
  */
 static void devfreq_dev_release(struct device *dev)
 {
 	struct devfreq *devfreq = to_devfreq(dev);
+	int err;
 
 	mutex_lock(&devfreq_list_lock);
 	list_del(&devfreq->node);
 	mutex_unlock(&devfreq_list_lock);
 
+	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
+					 DEV_PM_QOS_MAX_FREQUENCY);
+	if (err)
+		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MAX_FREQUENCY notifier: %d\n",
+			 err);
+	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
+			DEV_PM_QOS_MIN_FREQUENCY);
+	if (err)
+		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MIN_FREQUENCY notifier: %d\n",
+			 err);
+
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
@@ -735,10 +800,22 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	if (err) {
 		put_device(&devfreq->dev);
 		goto err_out;
 	}
 
+	devfreq->nb_min.notifier_call = 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 = 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",
@@ -762,10 +839,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

[PATCH v9 8/8] 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 | 63 ++++++++++++++++++++++++++++-----------
 include/linux/devfreq.h   |  9 +++---
 2 files changed, 51 insertions(+), 21 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 46f699b84a22..4ff5fbc4ee85 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -139,14 +139,10 @@ static void get_freq_range(struct devfreq *devfreq,
 	*min_freq = max(*min_freq, (unsigned long)HZ_PER_KHZ * qos_min_freq);
 	if (qos_max_freq != PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE)
 		*max_freq = min(*max_freq,
 				(unsigned long)HZ_PER_KHZ * qos_max_freq);
 
-	/* Apply constraints from sysfs */
-	*min_freq = max(*min_freq, devfreq->min_freq);
-	*max_freq = min(*max_freq, devfreq->max_freq);
-
 	/* Apply 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);
@@ -690,10 +686,18 @@ static void devfreq_dev_release(struct device *dev)
 			 err);
 
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
+	err = dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
+	if (err)
+		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MAX_FREQUENCY request: %d\n",
+			 err);
+	err = dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
+	if (err)
+		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MIN_FREQUENCY request: %d\n",
+			 err);
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
 }
@@ -758,18 +762,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;
+
+	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,
+				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
+	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(
@@ -848,10 +860,16 @@ 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))
+		if (dev_pm_qos_remove_request(&devfreq->user_max_freq_req))
+			dev_warn(dev, "Failed to remove DEV_PM_QOS_MAX_FREQUENCY request\n");
+	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
+		if (dev_pm_qos_remove_request(&devfreq->user_min_freq_req))
+			dev_warn(dev, "Failed to remove DEV_PM_QOS_MIN_FREQUENCY request\n");
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
@@ -1392,14 +1410,15 @@ 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 PM QoS */
+	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,
@@ -1424,18 +1443,28 @@ 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);
-
-	if (!value)
-		value = ULONG_MAX;
+	/*
+	 * PM QoS frequencies are in kHz so we need to convert. Convert by
+	 * rounding upwards so that the acceptable interval never shrinks.
+	 *
+	 * For example if the user writes "666666666" to sysfs this value will
+	 * be converted to 666667 kHz and back to 666667000 Hz before an OPP
+	 * lookup, this ensures that an OPP of 666666666Hz is still accepted.
+	 *
+	 * A value of zero means "no limit".
+	 */
+	if (value)
+		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
+	else
+		value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
 
-	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..fb376b5b7281 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 minimum frequency request from user (via sysfs)
+ * @user_max_freq_req:	PM QoS maximum 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

Re: [PATCH v9 2/8] PM / devfreq: Fix devfreq_notifier_call returning errno

[Matthias Kaehlcke]

On Wed, Oct 02, 2019 at 10:25:05PM +0300, Leonard Crestez wrote:
> Notifier callbacks shouldn't return negative errno but one of the
> NOTIFY_OK/DONE/BAD values.
> 
> The OPP core will ignore return values from notifiers but returning a
> value that matches NOTIFY_STOP_MASK will stop the notification chain.
> 
> Fix by always returning NOTIFY_OK.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 7dc899da1172..32bbf6e80380 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -548,30 +548,32 @@ EXPORT_SYMBOL(devfreq_interval_update);
>   */
>  static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  				 void *devp)
>  {
>  	struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
> -	int ret;
> +	int err = -EINVAL;
>  
>  	mutex_lock(&devfreq->lock);
>  
>  	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
> -	if (!devfreq->scaling_min_freq) {
> -		mutex_unlock(&devfreq->lock);
> -		return -EINVAL;
> -	}
> +	if (!devfreq->scaling_min_freq)
> +		goto out;
>  
>  	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
> -	if (!devfreq->scaling_max_freq) {
> -		mutex_unlock(&devfreq->lock);
> -		return -EINVAL;
> -	}
> +	if (!devfreq->scaling_max_freq)
> +		goto out;
> +
> +	err = update_devfreq(devfreq);
>  
> -	ret = update_devfreq(devfreq);
> +out:
>  	mutex_unlock(&devfreq->lock);
> +	if (err)
> +		dev_err(devfreq->dev.parent,
> +			"failed to update frequency from OPP notifier (%d)\n",
> +			err);

In case an OPP freq can't be found the log doesn't provide clues
about what the problem could be. I couldn't find a clearly superior
errno value though, so I guess this is as good as it gets, unless
you want to have a dedicated message for those errors. Should be a
rare exception anyway, and previously there was no log at all.

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

Re: [PATCH v9 3/8] PM / devfreq: Set scaling_max_freq to max on OPP notifier error

[Matthias Kaehlcke]

On Wed, Oct 02, 2019 at 10:25:06PM +0300, Leonard Crestez wrote:
> The devfreq_notifier_call functions will update scaling_min_freq and
> scaling_max_freq when the OPP table is updated.
> 
> If fetching the maximum frequency fails then scaling_max_freq remains
> set to zero which is confusing. Set to ULONG_MAX instead so we don't
> need special handling for this case in other places.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 32bbf6e80380..3e0e936185a3 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -557,12 +557,14 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>  	if (!devfreq->scaling_min_freq)
>  		goto out;
>  
>  	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
> -	if (!devfreq->scaling_max_freq)
> +	if (!devfreq->scaling_max_freq) {
> +		devfreq->scaling_max_freq = ULONG_MAX;
>  		goto out;
> +	}
>  
>  	err = update_devfreq(devfreq);
>  
>  out:
>  	mutex_unlock(&devfreq->lock);
> -- 
> 2.17.1
> 

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

Re: [PATCH v9 7/8] PM / devfreq: Add PM QoS support

[Matthias Kaehlcke]

On Wed, Oct 02, 2019 at 10:25:10PM +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 | 78 +++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq.h   |  5 +++
>  2 files changed, 83 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2d63692903ff..46f699b84a22 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -22,15 +22,18 @@
>  #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 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
> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>  static void get_freq_range(struct devfreq *devfreq,
>  			   unsigned long *min_freq,
>  			   unsigned long *max_freq)
>  {
>  	unsigned long *freq_table = devfreq->profile->freq_table;
> +	s32 qos_min_freq, qos_max_freq;
>  
>  	lockdep_assert_held(&devfreq->lock);
>  
>  	/*
>  	 * Initialize minimum/maximum frequency from freq table.
> @@ -125,10 +129,20 @@ static void get_freq_range(struct devfreq *devfreq,
>  	} else {
>  		*min_freq = freq_table[devfreq->profile->max_state - 1];
>  		*max_freq = freq_table[0];
>  	}
>  
> +	/* Apply constraints from PM QoS */
> +	qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent,
> +					     DEV_PM_QOS_MIN_FREQUENCY);
> +	qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
> +					     DEV_PM_QOS_MAX_FREQUENCY);
> +	*min_freq = max(*min_freq, (unsigned long)HZ_PER_KHZ * qos_min_freq);
> +	if (qos_max_freq != PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE)
> +		*max_freq = min(*max_freq,
> +				(unsigned long)HZ_PER_KHZ * qos_max_freq);
> +
>  	/* Apply constraints from sysfs */
>  	*min_freq = max(*min_freq, devfreq->min_freq);
>  	*max_freq = min(*max_freq, devfreq->max_freq);
>  
>  	/* Apply constraints from OPP interface */
> @@ -608,24 +622,75 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  			err);
>  
>  	return NOTIFY_OK;
>  }
>  
> +/**
> + * qos_notifier_call() - Common handler for QoS constraints.
> + * @devfreq:    the devfreq instance.
> + */
> +static int 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.parent,
> +			"failed to update frequency from PM QoS (%d)\n",
> +			err);
> +
> +	return NOTIFY_OK;
> +}
> +
> +/**
> + * qos_min_notifier_call() - Callback for QoS min_freq changes.
> + * @nb:		Should be devfreq->nb_min
> + */
> +static int qos_min_notifier_call(struct notifier_block *nb,
> +					 unsigned long val, void *ptr)
> +{
> +	return qos_notifier_call(container_of(nb, struct devfreq, nb_min));
> +}
> +
> +/**
> + * qos_max_notifier_call() - Callback for QoS max_freq changes.
> + * @nb:		Should be devfreq->nb_max
> + */
> +static int qos_max_notifier_call(struct notifier_block *nb,
> +					 unsigned long val, void *ptr)
> +{
> +	return qos_notifier_call(container_of(nb, struct devfreq, nb_max));
> +}
> +
>  /**
>   * devfreq_dev_release() - Callback for struct device to release the device.
>   * @dev:	the devfreq device
>   *
>   * Remove devfreq from the list and release its resources.
>   */
>  static void devfreq_dev_release(struct device *dev)
>  {
>  	struct devfreq *devfreq = to_devfreq(dev);
> +	int err;
>  
>  	mutex_lock(&devfreq_list_lock);
>  	list_del(&devfreq->node);
>  	mutex_unlock(&devfreq_list_lock);
>  
> +	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
> +					 DEV_PM_QOS_MAX_FREQUENCY);
> +	if (err)
> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MAX_FREQUENCY notifier: %d\n",
> +			 err);
> +	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
> +			DEV_PM_QOS_MIN_FREQUENCY);
> +	if (err)
> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MIN_FREQUENCY notifier: %d\n",
> +			 err);
> +
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>

I was concerned there might be a race where the notifier is still
running, but I confirmed that dev_pm_qos_mtx is held by
dev_pm_qos_remove_notifier() and also when the notifiers are called.

>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
> @@ -735,10 +800,22 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	if (err) {
>  		put_device(&devfreq->dev);
>  		goto err_out;
>  	}
>  
> +	devfreq->nb_min.notifier_call = 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 = 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",
> @@ -762,10 +839,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
> 

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

Re: [PATCH v9 2/8] PM / devfreq: Fix devfreq_notifier_call returning errno

[Leonard Crestez]

On 2019-10-03 12:24 AM, Matthias Kaehlcke wrote:
> On Wed, Oct 02, 2019 at 10:25:05PM +0300, Leonard Crestez wrote:
>> Notifier callbacks shouldn't return negative errno but one of the
>> NOTIFY_OK/DONE/BAD values.
>>
>> The OPP core will ignore return values from notifiers but returning a
>> value that matches NOTIFY_STOP_MASK will stop the notification chain.
>>
>> Fix by always returning NOTIFY_OK.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/devfreq/devfreq.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 7dc899da1172..32bbf6e80380 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -548,30 +548,32 @@ EXPORT_SYMBOL(devfreq_interval_update);
>>    */
>>   static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>>   				 void *devp)
>>   {
>>   	struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
>> -	int ret;
>> +	int err = -EINVAL;
>>   
>>   	mutex_lock(&devfreq->lock);
>>   
>>   	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>> -	if (!devfreq->scaling_min_freq) {
>> -		mutex_unlock(&devfreq->lock);
>> -		return -EINVAL;
>> -	}
>> +	if (!devfreq->scaling_min_freq)
>> +		goto out;
>>   
>>   	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>> -	if (!devfreq->scaling_max_freq) {
>> -		mutex_unlock(&devfreq->lock);
>> -		return -EINVAL;
>> -	}
>> +	if (!devfreq->scaling_max_freq)
>> +		goto out;
>> +
>> +	err = update_devfreq(devfreq);
>>   
>> -	ret = update_devfreq(devfreq);
>> +out:
>>   	mutex_unlock(&devfreq->lock);
>> +	if (err)
>> +		dev_err(devfreq->dev.parent,
>> +			"failed to update frequency from OPP notifier (%d)\n",
>> +			err);
> 
> In case an OPP freq can't be found the log doesn't provide clues
> about what the problem could be. I couldn't find a clearly superior
> errno value though, so I guess this is as good as it gets, unless
> you want to have a dedicated message for those errors. Should be a
> rare exception anyway, and previously there was no log at all.

I guess it could happen if all OPPs are disabled after probe?

The devfreq core will attempt to switch away if the current OPP get 
disabled but if nothing else is available then printing an error and 
sticking to the current frequency seems reasonable.

It would indicate a bug somewhere else.

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

Re: [PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper

[Matthias Kaehlcke]

On Wed, Oct 02, 2019 at 10:25:09PM +0300, Leonard Crestez wrote:
> Moving handling of min/max freq to a single function and call it from
> update_devfreq and for printing min/max freq values in sysfs.
> 
> This changes the behavior of out-of-range min_freq/max_freq: clamping
> is now done at evaluation time. This means that if an out-of-range
> constraint is imposed by sysfs and it later becomes valid then it will
> be enforced.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/devfreq/devfreq.c | 110 +++++++++++++++++++++-----------------
>  1 file changed, 62 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 87eff789ce24..2d63692903ff 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
>
> ...
>
>  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);
> +	get_freq_range(df, &min_freq, &max_freq);

With this min/max_freq shown aren't necessarily those set through sysfs,
but the aggregated PM QoS values (plus OPP constraints).

I did some testing with a WIP patch that converts devfreq_cooling.c to
PM QoS. When reading sysfs min/max values to double check the limits
set earlier I found it utterly confusing to see the sysfs min/max values
fluctuating due to thermal throttling, and not being able to see the
configured values.

Looks like cpufreq does the same, but I'm not convinced this is a good
idea. I think we want to display the values set by userspace, as done
before managing the limits through PM QoS. Viresh, was this change of
userspace visible behavior done intentionally for cpufreq?

Re: [PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper

[Leonard Crestez]

On 03.10.2019 21:19, Matthias Kaehlcke wrote:
> On Wed, Oct 02, 2019 at 10:25:09PM +0300, Leonard Crestez wrote:
>> Moving handling of min/max freq to a single function and call it from
>> update_devfreq and for printing min/max freq values in sysfs.
>>
>> This changes the behavior of out-of-range min_freq/max_freq: clamping
>> is now done at evaluation time. This means that if an out-of-range
>> constraint is imposed by sysfs and it later becomes valid then it will
>> be enforced.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>   drivers/devfreq/devfreq.c | 110 +++++++++++++++++++++-----------------
>>   1 file changed, 62 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 87eff789ce24..2d63692903ff 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>>
>> ...
>>
>>   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);
>> +	get_freq_range(df, &min_freq, &max_freq);
> 
> With this min/max_freq shown aren't necessarily those set through sysfs,
> but the aggregated PM QoS values (plus OPP constraints).
> 
> I did some testing with a WIP patch that converts devfreq_cooling.c to
> PM QoS. When reading sysfs min/max values to double check the limits
> set earlier I found it utterly confusing to see the sysfs min/max values
> fluctuating due to thermal throttling, and not being able to see the
> configured values.

Isn't current devfreq_cooling based on OPP disabling which modifies 
scaling_max_freq? This is not a behavior change: reading back always 
showed the "effective maximum" rather than the value explicitly written 
to max_freq.

This behavior is indeed confusing but can be fixed by adding two new 
files: user_min/max_freq and user_max_freq. These would act like current 
min/max_freq on write but on read would only show the value explicitly 
configured by the user.

> Looks like cpufreq does the same, but I'm not convinced this is a good
> idea. I think we want to display the values set by userspace, as done
> before managing the limits through PM QoS. Viresh, was this change of
> userspace visible behavior done intentionally for cpufreq?

Re: [PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper

[Matthias Kaehlcke]

On Thu, Oct 03, 2019 at 07:16:03PM +0000, Leonard Crestez wrote:
> On 03.10.2019 21:19, Matthias Kaehlcke wrote:
> > On Wed, Oct 02, 2019 at 10:25:09PM +0300, Leonard Crestez wrote:
> >> Moving handling of min/max freq to a single function and call it from
> >> update_devfreq and for printing min/max freq values in sysfs.
> >>
> >> This changes the behavior of out-of-range min_freq/max_freq: clamping
> >> is now done at evaluation time. This means that if an out-of-range
> >> constraint is imposed by sysfs and it later becomes valid then it will
> >> be enforced.
> >>
> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> >> ---
> >>   drivers/devfreq/devfreq.c | 110 +++++++++++++++++++++-----------------
> >>   1 file changed, 62 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >> index 87eff789ce24..2d63692903ff 100644
> >> --- a/drivers/devfreq/devfreq.c
> >> +++ b/drivers/devfreq/devfreq.c
> >>
> >> ...
> >>
> >>   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);
> >> +	get_freq_range(df, &min_freq, &max_freq);
> > 
> > With this min/max_freq shown aren't necessarily those set through sysfs,
> > but the aggregated PM QoS values (plus OPP constraints).
> > 
> > I did some testing with a WIP patch that converts devfreq_cooling.c to
> > PM QoS. When reading sysfs min/max values to double check the limits
> > set earlier I found it utterly confusing to see the sysfs min/max values
> > fluctuating due to thermal throttling, and not being able to see the
> > configured values.
> 
> Isn't current devfreq_cooling based on OPP disabling which modifies 
> scaling_max_freq? This is not a behavior change: reading back always 
> showed the "effective maximum" rather than the value explicitly written 
> to max_freq.

I stand corrected, for devfreq indeed this isn't a behavioral change, and
looking at the diff would have told me. I just expected it to do the
reasonable thing and what cpufreq did in the past.

> This behavior is indeed confusing but can be fixed by adding two new 
> files: user_min/max_freq and user_max_freq. These would act like current 
> min/max_freq on write but on read would only show the value explicitly 
> configured by the user.

It seems the reasonable thing to do, though it's not great to alter
userspace visible behavior :( I doubt though that userspace would really
depend on it, since any min/max value read might change inmediately
afterwards. If there's really value in exposing the aggregate limits it
would probably make sense to do this through separate attributes. Let's
see what devfreq maintainers think.

In any case the patch should be fine as is, since it doesn't introduce the
(IMO) odd behavior.

Re: [PATCH v9 7/8] PM / devfreq: Add PM QoS support

[Matthias Kaehlcke]

On Wed, Oct 02, 2019 at 10:25:10PM +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 | 78 +++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq.h   |  5 +++
>  2 files changed, 83 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2d63692903ff..46f699b84a22 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -22,15 +22,18 @@
>  #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 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
> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>  static void get_freq_range(struct devfreq *devfreq,
>  			   unsigned long *min_freq,
>  			   unsigned long *max_freq)
>  {
>  	unsigned long *freq_table = devfreq->profile->freq_table;
> +	s32 qos_min_freq, qos_max_freq;
>  
>  	lockdep_assert_held(&devfreq->lock);
>  
>  	/*
>  	 * Initialize minimum/maximum frequency from freq table.
> @@ -125,10 +129,20 @@ static void get_freq_range(struct devfreq *devfreq,
>  	} else {
>  		*min_freq = freq_table[devfreq->profile->max_state - 1];
>  		*max_freq = freq_table[0];
>  	}
>  
> +	/* Apply constraints from PM QoS */
> +	qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent,
> +					     DEV_PM_QOS_MIN_FREQUENCY);
> +	qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
> +					     DEV_PM_QOS_MAX_FREQUENCY);
> +	*min_freq = max(*min_freq, (unsigned long)HZ_PER_KHZ * qos_min_freq);
> +	if (qos_max_freq != PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE)
> +		*max_freq = min(*max_freq,
> +				(unsigned long)HZ_PER_KHZ * qos_max_freq);
> +
>  	/* Apply constraints from sysfs */
>  	*min_freq = max(*min_freq, devfreq->min_freq);
>  	*max_freq = min(*max_freq, devfreq->max_freq);
>  
>  	/* Apply constraints from OPP interface */
> @@ -608,24 +622,75 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  			err);
>  
>  	return NOTIFY_OK;
>  }
>  
> +/**
> + * qos_notifier_call() - Common handler for QoS constraints.
> + * @devfreq:    the devfreq instance.
> + */
> +static int 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.parent,
> +			"failed to update frequency from PM QoS (%d)\n",
> +			err);
> +
> +	return NOTIFY_OK;
> +}
> +
> +/**
> + * qos_min_notifier_call() - Callback for QoS min_freq changes.
> + * @nb:		Should be devfreq->nb_min
> + */
> +static int qos_min_notifier_call(struct notifier_block *nb,
> +					 unsigned long val, void *ptr)
> +{
> +	return qos_notifier_call(container_of(nb, struct devfreq, nb_min));
> +}
> +
> +/**
> + * qos_max_notifier_call() - Callback for QoS max_freq changes.
> + * @nb:		Should be devfreq->nb_max
> + */
> +static int qos_max_notifier_call(struct notifier_block *nb,
> +					 unsigned long val, void *ptr)
> +{
> +	return qos_notifier_call(container_of(nb, struct devfreq, nb_max));
> +}
> +
>  /**
>   * devfreq_dev_release() - Callback for struct device to release the device.
>   * @dev:	the devfreq device
>   *
>   * Remove devfreq from the list and release its resources.
>   */
>  static void devfreq_dev_release(struct device *dev)
>  {
>  	struct devfreq *devfreq = to_devfreq(dev);
> +	int err;
>  
>  	mutex_lock(&devfreq_list_lock);
>  	list_del(&devfreq->node);
>  	mutex_unlock(&devfreq_list_lock);
>  
> +	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
> +					 DEV_PM_QOS_MAX_FREQUENCY);
> +	if (err)
> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MAX_FREQUENCY notifier: %d\n",
> +			 err);
> +	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
> +			DEV_PM_QOS_MIN_FREQUENCY);
> +	if (err)
> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MIN_FREQUENCY notifier: %d\n",
> +			 err);
> +
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
> @@ -735,10 +800,22 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	if (err) {
>  		put_device(&devfreq->dev);
>  		goto err_out;
>  	}
>  
> +	devfreq->nb_min.notifier_call = 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 = 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",
> @@ -762,10 +839,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
> 

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

Re: [PATCH v9 8/8] PM / devfreq: Use PM QoS for sysfs min/max_freq

[Matthias Kaehlcke]

On Wed, Oct 02, 2019 at 10:25:11PM +0300, 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 | 63 ++++++++++++++++++++++++++++-----------
>  include/linux/devfreq.h   |  9 +++---
>  2 files changed, 51 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 46f699b84a22..4ff5fbc4ee85 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -139,14 +139,10 @@ static void get_freq_range(struct devfreq *devfreq,
>  	*min_freq = max(*min_freq, (unsigned long)HZ_PER_KHZ * qos_min_freq);
>  	if (qos_max_freq != PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE)
>  		*max_freq = min(*max_freq,
>  				(unsigned long)HZ_PER_KHZ * qos_max_freq);
>  
> -	/* Apply constraints from sysfs */
> -	*min_freq = max(*min_freq, devfreq->min_freq);
> -	*max_freq = min(*max_freq, devfreq->max_freq);
> -
>  	/* Apply 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);
> @@ -690,10 +686,18 @@ static void devfreq_dev_release(struct device *dev)
>  			 err);
>  
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
> +	err = dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> +	if (err)
> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MAX_FREQUENCY request: %d\n",
> +			 err);
> +	err = dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
> +	if (err)
> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MIN_FREQUENCY request: %d\n",
> +			 err);
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
>  }
> @@ -758,18 +762,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;
> +
> +	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,
> +				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> +	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(
> @@ -848,10 +860,16 @@ 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))
> +		if (dev_pm_qos_remove_request(&devfreq->user_max_freq_req))
> +			dev_warn(dev, "Failed to remove DEV_PM_QOS_MAX_FREQUENCY request\n");
> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
> +		if (dev_pm_qos_remove_request(&devfreq->user_min_freq_req))
> +			dev_warn(dev, "Failed to remove DEV_PM_QOS_MIN_FREQUENCY request\n");
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
> @@ -1392,14 +1410,15 @@ 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 PM QoS */
> +	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,
> @@ -1424,18 +1443,28 @@ 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);
> -
> -	if (!value)
> -		value = ULONG_MAX;
> +	/*
> +	 * PM QoS frequencies are in kHz so we need to convert. Convert by
> +	 * rounding upwards so that the acceptable interval never shrinks.
> +	 *
> +	 * For example if the user writes "666666666" to sysfs this value will
> +	 * be converted to 666667 kHz and back to 666667000 Hz before an OPP
> +	 * lookup, this ensures that an OPP of 666666666Hz is still accepted.
> +	 *
> +	 * A value of zero means "no limit".
> +	 */
> +	if (value)
> +		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
> +	else
> +		value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
>  
> -	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..fb376b5b7281 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 minimum frequency request from user (via sysfs)
> + * @user_max_freq_req:	PM QoS maximum 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
> 

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

Re: [PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper

[Matthias Kaehlcke]

On Thu, Oct 03, 2019 at 11:19:38AM -0700, Matthias Kaehlcke wrote:
> On Wed, Oct 02, 2019 at 10:25:09PM +0300, Leonard Crestez wrote:
> > Moving handling of min/max freq to a single function and call it from
> > update_devfreq and for printing min/max freq values in sysfs.
> > 
> > This changes the behavior of out-of-range min_freq/max_freq: clamping
> > is now done at evaluation time. This means that if an out-of-range
> > constraint is imposed by sysfs and it later becomes valid then it will
> > be enforced.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  drivers/devfreq/devfreq.c | 110 +++++++++++++++++++++-----------------
> >  1 file changed, 62 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 87eff789ce24..2d63692903ff 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> >
> > ...
> >
> >  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);
> > +	get_freq_range(df, &min_freq, &max_freq);
> 
> With this min/max_freq shown aren't necessarily those set through sysfs,
> but the aggregated PM QoS values (plus OPP constraints).
> 
> I did some testing with a WIP patch that converts devfreq_cooling.c to
> PM QoS. When reading sysfs min/max values to double check the limits
> set earlier I found it utterly confusing to see the sysfs min/max values
> fluctuating due to thermal throttling, and not being able to see the
> configured values.
> 
> Looks like cpufreq does the same, but I'm not convinced this is a good
> idea. I think we want to display the values set by userspace, as done
> before managing the limits through PM QoS. Viresh, was this change of
> userspace visible behavior done intentionally for cpufreq?

ping

Viresh / devfreq maintainers:

Do we really want to expose through sysfs the potentially constantly
changing aggregate min/max values, instead of those set by userspace?
At least for cpufreq that's a divergence from previous behavior, and
from a userspace perspective it seems odd that you can't reliably read
back the limit set by userspace.

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

[Leonard Crestez]

On 2019-10-02 10:25 PM, Leonard Crestez wrote:
> 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.
> 
> Some of first patches in the series are bugfixes and cleanups, they could be
> applied separately.

Hello,

This series was posted a few ago and all patches have been 
reviewed/tested-by multiple people. Possible minor hangups:

1) Matthias found it confusing that min/max_freq in sysfs changes 
on-the-fly. This is not a behavior change and I believe a decent 
workaround would be to implement separate user_min/max_freq files from 
which userspace will always read back the contraints it has written.

2) There was an objection to removing devm from two allocs in PATCH 4. I 
believe current solution is acceptable but a possible alternative would 
be to split device_register into device_initialize and device_add: this 
should allow devm sooner.
Link: https://patchwork.kernel.org/patch/11158385/#22902151

Let me know if you think I should implement the options above and resend.

The bigger problem is that DEV_PM_QOS_MIN/MAX_FREQUENCY was removed from 
pm core because only user (cpufreq) was refactored to use a new 
interface on top of cpufreq_policy. Links to discussion:
     https://patchwork.kernel.org/cover/11193021/
 
https://lore.kernel.org/linux-pm/VI1PR04MB7023DF47D046AEADB4E051EBEE680@VI1PR04MB7023.eurprd04.prod.outlook.com/T/#u

I believe there is still significant value in supporting min/max 
frequency requests on a per-target-device basis. This makes much more 
sense for devfreq that for cpufreq because the whole point of devfreq is 
scaling arbitrary independent devices.

> ---
> Changes since v8:
> * Fix incorrectly reading qos max twice in get_freq_range.
> * Make devfreq_notifier_call set scaling_max_freq to ULONG_MAX instead of 0 on
> error. This avoids special-casing this in get_freq_range.
> * Add a fix for devfreq_notifier_call returning -errno.
> * Replace S32_MAX with PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE. This seems cleaner
> and avoids overflow when multiplying S32_MAX with 1000 on 32bit platforms. It
> overflowed to 0xfffffc18 hz so it was mostly safe anyway.
> * Warn when encountering errors on cleanup (but continue).
> * Incorporate other smaller comment and printk adjustments from review.
> Link to v8:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11158383%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Ca6b53046361642d0fd5708d7476e40e4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637056411174276846&sdata=YKKW1RJl1YArhtjlA859DeRxys4d4iiB%2FQIz3Nl8OtU%3D&reserved=0
> 
> Changes since v7:
> * Only #define HZ_PER_KHZ in patch where it's used.
> * Drop devfreq_ prefix for some internal functions.
> * Improve qos update error message.
> * Remove some unnecessary comments.
> * Collect reviews
> Link to v7: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11157649%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Ca6b53046361642d0fd5708d7476e40e4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637056411174276846&sdata=0s6mH192c3dvDlfrFEVdqdMqoFRM4QJiLZiRg4c89nQ%3D&reserved=0
> 
> Changes since v6:
> * Don't return errno from devfreq_qos_notifier_call, return NOTIFY_DONE
> and print the error.
> * More spelling and punctuation nits
> Link to v6: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11157201%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Ca6b53046361642d0fd5708d7476e40e4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637056411174276846&sdata=6hcaCgESymHhCk5UbFC182FrgFVdJdDoorAJhZXKuTg%3D&reserved=0
> 
> Changes since v5:
> * 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11149497%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Ca6b53046361642d0fd5708d7476e40e4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637056411174276846&sdata=oDuaTI7bpOlCEs9mFRgY5eUvGX%2FtpP6m0U5t4SYNKaw%3D&reserved=0
> 
> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11114657%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Ca6b53046361642d0fd5708d7476e40e4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637056411174276846&sdata=ItIVLxJZVOpO2XL2MBqONWLQ1lHFu2gA7GC1yb%2BQgEE%3D&reserved=0
> 
> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11104061%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Ca6b53046361642d0fd5708d7476e40e4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637056411174276846&sdata=unedjnBbjVtifB8koQ0B8eOjqwCnnAeqHGI8QYuH%2Fv4%3D&reserved=0
> 
> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11084279%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Ca6b53046361642d0fd5708d7476e40e4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637056411174276846&sdata=boHOmdwyUhKlmk7F3X8oYbLq1MvR4dQWlF14I0AXXes%3D&reserved=0
> 
> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11078475%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Ca6b53046361642d0fd5708d7476e40e4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637056411174276846&sdata=DZlQAKLqQc4Q46a7v%2FtEBexsp1jDSLB8yuZcLXPEHl4%3D&reserved=0
> 
> Leonard Crestez (8):
>    PM / devfreq: Don't fail devfreq_dev_release if not in list
>    PM / devfreq: Fix devfreq_notifier_call returning errno
>    PM / devfreq: Set scaling_max_freq to max on OPP notifier error
>    PM / devfreq: Move more initialization before registration
>    PM / devfreq: Don't take lock in devfreq_add_device
>    PM / devfreq: Introduce get_freq_range helper
>    PM / devfreq: Add PM QoS support
>    PM / devfreq: Use PM QoS for sysfs min/max_freq
> 
>   drivers/devfreq/devfreq.c | 310 ++++++++++++++++++++++++++------------
>   include/linux/devfreq.h   |  14 +-
>   2 files changed, 224 insertions(+), 100 deletions(-)
> 

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

[Matthias Kaehlcke]

On Wed, Oct 23, 2019 at 02:06:40PM +0000, Leonard Crestez wrote:
> On 2019-10-02 10:25 PM, Leonard Crestez wrote:
> > 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.
> > 
> > Some of first patches in the series are bugfixes and cleanups, they could be
> > applied separately.
> 
> Hello,
> 
> This series was posted a few ago and all patches have been 
> reviewed/tested-by multiple people. Possible minor hangups:
> 
> 1) Matthias found it confusing that min/max_freq in sysfs changes 
> on-the-fly. This is not a behavior change and I believe a decent 
> workaround would be to implement separate user_min/max_freq files from 
> which userspace will always read back the contraints it has written.

As you said, it isn't a behavioral change, so it shouldn't be an issue
for this series.

Regarding the workaround I think it would be clearer to have new
xyx_min/max_freq files for the aggregate values. min/max_freq are the
interface userspace uses to specify the limits, it would be strange to
use different files to read them out.

In any case the aggregate values seem to be of little practical value,
except perhaps for monitoring, since they could be stale right after
userspace read them out. We could start with not providing them, and add
them if it turns out that they are actually needed.

> 2) There was an objection to removing devm from two allocs in PATCH 4. I 
> believe current solution is acceptable but a possible alternative would 
> be to split device_register into device_initialize and device_add: this 
> should allow devm sooner.
> Link: https://patchwork.kernel.org/patch/11158385/#22902151
> 
> Let me know if you think I should implement the options above and resend.
> 
> The bigger problem is that DEV_PM_QOS_MIN/MAX_FREQUENCY was removed from 
> pm core because only user (cpufreq) was refactored to use a new 
> interface on top of cpufreq_policy. Links to discussion:
>      https://patchwork.kernel.org/cover/11193021/
>  
> https://lore.kernel.org/linux-pm/VI1PR04MB7023DF47D046AEADB4E051EBEE680@VI1PR04MB7023.eurprd04.prod.outlook.com/T/#u
> 
> I believe there is still significant value in supporting min/max 
> frequency requests on a per-target-device basis. This makes much more 
> sense for devfreq that for cpufreq because the whole point of devfreq is 
> scaling arbitrary independent devices.

Agreed.

It seems Rafael would be ok with reverting the patch that removes
DEV_PM_QOS_MIN/MAX_FREQUENCY, IIUC he just doesn't want to keep it around
at this time because with the rest of his series there remain no in-tree
users.

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

[Leonard Crestez]

On 23.10.2019 19:34, Matthias Kaehlcke wrote:
> On Wed, Oct 23, 2019 at 02:06:40PM +0000, Leonard Crestez wrote:
>> On 2019-10-02 10:25 PM, Leonard Crestez wrote:
>>> 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.
>>>
>>> Some of first patches in the series are bugfixes and cleanups, they could be
>>> applied separately.
>>
>> Hello,
>>
>> This series was posted a few ago and all patches have been
>> reviewed/tested-by multiple people. Possible minor hangups:
>>
>> 1) Matthias found it confusing that min/max_freq in sysfs changes
>> on-the-fly. This is not a behavior change and I believe a decent
>> workaround would be to implement separate user_min/max_freq files from
>> which userspace will always read back the contraints it has written.
> 
> As you said, it isn't a behavioral change, so it shouldn't be an issue
> for this series.
> 
> Regarding the workaround I think it would be clearer to have new
> xyx_min/max_freq files for the aggregate values. min/max_freq are the
> interface userspace uses to specify the limits, it would be strange to
> use different files to read them out.
> 
> In any case the aggregate values seem to be of little practical value,
> except perhaps for monitoring, since they could be stale right after
> userspace read them out. We could start with not providing them, and add
> them if it turns out that they are actually needed.

But the min/max_freq files right now already are already aggregates and 
sysfs is considered ABI. I wouldn't be surprised if somebody has an 
userspace daemon which uses them.

My proposal is to add user_min/max_freq as a new finer-grained interface 
that you can both read and write to, no confusion here.

Writes to min/max_freq would still be supported but only for 
compatibility with older releases.

>> 2) There was an objection to removing devm from two allocs in PATCH 4. I
>> believe current solution is acceptable but a possible alternative would
>> be to split device_register into device_initialize and device_add: this
>> should allow devm sooner.
>> Link: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11158385%2F%2322902151&data=02%7C01%7Cleonard.crestez%40nxp.com%7Cb89f3efc3c934030fb6108d757d6ecb2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637074452911403311&sdata=DeOUpVjT1yZ2EWs50CFL98OoTjVMCpQDCM3qjCtKuW0%3D&reserved=0
>>
>> Let me know if you think I should implement the options above and resend.
>>
>> The bigger problem is that DEV_PM_QOS_MIN/MAX_FREQUENCY was removed from
>> pm core because only user (cpufreq) was refactored to use a new
>> interface on top of cpufreq_policy. Links to discussion:
>>       https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11193021%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Cb89f3efc3c934030fb6108d757d6ecb2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637074452911408301&sdata=DxfUtaGch6MilSy5fX8AHN3%2BDIp8MrbQrHH%2B6VdRb%2FI%3D&reserved=0
>>   
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-pm%2FVI1PR04MB7023DF47D046AEADB4E051EBEE680%40VI1PR04MB7023.eurprd04.prod.outlook.com%2FT%2F%23u&data=02%7C01%7Cleonard.crestez%40nxp.com%7Cb89f3efc3c934030fb6108d757d6ecb2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637074452911408301&sdata=sYQZUbzEk2DsWGQ5eQnu2m2%2Bsp%2BYBO16Abyjwf7Z1sQ%3D&reserved=0
>>
>> I believe there is still significant value in supporting min/max
>> frequency requests on a per-target-device basis. This makes much more
>> sense for devfreq that for cpufreq because the whole point of devfreq is
>> scaling arbitrary independent devices.
> 
> Agreed.
> 
> It seems Rafael would be ok with reverting the patch that removes
> DEV_PM_QOS_MIN/MAX_FREQUENCY, IIUC he just doesn't want to keep it around
> at this time because with the rest of his series there remain no in-tree
> users.

Re: [PATCH v9 3/8] PM / devfreq: Set scaling_max_freq to max on OPP notifier error

[Chanwoo Choi]

On 19. 10. 3. 오전 4:25, Leonard Crestez wrote:
> The devfreq_notifier_call functions will update scaling_min_freq and
> scaling_max_freq when the OPP table is updated.
> 
> If fetching the maximum frequency fails then scaling_max_freq remains
> set to zero which is confusing. Set to ULONG_MAX instead so we don't
> need special handling for this case in other places.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 32bbf6e80380..3e0e936185a3 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -557,12 +557,14 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>  	if (!devfreq->scaling_min_freq)
>  		goto out;
>  
>  	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
> -	if (!devfreq->scaling_max_freq)
> +	if (!devfreq->scaling_max_freq) {
> +		devfreq->scaling_max_freq = ULONG_MAX;
>  		goto out;
> +	}
>  
>  	err = update_devfreq(devfreq);
>  
>  out:
>  	mutex_unlock(&devfreq->lock);
> 

Sorry for the late reply.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v9 2/8] PM / devfreq: Fix devfreq_notifier_call returning errno

[Chanwoo Choi]

On 19. 10. 3. 오전 4:25, Leonard Crestez wrote:
> Notifier callbacks shouldn't return negative errno but one of the
> NOTIFY_OK/DONE/BAD values.
> 
> The OPP core will ignore return values from notifiers but returning a
> value that matches NOTIFY_STOP_MASK will stop the notification chain.
> 
> Fix by always returning NOTIFY_OK.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 7dc899da1172..32bbf6e80380 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -548,30 +548,32 @@ EXPORT_SYMBOL(devfreq_interval_update);
>   */
>  static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  				 void *devp)
>  {
>  	struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
> -	int ret;
> +	int err = -EINVAL;
>  
>  	mutex_lock(&devfreq->lock);
>  
>  	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
> -	if (!devfreq->scaling_min_freq) {
> -		mutex_unlock(&devfreq->lock);
> -		return -EINVAL;
> -	}
> +	if (!devfreq->scaling_min_freq)
> +		goto out;
>  
>  	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
> -	if (!devfreq->scaling_max_freq) {
> -		mutex_unlock(&devfreq->lock);
> -		return -EINVAL;
> -	}
> +	if (!devfreq->scaling_max_freq)
> +		goto out;
> +
> +	err = update_devfreq(devfreq);
>  
> -	ret = update_devfreq(devfreq);
> +out:
>  	mutex_unlock(&devfreq->lock);
> +	if (err)
> +		dev_err(devfreq->dev.parent,
> +			"failed to update frequency from OPP notifier (%d)\n",
> +			err);
>  
> -	return ret;
> +	return NOTIFY_OK;
>  }
>  
>  /**
>   * devfreq_dev_release() - Callback for struct device to release the device.
>   * @dev:	the devfreq device
> 

Sorry for the late reply.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper

[Chanwoo Choi]

On 19. 10. 3. 오전 4:25, 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 | 110 +++++++++++++++++++++-----------------
>  1 file changed, 62 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 87eff789ce24..2d63692903ff 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -96,10 +96,53 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>  		dev_pm_opp_put(opp);
>  
>  	return max_freq;
>  }
>  
> +/**
> + * 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 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);
> +
> +	/*
> +	 * Initialize minimum/maximum frequency from freq table.
> +	 * The devfreq drivers can initialize this in either ascending or
> +	 * descending order and devfreq core supports both.
> +	 */
> +	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];
> +	}
> +
> +	/* Apply constraints from sysfs */
> +	*min_freq = max(*min_freq, devfreq->min_freq);
> +	*max_freq = min(*max_freq, devfreq->max_freq);
> +
> +	/* Apply 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)

nitpick:
This if statement is necessary?

The patch3 in this series initializes the 'devfreq->scaling_max_freq'
is ULONG_MAX if failed to get the available frequency from find_available_max_freq.

> +		*max_freq = min(*max_freq, devfreq->scaling_max_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
>   */
> @@ -348,20 +391,11 @@ int update_devfreq(struct devfreq *devfreq)
>  
>  	/* Reevaluate the proper frequency */
>  	err = devfreq->governor->get_target_freq(devfreq, &freq);
>  	if (err)
>  		return err;
> -
> -	/*
> -	 * 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);
> +	get_freq_range(devfreq, &min_freq, &max_freq);
>  
>  	if (freq < min_freq) {
>  		freq = min_freq;
>  		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>  	}
> @@ -1281,40 +1315,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);
> +	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)
>  {
> @@ -1326,40 +1348,32 @@ 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];
> -	}
> +	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);
> +	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,
> 

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper

[Chanwoo Choi]

On 19. 10. 12. 오전 3:29, Matthias Kaehlcke wrote:
> On Thu, Oct 03, 2019 at 11:19:38AM -0700, Matthias Kaehlcke wrote:
>> On Wed, Oct 02, 2019 at 10:25:09PM +0300, Leonard Crestez wrote:
>>> Moving handling of min/max freq to a single function and call it from
>>> update_devfreq and for printing min/max freq values in sysfs.
>>>
>>> This changes the behavior of out-of-range min_freq/max_freq: clamping
>>> is now done at evaluation time. This means that if an out-of-range
>>> constraint is imposed by sysfs and it later becomes valid then it will
>>> be enforced.
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>> ---
>>>  drivers/devfreq/devfreq.c | 110 +++++++++++++++++++++-----------------
>>>  1 file changed, 62 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 87eff789ce24..2d63692903ff 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>>
>>> ...
>>>
>>>  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);
>>> +	get_freq_range(df, &min_freq, &max_freq);
>>
>> With this min/max_freq shown aren't necessarily those set through sysfs,
>> but the aggregated PM QoS values (plus OPP constraints).
>>
>> I did some testing with a WIP patch that converts devfreq_cooling.c to
>> PM QoS. When reading sysfs min/max values to double check the limits
>> set earlier I found it utterly confusing to see the sysfs min/max values
>> fluctuating due to thermal throttling, and not being able to see the
>> configured values.
>>
>> Looks like cpufreq does the same, but I'm not convinced this is a good
>> idea. I think we want to display the values set by userspace, as done
>> before managing the limits through PM QoS. Viresh, was this change of
>> userspace visible behavior done intentionally for cpufreq?
> 
> ping
> 
> Viresh / devfreq maintainers:
> 
> Do we really want to expose through sysfs the potentially constantly
> changing aggregate min/max values, instead of those set by userspace?

Until now, the devfreq core show the available min/max frequency
through sysfs interface. I think the available min/max frequency is proper
for sysfs.

> At least for cpufreq that's a divergence from previous behavior, and
> from a userspace perspective it seems odd that you can't reliably read
> back the limit set by userspace.
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v9 7/8] PM / devfreq: Add PM QoS support

[Chanwoo Choi]

Hi Leonard,

It looks good to me. Thanks for your effort.
But, I have one minor comment related to 'over 80 char'.

In the devfreq_dev_release(), please edit this line
as following to protect the over 80 char on one line
if there are no specific reason.

		dev_warn(dev->parent,
			"Failed to remove DEV_PM_QOS_MAX_FREQUENCY notifier: %d\n",
			err);

		dev_warn(dev->parent,
			"Failed to remove DEV_PM_QOS_MIN_FREQUENCY notifier: %d\n",
			err);


If you edit them, feel free to add my reviewed-by tag:

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>


On 19. 10. 3. 오전 4:25, 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 | 78 +++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq.h   |  5 +++
>  2 files changed, 83 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2d63692903ff..46f699b84a22 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -22,15 +22,18 @@
>  #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 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
> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>  static void get_freq_range(struct devfreq *devfreq,
>  			   unsigned long *min_freq,
>  			   unsigned long *max_freq)
>  {
>  	unsigned long *freq_table = devfreq->profile->freq_table;
> +	s32 qos_min_freq, qos_max_freq;
>  
>  	lockdep_assert_held(&devfreq->lock);
>  
>  	/*
>  	 * Initialize minimum/maximum frequency from freq table.
> @@ -125,10 +129,20 @@ static void get_freq_range(struct devfreq *devfreq,
>  	} else {
>  		*min_freq = freq_table[devfreq->profile->max_state - 1];
>  		*max_freq = freq_table[0];
>  	}
>  
> +	/* Apply constraints from PM QoS */
> +	qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent,
> +					     DEV_PM_QOS_MIN_FREQUENCY);
> +	qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
> +					     DEV_PM_QOS_MAX_FREQUENCY);
> +	*min_freq = max(*min_freq, (unsigned long)HZ_PER_KHZ * qos_min_freq);
> +	if (qos_max_freq != PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE)
> +		*max_freq = min(*max_freq,
> +				(unsigned long)HZ_PER_KHZ * qos_max_freq);
> +
>  	/* Apply constraints from sysfs */
>  	*min_freq = max(*min_freq, devfreq->min_freq);
>  	*max_freq = min(*max_freq, devfreq->max_freq);
>  
>  	/* Apply constraints from OPP interface */
> @@ -608,24 +622,75 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  			err);
>  
>  	return NOTIFY_OK;
>  }
>  
> +/**
> + * qos_notifier_call() - Common handler for QoS constraints.
> + * @devfreq:    the devfreq instance.
> + */
> +static int 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.parent,
> +			"failed to update frequency from PM QoS (%d)\n",
> +			err);
> +
> +	return NOTIFY_OK;
> +}
> +
> +/**
> + * qos_min_notifier_call() - Callback for QoS min_freq changes.
> + * @nb:		Should be devfreq->nb_min
> + */
> +static int qos_min_notifier_call(struct notifier_block *nb,
> +					 unsigned long val, void *ptr)
> +{
> +	return qos_notifier_call(container_of(nb, struct devfreq, nb_min));
> +}
> +
> +/**
> + * qos_max_notifier_call() - Callback for QoS max_freq changes.
> + * @nb:		Should be devfreq->nb_max
> + */
> +static int qos_max_notifier_call(struct notifier_block *nb,
> +					 unsigned long val, void *ptr)
> +{
> +	return qos_notifier_call(container_of(nb, struct devfreq, nb_max));
> +}
> +
>  /**
>   * devfreq_dev_release() - Callback for struct device to release the device.
>   * @dev:	the devfreq device
>   *
>   * Remove devfreq from the list and release its resources.
>   */
>  static void devfreq_dev_release(struct device *dev)
>  {
>  	struct devfreq *devfreq = to_devfreq(dev);
> +	int err;
>  
>  	mutex_lock(&devfreq_list_lock);
>  	list_del(&devfreq->node);
>  	mutex_unlock(&devfreq_list_lock);
>  
> +	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
> +					 DEV_PM_QOS_MAX_FREQUENCY);
> +	if (err)
> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MAX_FREQUENCY notifier: %d\n",
> +			 err);

Please edit this line as following to protect the over 80 char
on one line if there are no specific reason.

		dev_warn(dev->parent,
			"Failed to remove DEV_PM_QOS_MAX_FREQUENCY notifier: %d\n",
			err);


> +	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
> +			DEV_PM_QOS_MIN_FREQUENCY);
> +	if (err)
> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MIN_FREQUENCY notifier: %d\n",
> +			 err);
> +

Please edit this line as following to protect the over 80 char
on one line if there are no specific reason.


		dev_warn(dev->parent,
			"Failed to remove DEV_PM_QOS_MIN_FREQUENCY notifier: %d\n",
			err);


>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
> @@ -735,10 +800,22 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	if (err) {
>  		put_device(&devfreq->dev);
>  		goto err_out;
>  	}
>  
> +	devfreq->nb_min.notifier_call = 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 = 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",
> @@ -762,10 +839,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

Re: [PATCH v9 8/8] PM / devfreq: Use PM QoS for sysfs min/max_freq

[Chanwoo Choi]

Hi Leonard,

It looks good to me. Thanks for your effort.
But, I have one minor comment related to 'over 80 char'.

Please edit this line as following in order to prevent the 'over 80 char'

	dev_warn(dev->parent,
		"Failed to remove DEV_PM_QOS_MAX_FREQUENCY request: %d\n", err);

	dev_warn(dev->parent,
		"Failed to remove DEV_PM_QOS_MIN_FREQUENCY request: %d\n", err);


If you edit them, feel free to add my reviewed-by tag:

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

On 19. 10. 3. 오전 4:25, 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 | 63 ++++++++++++++++++++++++++++-----------
>  include/linux/devfreq.h   |  9 +++---
>  2 files changed, 51 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 46f699b84a22..4ff5fbc4ee85 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -139,14 +139,10 @@ static void get_freq_range(struct devfreq *devfreq,
>  	*min_freq = max(*min_freq, (unsigned long)HZ_PER_KHZ * qos_min_freq);
>  	if (qos_max_freq != PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE)
>  		*max_freq = min(*max_freq,
>  				(unsigned long)HZ_PER_KHZ * qos_max_freq);
>  
> -	/* Apply constraints from sysfs */
> -	*min_freq = max(*min_freq, devfreq->min_freq);
> -	*max_freq = min(*max_freq, devfreq->max_freq);
> -
>  	/* Apply 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);
> @@ -690,10 +686,18 @@ static void devfreq_dev_release(struct device *dev)
>  			 err);
>  
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
> +	err = dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> +	if (err)
> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MAX_FREQUENCY request: %d\n",
> +			 err);

Please edit this line as following in order to prevent the 'over 80 char'

		dev_warn(dev->parent,
			"Failed to remove DEV_PM_QOS_MAX_FREQUENCY request: %d\n", err);

> +	err = dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
> +	if (err)
> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MIN_FREQUENCY request: %d\n",
> +			 err);

ditto. Please edit this line as following:
		dev_warn(dev->parent,
			"Failed to remove DEV_PM_QOS_MIN_FREQUENCY request: %d\n", err);

>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
>  }
> @@ -758,18 +762,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;
> +
> +	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,
> +				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> +	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(
> @@ -848,10 +860,16 @@ 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))
> +		if (dev_pm_qos_remove_request(&devfreq->user_max_freq_req))
> +			dev_warn(dev, "Failed to remove DEV_PM_QOS_MAX_FREQUENCY request\n");
> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
> +		if (dev_pm_qos_remove_request(&devfreq->user_min_freq_req))
> +			dev_warn(dev, "Failed to remove DEV_PM_QOS_MIN_FREQUENCY request\n");
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
> @@ -1392,14 +1410,15 @@ 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 PM QoS */
> +	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,
> @@ -1424,18 +1443,28 @@ 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);
> -
> -	if (!value)
> -		value = ULONG_MAX;
> +	/*
> +	 * PM QoS frequencies are in kHz so we need to convert. Convert by
> +	 * rounding upwards so that the acceptable interval never shrinks.
> +	 *
> +	 * For example if the user writes "666666666" to sysfs this value will
> +	 * be converted to 666667 kHz and back to 666667000 Hz before an OPP
> +	 * lookup, this ensures that an OPP of 666666666Hz is still accepted.
> +	 *
> +	 * A value of zero means "no limit".
> +	 */
> +	if (value)
> +		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
> +	else
> +		value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
>  
> -	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..fb376b5b7281 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 minimum frequency request from user (via sysfs)
> + * @user_max_freq_req:	PM QoS maximum 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;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

Hi Leonard,

This patch didn't get the acked-by from devfreq maintainer.
I think that we need to discuss this patch with more time.
Also, it is possible to make it as the separate patch
from this series. 

IMHO, if you make the separate patch for this and
resend the separate patch on later, I think that
we can merge the remained patch related to PM_QOS.


On 19. 10. 3. 오전 4:25, 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 remove locking the partially initialized
> object and simplifies the code. However devm is not available before
> device_register (only after the device_initialize step) so the two
> allocations need to be managed manually.
> 
> 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 3e0e936185a3..0b40f40ee7aa 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -591,10 +591,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);
>  }
>  
>  /**
> @@ -674,44 +676,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);
> @@ -737,14 +738,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

Re: [PATCH v9 6/8] PM / devfreq: Introduce get_freq_range helper

[Leonard Crestez]

On 31.10.2019 04:37, Chanwoo Choi wrote:
> On 19. 10. 3. 오전 4:25, 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 | 110 +++++++++++++++++++++-----------------
>>   1 file changed, 62 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 87eff789ce24..2d63692903ff 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -96,10 +96,53 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>   		dev_pm_opp_put(opp);
>>   
>>   	return max_freq;
>>   }
>>   
>> +/**
>> + * 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 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);
>> +
>> +	/*
>> +	 * Initialize minimum/maximum frequency from freq table.
>> +	 * The devfreq drivers can initialize this in either ascending or
>> +	 * descending order and devfreq core supports both.
>> +	 */
>> +	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];
>> +	}
>> +
>> +	/* Apply constraints from sysfs */
>> +	*min_freq = max(*min_freq, devfreq->min_freq);
>> +	*max_freq = min(*max_freq, devfreq->max_freq);
>> +
>> +	/* Apply 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)
> 
> nitpick:
> This if statement is necessary?
> 
> The patch3 in this series initializes the 'devfreq->scaling_max_freq'
> is ULONG_MAX if failed to get the available frequency from find_available_max_freq.

Ok, seem I forgot to drop the if statement.

> 
>> +		*max_freq = min(*max_freq, devfreq->scaling_max_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
>>    */
>> @@ -348,20 +391,11 @@ int update_devfreq(struct devfreq *devfreq)
>>   
>>   	/* Reevaluate the proper frequency */
>>   	err = devfreq->governor->get_target_freq(devfreq, &freq);
>>   	if (err)
>>   		return err;
>> -
>> -	/*
>> -	 * 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);
>> +	get_freq_range(devfreq, &min_freq, &max_freq);
>>   
>>   	if (freq < min_freq) {
>>   		freq = min_freq;
>>   		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>>   	}
>> @@ -1281,40 +1315,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);
>> +	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)
>>   {
>> @@ -1326,40 +1348,32 @@ 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];
>> -	}
>> +	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);
>> +	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,
>>
> 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> 

Re: [PATCH v9 7/8] PM / devfreq: Add PM QoS support

[Leonard Crestez]

On 31.10.2019 04:55, Chanwoo Choi wrote:
> Hi Leonard,
> 
> It looks good to me. Thanks for your effort.
> But, I have one minor comment related to 'over 80 char'.
> 
> In the devfreq_dev_release(), please edit this line
> as following to protect the over 80 char on one line
> if there are no specific reason.
> 
> 		dev_warn(dev->parent,
> 			"Failed to remove DEV_PM_QOS_MAX_FREQUENCY notifier: %d\n",
> 			err);
> 
> 		dev_warn(dev->parent,
> 			"Failed to remove DEV_PM_QOS_MIN_FREQUENCY notifier: %d\n",
> 			err);
> 
> 
> If you edit them, feel free to add my reviewed-by tag:

Still didn't find in 80 chars so I shrunk "DEV_PM_QOS_MIN_FREQUENCY to 
"min_freq" instead. Same for PATCH 8/8

> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> 
> On 19. 10. 3. 오전 4:25, 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 | 78 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/devfreq.h   |  5 +++
>>   2 files changed, 83 insertions(+)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 2d63692903ff..46f699b84a22 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -22,15 +22,18 @@
>>   #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 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
>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>   static void get_freq_range(struct devfreq *devfreq,
>>   			   unsigned long *min_freq,
>>   			   unsigned long *max_freq)
>>   {
>>   	unsigned long *freq_table = devfreq->profile->freq_table;
>> +	s32 qos_min_freq, qos_max_freq;
>>   
>>   	lockdep_assert_held(&devfreq->lock);
>>   
>>   	/*
>>   	 * Initialize minimum/maximum frequency from freq table.
>> @@ -125,10 +129,20 @@ static void get_freq_range(struct devfreq *devfreq,
>>   	} else {
>>   		*min_freq = freq_table[devfreq->profile->max_state - 1];
>>   		*max_freq = freq_table[0];
>>   	}
>>   
>> +	/* Apply constraints from PM QoS */
>> +	qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>> +					     DEV_PM_QOS_MIN_FREQUENCY);
>> +	qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>> +					     DEV_PM_QOS_MAX_FREQUENCY);
>> +	*min_freq = max(*min_freq, (unsigned long)HZ_PER_KHZ * qos_min_freq);
>> +	if (qos_max_freq != PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE)
>> +		*max_freq = min(*max_freq,
>> +				(unsigned long)HZ_PER_KHZ * qos_max_freq);
>> +
>>   	/* Apply constraints from sysfs */
>>   	*min_freq = max(*min_freq, devfreq->min_freq);
>>   	*max_freq = min(*max_freq, devfreq->max_freq);
>>   
>>   	/* Apply constraints from OPP interface */
>> @@ -608,24 +622,75 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>>   			err);
>>   
>>   	return NOTIFY_OK;
>>   }
>>   
>> +/**
>> + * qos_notifier_call() - Common handler for QoS constraints.
>> + * @devfreq:    the devfreq instance.
>> + */
>> +static int 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.parent,
>> +			"failed to update frequency from PM QoS (%d)\n",
>> +			err);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +/**
>> + * qos_min_notifier_call() - Callback for QoS min_freq changes.
>> + * @nb:		Should be devfreq->nb_min
>> + */
>> +static int qos_min_notifier_call(struct notifier_block *nb,
>> +					 unsigned long val, void *ptr)
>> +{
>> +	return qos_notifier_call(container_of(nb, struct devfreq, nb_min));
>> +}
>> +
>> +/**
>> + * qos_max_notifier_call() - Callback for QoS max_freq changes.
>> + * @nb:		Should be devfreq->nb_max
>> + */
>> +static int qos_max_notifier_call(struct notifier_block *nb,
>> +					 unsigned long val, void *ptr)
>> +{
>> +	return qos_notifier_call(container_of(nb, struct devfreq, nb_max));
>> +}
>> +
>>   /**
>>    * devfreq_dev_release() - Callback for struct device to release the device.
>>    * @dev:	the devfreq device
>>    *
>>    * Remove devfreq from the list and release its resources.
>>    */
>>   static void devfreq_dev_release(struct device *dev)
>>   {
>>   	struct devfreq *devfreq = to_devfreq(dev);
>> +	int err;
>>   
>>   	mutex_lock(&devfreq_list_lock);
>>   	list_del(&devfreq->node);
>>   	mutex_unlock(&devfreq_list_lock);
>>   
>> +	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
>> +					 DEV_PM_QOS_MAX_FREQUENCY);
>> +	if (err)
>> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MAX_FREQUENCY notifier: %d\n",
>> +			 err);
> 
> Please edit this line as following to protect the over 80 char
> on one line if there are no specific reason.
> 
> 		dev_warn(dev->parent,
> 			"Failed to remove DEV_PM_QOS_MAX_FREQUENCY notifier: %d\n",
> 			err);
> 
> 
>> +	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
>> +			DEV_PM_QOS_MIN_FREQUENCY);
>> +	if (err)
>> +		dev_warn(dev->parent, "Failed to remove DEV_PM_QOS_MIN_FREQUENCY notifier: %d\n",
>> +			 err);
>> +
> 
> Please edit this line as following to protect the over 80 char
> on one line if there are no specific reason.
> 
> 
> 		dev_warn(dev->parent,
> 			"Failed to remove DEV_PM_QOS_MIN_FREQUENCY notifier: %d\n",
> 			err);
> 
> 
>>   	if (devfreq->profile->exit)
>>   		devfreq->profile->exit(devfreq->dev.parent);
>>   
>>   	kfree(devfreq->time_in_state);
>>   	kfree(devfreq->trans_table);
>> @@ -735,10 +800,22 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   	if (err) {
>>   		put_device(&devfreq->dev);
>>   		goto err_out;
>>   	}
>>   
>> +	devfreq->nb_min.notifier_call = 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 = 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",
>> @@ -762,10 +839,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;
>>
> 
> 

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

[Leonard Crestez]

On 31.10.2019 05:10, Chanwoo Choi wrote:
> Hi Leonard,
> 
> This patch didn't get the acked-by from devfreq maintainer.
> I think that we need to discuss this patch with more time.
> Also, it is possible to make it as the separate patch
> from this series.
> 
> IMHO, if you make the separate patch for this and
> resend the separate patch on later, I think that
> we can merge the remained patch related to PM_QOS.

The devfreq initialization cleanups are required for dev_pm_qos support, 
otherwise lockdep warnings are triggered. I can post the cleanups as a 
separate series but the PM QoS one would depend on the cleanups.

Do you prefer multiple smaller series?

I try to order my patches with uncontroversial fixes and cleanups first 
so in theory the earlier parts could be applied separately. It's very 
rare to see series partially applied though.

Earlier objection was that devm should be kept, I think this can be 
accomplished by splitting device_register into device_initialize and 
device_add.

> On 19. 10. 3. 오전 4:25, 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 remove locking the partially initialized
>> object and simplifies the code. However devm is not available before
>> device_register (only after the device_initialize step) so the two
>> allocations need to be managed manually.
>>
>> 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 3e0e936185a3..0b40f40ee7aa 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -591,10 +591,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);
>>   }
>>   
>>   /**
>> @@ -674,44 +676,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);
>> @@ -737,14 +738,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);

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

[Chanwoo Choi]

On 19. 10. 31. 오후 10:31, Leonard Crestez wrote:
> On 31.10.2019 05:10, Chanwoo Choi wrote:
>> Hi Leonard,
>>
>> This patch didn't get the acked-by from devfreq maintainer.
>> I think that we need to discuss this patch with more time.
>> Also, it is possible to make it as the separate patch
>> from this series.
>>
>> IMHO, if you make the separate patch for this and
>> resend the separate patch on later, I think that
>> we can merge the remained patch related to PM_QOS.
> 
> The devfreq initialization cleanups are required for dev_pm_qos support, 
> otherwise lockdep warnings are triggered. I can post the cleanups as a 
> separate series but the PM QoS one would depend on the cleanups.
> 
> Do you prefer multiple smaller series?

After read the v10, I think v9 is better than v10
for this issue. 

> 
> I try to order my patches with uncontroversial fixes and cleanups first 
> so in theory the earlier parts could be applied separately. It's very 
> rare to see series partially applied though.
> 
> Earlier objection was that devm should be kept, I think this can be 
> accomplished by splitting device_register into device_initialize and 
> device_add.
> 
>> On 19. 10. 3. 오전 4:25, 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 remove locking the partially initialized
>>> object and simplifies the code. However devm is not available before
>>> device_register (only after the device_initialize step) so the two
>>> allocations need to be managed manually.
>>>
>>> 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 3e0e936185a3..0b40f40ee7aa 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -591,10 +591,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);
>>>   }
>>>   
>>>   /**
>>> @@ -674,44 +676,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;

err_out -> err_dev
When failed to register, have to free resource.

>>> +	}
>>> +
>>>   	mutex_unlock(&devfreq->lock);
>>>   
>>>   	mutex_lock(&devfreq_list_lock);
>>>   
>>>   	governor = try_then_request_governor(devfreq->governor_name);
>>> @@ -737,14 +738,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);


It is not proper to return on the middle 
of the exception handling. Need to consider more clean method.

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

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

[Leonard Crestez]

On 01.11.2019 10:25, Chanwoo Choi wrote:
> On 19. 10. 31. 오후 10:31, Leonard Crestez wrote:
>> On 31.10.2019 05:10, Chanwoo Choi wrote:
>>> Hi Leonard,
>>>
>>> This patch didn't get the acked-by from devfreq maintainer.
>>> I think that we need to discuss this patch with more time.
>>> Also, it is possible to make it as the separate patch
>>> from this series.
>>>
>>> IMHO, if you make the separate patch for this and
>>> resend the separate patch on later, I think that
>>> we can merge the remained patch related to PM_QOS.
>>
>> The devfreq initialization cleanups are required for dev_pm_qos support,
>> otherwise lockdep warnings are triggered. I can post the cleanups as a
>> separate series but the PM QoS one would depend on the cleanups.
>>
>> Do you prefer multiple smaller series?
> 
> After read the v10, I think v9 is better than v10
> for this issue. >>
>> I try to order my patches with uncontroversial fixes and cleanups first
>> so in theory the earlier parts could be applied separately. It's very
>> rare to see series partially applied though.
>>
>> Earlier objection was that devm should be kept, I think this can be
>> accomplished by splitting device_register into device_initialize and
>> device_add.
>>
>>> On 19. 10. 3. 오전 4:25, 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 remove locking the partially initialized
>>>> object and simplifies the code. However devm is not available before
>>>> device_register (only after the device_initialize step) so the two
>>>> allocations need to be managed manually.
>>>>
>>>> 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 3e0e936185a3..0b40f40ee7aa 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -591,10 +591,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);
>>>>    }
>>>>    
>>>>    /**
>>>> @@ -674,44 +676,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;
> 
> err_out -> err_dev
> When failed to register, have to free resource.

According documentation on device_register resources need to be freed 
via put_device (which calls dev.release = devfreq_dev_release).

This chunk isn't new; it just appears so because other code was moved 
around it.

> 
>>>> +	}
>>>> +
>>>>    	mutex_unlock(&devfreq->lock);
>>>>    
>>>>    	mutex_lock(&devfreq_list_lock);
>>>>    
>>>>    	governor = try_then_request_governor(devfreq->governor_name);
>>>> @@ -737,14 +738,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);
> 
> 
> It is not proper to return on the middle
> of the exception handling. Need to consider more clean method.

Current code already does "devfreq = NULL" in order to skip the kfree 
below. There are already many cleanup/exit paths, it's just not very 
obvious.

Cleanup for errors before and after "device_register" is different and 
this is required by device core.

This can be improved by splitting device_register into device_initialize 
and device_add: this means that all early error paths do put_device and 
there is no longer any need to manually kfree(devfreq) because 
devfreq_dev_release is called on all cleanup paths. It also preserves devm:

https://patchwork.kernel.org/patch/11221873/

However there is still a difference between cleanup before/after 
device_add: if something fails after device_add you need to call 
devfreq_remove_device (which does device_unregister) and otherwise 
device_put.
>>>> +
>>>>    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);