On Sunday, November 17, 2013 09:43:14 AM viresh kumar wrote:
> On 16 November 2013 21:06, Lan Tianyu <tianyu.lan@xxxxxxxxx> wrote:
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > @@ -1818,9 +1818,13 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> > mutex_unlock(&cpufreq_governor_lock);
> > }
> >
> > - if (((event == CPUFREQ_GOV_POLICY_INIT) && ret) ||
> > - ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
> > + if ((event == CPUFREQ_GOV_POLICY_INIT) && ret) {
> > + module_put(policy->governor->owner);
> > + if (ret == -EALREADY)
> > + return 0;
> > + } else if ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret) {
> > module_put(policy->governor->owner);
> > + }
>
> This can still be written more efficiently I believe:
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 138ebe9..54e28e1 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1866,10 +1866,12 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> ret = policy->governor->governor(policy, event);
>
> if (!ret) {
> - if (event == CPUFREQ_GOV_POLICY_INIT)
> + if (event == CPUFREQ_GOV_POLICY_INIT) {
> policy->governor->initialized++;
> - else if (event == CPUFREQ_GOV_POLICY_EXIT)
> + } else if (event == CPUFREQ_GOV_POLICY_EXIT) {
> policy->governor->initialized--;
> + module_put(policy->governor->owner);
> + }
> } else {
> /* Restore original values */
> mutex_lock(&cpufreq_governor_lock);
> @@ -1877,13 +1879,14 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> policy->governor_enabled = true;
> else if (event == CPUFREQ_GOV_START)
> policy->governor_enabled = false;
> + else if (event == CPUFREQ_GOV_POLICY_INIT)
> + if (ret == -EALREADY) {
You can write this as
else if (event == CPUFREQ_GOV_POLICY_INIT && ret == -EALREADY) {
> + module_put(policy->governor->owner);
> + ret = 0;
> + }
> mutex_unlock(&cpufreq_governor_lock);
> }
>
> - if (((event == CPUFREQ_GOV_POLICY_INIT) && ret) ||
> - ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
> - module_put(policy->governor->owner);
> -
> return ret;
Apart from the above I agree.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
