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

[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