[PATCH V2] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks

[PATCH V2] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks

[Viresh Kumar]

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.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V1->V2:
- Added Reviewed-by from Chanwoo.
- Fixed an issue reported by buildbot.

 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..75333050cdea 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);
 	}
+	data->rate = dev_pm_opp_get_freq(opp);
+	data->volt = dev_pm_opp_get_voltage(opp);
 	rcu_read_unlock();
-	data->curr_opp = opp;
 
 	rk3399_devfreq_dmc_profile.initial_freq = data->rate;
 
-- 
2.7.1.410.g6faf27b

Re: [PATCH V2] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks

[Rafael J. Wysocki]

Hi MyungJoo,

On Mon, Dec 5, 2016 at 4:23 AM, Viresh Kumar <viresh.kumar@linaro.org> 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.
>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V1->V2:
> - Added Reviewed-by from Chanwoo.
> - Fixed an issue reported by buildbot.

I think this along with https://patchwork.kernel.org/patch/9455789/
and https://patchwork.kernel.org/patch/9455757/ should go into
4.10-rc1.

Are you going to send a pull request for this before the merge window
or do you want me to apply them directly?


>  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..75333050cdea 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);
>         }
> +       data->rate = dev_pm_opp_get_freq(opp);
> +       data->volt = dev_pm_opp_get_voltage(opp);
>         rcu_read_unlock();
> -       data->curr_opp = opp;
>
>         rk3399_devfreq_dmc_profile.initial_freq = data->rate;
>
> --

Thanks,
Rafael

Re: [PATCH V2] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks

[Rafael J. Wysocki]

On Tuesday, December 06, 2016 11:40:18 PM Rafael J. Wysocki wrote:
> Hi MyungJoo,
> 
> On Mon, Dec 5, 2016 at 4:23 AM, Viresh Kumar <viresh.kumar@linaro.org> 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.
> >
> > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > V1->V2:
> > - Added Reviewed-by from Chanwoo.
> > - Fixed an issue reported by buildbot.
> 
> I think this along with https://patchwork.kernel.org/patch/9455789/
> and https://patchwork.kernel.org/patch/9455757/ should go into
> 4.10-rc1.
> 
> Are you going to send a pull request for this before the merge window
> or do you want me to apply them directly?

Due to the lack of response, I'm going to apply these three patches
directly.

Thanks,
Rafael