The OPP structures are abused to the best here, without understanding how the OPP core and RCU locks work.
In short, the OPP pointer saved in 'rk3399_dmcfreq' can become invalid under your nose, as the OPP core may free it.
Fix various abuses around OPP structures and calls.
Compile tested only.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- I would like it to go via the PM tree. Perhaps that's already the default tree for this.
drivers/devfreq/rk3399_dmc.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c index 5063ac1a5939..cf14631b1945 100644 --- a/drivers/devfreq/rk3399_dmc.c +++ b/drivers/devfreq/rk3399_dmc.c @@ -80,7 +80,6 @@ struct rk3399_dmcfreq { struct regulator *vdd_center; unsigned long rate, target_rate; unsigned long volt, target_volt; - struct dev_pm_opp *curr_opp; };
static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, @@ -102,9 +101,6 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, target_rate = dev_pm_opp_get_freq(opp); target_volt = dev_pm_opp_get_voltage(opp);
- dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp); - dmcfreq->volt = dev_pm_opp_get_voltage(dmcfreq->curr_opp); - rcu_read_unlock();
if (dmcfreq->rate == target_rate) @@ -165,7 +161,9 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, if (err) dev_err(dev, "Cannot to set vol %lu uV\n", target_volt);
- dmcfreq->curr_opp = opp; + dmcfreq->rate = target_rate; + dmcfreq->volt = target_volt; + out: mutex_unlock(&dmcfreq->lock); return err; @@ -431,8 +429,9 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) rcu_read_unlock(); return PTR_ERR(opp); } + dmcfreq->rate = dev_pm_opp_get_freq(opp); + dmcfreq->volt = dev_pm_opp_get_voltage(opp); rcu_read_unlock(); - data->curr_opp = opp;
rk3399_devfreq_dmc_profile.initial_freq = data->rate;
Hi Viresh,
On 2016년 12월 01일 19:38, Viresh Kumar wrote:
The OPP structures are abused to the best here, without understanding how the OPP core and RCU locks work.
In short, the OPP pointer saved in 'rk3399_dmcfreq' can become invalid under your nose, as the OPP core may free it.
Fix various abuses around OPP structures and calls.
Compile tested only.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I would like it to go via the PM tree. Perhaps that's already the default tree for this.
drivers/devfreq/rk3399_dmc.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c index 5063ac1a5939..cf14631b1945 100644 --- a/drivers/devfreq/rk3399_dmc.c +++ b/drivers/devfreq/rk3399_dmc.c @@ -80,7 +80,6 @@ struct rk3399_dmcfreq { struct regulator *vdd_center; unsigned long rate, target_rate; unsigned long volt, target_volt;
- struct dev_pm_opp *curr_opp;
}; static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, @@ -102,9 +101,6 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, target_rate = dev_pm_opp_get_freq(opp); target_volt = dev_pm_opp_get_voltage(opp);
- dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp);
- dmcfreq->volt = dev_pm_opp_get_voltage(dmcfreq->curr_opp);
- rcu_read_unlock();
if (dmcfreq->rate == target_rate)
dmcfreq->rate is used on here. Maybe struct rk3399_dmcfreq need to add the new 'curr_freq' field.
@@ -165,7 +161,9 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, if (err) dev_err(dev, "Cannot to set vol %lu uV\n", target_volt);
- dmcfreq->curr_opp = opp;
- dmcfreq->rate = target_rate;
- dmcfreq->volt = target_volt;
out: mutex_unlock(&dmcfreq->lock); return err; @@ -431,8 +429,9 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) rcu_read_unlock(); return PTR_ERR(opp); }
- dmcfreq->rate = dev_pm_opp_get_freq(opp);
- dmcfreq->volt = dev_pm_opp_get_voltage(opp); rcu_read_unlock();
- data->curr_opp = opp;
rk3399_devfreq_dmc_profile.initial_freq = data->rate;
Hi Chanwoo,
Thanks for trying to review all these patches.
On 02-12-16, 17:42, Chanwoo Choi wrote:
Hi Viresh,
On 2016년 12월 01일 19:38, Viresh Kumar wrote:
The OPP structures are abused to the best here, without understanding how the OPP core and RCU locks work.
In short, the OPP pointer saved in 'rk3399_dmcfreq' can become invalid under your nose, as the OPP core may free it.
Fix various abuses around OPP structures and calls.
Compile tested only.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I would like it to go via the PM tree. Perhaps that's already the default tree for this.
drivers/devfreq/rk3399_dmc.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c index 5063ac1a5939..cf14631b1945 100644 --- a/drivers/devfreq/rk3399_dmc.c +++ b/drivers/devfreq/rk3399_dmc.c @@ -80,7 +80,6 @@ struct rk3399_dmcfreq { struct regulator *vdd_center; unsigned long rate, target_rate; unsigned long volt, target_volt;
- struct dev_pm_opp *curr_opp;
}; static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, @@ -102,9 +101,6 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, target_rate = dev_pm_opp_get_freq(opp); target_volt = dev_pm_opp_get_voltage(opp);
- dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp);
- dmcfreq->volt = dev_pm_opp_get_voltage(dmcfreq->curr_opp);
- rcu_read_unlock();
if (dmcfreq->rate == target_rate)
dmcfreq->rate is used on here.
Are you trying to say that dmcfreq->rate is required to have the right value as we will be using it here for comparison? If yes, then ...
Maybe struct rk3399_dmcfreq need to add the new 'curr_freq' field.
@@ -165,7 +161,9 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, if (err) dev_err(dev, "Cannot to set vol %lu uV\n", target_volt);
- dmcfreq->curr_opp = opp;
- dmcfreq->rate = target_rate;
- dmcfreq->volt = target_volt;
This takes care of it for all cases except the first call to target()...
out: mutex_unlock(&dmcfreq->lock); return err; @@ -431,8 +429,9 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) rcu_read_unlock(); return PTR_ERR(opp); }
- dmcfreq->rate = dev_pm_opp_get_freq(opp);
- dmcfreq->volt = dev_pm_opp_get_voltage(opp);
And this takes care of the first call to target().
Am I still missing something ?
rcu_read_unlock();
- data->curr_opp = opp;
rk3399_devfreq_dmc_profile.initial_freq = data->rate;
-- Best Regards, Chanwoo Choi
Hi Viresh,
2016-12-02 18:00 GMT+09:00 Viresh Kumar viresh.kumar@linaro.org:
Hi Chanwoo,
Thanks for trying to review all these patches.
On 02-12-16, 17:42, Chanwoo Choi wrote:
Hi Viresh,
On 2016년 12월 01일 19:38, Viresh Kumar wrote:
The OPP structures are abused to the best here, without understanding how the OPP core and RCU locks work.
In short, the OPP pointer saved in 'rk3399_dmcfreq' can become invalid under your nose, as the OPP core may free it.
Fix various abuses around OPP structures and calls.
Compile tested only.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I would like it to go via the PM tree. Perhaps that's already the default tree for this.
drivers/devfreq/rk3399_dmc.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c index 5063ac1a5939..cf14631b1945 100644 --- a/drivers/devfreq/rk3399_dmc.c +++ b/drivers/devfreq/rk3399_dmc.c @@ -80,7 +80,6 @@ struct rk3399_dmcfreq { struct regulator *vdd_center; unsigned long rate, target_rate; unsigned long volt, target_volt;
- struct dev_pm_opp *curr_opp;
};
static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, @@ -102,9 +101,6 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, target_rate = dev_pm_opp_get_freq(opp); target_volt = dev_pm_opp_get_voltage(opp);
dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp);
dmcfreq->volt = dev_pm_opp_get_voltage(dmcfreq->curr_opp);
rcu_read_unlock();
if (dmcfreq->rate == target_rate)
dmcfreq->rate is used on here.
Are you trying to say that dmcfreq->rate is required to have the right value as we will be using it here for comparison? If yes, then ...
Maybe struct rk3399_dmcfreq need to add the new 'curr_freq' field.
@@ -165,7 +161,9 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, if (err) dev_err(dev, "Cannot to set vol %lu uV\n", target_volt);
- dmcfreq->curr_opp = opp;
- dmcfreq->rate = target_rate;
- dmcfreq->volt = target_volt;
This takes care of it for all cases except the first call to target()...
out: mutex_unlock(&dmcfreq->lock); return err; @@ -431,8 +429,9 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) rcu_read_unlock(); return PTR_ERR(opp); }
- dmcfreq->rate = dev_pm_opp_get_freq(opp);
- dmcfreq->volt = dev_pm_opp_get_voltage(opp);
And this takes care of the first call to target().
Am I still missing something ?
rcu_read_unlock();
data->curr_opp = opp;
rk3399_devfreq_dmc_profile.initial_freq = data->rate;
-- Best Regards, Chanwoo Choi
-- viresh _______________________________________________ linaro-kernel mailing list linaro-kernel@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-kernel
Hi Viresh,
I'm sorry. I sent the mail without any message. It is my mistake.
2016-12-02 22:21 GMT+09:00 Chanwoo Choi cwchoi00@gmail.com:
Hi Viresh,
2016-12-02 18:00 GMT+09:00 Viresh Kumar viresh.kumar@linaro.org:
Hi Chanwoo,
Thanks for trying to review all these patches.
On 02-12-16, 17:42, Chanwoo Choi wrote:
Hi Viresh,
On 2016년 12월 01일 19:38, Viresh Kumar wrote:
The OPP structures are abused to the best here, without understanding how the OPP core and RCU locks work.
In short, the OPP pointer saved in 'rk3399_dmcfreq' can become invalid under your nose, as the OPP core may free it.
Fix various abuses around OPP structures and calls.
Compile tested only.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I would like it to go via the PM tree. Perhaps that's already the default tree for this.
drivers/devfreq/rk3399_dmc.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c index 5063ac1a5939..cf14631b1945 100644 --- a/drivers/devfreq/rk3399_dmc.c +++ b/drivers/devfreq/rk3399_dmc.c @@ -80,7 +80,6 @@ struct rk3399_dmcfreq { struct regulator *vdd_center; unsigned long rate, target_rate; unsigned long volt, target_volt;
- struct dev_pm_opp *curr_opp;
};
static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, @@ -102,9 +101,6 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, target_rate = dev_pm_opp_get_freq(opp); target_volt = dev_pm_opp_get_voltage(opp);
dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp);
dmcfreq->volt = dev_pm_opp_get_voltage(dmcfreq->curr_opp);
rcu_read_unlock();
if (dmcfreq->rate == target_rate)
dmcfreq->rate is used on here.
Are you trying to say that dmcfreq->rate is required to have the right value as we will be using it here for comparison? If yes, then ...
Maybe struct rk3399_dmcfreq need to add the new 'curr_freq' field.
@@ -165,7 +161,9 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, if (err) dev_err(dev, "Cannot to set vol %lu uV\n", target_volt);
- dmcfreq->curr_opp = opp;
- dmcfreq->rate = target_rate;
- dmcfreq->volt = target_volt;
This takes care of it for all cases except the first call to target()...
You're right. I could not think of it.
As you mentioned, except the first call to target(), this patch looks good to me. On the first call, the target() should change the appropriate freq/voltage according to the real utilization.
I'm sorry for my confusion. Feel free to add my reviewed tag.
Reviewed-by: Chanwoo Choi cw00.choi@samsung.com
[snip]
On 02-12-16, 22:26, Chanwoo Choi wrote:
Hi Viresh,
I'm sorry. I sent the mail without any message. It is my mistake.
2016-12-02 22:21 GMT+09:00 Chanwoo Choi cwchoi00@gmail.com:
Hi Viresh,
2016-12-02 18:00 GMT+09:00 Viresh Kumar viresh.kumar@linaro.org:
Hi Chanwoo,
Thanks for trying to review all these patches.
On 02-12-16, 17:42, Chanwoo Choi wrote:
Hi Viresh,
On 2016년 12월 01일 19:38, Viresh Kumar wrote:
The OPP structures are abused to the best here, without understanding how the OPP core and RCU locks work.
In short, the OPP pointer saved in 'rk3399_dmcfreq' can become invalid under your nose, as the OPP core may free it.
Fix various abuses around OPP structures and calls.
Compile tested only.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I would like it to go via the PM tree. Perhaps that's already the default tree for this.
drivers/devfreq/rk3399_dmc.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c index 5063ac1a5939..cf14631b1945 100644 --- a/drivers/devfreq/rk3399_dmc.c +++ b/drivers/devfreq/rk3399_dmc.c @@ -80,7 +80,6 @@ struct rk3399_dmcfreq { struct regulator *vdd_center; unsigned long rate, target_rate; unsigned long volt, target_volt;
- struct dev_pm_opp *curr_opp;
};
static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, @@ -102,9 +101,6 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, target_rate = dev_pm_opp_get_freq(opp); target_volt = dev_pm_opp_get_voltage(opp);
dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp);
dmcfreq->volt = dev_pm_opp_get_voltage(dmcfreq->curr_opp);
rcu_read_unlock();
if (dmcfreq->rate == target_rate)
dmcfreq->rate is used on here.
Are you trying to say that dmcfreq->rate is required to have the right value as we will be using it here for comparison? If yes, then ...
Maybe struct rk3399_dmcfreq need to add the new 'curr_freq' field.
@@ -165,7 +161,9 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, if (err) dev_err(dev, "Cannot to set vol %lu uV\n", target_volt);
- dmcfreq->curr_opp = opp;
- dmcfreq->rate = target_rate;
- dmcfreq->volt = target_volt;
This takes care of it for all cases except the first call to target()...
You're right. I could not think of it.
As you mentioned, except the first call to target(), this patch looks good to me. On the first call, the target() should change the appropriate freq/voltage according to the real utilization.
I'm sorry for my confusion. Feel free to add my reviewed tag.
Reviewed-by: Chanwoo Choi cw00.choi@samsung.com
Perhaps you missed part of my reply. The first call is also taken care of. see below.
@@ -431,8 +429,9 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) rcu_read_unlock(); return PTR_ERR(opp); }
- dmcfreq->rate = dev_pm_opp_get_freq(opp);
- dmcfreq->volt = dev_pm_opp_get_voltage(opp);
And this takes care of the first call to target().
linaro-kernel@lists.linaro.org
-
Chanwoo Choi -
Chanwoo Choi -
Viresh Kumar