devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks

Message ID 22bf6dfc155a6a241c2ee4e2b44a615d0e82064d.1480588658.git.viresh.kumar@linaro.org
State Superseded
Headers show

Commit Message

Viresh Kumar Dec. 1, 2016, 10:38 a.m.
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(-)

-- 
2.7.1.410.g6faf27b

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Chanwoo Choi Dec. 2, 2016, 8:42 a.m. | #1
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;

>  

> 


-- 
Best Regards,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 2, 2016, 9 a.m. | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chanwoo Choi Dec. 2, 2016, 1:21 p.m. | #3
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




-- 
Best Regards,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chanwoo Choi Dec. 2, 2016, 1:26 p.m. | #4
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]

-- 
Best Regards,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 2, 2016, 1:53 p.m. | #5
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().

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;