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

["Menon, Nishanth" <nm@ti.com>]

On Wed, May 25, 2011 at 17:16, Kevin Hilman <khilman@ti.com> wrote:
>
> 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?

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@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

[..]
> > @@ -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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html