linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks
@ 2016-12-01 10:38 ` Viresh Kumar
  2016-12-02  8:42   ` Chanwoo Choi
  0 siblings, 1 reply; 6+ messages in thread
From: Viresh Kumar @ 2016-12-01 10:38 UTC (permalink / raw)
  To: Rafael Wysocki, MyungJoo Ham, Kyungmin Park
  Cc: linaro-kernel, linux-pm, Stephen Boyd, nm, Vincent Guittot,
	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.

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;
 
-- 
2.7.1.410.g6faf27b


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks
  2016-12-01 10:38 ` [PATCH] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks Viresh Kumar
@ 2016-12-02  8:42   ` Chanwoo Choi
  2016-12-02  9:00     ` Viresh Kumar
  0 siblings, 1 reply; 6+ messages in thread
From: Chanwoo Choi @ 2016-12-02  8:42 UTC (permalink / raw)
  To: Viresh Kumar, Rafael Wysocki, MyungJoo Ham, Kyungmin Park
  Cc: linaro-kernel, linux-pm, Stephen Boyd, nm, Vincent Guittot

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks
  2016-12-02  8:42   ` Chanwoo Choi
@ 2016-12-02  9:00     ` Viresh Kumar
  2016-12-02 13:21       ` Chanwoo Choi
  0 siblings, 1 reply; 6+ messages in thread
From: Viresh Kumar @ 2016-12-02  9:00 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Rafael Wysocki, MyungJoo Ham, Kyungmin Park, linaro-kernel,
	linux-pm, Stephen Boyd, nm, Vincent Guittot

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks
  2016-12-02  9:00     ` Viresh Kumar
@ 2016-12-02 13:21       ` Chanwoo Choi
  2016-12-02 13:26         ` Chanwoo Choi
  0 siblings, 1 reply; 6+ messages in thread
From: Chanwoo Choi @ 2016-12-02 13:21 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Chanwoo Choi, nm, linaro-kernel, linux-pm@vger.kernel.org,
	Rafael Wysocki, Stephen Boyd, Kyungmin Park, MyungJoo Ham

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks
  2016-12-02 13:21       ` Chanwoo Choi
@ 2016-12-02 13:26         ` Chanwoo Choi
  2016-12-02 13:53           ` Viresh Kumar
  0 siblings, 1 reply; 6+ messages in thread
From: Chanwoo Choi @ 2016-12-02 13:26 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Chanwoo Choi, nm, linaro-kernel, linux-pm@vger.kernel.org,
	Rafael Wysocki, Stephen Boyd, Kyungmin Park, MyungJoo Ham

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks
  2016-12-02 13:26         ` Chanwoo Choi
@ 2016-12-02 13:53           ` Viresh Kumar
  0 siblings, 0 replies; 6+ messages in thread
From: Viresh Kumar @ 2016-12-02 13:53 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Chanwoo Choi, nm, linaro-kernel, linux-pm@vger.kernel.org,
	Rafael Wysocki, Stephen Boyd, Kyungmin Park, MyungJoo Ham

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-12-02 13:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <CGME20161201103813epcas4p116430ac17cea842a7f09b35f0969ccb0@epcas4p1.samsung.com>
2016-12-01 10:38 ` [PATCH] devfreq: rk3399_dmc: Don't use OPP structures outside of RCU locks Viresh Kumar
2016-12-02  8:42   ` Chanwoo Choi
2016-12-02  9:00     ` Viresh Kumar
2016-12-02 13:21       ` Chanwoo Choi
2016-12-02 13:26         ` Chanwoo Choi
2016-12-02 13:53           ` Viresh Kumar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).