| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 03 Feb 2016 17:37:28 -0800
From: Saravana Kannan <skannan@...eaurora.org>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
CC: Linux PM list <linux-pm@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Viresh Kumar <viresh.kumar@...aro.org>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Juri Lelli <juri.lelli@....com>,
Steve Muckle <steve.muckle@...aro.org>
Subject: Re: [PATCH 4/11] cpufreq: governor: Avoid passing dbs_data pointers
around unnecessarily
On 02/03/2016 03:29 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> Do not pass struct dbs_data pointers to the family of functions
> implementing governor operations in cpufreq_governor.c as they can
> take that pointer from policy->governor by themselves.
>
> The cpufreq_governor_init() case is slightly more complicated, since
> policy->governor may be NULL when it is invoked, but then it can use
> global_dbs_data directly just fine.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
> drivers/cpufreq/cpufreq_governor.c | 74 +++++++++++++++----------------------
> 1 file changed, 30 insertions(+), 44 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
> @@ -330,9 +330,9 @@ static void free_common_dbs_info(struct
> }
>
> static int cpufreq_governor_init(struct cpufreq_policy *policy,
> - struct dbs_data *dbs_data,
> struct common_dbs_data *cdata)
> {
> + struct dbs_data *dbs_data;
> unsigned int latency;
> int ret;
>
> @@ -340,7 +340,7 @@ static int cpufreq_governor_init(struct
> if (policy->governor_data)
> return -EBUSY;
>
> - if (dbs_data) {
> + if (global_dbs_data) {
> if (WARN_ON(have_governor_per_policy()))
> return -EINVAL;
I'm not sure if this code is functionally equivalent to what was there
before.
Old: If the dbs_data is not NULL (whether it's global or not), but we
have gov per policy enabled, warn and bail out.
New: If the global_dbs_data is not NULL, but we have gov per policy
enabled, warn and bail out.
Both of these are bad cases, but the meaning of the code here definitely
changes. My guess is that the old code was put in here to catch the
"Old" case since that's more likely to happen with the crappy locking we
have/had. I don't think the "New" case is even possible given the code
-- we can probably prove it will never happen if we trust the compiler.
I'm not necessarily asking you to go back to the old code (I don't have
a strong preference either way), but just wanted to point out the
difference and let the rest of you decide.
>
> @@ -348,8 +348,8 @@ static int cpufreq_governor_init(struct
> if (ret)
> return ret;
>
> - dbs_data->usage_count++;
> - policy->governor_data = dbs_data;
> + global_dbs_data->usage_count++;
> + policy->governor_data = global_dbs_data;
> return 0;
> }
>
> @@ -401,9 +401,9 @@ free_dbs_data:
> return ret;
> }
>
> -static int cpufreq_governor_exit(struct cpufreq_policy *policy,
> - struct dbs_data *dbs_data)
> +static int cpufreq_governor_exit(struct cpufreq_policy *policy)
> {
> + struct dbs_data *dbs_data = policy->governor_data;
> struct common_dbs_data *cdata = dbs_data->cdata;
> struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs(policy->cpu);
>
> @@ -430,9 +430,9 @@ static int cpufreq_governor_exit(struct
> return 0;
> }
>
> -static int cpufreq_governor_start(struct cpufreq_policy *policy,
> - struct dbs_data *dbs_data)
> +static int cpufreq_governor_start(struct cpufreq_policy *policy)
> {
> + struct dbs_data *dbs_data = policy->governor_data;
> struct common_dbs_data *cdata = dbs_data->cdata;
> unsigned int sampling_rate, ignore_nice, j, cpu = policy->cpu;
> struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs(cpu);
> @@ -498,9 +498,9 @@ static int cpufreq_governor_start(struct
> return 0;
> }
>
> -static int cpufreq_governor_stop(struct cpufreq_policy *policy,
> - struct dbs_data *dbs_data)
> +static int cpufreq_governor_stop(struct cpufreq_policy *policy)
> {
> + struct dbs_data *dbs_data = policy->governor_data;
> struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(policy->cpu);
> struct cpu_common_dbs_info *shared = cdbs->shared;
>
> @@ -514,9 +514,9 @@ static int cpufreq_governor_stop(struct
> return 0;
> }
>
> -static int cpufreq_governor_limits(struct cpufreq_policy *policy,
> - struct dbs_data *dbs_data)
> +static int cpufreq_governor_limits(struct cpufreq_policy *policy)
> {
> + struct dbs_data *dbs_data = policy->governor_data;
> struct common_dbs_data *cdata = dbs_data->cdata;
> unsigned int cpu = policy->cpu;
> struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs(cpu);
> @@ -541,45 +541,31 @@ static int cpufreq_governor_limits(struc
> int cpufreq_governor_dbs(struct cpufreq_policy *policy,
> struct common_dbs_data *cdata, unsigned int event)
> {
> - struct dbs_data *dbs_data;
> - int ret;
> + int ret = -EINVAL;
>
> /* Lock governor to block concurrent initialization of governor */
> mutex_lock(&dbs_data_mutex);
>
> - if (have_governor_per_policy())
> - dbs_data = policy->governor_data;
> - else
> - dbs_data = global_dbs_data;
> -
> - if (!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT)) {
> - ret = -EINVAL;
> - goto unlock;
> + if (event == CPUFREQ_GOV_POLICY_INIT) {
> + ret = cpufreq_governor_init(policy, cdata);
> + } else if (policy->governor_data) {
> + switch (event) {
> + case CPUFREQ_GOV_POLICY_EXIT:
> + ret = cpufreq_governor_exit(policy);
> + break;
> + case CPUFREQ_GOV_START:
> + ret = cpufreq_governor_start(policy);
> + break;
> + case CPUFREQ_GOV_STOP:
> + ret = cpufreq_governor_stop(policy);
> + break;
> + case CPUFREQ_GOV_LIMITS:
> + ret = cpufreq_governor_limits(policy);
> + break;
> + }
> }
>
> - switch (event) {
> - case CPUFREQ_GOV_POLICY_INIT:
> - ret = cpufreq_governor_init(policy, dbs_data, cdata);
> - break;
> - case CPUFREQ_GOV_POLICY_EXIT:
> - ret = cpufreq_governor_exit(policy, dbs_data);
> - break;
> - case CPUFREQ_GOV_START:
> - ret = cpufreq_governor_start(policy, dbs_data);
> - break;
> - case CPUFREQ_GOV_STOP:
> - ret = cpufreq_governor_stop(policy, dbs_data);
> - break;
> - case CPUFREQ_GOV_LIMITS:
> - ret = cpufreq_governor_limits(policy, dbs_data);
> - break;
> - default:
> - ret = -EINVAL;
> - }
> -
> -unlock:
> mutex_unlock(&dbs_data_mutex);
> -
Po-tay-to, Po-tah-to.
> return ret;
> }
> EXPORT_SYMBOL_GPL(cpufreq_governor_dbs);
>
Agree with the general idea of the patch though.
Conditional on the comment above being resolve amongst the others:
Acked-by: Saravana Kannan <skannan@...eaurora.org>
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Powered by blists - more mailing lists