| Message ID | 22bf6dfc155a6a241c2ee4e2b44a615d0e82064d.1480588658.git.viresh.kumar@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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;
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