On Wed, May 25, 2011 at 17:16, Kevin Hilman <khilman@xxxxxx> wrote:
>
> Nishanth Menon <nm@xxxxxx> 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?
We just have one freq_table for both cpu0 and cpu1. We have common
data structure(freq_table and users) which is modifiable in two
APIs(init/exit) and a set of reads. What if there is a read path while
free occurs - I may be mistaken, but my understanding is that the
datastructure used in my code should be secured in my code and I
cannot depend on higher layer(cpufreq/governors) to ensure that.
>
> > Signed-off-by: Nishanth Menon <nm@xxxxxx>
> > ---
> > 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
[..]
> > @@ -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;
ok
>
> 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.
The mutex lock was to protect both the freq_table and the count as
they protect the same resource - freq_table
>
> However, if you get rid of the mutex (and I think you should), you could
> use an atomic variable here
yes, we can use just atomic to protect alloc Vs free - but we cannot
protect read Vs free
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
