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

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

[Leonard Crestez]

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

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

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

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

---

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

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

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

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

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

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

Leonard Crestez (6):
  PM / devfreq: Don't fail devfreq_dev_release if not in list
  PM / devfreq: Move more initialization before registration
  PM / devfreq: Don't take lock in devfreq_add_device
  PM / devfreq: Introduce devfreq_get_freq_range
  PM / devfreq: Add PM QoS support
  PM / devfreq: Use PM QoS for sysfs min/max_freq

 drivers/devfreq/devfreq.c | 264 +++++++++++++++++++++++++-------------
 include/linux/devfreq.h   |  14 +-
 2 files changed, 188 insertions(+), 90 deletions(-)

-- 
2.17.1

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

[Leonard Crestez]

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

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

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

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

---

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

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

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

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

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

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

Leonard Crestez (6):
  PM / devfreq: Don't fail devfreq_dev_release if not in list
  PM / devfreq: Move more initialization before registration
  PM / devfreq: Don't take lock in devfreq_add_device
  PM / devfreq: Introduce devfreq_get_freq_range
  PM / devfreq: Add PM QoS support
  PM / devfreq: Use PM QoS for sysfs min/max_freq

 drivers/devfreq/devfreq.c | 264 +++++++++++++++++++++++++-------------
 include/linux/devfreq.h   |  14 +-
 2 files changed, 188 insertions(+), 90 deletions(-)

-- 
2.17.1


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

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

[Leonard Crestez]

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

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

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index eefd481885dd..323d43315d1e 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 v6 1/6] PM / devfreq: Don't fail devfreq_dev_release if not in list

[Leonard Crestez]

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

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

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index eefd481885dd..323d43315d1e 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


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

[PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Leonard Crestez]

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

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

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 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 323d43315d1e..b4d2bfebb140 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -587,10 +587,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);
 }
 
 /**
@@ -670,44 +672,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);
@@ -733,14 +734,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 v6 2/6] PM / devfreq: Move more initialization before registration

[Leonard Crestez]

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

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

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 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 323d43315d1e..b4d2bfebb140 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -587,10 +587,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);
 }
 
 /**
@@ -670,44 +672,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);
@@ -733,14 +734,20 @@ struct devfreq *devfreq_add_device(struct device *dev,
 
 	return devfreq;
 
 err_init:
 	mutex_unlock(&devfreq_list_lock);
-err_devfreq:
 	devfreq_remove_device(devfreq);
-	devfreq = NULL;
+	return ERR_PTR(err);
+
 err_dev:
+	/*
+	 * Cleanup path for errors that happen before registration.
+	 * Otherwise we rely on devfreq_dev_release
+	 */
+	kfree(devfreq->time_in_state);
+	kfree(devfreq->trans_table);
 	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL(devfreq_add_device);
-- 
2.17.1


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

[PATCH v6 3/6] PM / devfreq: Don't take lock in devfreq_add_device

[Leonard Crestez]

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

This simplifies the code and helps avoid recursive lock warnings.

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index b4d2bfebb140..0eee4dd79fbb 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -633,11 +633,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;
@@ -646,28 +645,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;
 
@@ -678,20 +673,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;
@@ -700,17 +693,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 v6 3/6] PM / devfreq: Don't take lock in devfreq_add_device

[Leonard Crestez]

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

This simplifies the code and helps avoid recursive lock warnings.

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index b4d2bfebb140..0eee4dd79fbb 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -633,11 +633,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;
@@ -646,28 +645,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;
 
@@ -678,20 +673,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;
@@ -700,17 +693,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
 
 	dev_set_name(&devfreq->dev, "devfreq%d",
 				atomic_inc_return(&devfreq_no));
 	err = device_register(&devfreq->dev);
 	if (err) {
-		mutex_unlock(&devfreq->lock);
 		put_device(&devfreq->dev);
 		goto err_out;
 	}
 
-	mutex_unlock(&devfreq->lock);
-
 	mutex_lock(&devfreq_list_lock);
 
 	governor = try_then_request_governor(devfreq->governor_name);
 	if (IS_ERR(governor)) {
 		dev_err(dev, "%s: Unable to find governor for the device\n",
-- 
2.17.1


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

[PATCH v6 4/6] PM / devfreq: Introduce devfreq_get_freq_range

[Leonard Crestez]

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

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

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

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

[PATCH v6 4/6] PM / devfreq: Introduce devfreq_get_freq_range

[Leonard Crestez]

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

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

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

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


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

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

[Leonard Crestez]

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

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

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

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

[Leonard Crestez]

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

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

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


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

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

[Leonard Crestez]

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

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

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

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index a4c7dde17a06..4c58fbf7d4e4 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
 	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
 				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
 	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
 				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
 
-	/* constraints from sysfs: */
-	*min_freq = max(*min_freq, devfreq->min_freq);
-	*max_freq = min(*max_freq, devfreq->max_freq);
-
 	/* constraints from OPP interface: */
 	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
 	/* scaling_max_freq can be zero on error */
 	if (devfreq->scaling_max_freq)
 		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
@@ -674,10 +670,12 @@ static void devfreq_dev_release(struct device *dev)
 			DEV_PM_QOS_MIN_FREQUENCY);
 
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
+	dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
+	dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
 }
@@ -742,18 +740,26 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
 	if (!devfreq->scaling_min_freq) {
 		err = -EINVAL;
 		goto err_dev;
 	}
-	devfreq->min_freq = devfreq->scaling_min_freq;
 
 	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
 	if (!devfreq->scaling_max_freq) {
 		err = -EINVAL;
 		goto err_dev;
 	}
-	devfreq->max_freq = devfreq->scaling_max_freq;
+
+	/* PM QoS requests for min/max freq from sysfs */
+	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (err < 0)
+		goto err_dev;
+	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
+				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
+	if (err < 0)
+		goto err_dev;
 
 	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
 	atomic_set(&devfreq->suspend_count, 0);
 
 	devfreq->trans_table = kzalloc(
@@ -837,10 +843,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
 err_dev:
 	/*
 	 * Cleanup path for errors that happen before registration.
 	 * Otherwise we rely on devfreq_dev_release
 	 */
+	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
+		dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
+	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
+		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
@@ -1401,14 +1411,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
 		return -EINVAL;
 
-	mutex_lock(&df->lock);
-	df->min_freq = value;
-	update_devfreq(df);
-	mutex_unlock(&df->lock);
+	/* round down to kHz for dev_pm_qos */
+	if (value)
+		value = value / HZ_PER_KHZ;
+
+	ret = dev_pm_qos_update_request(&df->user_min_freq_req, value);
+	if (ret < 0)
+		return ret;
 
 	return count;
 }
 
 static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
@@ -1433,19 +1446,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
 		return -EINVAL;
 
-	mutex_lock(&df->lock);
-
-	/* Interpret zero as "don't care" */
-	if (!value)
-		value = ULONG_MAX;
+	/* round up to kHz for dev_pm_qos and interpret zero as "don't care" */
+	if (value)
+		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
+	else
+		value = S32_MAX;
 
-	df->max_freq = value;
-	update_devfreq(df);
-	mutex_unlock(&df->lock);
+	ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
+	if (ret < 0)
+		return ret;
 
 	return count;
 }
 static DEVICE_ATTR_RW(min_freq);
 
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index dac0dffeabb4..7849fe4c666d 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -11,10 +11,11 @@
 #define __LINUX_DEVFREQ_H__
 
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/pm_opp.h>
+#include <linux/pm_qos.h>
 
 #define DEVFREQ_NAME_LEN 16
 
 /* DEVFREQ governor name */
 #define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
@@ -121,12 +122,12 @@ struct devfreq_dev_profile {
  *		devfreq.nb to the corresponding register notifier call chain.
  * @work:	delayed work for load monitoring.
  * @previous_freq:	previously configured frequency value.
  * @data:	Private data of the governor. The devfreq framework does not
  *		touch this.
- * @min_freq:	Limit minimum frequency requested by user (0: none)
- * @max_freq:	Limit maximum frequency requested by user (0: none)
+ * @user_min_freq_req:	PM QoS min frequency request from user (via sysfs)
+ * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)
  * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
  * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
  * @stop_polling:	 devfreq polling status of a device.
  * @suspend_freq:	 frequency of a device set during suspend phase.
  * @resume_freq:	 frequency of a device set in resume phase.
@@ -161,12 +162,12 @@ struct devfreq {
 	unsigned long previous_freq;
 	struct devfreq_dev_status last_status;
 
 	void *data; /* private data for governors */
 
-	unsigned long min_freq;
-	unsigned long max_freq;
+	struct dev_pm_qos_request user_min_freq_req;
+	struct dev_pm_qos_request user_max_freq_req;
 	unsigned long scaling_min_freq;
 	unsigned long scaling_max_freq;
 	bool stop_polling;
 
 	unsigned long suspend_freq;
-- 
2.17.1

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

[Leonard Crestez]

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

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

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

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index a4c7dde17a06..4c58fbf7d4e4 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
 	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
 				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
 	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
 				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
 
-	/* constraints from sysfs: */
-	*min_freq = max(*min_freq, devfreq->min_freq);
-	*max_freq = min(*max_freq, devfreq->max_freq);
-
 	/* constraints from OPP interface: */
 	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
 	/* scaling_max_freq can be zero on error */
 	if (devfreq->scaling_max_freq)
 		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
@@ -674,10 +670,12 @@ static void devfreq_dev_release(struct device *dev)
 			DEV_PM_QOS_MIN_FREQUENCY);
 
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
+	dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
+	dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
 }
@@ -742,18 +740,26 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
 	if (!devfreq->scaling_min_freq) {
 		err = -EINVAL;
 		goto err_dev;
 	}
-	devfreq->min_freq = devfreq->scaling_min_freq;
 
 	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
 	if (!devfreq->scaling_max_freq) {
 		err = -EINVAL;
 		goto err_dev;
 	}
-	devfreq->max_freq = devfreq->scaling_max_freq;
+
+	/* PM QoS requests for min/max freq from sysfs */
+	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (err < 0)
+		goto err_dev;
+	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
+				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
+	if (err < 0)
+		goto err_dev;
 
 	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
 	atomic_set(&devfreq->suspend_count, 0);
 
 	devfreq->trans_table = kzalloc(
@@ -837,10 +843,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
 err_dev:
 	/*
 	 * Cleanup path for errors that happen before registration.
 	 * Otherwise we rely on devfreq_dev_release
 	 */
+	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
+		dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
+	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
+		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
 	kfree(devfreq->time_in_state);
 	kfree(devfreq->trans_table);
 	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
@@ -1401,14 +1411,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
 		return -EINVAL;
 
-	mutex_lock(&df->lock);
-	df->min_freq = value;
-	update_devfreq(df);
-	mutex_unlock(&df->lock);
+	/* round down to kHz for dev_pm_qos */
+	if (value)
+		value = value / HZ_PER_KHZ;
+
+	ret = dev_pm_qos_update_request(&df->user_min_freq_req, value);
+	if (ret < 0)
+		return ret;
 
 	return count;
 }
 
 static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
@@ -1433,19 +1446,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
 		return -EINVAL;
 
-	mutex_lock(&df->lock);
-
-	/* Interpret zero as "don't care" */
-	if (!value)
-		value = ULONG_MAX;
+	/* round up to kHz for dev_pm_qos and interpret zero as "don't care" */
+	if (value)
+		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
+	else
+		value = S32_MAX;
 
-	df->max_freq = value;
-	update_devfreq(df);
-	mutex_unlock(&df->lock);
+	ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
+	if (ret < 0)
+		return ret;
 
 	return count;
 }
 static DEVICE_ATTR_RW(min_freq);
 
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index dac0dffeabb4..7849fe4c666d 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -11,10 +11,11 @@
 #define __LINUX_DEVFREQ_H__
 
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/pm_opp.h>
+#include <linux/pm_qos.h>
 
 #define DEVFREQ_NAME_LEN 16
 
 /* DEVFREQ governor name */
 #define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
@@ -121,12 +122,12 @@ struct devfreq_dev_profile {
  *		devfreq.nb to the corresponding register notifier call chain.
  * @work:	delayed work for load monitoring.
  * @previous_freq:	previously configured frequency value.
  * @data:	Private data of the governor. The devfreq framework does not
  *		touch this.
- * @min_freq:	Limit minimum frequency requested by user (0: none)
- * @max_freq:	Limit maximum frequency requested by user (0: none)
+ * @user_min_freq_req:	PM QoS min frequency request from user (via sysfs)
+ * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)
  * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
  * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
  * @stop_polling:	 devfreq polling status of a device.
  * @suspend_freq:	 frequency of a device set during suspend phase.
  * @resume_freq:	 frequency of a device set in resume phase.
@@ -161,12 +162,12 @@ struct devfreq {
 	unsigned long previous_freq;
 	struct devfreq_dev_status last_status;
 
 	void *data; /* private data for governors */
 
-	unsigned long min_freq;
-	unsigned long max_freq;
+	struct dev_pm_qos_request user_min_freq_req;
+	struct dev_pm_qos_request user_max_freq_req;
 	unsigned long scaling_min_freq;
 	unsigned long scaling_max_freq;
 	bool stop_polling;
 
 	unsigned long suspend_freq;
-- 
2.17.1


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

Re: [PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 06:51:05PM +0300, Leonard Crestez wrote:
> In general it is a better to initialize an object before making it
> accessible externally (through device_register).
> 
> This makes it possible to avoid relying on locking a partially
> initialized object.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  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 323d43315d1e..b4d2bfebb140 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -587,10 +587,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);
>  }
>  
>  /**
> @@ -670,44 +672,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;

As per my comment on v5 I think the goto needs to go to 'err_dev'. The
device registration failed, hence devfreq_dev_release() won't be
called to free allocated memory.

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

nit: add a period at the end of the second sentence.

> +	 */
> +	kfree(devfreq->time_in_state);
> +	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL(devfreq_add_device);

Re: [PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 06:51:05PM +0300, Leonard Crestez wrote:
> In general it is a better to initialize an object before making it
> accessible externally (through device_register).
> 
> This makes it possible to avoid relying on locking a partially
> initialized object.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  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 323d43315d1e..b4d2bfebb140 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -587,10 +587,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);
>  }
>  
>  /**
> @@ -670,44 +672,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;

As per my comment on v5 I think the goto needs to go to 'err_dev'. The
device registration failed, hence devfreq_dev_release() won't be
called to free allocated memory.

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

nit: add a period at the end of the second sentence.

> +	 */
> +	kfree(devfreq->time_in_state);
> +	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL(devfreq_add_device);

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

Re: [PATCH v6 4/6] PM / devfreq: Introduce devfreq_get_freq_range

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 06:51:07PM +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 | 111 +++++++++++++++++++++-----------------
>  1 file changed, 63 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 0eee4dd79fbb..b6acb827fee5 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -24,10 +24,12 @@
>  #include <linux/printk.h>
>  #include <linux/hrtimer.h>
>  #include <linux/of.h>
>  #include "governor.h"
>  
> +#define HZ_PER_KHZ 1000
> +
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/devfreq.h>
>  
>  static struct class *devfreq_class;
>  
> @@ -96,10 +98,50 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>  		dev_pm_opp_put(opp);
>  
>  	return max_freq;
>  }
>  
> +/**
> + * devfreq_get_freq_range() - Get the current freq range
> + * @devfreq:	the devfreq instance
> + * @min_freq:	the min frequency
> + * @max_freq:	the max frequency
> + *
> + * This takes into consideration all constraints.
> + */
> +static void devfreq_get_freq_range(struct devfreq *devfreq,
> +				   unsigned long *min_freq,
> +				   unsigned long *max_freq)
> +{
> +	unsigned long *freq_table = devfreq->profile->freq_table;
> +
> +	lockdep_assert_held(&devfreq->lock);
> +
> +	/* Init min/max frequency from freq table */
> +	if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
> +		*min_freq = freq_table[0];
> +		*max_freq = freq_table[devfreq->profile->max_state - 1];
> +	} else {
> +		*min_freq = freq_table[devfreq->profile->max_state - 1];
> +		*max_freq = freq_table[0];
> +	}
> +
> +	/* constraints from sysfs: */

one more nit if you are respinning anyway: remove colon

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

ditto

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

Re: [PATCH v6 4/6] PM / devfreq: Introduce devfreq_get_freq_range

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 06:51:07PM +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 | 111 +++++++++++++++++++++-----------------
>  1 file changed, 63 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 0eee4dd79fbb..b6acb827fee5 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -24,10 +24,12 @@
>  #include <linux/printk.h>
>  #include <linux/hrtimer.h>
>  #include <linux/of.h>
>  #include "governor.h"
>  
> +#define HZ_PER_KHZ 1000
> +
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/devfreq.h>
>  
>  static struct class *devfreq_class;
>  
> @@ -96,10 +98,50 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>  		dev_pm_opp_put(opp);
>  
>  	return max_freq;
>  }
>  
> +/**
> + * devfreq_get_freq_range() - Get the current freq range
> + * @devfreq:	the devfreq instance
> + * @min_freq:	the min frequency
> + * @max_freq:	the max frequency
> + *
> + * This takes into consideration all constraints.
> + */
> +static void devfreq_get_freq_range(struct devfreq *devfreq,
> +				   unsigned long *min_freq,
> +				   unsigned long *max_freq)
> +{
> +	unsigned long *freq_table = devfreq->profile->freq_table;
> +
> +	lockdep_assert_held(&devfreq->lock);
> +
> +	/* Init min/max frequency from freq table */
> +	if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
> +		*min_freq = freq_table[0];
> +		*max_freq = freq_table[devfreq->profile->max_state - 1];
> +	} else {
> +		*min_freq = freq_table[devfreq->profile->max_state - 1];
> +		*max_freq = freq_table[0];
> +	}
> +
> +	/* constraints from sysfs: */

one more nit if you are respinning anyway: remove colon

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

ditto

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

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

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

[Matthias Kaehlcke]

Please see my comments on v5: https://patchwork.kernel.org/patch/11149485/

an additional nit inline

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

nit: remove colon

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

[Matthias Kaehlcke]

Please see my comments on v5: https://patchwork.kernel.org/patch/11149485/

an additional nit inline

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

nit: remove colon

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

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

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 06:51:09PM +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>
> ---
>  drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++--------------
>  include/linux/devfreq.h   |  9 +++----
>  2 files changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index a4c7dde17a06..4c58fbf7d4e4 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
>  	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>  				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
>  	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>  				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
>  
> -	/* constraints from sysfs: */
> -	*min_freq = max(*min_freq, devfreq->min_freq);
> -	*max_freq = min(*max_freq, devfreq->max_freq);
> -
>  	/* constraints from OPP interface: */
>  	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
>  	/* scaling_max_freq can be zero on error */
>  	if (devfreq->scaling_max_freq)
>  		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
> @@ -674,10 +670,12 @@ static void devfreq_dev_release(struct device *dev)
>  			DEV_PM_QOS_MIN_FREQUENCY);
>  
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
> +	dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> +	dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
>  }
> @@ -742,18 +740,26 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>  	if (!devfreq->scaling_min_freq) {
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
> -	devfreq->min_freq = devfreq->scaling_min_freq;
>  
>  	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>  	if (!devfreq->scaling_max_freq) {
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
> -	devfreq->max_freq = devfreq->scaling_max_freq;
> +
> +	/* PM QoS requests for min/max freq from sysfs */
> +	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +	if (err < 0)
> +		goto err_dev;
> +	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
> +				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
> +	if (err < 0)
> +		goto err_dev;
>  
>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>  	atomic_set(&devfreq->suspend_count, 0);
>  
>  	devfreq->trans_table = kzalloc(
> @@ -837,10 +843,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  err_dev:
>  	/*
>  	 * Cleanup path for errors that happen before registration.
>  	 * Otherwise we rely on devfreq_dev_release
>  	 */
> +	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
> @@ -1401,14 +1411,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>  
>  	ret = sscanf(buf, "%lu", &value);
>  	if (ret != 1)
>  		return -EINVAL;
>  
> -	mutex_lock(&df->lock);
> -	df->min_freq = value;
> -	update_devfreq(df);
> -	mutex_unlock(&df->lock);
> +	/* round down to kHz for dev_pm_qos */
> +	if (value)
> +		value = value / HZ_PER_KHZ;
> +
> +	ret = dev_pm_qos_update_request(&df->user_min_freq_req, value);
> +	if (ret < 0)
> +		return ret;
>  
>  	return count;
>  }
>  
>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
> @@ -1433,19 +1446,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  
>  	ret = sscanf(buf, "%lu", &value);
>  	if (ret != 1)
>  		return -EINVAL;
>  
> -	mutex_lock(&df->lock);
> -
> -	/* Interpret zero as "don't care" */
> -	if (!value)
> -		value = ULONG_MAX;
> +	/* round up to kHz for dev_pm_qos and interpret zero as "don't care" */
> +	if (value)
> +		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
> +	else
> +		value = S32_MAX;
>  
> -	df->max_freq = value;
> -	update_devfreq(df);
> -	mutex_unlock(&df->lock);
> +	ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
> +	if (ret < 0)
> +		return ret;
>  
>  	return count;
>  }
>  static DEVICE_ATTR_RW(min_freq);
>  
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index dac0dffeabb4..7849fe4c666d 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -11,10 +11,11 @@
>  #define __LINUX_DEVFREQ_H__
>  
>  #include <linux/device.h>
>  #include <linux/notifier.h>
>  #include <linux/pm_opp.h>
> +#include <linux/pm_qos.h>
>  
>  #define DEVFREQ_NAME_LEN 16
>  
>  /* DEVFREQ governor name */
>  #define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
> @@ -121,12 +122,12 @@ struct devfreq_dev_profile {
>   *		devfreq.nb to the corresponding register notifier call chain.
>   * @work:	delayed work for load monitoring.
>   * @previous_freq:	previously configured frequency value.
>   * @data:	Private data of the governor. The devfreq framework does not
>   *		touch this.
> - * @min_freq:	Limit minimum frequency requested by user (0: none)
> - * @max_freq:	Limit maximum frequency requested by user (0: none)
> + * @user_min_freq_req:	PM QoS min frequency request from user (via sysfs)
> + * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)
>   * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
>   * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
>   * @stop_polling:	 devfreq polling status of a device.
>   * @suspend_freq:	 frequency of a device set during suspend phase.
>   * @resume_freq:	 frequency of a device set in resume phase.
> @@ -161,12 +162,12 @@ struct devfreq {
>  	unsigned long previous_freq;
>  	struct devfreq_dev_status last_status;
>  
>  	void *data; /* private data for governors */
>  
> -	unsigned long min_freq;
> -	unsigned long max_freq;
> +	struct dev_pm_qos_request user_min_freq_req;
> +	struct dev_pm_qos_request user_max_freq_req;
>  	unsigned long scaling_min_freq;
>  	unsigned long scaling_max_freq;
>  	bool stop_polling;
>  
>  	unsigned long suspend_freq;

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

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

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 06:51:09PM +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>
> ---
>  drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++--------------
>  include/linux/devfreq.h   |  9 +++----
>  2 files changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index a4c7dde17a06..4c58fbf7d4e4 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
>  	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>  				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
>  	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>  				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
>  
> -	/* constraints from sysfs: */
> -	*min_freq = max(*min_freq, devfreq->min_freq);
> -	*max_freq = min(*max_freq, devfreq->max_freq);
> -
>  	/* constraints from OPP interface: */
>  	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
>  	/* scaling_max_freq can be zero on error */
>  	if (devfreq->scaling_max_freq)
>  		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
> @@ -674,10 +670,12 @@ static void devfreq_dev_release(struct device *dev)
>  			DEV_PM_QOS_MIN_FREQUENCY);
>  
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
> +	dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> +	dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
>  }
> @@ -742,18 +740,26 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>  	if (!devfreq->scaling_min_freq) {
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
> -	devfreq->min_freq = devfreq->scaling_min_freq;
>  
>  	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>  	if (!devfreq->scaling_max_freq) {
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
> -	devfreq->max_freq = devfreq->scaling_max_freq;
> +
> +	/* PM QoS requests for min/max freq from sysfs */
> +	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +	if (err < 0)
> +		goto err_dev;
> +	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
> +				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
> +	if (err < 0)
> +		goto err_dev;
>  
>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>  	atomic_set(&devfreq->suspend_count, 0);
>  
>  	devfreq->trans_table = kzalloc(
> @@ -837,10 +843,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  err_dev:
>  	/*
>  	 * Cleanup path for errors that happen before registration.
>  	 * Otherwise we rely on devfreq_dev_release
>  	 */
> +	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>  	kfree(devfreq->time_in_state);
>  	kfree(devfreq->trans_table);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
> @@ -1401,14 +1411,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>  
>  	ret = sscanf(buf, "%lu", &value);
>  	if (ret != 1)
>  		return -EINVAL;
>  
> -	mutex_lock(&df->lock);
> -	df->min_freq = value;
> -	update_devfreq(df);
> -	mutex_unlock(&df->lock);
> +	/* round down to kHz for dev_pm_qos */
> +	if (value)
> +		value = value / HZ_PER_KHZ;
> +
> +	ret = dev_pm_qos_update_request(&df->user_min_freq_req, value);
> +	if (ret < 0)
> +		return ret;
>  
>  	return count;
>  }
>  
>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
> @@ -1433,19 +1446,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  
>  	ret = sscanf(buf, "%lu", &value);
>  	if (ret != 1)
>  		return -EINVAL;
>  
> -	mutex_lock(&df->lock);
> -
> -	/* Interpret zero as "don't care" */
> -	if (!value)
> -		value = ULONG_MAX;
> +	/* round up to kHz for dev_pm_qos and interpret zero as "don't care" */
> +	if (value)
> +		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
> +	else
> +		value = S32_MAX;
>  
> -	df->max_freq = value;
> -	update_devfreq(df);
> -	mutex_unlock(&df->lock);
> +	ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
> +	if (ret < 0)
> +		return ret;
>  
>  	return count;
>  }
>  static DEVICE_ATTR_RW(min_freq);
>  
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index dac0dffeabb4..7849fe4c666d 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -11,10 +11,11 @@
>  #define __LINUX_DEVFREQ_H__
>  
>  #include <linux/device.h>
>  #include <linux/notifier.h>
>  #include <linux/pm_opp.h>
> +#include <linux/pm_qos.h>
>  
>  #define DEVFREQ_NAME_LEN 16
>  
>  /* DEVFREQ governor name */
>  #define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
> @@ -121,12 +122,12 @@ struct devfreq_dev_profile {
>   *		devfreq.nb to the corresponding register notifier call chain.
>   * @work:	delayed work for load monitoring.
>   * @previous_freq:	previously configured frequency value.
>   * @data:	Private data of the governor. The devfreq framework does not
>   *		touch this.
> - * @min_freq:	Limit minimum frequency requested by user (0: none)
> - * @max_freq:	Limit maximum frequency requested by user (0: none)
> + * @user_min_freq_req:	PM QoS min frequency request from user (via sysfs)
> + * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)
>   * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
>   * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
>   * @stop_polling:	 devfreq polling status of a device.
>   * @suspend_freq:	 frequency of a device set during suspend phase.
>   * @resume_freq:	 frequency of a device set in resume phase.
> @@ -161,12 +162,12 @@ struct devfreq {
>  	unsigned long previous_freq;
>  	struct devfreq_dev_status last_status;
>  
>  	void *data; /* private data for governors */
>  
> -	unsigned long min_freq;
> -	unsigned long max_freq;
> +	struct dev_pm_qos_request user_min_freq_req;
> +	struct dev_pm_qos_request user_max_freq_req;
>  	unsigned long scaling_min_freq;
>  	unsigned long scaling_max_freq;
>  	bool stop_polling;
>  
>  	unsigned long suspend_freq;

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

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

Re: [PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Leonard Crestez]

On 23.09.2019 21:11, Matthias Kaehlcke wrote:
> On Mon, Sep 23, 2019 at 06:51:05PM +0300, Leonard Crestez wrote:
>> In general it is a better to initialize an object before making it
>> accessible externally (through device_register).
>>
>> This makes it possible to avoid relying on locking a partially
>> initialized object.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   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 323d43315d1e..b4d2bfebb140 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -587,10 +587,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);
>>   }
>>   
>>   /**
>> @@ -670,44 +672,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;
> 
> As per my comment on v5 I think the goto needs to go to 'err_dev'. The
> device registration failed, hence devfreq_dev_release() won't be
> called to free allocated memory.

This code is not modified in the patch, it only shows up as +added 
because diff got confused but there is an identical -removed chunk 
higher up.

The device_register documentation mentions the following:

  * NOTE: _Never_ directly free @dev after calling this function, even
  * if it returned an error! Always use put_device() to give up the
  * reference initialized in this function instead.

Cleanup path then goes like this (from a hacked error in device_add):
  dump_stack+0xdc/0x144 
 

  devfreq_dev_release+0x38/0xc0 
 

  device_release+0x34/0x90 
 

  kobject_put+0x8c/0x1f0 
 

  put_device+0x24/0x30 
 

  devfreq_add_device+0x540/0x570 
 

  devm_devfreq_add_device+0x60/0xd0 
 

  imx_ddrc_probe+0x35c/0x4c8

Can I add your "Reviewed-By" for the rest of the series if I fix the nits?

--
Regards,
Leonard

Re: [PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Leonard Crestez]

On 23.09.2019 21:11, Matthias Kaehlcke wrote:
> On Mon, Sep 23, 2019 at 06:51:05PM +0300, Leonard Crestez wrote:
>> In general it is a better to initialize an object before making it
>> accessible externally (through device_register).
>>
>> This makes it possible to avoid relying on locking a partially
>> initialized object.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   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 323d43315d1e..b4d2bfebb140 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -587,10 +587,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);
>>   }
>>   
>>   /**
>> @@ -670,44 +672,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;
> 
> As per my comment on v5 I think the goto needs to go to 'err_dev'. The
> device registration failed, hence devfreq_dev_release() won't be
> called to free allocated memory.

This code is not modified in the patch, it only shows up as +added 
because diff got confused but there is an identical -removed chunk 
higher up.

The device_register documentation mentions the following:

  * NOTE: _Never_ directly free @dev after calling this function, even
  * if it returned an error! Always use put_device() to give up the
  * reference initialized in this function instead.

Cleanup path then goes like this (from a hacked error in device_add):
  dump_stack+0xdc/0x144 
 

  devfreq_dev_release+0x38/0xc0 
 

  device_release+0x34/0x90 
 

  kobject_put+0x8c/0x1f0 
 

  put_device+0x24/0x30 
 

  devfreq_add_device+0x540/0x570 
 

  devm_devfreq_add_device+0x60/0xd0 
 

  imx_ddrc_probe+0x35c/0x4c8

Can I add your "Reviewed-By" for the rest of the series if I fix the nits?

--
Regards,
Leonard

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

Re: [PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 06:56:28PM +0000, Leonard Crestez wrote:
> On 23.09.2019 21:11, Matthias Kaehlcke wrote:
> > On Mon, Sep 23, 2019 at 06:51:05PM +0300, Leonard Crestez wrote:
> >> In general it is a better to initialize an object before making it
> >> accessible externally (through device_register).
> >>
> >> This makes it possible to avoid relying on locking a partially
> >> initialized object.
> >>
> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >> ---
> >>   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 323d43315d1e..b4d2bfebb140 100644
> >> --- a/drivers/devfreq/devfreq.c
> >> +++ b/drivers/devfreq/devfreq.c
> >> @@ -587,10 +587,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);
> >>   }
> >>   
> >>   /**
> >> @@ -670,44 +672,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;
> > 
> > As per my comment on v5 I think the goto needs to go to 'err_dev'. The
> > device registration failed, hence devfreq_dev_release() won't be
> > called to free allocated memory.
> 
> This code is not modified in the patch, it only shows up as +added 
> because diff got confused but there is an identical -removed chunk 
> higher up.
> 
> The device_register documentation mentions the following:
> 
>   * NOTE: _Never_ directly free @dev after calling this function, even
>   * if it returned an error! Always use put_device() to give up the
>   * reference initialized in this function instead.
> 
> Cleanup path then goes like this (from a hacked error in device_add):
>   dump_stack+0xdc/0x144 
>  
> 
>   devfreq_dev_release+0x38/0xc0 
>  
> 
>   device_release+0x34/0x90 
>  
> 
>   kobject_put+0x8c/0x1f0 
>  
> 
>   put_device+0x24/0x30 
>  
> 
>   devfreq_add_device+0x540/0x570 
>  
> 
>   devm_devfreq_add_device+0x60/0xd0 
>  
> 
>   imx_ddrc_probe+0x35c/0x4c8

Good to know, thanks for the pointer!

> Can I add your "Reviewed-By" for the rest of the series if I fix the nits?

By now you should have it for most patches. For this one:

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

There is one doubt I have left on "PM / devfreq: Add PM QoS support" that I
posted on v5:

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

Could you clarify this replying to the thread? Besides that and the
nits (which are optional to fix) the patch looks good to me.

Re: [PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 06:56:28PM +0000, Leonard Crestez wrote:
> On 23.09.2019 21:11, Matthias Kaehlcke wrote:
> > On Mon, Sep 23, 2019 at 06:51:05PM +0300, Leonard Crestez wrote:
> >> In general it is a better to initialize an object before making it
> >> accessible externally (through device_register).
> >>
> >> This makes it possible to avoid relying on locking a partially
> >> initialized object.
> >>
> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >> ---
> >>   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 323d43315d1e..b4d2bfebb140 100644
> >> --- a/drivers/devfreq/devfreq.c
> >> +++ b/drivers/devfreq/devfreq.c
> >> @@ -587,10 +587,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);
> >>   }
> >>   
> >>   /**
> >> @@ -670,44 +672,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;
> > 
> > As per my comment on v5 I think the goto needs to go to 'err_dev'. The
> > device registration failed, hence devfreq_dev_release() won't be
> > called to free allocated memory.
> 
> This code is not modified in the patch, it only shows up as +added 
> because diff got confused but there is an identical -removed chunk 
> higher up.
> 
> The device_register documentation mentions the following:
> 
>   * NOTE: _Never_ directly free @dev after calling this function, even
>   * if it returned an error! Always use put_device() to give up the
>   * reference initialized in this function instead.
> 
> Cleanup path then goes like this (from a hacked error in device_add):
>   dump_stack+0xdc/0x144 
>  
> 
>   devfreq_dev_release+0x38/0xc0 
>  
> 
>   device_release+0x34/0x90 
>  
> 
>   kobject_put+0x8c/0x1f0 
>  
> 
>   put_device+0x24/0x30 
>  
> 
>   devfreq_add_device+0x540/0x570 
>  
> 
>   devm_devfreq_add_device+0x60/0xd0 
>  
> 
>   imx_ddrc_probe+0x35c/0x4c8

Good to know, thanks for the pointer!

> Can I add your "Reviewed-By" for the rest of the series if I fix the nits?

By now you should have it for most patches. For this one:

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

There is one doubt I have left on "PM / devfreq: Add PM QoS support" that I
posted on v5:

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

Could you clarify this replying to the thread? Besides that and the
nits (which are optional to fix) the patch looks good to me.




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

Re: [PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Leonard Crestez]

On 23.09.2019 22:11, Matthias Kaehlcke wrote:
> On Mon, Sep 23, 2019 at 06:56:28PM +0000, Leonard Crestez wrote:
>> On 23.09.2019 21:11, Matthias Kaehlcke wrote:
>>> On Mon, Sep 23, 2019 at 06:51:05PM +0300, Leonard Crestez wrote:
>>>> In general it is a better to initialize an object before making it
>>>> accessible externally (through device_register).
>>>>
>>>> This makes it possible to avoid relying on locking a partially
>>>> initialized object.
>>>>
>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>> ---
>>>>    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 323d43315d1e..b4d2bfebb140 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -587,10 +587,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);
>>>>    }
>>>>    
>>>>    /**
>>>> @@ -670,44 +672,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;
>>>
>>> As per my comment on v5 I think the goto needs to go to 'err_dev'. The
>>> device registration failed, hence devfreq_dev_release() won't be
>>> called to free allocated memory.
>>
>> This code is not modified in the patch, it only shows up as +added
>> because diff got confused but there is an identical -removed chunk
>> higher up.
>>
>> The device_register documentation mentions the following:
>>
>>    * NOTE: _Never_ directly free @dev after calling this function, even
>>    * if it returned an error! Always use put_device() to give up the
>>    * reference initialized in this function instead.
>>
>> Cleanup path then goes like this (from a hacked error in device_add):
>>    dump_stack+0xdc/0x144
>>   
>>
>>    devfreq_dev_release+0x38/0xc0
>>   
>>
>>    device_release+0x34/0x90
>>   
>>
>>    kobject_put+0x8c/0x1f0
>>   
>>
>>    put_device+0x24/0x30
>>   
>>
>>    devfreq_add_device+0x540/0x570
>>   
>>
>>    devm_devfreq_add_device+0x60/0xd0
>>   
>>
>>    imx_ddrc_probe+0x35c/0x4c8
> 
> Good to know, thanks for the pointer!
> 
>> Can I add your "Reviewed-By" for the rest of the series if I fix the nits?
> 
> By now you should have it for most patches. For this one:
> 
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> 
> There is one doubt I have left on "PM / devfreq: Add PM QoS support" that I
> posted on v5:
> 
> "IIUC you rely on the notifiers being removed by devfreq_dev_release().
> Does dev_pm_qos_remove_notifier() behave gracefully if the notifier is
> not initialized/added or do we need to use BLOCKING_NOTIFIER_INIT() or
> similar?"

Sorry for missing that.
> Could you clarify this replying to the thread? Besides that and the
> nits (which are optional to fix) the patch looks good to me.

The blocking_notifier_head structs are managed by PM QoS inside 
dev_pm_qos_constraints_allocate and dev_pm_qos_constraints_destroy. The 
devfreq subsystem only registers a notifier_block, that's a 
NULL-terminated singly-linked list for which zero-initialization from 
kzalloc should be sufficient.

But now that I look at this again I should warn and return NOTIFY_DONE 
from devfreq_qos_notifier_call instead of propagating a negative errno.

--
Regards,
Leonard

Re: [PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Leonard Crestez]

On 23.09.2019 22:11, Matthias Kaehlcke wrote:
> On Mon, Sep 23, 2019 at 06:56:28PM +0000, Leonard Crestez wrote:
>> On 23.09.2019 21:11, Matthias Kaehlcke wrote:
>>> On Mon, Sep 23, 2019 at 06:51:05PM +0300, Leonard Crestez wrote:
>>>> In general it is a better to initialize an object before making it
>>>> accessible externally (through device_register).
>>>>
>>>> This makes it possible to avoid relying on locking a partially
>>>> initialized object.
>>>>
>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>> ---
>>>>    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 323d43315d1e..b4d2bfebb140 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -587,10 +587,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);
>>>>    }
>>>>    
>>>>    /**
>>>> @@ -670,44 +672,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;
>>>
>>> As per my comment on v5 I think the goto needs to go to 'err_dev'. The
>>> device registration failed, hence devfreq_dev_release() won't be
>>> called to free allocated memory.
>>
>> This code is not modified in the patch, it only shows up as +added
>> because diff got confused but there is an identical -removed chunk
>> higher up.
>>
>> The device_register documentation mentions the following:
>>
>>    * NOTE: _Never_ directly free @dev after calling this function, even
>>    * if it returned an error! Always use put_device() to give up the
>>    * reference initialized in this function instead.
>>
>> Cleanup path then goes like this (from a hacked error in device_add):
>>    dump_stack+0xdc/0x144
>>   
>>
>>    devfreq_dev_release+0x38/0xc0
>>   
>>
>>    device_release+0x34/0x90
>>   
>>
>>    kobject_put+0x8c/0x1f0
>>   
>>
>>    put_device+0x24/0x30
>>   
>>
>>    devfreq_add_device+0x540/0x570
>>   
>>
>>    devm_devfreq_add_device+0x60/0xd0
>>   
>>
>>    imx_ddrc_probe+0x35c/0x4c8
> 
> Good to know, thanks for the pointer!
> 
>> Can I add your "Reviewed-By" for the rest of the series if I fix the nits?
> 
> By now you should have it for most patches. For this one:
> 
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> 
> There is one doubt I have left on "PM / devfreq: Add PM QoS support" that I
> posted on v5:
> 
> "IIUC you rely on the notifiers being removed by devfreq_dev_release().
> Does dev_pm_qos_remove_notifier() behave gracefully if the notifier is
> not initialized/added or do we need to use BLOCKING_NOTIFIER_INIT() or
> similar?"

Sorry for missing that.
> Could you clarify this replying to the thread? Besides that and the
> nits (which are optional to fix) the patch looks good to me.

The blocking_notifier_head structs are managed by PM QoS inside 
dev_pm_qos_constraints_allocate and dev_pm_qos_constraints_destroy. The 
devfreq subsystem only registers a notifier_block, that's a 
NULL-terminated singly-linked list for which zero-initialization from 
kzalloc should be sufficient.

But now that I look at this again I should warn and return NOTIFY_DONE 
from devfreq_qos_notifier_call instead of propagating a negative errno.

--
Regards,
Leonard

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

Re: [PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 07:56:51PM +0000, Leonard Crestez wrote:
> On 23.09.2019 22:11, Matthias Kaehlcke wrote:
> > On Mon, Sep 23, 2019 at 06:56:28PM +0000, Leonard Crestez wrote:
> >> On 23.09.2019 21:11, Matthias Kaehlcke wrote:
> >>> On Mon, Sep 23, 2019 at 06:51:05PM +0300, Leonard Crestez wrote:
> >>>> In general it is a better to initialize an object before making it
> >>>> accessible externally (through device_register).
> >>>>
> >>>> This makes it possible to avoid relying on locking a partially
> >>>> initialized object.
> >>>>
> >>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >>>> ---
> >>>>    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 323d43315d1e..b4d2bfebb140 100644
> >>>> --- a/drivers/devfreq/devfreq.c
> >>>> +++ b/drivers/devfreq/devfreq.c
> >>>> @@ -587,10 +587,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);
> >>>>    }
> >>>>    
> >>>>    /**
> >>>> @@ -670,44 +672,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;
> >>>
> >>> As per my comment on v5 I think the goto needs to go to 'err_dev'. The
> >>> device registration failed, hence devfreq_dev_release() won't be
> >>> called to free allocated memory.
> >>
> >> This code is not modified in the patch, it only shows up as +added
> >> because diff got confused but there is an identical -removed chunk
> >> higher up.
> >>
> >> The device_register documentation mentions the following:
> >>
> >>    * NOTE: _Never_ directly free @dev after calling this function, even
> >>    * if it returned an error! Always use put_device() to give up the
> >>    * reference initialized in this function instead.
> >>
> >> Cleanup path then goes like this (from a hacked error in device_add):
> >>    dump_stack+0xdc/0x144
> >>   
> >>
> >>    devfreq_dev_release+0x38/0xc0
> >>   
> >>
> >>    device_release+0x34/0x90
> >>   
> >>
> >>    kobject_put+0x8c/0x1f0
> >>   
> >>
> >>    put_device+0x24/0x30
> >>   
> >>
> >>    devfreq_add_device+0x540/0x570
> >>   
> >>
> >>    devm_devfreq_add_device+0x60/0xd0
> >>   
> >>
> >>    imx_ddrc_probe+0x35c/0x4c8
> > 
> > Good to know, thanks for the pointer!
> > 
> >> Can I add your "Reviewed-By" for the rest of the series if I fix the nits?
> > 
> > By now you should have it for most patches. For this one:
> > 
> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > 
> > There is one doubt I have left on "PM / devfreq: Add PM QoS support" that I
> > posted on v5:
> > 
> > "IIUC you rely on the notifiers being removed by devfreq_dev_release().
> > Does dev_pm_qos_remove_notifier() behave gracefully if the notifier is
> > not initialized/added or do we need to use BLOCKING_NOTIFIER_INIT() or
> > similar?"
> 
> Sorry for missing that.
> > Could you clarify this replying to the thread? Besides that and the
> > nits (which are optional to fix) the patch looks good to me.
> 
> The blocking_notifier_head structs are managed by PM QoS inside 
> dev_pm_qos_constraints_allocate and dev_pm_qos_constraints_destroy. The 
> devfreq subsystem only registers a notifier_block, that's a 
> NULL-terminated singly-linked list for which zero-initialization from 
> kzalloc should be sufficient.

Right, thanks for the clarification!

> But now that I look at this again I should warn and return NOTIFY_DONE 
> from devfreq_qos_notifier_call instead of propagating a negative errno.

Ack

Re: [PATCH v6 2/6] PM / devfreq: Move more initialization before registration

[Matthias Kaehlcke]

On Mon, Sep 23, 2019 at 07:56:51PM +0000, Leonard Crestez wrote:
> On 23.09.2019 22:11, Matthias Kaehlcke wrote:
> > On Mon, Sep 23, 2019 at 06:56:28PM +0000, Leonard Crestez wrote:
> >> On 23.09.2019 21:11, Matthias Kaehlcke wrote:
> >>> On Mon, Sep 23, 2019 at 06:51:05PM +0300, Leonard Crestez wrote:
> >>>> In general it is a better to initialize an object before making it
> >>>> accessible externally (through device_register).
> >>>>
> >>>> This makes it possible to avoid relying on locking a partially
> >>>> initialized object.
> >>>>
> >>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >>>> ---
> >>>>    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 323d43315d1e..b4d2bfebb140 100644
> >>>> --- a/drivers/devfreq/devfreq.c
> >>>> +++ b/drivers/devfreq/devfreq.c
> >>>> @@ -587,10 +587,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);
> >>>>    }
> >>>>    
> >>>>    /**
> >>>> @@ -670,44 +672,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;
> >>>
> >>> As per my comment on v5 I think the goto needs to go to 'err_dev'. The
> >>> device registration failed, hence devfreq_dev_release() won't be
> >>> called to free allocated memory.
> >>
> >> This code is not modified in the patch, it only shows up as +added
> >> because diff got confused but there is an identical -removed chunk
> >> higher up.
> >>
> >> The device_register documentation mentions the following:
> >>
> >>    * NOTE: _Never_ directly free @dev after calling this function, even
> >>    * if it returned an error! Always use put_device() to give up the
> >>    * reference initialized in this function instead.
> >>
> >> Cleanup path then goes like this (from a hacked error in device_add):
> >>    dump_stack+0xdc/0x144
> >>   
> >>
> >>    devfreq_dev_release+0x38/0xc0
> >>   
> >>
> >>    device_release+0x34/0x90
> >>   
> >>
> >>    kobject_put+0x8c/0x1f0
> >>   
> >>
> >>    put_device+0x24/0x30
> >>   
> >>
> >>    devfreq_add_device+0x540/0x570
> >>   
> >>
> >>    devm_devfreq_add_device+0x60/0xd0
> >>   
> >>
> >>    imx_ddrc_probe+0x35c/0x4c8
> > 
> > Good to know, thanks for the pointer!
> > 
> >> Can I add your "Reviewed-By" for the rest of the series if I fix the nits?
> > 
> > By now you should have it for most patches. For this one:
> > 
> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > 
> > There is one doubt I have left on "PM / devfreq: Add PM QoS support" that I
> > posted on v5:
> > 
> > "IIUC you rely on the notifiers being removed by devfreq_dev_release().
> > Does dev_pm_qos_remove_notifier() behave gracefully if the notifier is
> > not initialized/added or do we need to use BLOCKING_NOTIFIER_INIT() or
> > similar?"
> 
> Sorry for missing that.
> > Could you clarify this replying to the thread? Besides that and the
> > nits (which are optional to fix) the patch looks good to me.
> 
> The blocking_notifier_head structs are managed by PM QoS inside 
> dev_pm_qos_constraints_allocate and dev_pm_qos_constraints_destroy. The 
> devfreq subsystem only registers a notifier_block, that's a 
> NULL-terminated singly-linked list for which zero-initialization from 
> kzalloc should be sufficient.

Right, thanks for the clarification!

> But now that I look at this again I should warn and return NOTIFY_DONE 
> from devfreq_qos_notifier_call instead of propagating a negative errno.

Ack

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