Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
Menon, Nishanth n...@ti.com writes:
On Wed, May 25, 2011 at 17:16, Kevin Hilman khil...@ti.com wrote:
Nishanth Menon n...@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 n...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
On Thu, May 26, 2011 at 10:11, Kevin Hilman khil...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
Nishanth Menon n...@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 n...@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);
Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
On Wed, May 25, 2011 at 17:16, Kevin Hilman khil...@ti.com wrote:
Nishanth Menon n...@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 n...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
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 n...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
On Wed, May 25, 2011 at 18:25, Todd Poynor toddpoy...@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 n...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
