diff mbox

[V3,07/14] cpufreq: Don't allow updating inactive-policies from sysfs

Message ID 6ad4377106496d9d148005c902602c66eb307930.1431065963.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar May 8, 2015, 6:23 a.m. UTC
Later commits would change the way policies are managed today. Policies
wouldn't be freed on cpu hotplug (currently they aren't freed only for
suspend), and while the CPU is offline, the sysfs cpufreq files would
still be present.

User may accidentally try to update the sysfs files in following
directory: '/sys/devices/system/cpu/cpuX/cpufreq/'. And that would
result in undefined behavior as policy wouldn't be active then.

Apart from updating the store() routine, we also update __cpufreq_get()
which can call cpufreq_out_of_sync(). The later routine tries to update
policy->cur and starts notifying kernel about it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Viresh Kumar May 16, 2015, 2:01 a.m. UTC | #1
On 16-05-15, 03:10, Rafael J. Wysocki wrote:
> The dash after "inactive" in the subject is not necessary IMO.

Okay

> On Friday, May 08, 2015 11:53:50 AM Viresh Kumar wrote:
> > +	/*
> > +	 * Policy might not be active currently, and so we shouldn't try
> > +	 * updating any values here. policy->cpus is cleared for inactive policy
> > +	 * and so cpufreq_cpu_get_raw() should fail.
> 
> These comments don't really clarify things.  It'd be better to say something
> like "Updating inactive policies is invalid, so avoid doing that."

Okay..

> > +	 */
> > +	if (unlikely(policy_is_inactive(policy))) {
> > +		ret = -EPERM;
> 
> This doesn't seem to be the appropriate error code to return here.
> 
> -EBUSY or -EAGAIN would be better IMO.

Okay
diff mbox

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index bd8a47b5054e..652a843a553b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -887,11 +887,22 @@  static ssize_t store(struct kobject *kobj, struct attribute *attr,
 
 	down_write(&policy->rwsem);
 
+	/*
+	 * Policy might not be active currently, and so we shouldn't try
+	 * updating any values here. policy->cpus is cleared for inactive policy
+	 * and so cpufreq_cpu_get_raw() should fail.
+	 */
+	if (unlikely(policy_is_inactive(policy))) {
+		ret = -EPERM;
+		goto unlock_policy_rwsem;
+	}
+
 	if (fattr->store)
 		ret = fattr->store(policy, buf, count);
 	else
 		ret = -EIO;
 
+unlock_policy_rwsem:
 	up_write(&policy->rwsem);
 
 	up_read(&cpufreq_rwsem);
@@ -1619,6 +1630,14 @@  static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
 
 	ret_freq = cpufreq_driver->get(policy->cpu);
 
+	/*
+	 * Policy might not be active currently, and so we shouldn't try
+	 * updating any values here. policy->cpus is cleared for inactive policy
+	 * and so cpufreq_cpu_get_raw() should fail.
+	 */
+	if (unlikely(policy_is_inactive(policy)))
+		return ret_freq;
+
 	if (ret_freq && policy->cur &&
 		!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
 		/* verify no discrepancy between actual and