linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC 0/2] PM / devfreq: Add dev_pm_qos support with minimal changes
@ 2019-11-19 16:12 Leonard Crestez
  2019-11-19 16:12 ` [PATCH RFC 1/2] PM / devfreq: Add PM QoS support Leonard Crestez
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Leonard Crestez @ 2019-11-19 16:12 UTC (permalink / raw)
  To: Chanwoo Choi, Matthias Kaehlcke, Rafael J. Wysocki
  Cc: Kyungmin Park, MyungJoo Ham, Artur Świgoń,
	Saravana Kannan, Krzysztof Kozlowski, Alexandre Bailon,
	Georgi Djakov, Jacky Bai, Viresh Kumar, NXP Linux Team, linux-pm,
	linux-arm-kernel

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, this means that calls into dev_pm_qos while holding
devfreq->lock are not allowed (lockdep warns about possible deadlocks).

Fix this by only adding the qos request and notifiers after devfreq->lock is
released inside devfreq_add_device. In theory this means sysfs writes
are possible before the min/max requests are initialized so we guard
against that explictly. The dev_pm_qos_update_request function would
otherwise print a big WARN splat.

Alternatively devfreq initialization could be refactored to avoid taking
devfreq->lock but that requires several intricate changes:

	https://patchwork.kernel.org/cover/11242865/

I considered making dev_pm_qos call notifiers outside the lock but
that's another complex refactoring and it's difficult to ensure
correctness. If two identical qos requests are made in parallel then the
second shouldn't return until all notifiers are completely executed for
the first and QOS is enforced; otherwise it mostly defeats the purpose
of making proactive requests.

This series implements the minimal changes in order to implement dev_pm_qos
support for devfreq. It only costs a little defensive programming.

This series is also marked as [RFC] because it depends on restoring
DEV_PM_QOS_MIN/MAX_FREQUENCY inside the pm core:

	https://patchwork.kernel.org/cover/11250413/

---
Changes since "big version" v10:
* Drop accepted cleanups
* Work with current locking approach (split cleanups into other series)
* Drop acks and deliberately relabel as a new series. It still incorporates
most previous discussion but takes a different approach to locking.
* Don't print errors if devfreq_dev_release is called on error cleanup from
devfreq_add_device, just accept that requests and notifiers might not be
registered yet. I wish dev_pm_qos cleanups behaved like standard "kfree" and
silently did nothing when there's nothing to be done.
Link to v10: https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=196443

Leonard Crestez (2):
  PM / devfreq: Add PM QoS support
  PM / devfreq: Use PM QoS for sysfs min/max_freq

 drivers/devfreq/devfreq.c | 151 ++++++++++++++++++++++++++++++++++----
 include/linux/devfreq.h   |  14 +++-
 2 files changed, 145 insertions(+), 20 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH RFC 1/2] PM / devfreq: Add PM QoS support
  2019-11-19 16:12 [PATCH RFC 0/2] PM / devfreq: Add dev_pm_qos support with minimal changes Leonard Crestez
@ 2019-11-19 16:12 ` Leonard Crestez
  2019-12-02  1:13   ` Chanwoo Choi
  2019-11-19 16:12 ` [PATCH RFC 2/2] PM / devfreq: Use PM QoS for sysfs min/max_freq Leonard Crestez
  2019-12-04 10:46 ` [PATCH RFC 0/2] PM / devfreq: Add dev_pm_qos support with minimal changes Rafael J. Wysocki
  2 siblings, 1 reply; 9+ messages in thread
From: Leonard Crestez @ 2019-11-19 16:12 UTC (permalink / raw)
  To: Chanwoo Choi, Matthias Kaehlcke, Rafael J. Wysocki
  Cc: Kyungmin Park, MyungJoo Ham, Artur Świgoń,
	Saravana Kannan, Krzysztof Kozlowski, Alexandre Bailon,
	Georgi Djakov, Jacky Bai, Viresh Kumar, NXP Linux Team, linux-pm,
	linux-arm-kernel

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 | 77 +++++++++++++++++++++++++++++++++++++++
 include/linux/devfreq.h   |  5 +++
 2 files changed, 82 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 65a4b6cf3fa5..34bbaac6ea7e 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 */
@@ -624,29 +638,80 @@ 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);
 	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);
 
+	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
+					 DEV_PM_QOS_MAX_FREQUENCY);
+	if (err && err != -ENOENT)
+		dev_warn(dev->parent,
+			"Failed to remove max_freq notifier: %d\n", err);
+	err = dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
+					 DEV_PM_QOS_MIN_FREQUENCY);
+	if (err && err != -ENOENT)
+		dev_warn(dev->parent,
+			"Failed to remove min_freq notifier: %d\n", err);
+
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
@@ -764,10 +829,22 @@ struct devfreq *devfreq_add_device(struct device *dev,
 
 	srcu_init_notifier_head(&devfreq->transition_notifier_list);
 
 	mutex_unlock(&devfreq->lock);
 
+	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",
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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH RFC 2/2] PM / devfreq: Use PM QoS for sysfs min/max_freq
  2019-11-19 16:12 [PATCH RFC 0/2] PM / devfreq: Add dev_pm_qos support with minimal changes Leonard Crestez
  2019-11-19 16:12 ` [PATCH RFC 1/2] PM / devfreq: Add PM QoS support Leonard Crestez
@ 2019-11-19 16:12 ` Leonard Crestez
  2019-11-21 23:16   ` Matthias Kaehlcke
  2019-12-02  1:18   ` Chanwoo Choi
  2019-12-04 10:46 ` [PATCH RFC 0/2] PM / devfreq: Add dev_pm_qos support with minimal changes Rafael J. Wysocki
  2 siblings, 2 replies; 9+ messages in thread
From: Leonard Crestez @ 2019-11-19 16:12 UTC (permalink / raw)
  To: Chanwoo Choi, Matthias Kaehlcke, Rafael J. Wysocki
  Cc: Kyungmin Park, MyungJoo Ham, Artur Świgoń,
	Saravana Kannan, Krzysztof Kozlowski, Alexandre Bailon,
	Georgi Djakov, Jacky Bai, Viresh Kumar, NXP Linux Team, linux-pm,
	linux-arm-kernel

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>
---
 drivers/devfreq/devfreq.c | 76 ++++++++++++++++++++++++++++++---------
 include/linux/devfreq.h   |  9 ++---
 2 files changed, 64 insertions(+), 21 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 34bbaac6ea7e..518dd2745d06 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);
 	*max_freq = min(*max_freq, devfreq->scaling_max_freq);
 
 	if (*min_freq > *max_freq)
@@ -708,10 +704,23 @@ static void devfreq_dev_release(struct device *dev)
 					 DEV_PM_QOS_MIN_FREQUENCY);
 	if (err && err != -ENOENT)
 		dev_warn(dev->parent,
 			"Failed to remove min_freq notifier: %d\n", err);
 
+	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req)) {
+		err = dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
+		if (err)
+			dev_warn(dev->parent,
+				"Failed to remove max_freq request: %d\n", err);
+	}
+	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) {
+		err = dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
+		if (err)
+			dev_warn(dev->parent,
+				"Failed to remove min_freq request: %d\n", err);
+	}
+
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
@@ -780,19 +789,17 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	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;
 
 	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
 	atomic_set(&devfreq->suspend_count, 0);
 
 	dev_set_name(&devfreq->dev, "devfreq%d",
@@ -829,10 +836,20 @@ struct devfreq *devfreq_add_device(struct device *dev,
 
 	srcu_init_notifier_head(&devfreq->transition_notifier_list);
 
 	mutex_unlock(&devfreq->lock);
 
+	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (err < 0)
+		goto err_devfreq;
+	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_devfreq;
+
 	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;
@@ -1414,18 +1431,26 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 {
 	struct devfreq *df = to_devfreq(dev);
 	unsigned long value;
 	int ret;
 
+	/*
+	 * Protect against theoretical sysfs writes between
+	 * device_add and dev_pm_qos_add_request
+	 */
+	if (!dev_pm_qos_request_active(&df->user_min_freq_req))
+		return -EINVAL;
+
 	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,
@@ -1446,22 +1471,39 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 {
 	struct devfreq *df = to_devfreq(dev);
 	unsigned long value;
 	int ret;
 
+	/*
+	 * Protect against theoretical sysfs writes between
+	 * device_add and dev_pm_qos_add_request
+	 */
+	if (!dev_pm_qos_request_active(&df->user_max_freq_req))
+		return -EINVAL;
+
 	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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 2/2] PM / devfreq: Use PM QoS for sysfs min/max_freq
  2019-11-19 16:12 ` [PATCH RFC 2/2] PM / devfreq: Use PM QoS for sysfs min/max_freq Leonard Crestez
@ 2019-11-21 23:16   ` Matthias Kaehlcke
  2019-11-25 16:46     ` Leonard Crestez
  2019-12-02  1:18   ` Chanwoo Choi
  1 sibling, 1 reply; 9+ messages in thread
From: Matthias Kaehlcke @ 2019-11-21 23:16 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Chanwoo Choi, Rafael J. Wysocki, Kyungmin Park, MyungJoo Ham,
	Artur Świgoń,
	Saravana Kannan, Krzysztof Kozlowski, Alexandre Bailon,
	Georgi Djakov, Jacky Bai, Viresh Kumar, NXP Linux Team, linux-pm,
	linux-arm-kernel

Hi Leonard,

this seems to work ok as long as there are no other ('external') PM QoS
requests, however the aggregate max_freq can be incorrect when there are
multiple requests.

I intended to test with devfreq_cooling using PM QoS, but this didn't work
out due to limitations on my development platform. Instead I added another
set of requests and sysfs attributes to devfreq (see patch below).

With this we get:

cat available_frequencies
  180000000 267000000 355000000 430000000 565000000 650000000 800000000

# ok
cat max_freq
  800000000

# wtf???
echo 650000000 > max_freq
cat max_freq
  800000000

# this looks better
echo 800000000 > test_max_freq
cat max_freq
  650000000


The problem appears to be:

#define PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE      (-1)

in include/linux/pm_qos.h (added by "PM / QoS: Restore
DEV_PM_QOS_MIN/MAX_FREQUENCY")

The aggregate value returned by
dev_pm_qos_read_value(dev, DEV_PM_QOS_MAX_FREQUENCY) is the
smallest of all requests, which is PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE
unless all requests have set an actual constraint.

You probably want to change the constant to resolve to S32_MAX or some
other big value.

Cheers

Matthias


diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 4e6b3c05b3706f..935098bfadf944 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -659,6 +659,18 @@ static void devfreq_dev_release(struct device *dev)
                        dev_warn(dev->parent,
                                "Failed to remove min_freq request: %d\n", err);
        }
+       if (dev_pm_qos_request_active(&devfreq->test_max_freq_req)) {
+               err = dev_pm_qos_remove_request(&devfreq->test_max_freq_req);
+               if (err)
+                       dev_warn(dev->parent,
+                               "Failed to remove test_max_freq request: %d\n", err);
+       }
+       if (dev_pm_qos_request_active(&devfreq->test_min_freq_req)) {
+               err = dev_pm_qos_remove_request(&devfreq->test_min_freq_req);
+               if (err)
+                       dev_warn(dev->parent,
+                               "Failed to remove test_min_freq request: %d\n", err);
+       }

        if (devfreq->profile->exit)
                devfreq->profile->exit(devfreq->dev.parent);
@@ -738,6 +750,16 @@ struct devfreq *devfreq_add_device(struct device *dev,
        if (err < 0)
                goto err_dev;

+       err = dev_pm_qos_add_request(dev, &devfreq->test_min_freq_req,
+                                    DEV_PM_QOS_MIN_FREQUENCY, 0);
+       if (err < 0)
+               goto err_dev;
+       err = dev_pm_qos_add_request(dev, &devfreq->test_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);
        devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
        if (IS_ERR(devfreq->opp_table))
@@ -1454,6 +1476,101 @@ static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(max_freq);

+static ssize_t test_min_freq_store(struct device *dev, struct device_attribute *attr,
+                             const char *buf, size_t count)
+{
+       struct devfreq *df = to_devfreq(dev);
+       unsigned long value;
+       int ret;
+
+       /*
+        * Protect against theoretical sysfs writes between
+        * device_add and dev_pm_qos_add_request
+        */
+       if (!dev_pm_qos_request_active(&df->test_min_freq_req))
+               return -EINVAL;
+
+       ret = sscanf(buf, "%lu", &value);
+       if (ret != 1)
+               return -EINVAL;
+
+       /* Round down to kHz for PM QoS */
+       ret = dev_pm_qos_update_request(&df->test_min_freq_req,
+                                       value / HZ_PER_KHZ);
+       if (ret < 0)
+               return ret;
+
+       return count;
+}
+
+static ssize_t test_min_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_freq);
+}
+
+static ssize_t test_max_freq_store(struct device *dev, struct device_attribute *attr,
+                             const char *buf, size_t count)
+{
+       struct devfreq *df = to_devfreq(dev);
+       unsigned long value;
+       int ret;
+
+       /*
+        * Protect against theoretical sysfs writes between
+        * device_add and dev_pm_qos_add_request
+        */
+       if (!dev_pm_qos_request_active(&df->test_max_freq_req))
+               return -EINVAL;
+
+       ret = sscanf(buf, "%lu", &value);
+       if (ret != 1)
+               return -EINVAL;
+
+       /*
+        * 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;
+
+       ret = dev_pm_qos_update_request(&df->test_max_freq_req, value);
+       if (ret < 0)
+               return ret;
+
+       return count;
+}
+static DEVICE_ATTR_RW(test_min_freq);
+
+static ssize_t test_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", max_freq);
+}
+static DEVICE_ATTR_RW(test_max_freq);
+
 static ssize_t available_frequencies_show(struct device *d,
                                          struct device_attribute *attr,
                                          char *buf)
@@ -1532,6 +1649,8 @@ static struct attribute *devfreq_attrs[] = {
        &dev_attr_polling_interval.attr,
        &dev_attr_min_freq.attr,
        &dev_attr_max_freq.attr,
+       &dev_attr_test_min_freq.attr,
+       &dev_attr_test_max_freq.attr,
        &dev_attr_trans_stat.attr,
        NULL,
 };
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 6cdd070d59c1f2..8f97c1efaacca5 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -166,6 +166,8 @@ struct devfreq {

        struct dev_pm_qos_request user_min_freq_req;
        struct dev_pm_qos_request user_max_freq_req;
+
+static ssize_t test_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", max_freq);
+}
+static DEVICE_ATTR_RW(test_max_freq);
+
 static ssize_t available_frequencies_show(struct device *d,
                                          struct device_attribute *attr,
                                          char *buf)
@@ -1532,6 +1649,8 @@ static struct attribute *devfreq_attrs[] = {
        &dev_attr_polling_interval.attr,
        &dev_attr_min_freq.attr,
        &dev_attr_max_freq.attr,
+       &dev_attr_test_min_freq.attr,
+       &dev_attr_test_max_freq.attr,
        &dev_attr_trans_stat.attr,
        NULL,
 };
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 6cdd070d59c1f2..8f97c1efaacca5 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -166,6 +166,8 @@ struct devfreq {

        struct dev_pm_qos_request user_min_freq_req;
        struct dev_pm_qos_request user_max_freq_req;
+       struct dev_pm_qos_request test_min_freq_req;
+       struct dev_pm_qos_request test_max_freq_req;
        bool stop_polling;

        unsigned long suspend_freq;


On Tue, Nov 19, 2019 at 06:12:14PM +0200, 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>
> ---
>  drivers/devfreq/devfreq.c | 76 ++++++++++++++++++++++++++++++---------
>  include/linux/devfreq.h   |  9 ++---
>  2 files changed, 64 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 34bbaac6ea7e..518dd2745d06 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);
>  	*max_freq = min(*max_freq, devfreq->scaling_max_freq);
>  
>  	if (*min_freq > *max_freq)
> @@ -708,10 +704,23 @@ static void devfreq_dev_release(struct device *dev)
>  					 DEV_PM_QOS_MIN_FREQUENCY);
>  	if (err && err != -ENOENT)
>  		dev_warn(dev->parent,
>  			"Failed to remove min_freq notifier: %d\n", err);
>  
> +	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req)) {
> +		err = dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> +		if (err)
> +			dev_warn(dev->parent,
> +				"Failed to remove max_freq request: %d\n", err);
> +	}
> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) {
> +		err = dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
> +		if (err)
> +			dev_warn(dev->parent,
> +				"Failed to remove min_freq request: %d\n", err);
> +	}
> +
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
> @@ -780,19 +789,17 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	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;
>  
>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>  	atomic_set(&devfreq->suspend_count, 0);
>  
>  	dev_set_name(&devfreq->dev, "devfreq%d",
> @@ -829,10 +836,20 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  
>  	srcu_init_notifier_head(&devfreq->transition_notifier_list);
>  
>  	mutex_unlock(&devfreq->lock);
>  
> +	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +	if (err < 0)
> +		goto err_devfreq;
> +	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_devfreq;
> +
>  	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;
> @@ -1414,18 +1431,26 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>  {
>  	struct devfreq *df = to_devfreq(dev);
>  	unsigned long value;
>  	int ret;
>  
> +	/*
> +	 * Protect against theoretical sysfs writes between
> +	 * device_add and dev_pm_qos_add_request
> +	 */
> +	if (!dev_pm_qos_request_active(&df->user_min_freq_req))
> +		return -EINVAL;
> +
>  	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,
> @@ -1446,22 +1471,39 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  {
>  	struct devfreq *df = to_devfreq(dev);
>  	unsigned long value;
>  	int ret;
>  
> +	/*
> +	 * Protect against theoretical sysfs writes between
> +	 * device_add and dev_pm_qos_add_request
> +	 */
> +	if (!dev_pm_qos_request_active(&df->user_max_freq_req))
> +		return -EINVAL;
> +
>  	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
> 

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 2/2] PM / devfreq: Use PM QoS for sysfs min/max_freq
  2019-11-21 23:16   ` Matthias Kaehlcke
@ 2019-11-25 16:46     ` Leonard Crestez
  0 siblings, 0 replies; 9+ messages in thread
From: Leonard Crestez @ 2019-11-25 16:46 UTC (permalink / raw)
  To: Matthias Kaehlcke, Rafael J. Wysocki, Chanwoo Choi
  Cc: Kyungmin Park, MyungJoo Ham, Artur Świgoń,
	Saravana Kannan, Krzysztof Kozlowski, Alexandre Bailon,
	Georgi Djakov, Jacky Bai, Viresh Kumar, dl-linux-imx, linux-pm,
	linux-arm-kernel

On 22.11.2019 01:16, Matthias Kaehlcke wrote:
> Hi Leonard,
> 
> this seems to work ok as long as there are no other ('external') PM QoS
> requests, however the aggregate max_freq can be incorrect when there are
> multiple requests.
> 
> I intended to test with devfreq_cooling using PM QoS, but this didn't work
> out due to limitations on my development platform. Instead I added another
> set of requests and sysfs attributes to devfreq (see patch below).

Thanks a lot for looking at this.

> With this we get:
> 
> cat available_frequencies
>    180000000 267000000 355000000 430000000 565000000 650000000 800000000
> 
> # ok
> cat max_freq
>    800000000
> 
> # wtf???
> echo 650000000 > max_freq
> cat max_freq
>    800000000
> 
> # this looks better
> echo 800000000 > test_max_freq
> cat max_freq
>    650000000
> 
> 
> The problem appears to be:
> 
> #define PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE      (-1)
> 
> in include/linux/pm_qos.h (added by "PM / QoS: Restore
> DEV_PM_QOS_MIN/MAX_FREQUENCY")
> 
> The aggregate value returned by
> dev_pm_qos_read_value(dev, DEV_PM_QOS_MAX_FREQUENCY) is the
> smallest of all requests, which is PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE
> unless all requests have set an actual constraint.
> 
> You probably want to change the constant to resolve to S32_MAX or some
> other big value.

Since dev_pm_qos is now layered on top of freq_qos that should be:

#define PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE FREQ_QOS_MAX_DEFAULT_VALUE

I wrote a synthetic test and it seems your issue can be reproduced using 
kunit on top of raw freq_qos. Looking a bit deeper into pm_qos 
implementation it seems that there isn't much special treatment for 
pm_qos_constraints. A reasonable solution would be:

#define FREQ_QOS_MAX_DEFAULT_VALUE	S32_MAX

Cpufreq is not affected because it always sets "real" values but I 
believe that creating requests that should be ignored is a valid approach.

Update on pm/qos here: https://patchwork.kernel.org/cover/11260627/

--
Regards,
Leonard


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 1/2] PM / devfreq: Add PM QoS support
  2019-11-19 16:12 ` [PATCH RFC 1/2] PM / devfreq: Add PM QoS support Leonard Crestez
@ 2019-12-02  1:13   ` Chanwoo Choi
  0 siblings, 0 replies; 9+ messages in thread
From: Chanwoo Choi @ 2019-12-02  1:13 UTC (permalink / raw)
  To: Leonard Crestez, Matthias Kaehlcke, Rafael J. Wysocki
  Cc: Kyungmin Park, MyungJoo Ham, Artur Świgoń,
	Saravana Kannan, Krzysztof Kozlowski, Alexandre Bailon,
	Georgi Djakov, Jacky Bai, Viresh Kumar, NXP Linux Team, linux-pm,
	linux-arm-kernel

On 11/20/19 1:12 AM, 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 | 77 +++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq.h   |  5 +++
>  2 files changed, 82 insertions(+)
> 

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

(snip)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 2/2] PM / devfreq: Use PM QoS for sysfs min/max_freq
  2019-11-19 16:12 ` [PATCH RFC 2/2] PM / devfreq: Use PM QoS for sysfs min/max_freq Leonard Crestez
  2019-11-21 23:16   ` Matthias Kaehlcke
@ 2019-12-02  1:18   ` Chanwoo Choi
  1 sibling, 0 replies; 9+ messages in thread
From: Chanwoo Choi @ 2019-12-02  1:18 UTC (permalink / raw)
  To: Leonard Crestez, Matthias Kaehlcke, Rafael J. Wysocki
  Cc: Kyungmin Park, MyungJoo Ham, Artur Świgoń,
	Saravana Kannan, Krzysztof Kozlowski, Alexandre Bailon,
	Georgi Djakov, Jacky Bai, Viresh Kumar, NXP Linux Team, linux-pm,
	linux-arm-kernel

On 11/20/19 1:12 AM, 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>
> ---
>  drivers/devfreq/devfreq.c | 76 ++++++++++++++++++++++++++++++---------
>  include/linux/devfreq.h   |  9 ++---
>  2 files changed, 64 insertions(+), 21 deletions(-)

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

Thanks.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 0/2] PM / devfreq: Add dev_pm_qos support with minimal changes
  2019-11-19 16:12 [PATCH RFC 0/2] PM / devfreq: Add dev_pm_qos support with minimal changes Leonard Crestez
  2019-11-19 16:12 ` [PATCH RFC 1/2] PM / devfreq: Add PM QoS support Leonard Crestez
  2019-11-19 16:12 ` [PATCH RFC 2/2] PM / devfreq: Use PM QoS for sysfs min/max_freq Leonard Crestez
@ 2019-12-04 10:46 ` Rafael J. Wysocki
  2019-12-05  0:52   ` Chanwoo Choi
  2 siblings, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2019-12-04 10:46 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Chanwoo Choi, Matthias Kaehlcke, Kyungmin Park, MyungJoo Ham,
	Artur Świgoń,
	Saravana Kannan, Krzysztof Kozlowski, Alexandre Bailon,
	Georgi Djakov, Jacky Bai, Viresh Kumar, NXP Linux Team, linux-pm,
	linux-arm-kernel

On Tuesday, November 19, 2019 5:12:12 PM CET 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, this means that calls into dev_pm_qos while holding
> devfreq->lock are not allowed (lockdep warns about possible deadlocks).
> 
> Fix this by only adding the qos request and notifiers after devfreq->lock is
> released inside devfreq_add_device. In theory this means sysfs writes
> are possible before the min/max requests are initialized so we guard
> against that explictly. The dev_pm_qos_update_request function would
> otherwise print a big WARN splat.
> 
> Alternatively devfreq initialization could be refactored to avoid taking
> devfreq->lock but that requires several intricate changes:
> 
> 	https://patchwork.kernel.org/cover/11242865/
> 
> I considered making dev_pm_qos call notifiers outside the lock but
> that's another complex refactoring and it's difficult to ensure
> correctness. If two identical qos requests are made in parallel then the
> second shouldn't return until all notifiers are completely executed for
> the first and QOS is enforced; otherwise it mostly defeats the purpose
> of making proactive requests.
> 
> This series implements the minimal changes in order to implement dev_pm_qos
> support for devfreq. It only costs a little defensive programming.
> 
> This series is also marked as [RFC] because it depends on restoring
> DEV_PM_QOS_MIN/MAX_FREQUENCY inside the pm core:
> 
> 	https://patchwork.kernel.org/cover/11250413/
> 
> ---
> Changes since "big version" v10:
> * Drop accepted cleanups
> * Work with current locking approach (split cleanups into other series)
> * Drop acks and deliberately relabel as a new series. It still incorporates
> most previous discussion but takes a different approach to locking.
> * Don't print errors if devfreq_dev_release is called on error cleanup from
> devfreq_add_device, just accept that requests and notifiers might not be
> registered yet. I wish dev_pm_qos cleanups behaved like standard "kfree" and
> silently did nothing when there's nothing to be done.
> Link to v10: https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=196443
> 
> Leonard Crestez (2):
>   PM / devfreq: Add PM QoS support
>   PM / devfreq: Use PM QoS for sysfs min/max_freq
> 
>  drivers/devfreq/devfreq.c | 151 ++++++++++++++++++++++++++++++++++----
>  include/linux/devfreq.h   |  14 +++-
>  2 files changed, 145 insertions(+), 20 deletions(-)

Please resend this series as non-RFC with the ACKs from Chanwoo included.

It may still be viable to push it for 5.5 during the -rc period.

Thanks!




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RFC 0/2] PM / devfreq: Add dev_pm_qos support with minimal changes
  2019-12-04 10:46 ` [PATCH RFC 0/2] PM / devfreq: Add dev_pm_qos support with minimal changes Rafael J. Wysocki
@ 2019-12-05  0:52   ` Chanwoo Choi
  0 siblings, 0 replies; 9+ messages in thread
From: Chanwoo Choi @ 2019-12-05  0:52 UTC (permalink / raw)
  To: Rafael J. Wysocki, Leonard Crestez
  Cc: Matthias Kaehlcke, Kyungmin Park, MyungJoo Ham,
	Artur Świgoń,
	Saravana Kannan, Krzysztof Kozlowski, Alexandre Bailon,
	Georgi Djakov, Jacky Bai, Viresh Kumar, NXP Linux Team, linux-pm,
	linux-arm-kernel

On 12/4/19 7:46 PM, Rafael J. Wysocki wrote:
> On Tuesday, November 19, 2019 5:12:12 PM CET 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, this means that calls into dev_pm_qos while holding
>> devfreq->lock are not allowed (lockdep warns about possible deadlocks).
>>
>> Fix this by only adding the qos request and notifiers after devfreq->lock is
>> released inside devfreq_add_device. In theory this means sysfs writes
>> are possible before the min/max requests are initialized so we guard
>> against that explictly. The dev_pm_qos_update_request function would
>> otherwise print a big WARN splat.
>>
>> Alternatively devfreq initialization could be refactored to avoid taking
>> devfreq->lock but that requires several intricate changes:
>>
>> 	https://protect2.fireeye.com/url?k=0cd18bf1-51473286-0cd000be-0cc47a312ab0-56d7069e720c53c0&u=https://patchwork.kernel.org/cover/11242865/
>>
>> I considered making dev_pm_qos call notifiers outside the lock but
>> that's another complex refactoring and it's difficult to ensure
>> correctness. If two identical qos requests are made in parallel then the
>> second shouldn't return until all notifiers are completely executed for
>> the first and QOS is enforced; otherwise it mostly defeats the purpose
>> of making proactive requests.
>>
>> This series implements the minimal changes in order to implement dev_pm_qos
>> support for devfreq. It only costs a little defensive programming.
>>
>> This series is also marked as [RFC] because it depends on restoring
>> DEV_PM_QOS_MIN/MAX_FREQUENCY inside the pm core:
>>
>> 	https://protect2.fireeye.com/url?k=304e4bce-6dd8f2b9-304fc081-0cc47a312ab0-ae5dd24df5aed564&u=https://patchwork.kernel.org/cover/11250413/
>>
>> ---
>> Changes since "big version" v10:
>> * Drop accepted cleanups
>> * Work with current locking approach (split cleanups into other series)
>> * Drop acks and deliberately relabel as a new series. It still incorporates
>> most previous discussion but takes a different approach to locking.
>> * Don't print errors if devfreq_dev_release is called on error cleanup from
>> devfreq_add_device, just accept that requests and notifiers might not be
>> registered yet. I wish dev_pm_qos cleanups behaved like standard "kfree" and
>> silently did nothing when there's nothing to be done.
>> Link to v10: https://protect2.fireeye.com/url?k=221a9dd6-7f8c24a1-221b1699-0cc47a312ab0-2fdb90bdac06bfe8&u=https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=196443
>>
>> Leonard Crestez (2):
>>   PM / devfreq: Add PM QoS support
>>   PM / devfreq: Use PM QoS for sysfs min/max_freq
>>
>>  drivers/devfreq/devfreq.c | 151 ++++++++++++++++++++++++++++++++++----
>>  include/linux/devfreq.h   |  14 +++-
>>  2 files changed, 145 insertions(+), 20 deletions(-)
> 
> Please resend this series as non-RFC with the ACKs from Chanwoo included.
> 
> It may still be viable to push it for 5.5 during the -rc period.
> 

If Leonard resend this patch with Ack, I'll send the pull-request for -rc period
with the related minimal patches. Thanks.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-12-05  0:45 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-19 16:12 [PATCH RFC 0/2] PM / devfreq: Add dev_pm_qos support with minimal changes Leonard Crestez
2019-11-19 16:12 ` [PATCH RFC 1/2] PM / devfreq: Add PM QoS support Leonard Crestez
2019-12-02  1:13   ` Chanwoo Choi
2019-11-19 16:12 ` [PATCH RFC 2/2] PM / devfreq: Use PM QoS for sysfs min/max_freq Leonard Crestez
2019-11-21 23:16   ` Matthias Kaehlcke
2019-11-25 16:46     ` Leonard Crestez
2019-12-02  1:18   ` Chanwoo Choi
2019-12-04 10:46 ` [PATCH RFC 0/2] PM / devfreq: Add dev_pm_qos support with minimal changes Rafael J. Wysocki
2019-12-05  0:52   ` Chanwoo Choi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).