Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak

From: Kevin Hilman <khilman@ti.com>
To: Nishanth Menon <nm@ti.com>
Cc: linux-omap <linux-omap@vger.kernel.org>
Subject: Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
Date: Wed, 25 May 2011 17:16:54 -0700	[thread overview]
Message-ID: <877h9ejoy1.fsf@ti.com> (raw)
In-Reply-To: <1306366733-8439-7-git-send-email-nm@ti.com> (Nishanth Menon's message of "Wed, 25 May 2011 16:38:51 -0700")

Nishanth Menon <nm@ti.com> writes:

> Since we have multiple CPUs, the cpuinit call for CPU1 causes
> freq_table of CPU0 to be overwritten. Instead, we maintain
> a counter to keep track of cpus who use the cpufreq table
> allocate it once(one freq table for all CPUs) and free them
> once the last user is done with it. We also need to protect
> freq_table and this new counter from updates from multiple
> contexts to be on a safe side.

Not sure I understand the need for all the locking here.  Once allocated
and filled, the freq_table isn't changing.  Also, all the functions are
only reading the freq_table, not changing it.    So what is it you're
trying to protect against?

> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  arch/arm/mach-omap2/omap2plus-cpufreq.c |   62 +++++++++++++++++++++++++++----
>  1 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> index 3ff3302..f026ac4 100644
> --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
> +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> @@ -39,6 +39,9 @@
>  #include <mach/hardware.h>
>  
>  static struct cpufreq_frequency_table *freq_table;
> +static int freq_table_users;
> +static DEFINE_MUTEX(freq_table_lock);
> +
>  static struct clk *mpu_clk;
>  static char *mpu_clk_name;
>  static struct device *mpu_dev;
> @@ -46,9 +49,17 @@ static bool use_opp;
>  
>  static int omap_verify_speed(struct cpufreq_policy *policy)
>  {
> +	int r = -EINVAL;
> +
> +	mutex_lock(&freq_table_lock);
>  	if (!freq_table)
> -		return -EINVAL;
> -	return cpufreq_frequency_table_verify(policy, freq_table);
> +		goto out;
> +
> +	r = cpufreq_frequency_table_verify(policy, freq_table);
> +
> +out:
> +	mutex_unlock(&freq_table_lock);
> +	return r;
>  }
>  
>  static unsigned int omap_getspeed(unsigned int cpu)
> @@ -74,9 +85,11 @@ static int omap_target(struct cpufreq_policy *policy,
>  	if (is_smp() && (num_online_cpus() < NR_CPUS))
>  		return ret;
>  
> +	mutex_lock(&freq_table_lock);
>  	if (!freq_table) {
>  		dev_err(mpu_dev, "%s: cpu%d: no freq table!\n", __func__,
>  				policy->cpu);
> +		mutex_unlock(&freq_table_lock);
>  		return -EINVAL;
>  	}
>  
> @@ -85,9 +98,13 @@ static int omap_target(struct cpufreq_policy *policy,
>  	if (ret) {
>  		dev_dbg(mpu_dev, "%s: cpu%d: no freq match for %d(ret=%d)\n",
>  			__func__, policy->cpu, target_freq, ret);
> +		mutex_unlock(&freq_table_lock);
>  		return ret;
>  	}
> +
>  	freqs.new = freq_table[i].frequency;
> +	mutex_unlock(&freq_table_lock);
> +
>  	if (!freqs.new) {
>  		dev_err(mpu_dev, "%s: cpu%d: no match for freq %d\n", __func__,
>  			policy->cpu, target_freq);
> @@ -156,22 +173,48 @@ skip_lpj:
>  
>  static int freq_table_alloc(void)
>  {
> -	if (use_opp)
> -		return opp_init_cpufreq_table(mpu_dev, &freq_table);
> +	int ret = 0;
>  
> -	clk_init_cpufreq_table(&freq_table);
> -	if (!freq_table)
> -		return -ENOMEM;
> +	mutex_lock(&freq_table_lock);
>  
> -	return 0;
> +	freq_table_users++;
> +	/* Did we allocate previously? */
> +	if (freq_table_users - 1)
> +		goto out;

Rather than the ' - 1', this can just be 

     if (freq_table_users++)
                goto out;

or better, you probably don't need this check protected by the mutex,
so this could just return directly, and then take the mutex_lock() after
this point.

However, if you get rid of the mutex (and I think you should), you could
use an atomic variable here 
> +	/* no, so we allocate */
> +	if (use_opp) {
> +		ret = opp_init_cpufreq_table(mpu_dev, &freq_table);
> +	} else {
> +		clk_init_cpufreq_table(&freq_table);
> +		if (!freq_table)
> +			ret = -ENOMEM;
> +	}
> +	/* if we did not allocate cleanly.. reduce user count */
> +	if (!ret)
> +		freq_table_users--;
> +
> +out:
> +	mutex_unlock(&freq_table_lock);
> +	return ret;
>  }
>  
>  static void freq_table_free(void)
>  {
> +	mutex_lock(&freq_table_lock);
> +
> +	if (!freq_table_users)
> +		goto out;
> +	freq_table_users--;
> +	if (freq_table_users)
> +		goto out;

Similarily:

	if (--freq_table_users)
		goto out;

>  	if (use_opp)
>  		opp_free_cpufreq_table(mpu_dev, &freq_table);
>  	else
>  		clk_exit_cpufreq_table(&freq_table);
> +out:
> +	mutex_unlock(&freq_table_lock);
>  }
>  
>  static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
> @@ -195,14 +238,17 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
>  		return result;
>  	}
>  
> +	mutex_lock(&freq_table_lock);
>  	result = cpufreq_frequency_table_cpuinfo(policy, freq_table);
>  	if (result) {
> +		mutex_unlock(&freq_table_lock);
>  		dev_err(mpu_dev, "%s: cpu%d: unable to get cpuinfo [%d]\n",
>  			__func__, policy->cpu, result);
>  		freq_table_free();
>  		return result;
>  	}
>  	cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> +	mutex_unlock(&freq_table_lock);
>  
>  	policy->min = policy->cpuinfo.min_freq;
>  	policy->max = policy->cpuinfo.max_freq;