[PATCH 0/6] Misc devfreq_add_device() fixes

[PATCH 0/6] Misc devfreq_add_device() fixes

[Bjorn Andersson]

The first 5 patches corrects error handling, reoder things and stop holding the
devfreq mutex in devfreq_add_device(). The last patch validates that a client
doesn't attempt to provide a freq_table, as the implementation relies on the
freq_table being an internal representation of the associated device's opp
table.

Bjorn Andersson (6):
  PM / devfreq: Free devfreq upon set_freq_table error
  PM / devfreq: Use the device release function for cleanup
  PM / devfreq: Reorder devfreq_add_device()
  PM / devfreq: Remove unnecessary locking
  PM / devfreq: Use put_device() on device_register error
  PM / devfreq: Reject client provided freq_table

 drivers/devfreq/devfreq.c | 73 ++++++++++++++++++++---------------------------
 1 file changed, 31 insertions(+), 42 deletions(-)

-- 
2.16.2

[PATCH 1/6] PM / devfreq: Free devfreq upon set_freq_table error

[Bjorn Andersson]

When set_freq_table() fails we must still free the previously allocated
devfreq context, so jump to the correct label for this. Also when this
happens devfreq will always be non-NULL, so drop the unnecessary
conditional.

Also destroy the devfreq mutex as we're cleaning up the devfreq object.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/devfreq/devfreq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2067cd229ce3..30a672397ff0 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -597,7 +597,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
 		mutex_unlock(&devfreq->lock);
 		err = set_freq_table(devfreq);
 		if (err < 0)
-			goto err_out;
+			goto err_dev;
 		mutex_lock(&devfreq->lock);
 	}
 
@@ -669,8 +669,8 @@ struct devfreq *devfreq_add_device(struct device *dev,
 
 	device_unregister(&devfreq->dev);
 err_dev:
-	if (devfreq)
-		kfree(devfreq);
+	mutex_destroy(&devfreq->lock);
+	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
 }
-- 
2.16.2

[PATCH 2/6] PM / devfreq: Use the device release function for cleanup

[Bjorn Andersson]

Removing the devfreq from the devfreq_list before calling unregister
causes the device's release function to not find the devfreq and as such
bails from the release function, resulting in the following log
entries if an invalid governor is specified.

 ufshcd-qcom 624000.ufshc: devfreq_add_device: Unable to find governor for the device
 devfreq devfreq0: releasing devfreq which doesn't exist

Drop the removal of devfreq from the list.

As the release function will end by freeing the devfreq context we have
to skip the freeing of this in our error handler, and hence need to pull
in the device_unregister() calls.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/devfreq/devfreq.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 30a672397ff0..d5b278b8f20e 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -647,8 +647,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	if (IS_ERR(governor)) {
 		dev_err(dev, "%s: Unable to find governor for the device\n",
 			__func__);
+		mutex_unlock(&devfreq_list_lock);
+		device_unregister(&devfreq->dev);
 		err = PTR_ERR(governor);
-		goto err_init;
+		goto err_out;
 	}
 
 	devfreq->governor = governor;
@@ -657,17 +659,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	if (err) {
 		dev_err(dev, "%s: Unable to start governor for the device\n",
 			__func__);
-		goto err_init;
+		mutex_unlock(&devfreq_list_lock);
+		device_unregister(&devfreq->dev);
+		goto err_out;
 	}
 	mutex_unlock(&devfreq_list_lock);
 
 	return devfreq;
 
-err_init:
-	list_del(&devfreq->node);
-	mutex_unlock(&devfreq_list_lock);
-
-	device_unregister(&devfreq->dev);
 err_dev:
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
-- 
2.16.2

[PATCH 3/6] PM / devfreq: Reorder devfreq_add_device()

[Bjorn Andersson]

The devfreq lock is used to ensure that the devfreq object is fully
initialized before e.g. the sysfs interface is accessing it. Moving all
these operations before the device_register() call ensures the same
thing without a lock; as it's yet to be shared outside the current
context.

This allows us to simplify the locking done in devfreq_add_device().

Instead of using devres for trans_table and time_in_state we use the
release function to free them as the devfreq device is released.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/devfreq/devfreq.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index d5b278b8f20e..2e50f5d9d92a 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -539,6 +539,8 @@ static void devfreq_dev_release(struct device *dev)
 		devfreq->profile->exit(devfreq->dev.parent);
 
 	mutex_destroy(&devfreq->lock);
+	kfree(devfreq->time_in_state);
+	kfree(devfreq->trans_table);
 	kfree(devfreq);
 }
 
@@ -617,6 +619,18 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	}
 	devfreq->scaling_max_freq = devfreq->max_freq;
 
+	devfreq->trans_table =	kcalloc(devfreq->profile->max_state *
+					devfreq->profile->max_state,
+					sizeof(unsigned int),
+					GFP_KERNEL);
+	devfreq->time_in_state = kcalloc(devfreq->profile->max_state,
+					 sizeof(unsigned long),
+					 GFP_KERNEL);
+
+	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);
@@ -625,19 +639,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
 		goto err_dev;
 	}
 
-	devfreq->trans_table =	devm_kzalloc(&devfreq->dev,
-						sizeof(unsigned int) *
-						devfreq->profile->max_state *
-						devfreq->profile->max_state,
-						GFP_KERNEL);
-	devfreq->time_in_state = devm_kzalloc(&devfreq->dev,
-						sizeof(unsigned long) *
-						devfreq->profile->max_state,
-						GFP_KERNEL);
-	devfreq->last_stat_updated = jiffies;
-
-	srcu_init_notifier_head(&devfreq->transition_notifier_list);
-
 	mutex_unlock(&devfreq->lock);
 
 	mutex_lock(&devfreq_list_lock);
@@ -669,6 +670,8 @@ struct devfreq *devfreq_add_device(struct device *dev,
 
 err_dev:
 	mutex_destroy(&devfreq->lock);
+	kfree(devfreq->time_in_state);
+	kfree(devfreq->trans_table);
 	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
-- 
2.16.2

[PATCH 4/6] PM / devfreq: Remove unnecessary locking

[Bjorn Andersson]

The devfreq lock is used to prevent concurrent access to the devfreq
object, but as all operations leading up to the registration of the
devfreq device are local to devfreq_add_device() there's no reason to
hold the lock.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/devfreq/devfreq.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2e50f5d9d92a..70588dc2032c 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -584,7 +584,6 @@ 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;
@@ -596,16 +595,13 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	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->min_freq = find_available_min_freq(devfreq);
 	if (!devfreq->min_freq) {
-		mutex_unlock(&devfreq->lock);
 		err = -EINVAL;
 		goto err_dev;
 	}
@@ -613,7 +609,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
 
 	devfreq->max_freq = find_available_max_freq(devfreq);
 	if (!devfreq->max_freq) {
-		mutex_unlock(&devfreq->lock);
 		err = -EINVAL;
 		goto err_dev;
 	}
@@ -635,12 +630,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
 				atomic_inc_return(&devfreq_no));
 	err = device_register(&devfreq->dev);
 	if (err) {
-		mutex_unlock(&devfreq->lock);
 		goto err_dev;
 	}
 
-	mutex_unlock(&devfreq->lock);
-
 	mutex_lock(&devfreq_list_lock);
 	list_add(&devfreq->node, &devfreq_list);
 
-- 
2.16.2

[PATCH 5/6] PM / devfreq: Use put_device() on device_register error

[Bjorn Andersson]

After calling device_register() the dev must be released using
put_device() rather than just freeing the carrying object.

The release function of the devfreq device will be called in this case,
but as we're yet to add the devfreq instance to the devfreq_list the
release function would print a warning and return.

But as the error path of devfreq_add_device() has been cleaned up the
release function is now the only function taking the devfreq off the
list, so we do not need to safe guard against the devfreq not being on
the list.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/devfreq/devfreq.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 70588dc2032c..c804bd72a644 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -523,11 +523,6 @@ 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);
 
@@ -583,6 +578,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
 		goto err_out;
 	}
 
+	INIT_LIST_HEAD(&devfreq->node);
 	mutex_init(&devfreq->lock);
 	devfreq->dev.parent = dev;
 	devfreq->dev.class = devfreq_class;
@@ -630,7 +626,8 @@ struct devfreq *devfreq_add_device(struct device *dev,
 				atomic_inc_return(&devfreq_no));
 	err = device_register(&devfreq->dev);
 	if (err) {
-		goto err_dev;
+		put_device(&devfreq->dev);
+		goto err_out;
 	}
 
 	mutex_lock(&devfreq_list_lock);
-- 
2.16.2

[PATCH 6/6] PM / devfreq: Reject client provided freq_table

[Bjorn Andersson]

The devfreq profile's freq_table is an internal resource used to keep
track of all levels described in the associated opp table, in order to
support levels being enabled and disabled in runtime by e.g.
devfreq_cooling.

As it is required by the implementation that the device has an
associated opp table and expected that the freq_table matches this, it
is not possible for clients to provide a freq_table through the devfreq
profile.

Check for this in devfreq_add_device() and remove the unnecessary
conditional generation of the internal freq_table.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/devfreq/devfreq.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index c804bd72a644..3d1f6a2edf68 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -557,7 +557,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	static atomic_t devfreq_no = ATOMIC_INIT(-1);
 	int err = 0;
 
-	if (!dev || !profile || !governor_name) {
+	if (!dev || !profile || !governor_name || profile->freq_table) {
 		dev_err(dev, "%s: Invalid parameters.\n", __func__);
 		return ERR_PTR(-EINVAL);
 	}
@@ -590,11 +590,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->data = data;
 	devfreq->nb.notifier_call = devfreq_notifier_call;
 
-	if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
-		err = set_freq_table(devfreq);
-		if (err < 0)
-			goto err_dev;
-	}
+	err = set_freq_table(devfreq);
+	if (err < 0)
+		goto err_dev;
 
 	devfreq->min_freq = find_available_min_freq(devfreq);
 	if (!devfreq->min_freq) {
-- 
2.16.2