[PATCH] cpufreq: Fix possible memory leak during CPU removal

[PATCH] cpufreq: Fix possible memory leak during CPU removal

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit 9b07109f06a1 (cpufreq: Fix double addition of sysfs
links) the second sif argument of __cpufreq_remove_dev_prepare()
and __cpufreq_remove_dev_finish() is not used by them any more.

However, there also is a problem in cpufreq_remove_dev() that if
any of the above functions returns an error, we'll fail to
clean up after a CPU that is going away (and it is going away
no matter what).

For this reason, replace the sif argument of __cpufreq_remove_dev_prepare()
and __cpufreq_remove_dev_finish() with a bool force argument that
will tell each of them to avoid aborting in case of a governor
callback failure and do the cleanup regardless.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

On top of:
http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=9b07109f06a1edd6e636b1e7397157eae0e6baa4

Note: The above commit is on a testing branch only at the moment.

---
 drivers/cpufreq/cpufreq.c |   23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1376,8 +1376,7 @@ out_release_rwsem:
 	return ret;
 }
 
-static int __cpufreq_remove_dev_prepare(struct device *dev,
-					struct subsys_interface *sif)
+static int __cpufreq_remove_dev_prepare(struct device *dev, bool force)
 {
 	unsigned int cpu = dev->id;
 	int ret = 0;
@@ -1395,7 +1394,8 @@ static int __cpufreq_remove_dev_prepare(
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 		if (ret) {
 			pr_err("%s: Failed to stop governor\n", __func__);
-			return ret;
+			if (!force)
+				return ret;
 		}
 	}
 
@@ -1429,8 +1429,7 @@ static int __cpufreq_remove_dev_prepare(
 	return ret;
 }
 
-static int __cpufreq_remove_dev_finish(struct device *dev,
-				       struct subsys_interface *sif)
+static int __cpufreq_remove_dev_finish(struct device *dev, bool force)
 {
 	unsigned int cpu = dev->id;
 	int ret;
@@ -1450,7 +1449,8 @@ static int __cpufreq_remove_dev_finish(s
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
 		if (ret) {
 			pr_err("%s: Failed to exit governor\n", __func__);
-			return ret;
+			if (!force)
+				return ret;
 		}
 	}
 
@@ -1480,11 +1480,8 @@ static int cpufreq_remove_dev(struct dev
 		return 0;
 
 	if (cpu_online(cpu)) {
-		ret = __cpufreq_remove_dev_prepare(dev, sif);
-		if (!ret)
-			ret = __cpufreq_remove_dev_finish(dev, sif);
-		if (ret)
-			return ret;
+		__cpufreq_remove_dev_prepare(dev, true);
+		__cpufreq_remove_dev_finish(dev, true);
 	}
 
 	/* sysfs links are removed only on subsys callback */
@@ -2376,11 +2373,11 @@ static int cpufreq_cpu_callback(struct n
 			break;
 
 		case CPU_DOWN_PREPARE:
-			__cpufreq_remove_dev_prepare(dev, NULL);
+			__cpufreq_remove_dev_prepare(dev, false);
 			break;
 
 		case CPU_POST_DEAD:
-			__cpufreq_remove_dev_finish(dev, NULL);
+			__cpufreq_remove_dev_finish(dev, false);
 			break;
 
 		case CPU_DOWN_FAILED:

[PATCH v2] cpufreq: Fix possible memory leak during CPU removal

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit 9b07109f06a1 (cpufreq: Fix double addition of sysfs
links) the second sif argument of __cpufreq_remove_dev_prepare()
and __cpufreq_remove_dev_finish() is not used by them any more.

However, there also is a problem in cpufreq_remove_dev() that
if any of the above functions returns an error, we'll fail to
clean up after a CPU that is going away (and it is going away
no matter what).  Moreover, error codes returned by them are
ignored by cpufreq_cpu_callback(), so even if any of them is
aborted and returns an error code, the caller of the notifier
callback will not know about that.

For this reason, make __cpufreq_remove_dev_prepare() and
__cpufreq_remove_dev_finish() never fail, change them to void
functions and drop the sif argument from them.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

On top of:
http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=9b07109f06a1edd6e636b1e7397157eae0e6baa4

Note: The commit mentioned above is on a testing branch only
at the moment.

Well, I seem to be blind.

v1 -> v2: *Notice* that the return values of the functions in question
are not checked by cpufreq_cpu_callback(), so they actually don't need to
return anything and therefore they may never fail.  Take that into account
and (1) make them void, (2) make them never abort and (3) really drop the
second arg from them (it isn't needed at all).

---
 drivers/cpufreq/cpufreq.c |   44 ++++++++++++++------------------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1376,11 +1376,9 @@ out_release_rwsem:
 	return ret;
 }
 
-static int __cpufreq_remove_dev_prepare(struct device *dev,
-					struct subsys_interface *sif)
+static void __cpufreq_remove_dev_prepare(struct device *dev)
 {
 	unsigned int cpu = dev->id;
-	int ret = 0;
 	struct cpufreq_policy *policy;
 
 	pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
@@ -1388,15 +1386,13 @@ static int __cpufreq_remove_dev_prepare(
 	policy = cpufreq_cpu_get_raw(cpu);
 	if (!policy) {
 		pr_debug("%s: No cpu_data found\n", __func__);
-		return -EINVAL;
+		return;
 	}
 
 	if (has_target()) {
-		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
-		if (ret) {
+		int ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
+		if (ret)
 			pr_err("%s: Failed to stop governor\n", __func__);
-			return ret;
-		}
 	}
 
 	down_write(&policy->rwsem);
@@ -1415,7 +1411,7 @@ static int __cpufreq_remove_dev_prepare(
 	/* Start governor again for active policy */
 	if (!policy_is_inactive(policy)) {
 		if (has_target()) {
-			ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
+			int ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
 			if (!ret)
 				ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 
@@ -1425,33 +1421,27 @@ static int __cpufreq_remove_dev_prepare(
 	} else if (cpufreq_driver->stop_cpu) {
 		cpufreq_driver->stop_cpu(policy);
 	}
-
-	return ret;
 }
 
-static int __cpufreq_remove_dev_finish(struct device *dev,
-				       struct subsys_interface *sif)
+static void __cpufreq_remove_dev_finish(struct device *dev)
 {
 	unsigned int cpu = dev->id;
-	int ret;
 	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 
 	if (!policy) {
 		pr_debug("%s: No cpu_data found\n", __func__);
-		return -EINVAL;
+		return;
 	}
 
 	/* Only proceed for inactive policies */
 	if (!policy_is_inactive(policy))
-		return 0;
+		return;
 
 	/* If cpu is last user of policy, free policy */
 	if (has_target()) {
-		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-		if (ret) {
+		int ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
+		if (ret)
 			pr_err("%s: Failed to exit governor\n", __func__);
-			return ret;
-		}
 	}
 
 	/*
@@ -1461,8 +1451,6 @@ static int __cpufreq_remove_dev_finish(s
 	 */
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
-
-	return 0;
 }
 
 /**
@@ -1474,17 +1462,13 @@ static int cpufreq_remove_dev(struct dev
 {
 	unsigned int cpu = dev->id;
 	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
-	int ret;
 
 	if (!policy)
 		return 0;
 
 	if (cpu_online(cpu)) {
-		ret = __cpufreq_remove_dev_prepare(dev, sif);
-		if (!ret)
-			ret = __cpufreq_remove_dev_finish(dev, sif);
-		if (ret)
-			return ret;
+		__cpufreq_remove_dev_prepare(dev);
+		__cpufreq_remove_dev_finish(dev);
 	}
 
 	/* sysfs links are removed only on subsys callback */
@@ -2376,11 +2360,11 @@ static int cpufreq_cpu_callback(struct n
 			break;
 
 		case CPU_DOWN_PREPARE:
-			__cpufreq_remove_dev_prepare(dev, NULL);
+			__cpufreq_remove_dev_prepare(dev);
 			break;
 
 		case CPU_POST_DEAD:
-			__cpufreq_remove_dev_finish(dev, NULL);
+			__cpufreq_remove_dev_finish(dev);
 			break;
 
 		case CPU_DOWN_FAILED:

Re: [PATCH v2] cpufreq: Fix possible memory leak during CPU removal

[Viresh Kumar]

On 23-07-15, 00:27, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After commit 9b07109f06a1 (cpufreq: Fix double addition of sysfs
> links) the second sif argument of __cpufreq_remove_dev_prepare()
> and __cpufreq_remove_dev_finish() is not used by them any more.
> 
> However, there also is a problem in cpufreq_remove_dev() that
> if any of the above functions returns an error, we'll fail to
> clean up after a CPU that is going away (and it is going away
> no matter what).  Moreover, error codes returned by them are
> ignored by cpufreq_cpu_callback(), so even if any of them is
> aborted and returns an error code, the caller of the notifier
> callback will not know about that.
> 
> For this reason, make __cpufreq_remove_dev_prepare() and
> __cpufreq_remove_dev_finish() never fail, change them to void
> functions and drop the sif argument from them.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> On top of:
> http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge&id=9b07109f06a1edd6e636b1e7397157eae0e6baa4
> 
> Note: The commit mentioned above is on a testing branch only
> at the moment.
> 
> Well, I seem to be blind.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh