| Message ID | 1371956540-8830-1-git-send-email-skannan@codeaurora.org (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
Hi Saravana, On 23 June 2013 08:32, Saravana Kannan <skannan@codeaurora.org> wrote: > The sysfs store ops need to grab the policy write semaphore to avoid race > with hotplug and cpufreq_update_policy() calls. Without this, we could end > up with simultaneous calls to cpufreq_driver->target() Interesting. > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 2d53f47..37db7f0 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -420,9 +420,13 @@ static ssize_t store_##file_name \ > if (ret != 1) \ > return -EINVAL; \ > \ > + lock_policy_rwsem_write(policy->cpu); \ > + \ > ret = __cpufreq_set_policy(policy, &new_policy); \ > policy->user_policy.object = policy->object; \ > \ > + unlock_policy_rwsem_write(policy->cpu); \ > + \ > return ret ? ret : count; \ > } As far as I know, this protection already exists. Check this function, which eventually calls all **store() related to struct freq_attr static ssize_t store(struct kobject *kobj, struct attribute *attr, const char *buf, size_t count) { struct cpufreq_policy *policy = to_policy(kobj); struct freq_attr *fattr = to_attr(attr); ssize_t ret = -EINVAL; policy = cpufreq_cpu_get_sysfs(policy->cpu); if (!policy) goto no_policy; if (lock_policy_rwsem_write(policy->cpu) < 0) goto fail; if (fattr->store) ret = fattr->store(policy, buf, count); else ret = -EIO; unlock_policy_rwsem_write(policy->cpu); fail: cpufreq_cpu_put_sysfs(policy); no_policy: return ret; } -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/23/2013 11:08 PM, Viresh Kumar wrote: > Hi Saravana, > > On 23 June 2013 08:32, Saravana Kannan <skannan@codeaurora.org> wrote: >> The sysfs store ops need to grab the policy write semaphore to avoid race >> with hotplug and cpufreq_update_policy() calls. Without this, we could end >> up with simultaneous calls to cpufreq_driver->target() > > Interesting. > >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 2d53f47..37db7f0 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -420,9 +420,13 @@ static ssize_t store_##file_name \ >> if (ret != 1) \ >> return -EINVAL; \ >> \ >> + lock_policy_rwsem_write(policy->cpu); \ >> + \ >> ret = __cpufreq_set_policy(policy, &new_policy); \ >> policy->user_policy.object = policy->object; \ >> \ >> + unlock_policy_rwsem_write(policy->cpu); \ >> + \ >> return ret ? ret : count; \ >> } > > As far as I know, this protection already exists. Check this > function, which eventually calls all **store() related to > struct freq_attr > > static ssize_t store(struct kobject *kobj, struct attribute *attr, > const char *buf, size_t count) > { > struct cpufreq_policy *policy = to_policy(kobj); > struct freq_attr *fattr = to_attr(attr); > ssize_t ret = -EINVAL; > policy = cpufreq_cpu_get_sysfs(policy->cpu); > if (!policy) > goto no_policy; > > if (lock_policy_rwsem_write(policy->cpu) < 0) > goto fail; > > if (fattr->store) > ret = fattr->store(policy, buf, count); > else > ret = -EIO; > > unlock_policy_rwsem_write(policy->cpu); > fail: > cpufreq_cpu_put_sysfs(policy); > no_policy: > return ret; > } > You are right. I did look at this, but looks like I skimmed some code too fast. But the race is certainly happening. I'll have to dig deeper I guess. I do see some patches you have for serializing driver->target() calls. Haven't looked at them yet, but maybe they'll help. I'll dig deeper. Thanks, Saravana
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2d53f47..37db7f0 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -420,9 +420,13 @@ static ssize_t store_##file_name \ if (ret != 1) \ return -EINVAL; \ \ + lock_policy_rwsem_write(policy->cpu); \ + \ ret = __cpufreq_set_policy(policy, &new_policy); \ policy->user_policy.object = policy->object; \ \ + unlock_policy_rwsem_write(policy->cpu); \ + \ return ret ? ret : count; \ } @@ -480,6 +484,8 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy, &new_policy.governor)) return -EINVAL; + lock_policy_rwsem_write(policy->cpu); + /* Do not use cpufreq_set_policy here or the user_policy.max will be wrongly overridden */ ret = __cpufreq_set_policy(policy, &new_policy); @@ -487,6 +493,8 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy, policy->user_policy.policy = policy->policy; policy->user_policy.governor = policy->governor; + unlock_policy_rwsem_write(policy->cpu); + if (ret) return ret; else @@ -572,7 +580,9 @@ static ssize_t store_scaling_setspeed(struct cpufreq_policy *policy, if (ret != 1) return -EINVAL; + lock_policy_rwsem_write(policy->cpu); policy->governor->store_setspeed(policy, freq); + unlock_policy_rwsem_write(policy->cpu); return count; } diff --git a/drivers/cpufreq/cpufreq_userspace.c b/drivers/cpufreq/cpufreq_userspace.c index bbeb9c0..b7dd5b8 100644 --- a/drivers/cpufreq/cpufreq_userspace.c +++ b/drivers/cpufreq/cpufreq_userspace.c @@ -87,16 +87,7 @@ static int cpufreq_set(struct cpufreq_policy *policy, unsigned int freq) if (freq > per_cpu(cpu_max_freq, policy->cpu)) freq = per_cpu(cpu_max_freq, policy->cpu); - /* - * We're safe from concurrent calls to ->target() here - * as we hold the userspace_mutex lock. If we were calling - * cpufreq_driver_target, a deadlock situation might occur: - * A: cpufreq_set (lock userspace_mutex) -> - * cpufreq_driver_target(lock policy->lock) - * B: cpufreq_set_policy(lock policy->lock) -> - * __cpufreq_governor -> - * cpufreq_governor_userspace (lock userspace_mutex) - */ + /* The cpufreq framework holds the lock before calling this op. */ ret = __cpufreq_driver_target(policy, freq, CPUFREQ_RELATION_L); err:
The sysfs store ops need to grab the policy write semaphore to avoid race with hotplug and cpufreq_update_policy() calls. Without this, we could end up with simultaneous calls to cpufreq_driver->target() Signed-off-by: Saravana Kannan <skannan@codeaurora.org> --- There still seem to be race conditions between cpufreq_update_policy() and the cpufreq hotplug path. But that seems more complicated to fix. So, leaving that for later. drivers/cpufreq/cpufreq.c | 10 ++++++++++ drivers/cpufreq/cpufreq_userspace.c | 11 +---------- 2 files changed, 11 insertions(+), 10 deletions(-)