| Message ID | 1306366733-8439-7-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; -- 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
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
On Wed, May 25, 2011 at 04:38:51PM -0700, Nishanth Menon wrote: > 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. > > Signed-off-by: Nishanth Menon <nm@ti.com> > --- ... > 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; > + > + /* 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--; "if (ret)" intended? ret == 0 means allocated OK. Todd -- 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
On Wed, May 25, 2011 at 18:25, Todd Poynor <toddpoynor@google.com> wrote: > On Wed, May 25, 2011 at 04:38:51PM -0700, Nishanth Menon wrote: >> 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. >> >> Signed-off-by: Nishanth Menon <nm@ti.com> >> --- > ... >> 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; >> + >> + /* 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--; > > "if (ret)" intended? ret == 0 means allocated OK. arrgh.. yikes.. 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
"Menon, Nishanth" <nm@ti.com> writes: > 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. The table is not modifiable in two places. It's only modified once, upon creation. After that the table contents are constant. What is changable is simply the existence of the table. This can be handled by simply checking the pointer (or using your counter.) > What if there is a read path while free occurs - Then the CPUfreq driver has a bug. If you want to be safe, check for a valid pointer or use your counter. > 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. When you're talking about potentially concurrent modification of data, that make sense. Here you're implementing a plugin for an existing framework, and you can (and have to) make assumptions about when the callbacks are used. Kevin >> >> > 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
On Thu, May 26, 2011 at 10:11, Kevin Hilman <khilman@ti.com> wrote: > > When you're talking about potentially concurrent modification of data, > that make sense. Here you're implementing a plugin for an existing > framework, and you can (and have to) make assumptions about when the > callbacks are used. ok, that is the one i was looking for. thanks. will use atomic ops and drop the mutex. 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
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; + + /* 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; + 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;
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. Signed-off-by: Nishanth Menon <nm@ti.com> --- arch/arm/mach-omap2/omap2plus-cpufreq.c | 62 +++++++++++++++++++++++++++---- 1 files changed, 54 insertions(+), 8 deletions(-)