| Message ID | 7017fa592bdaf73c260ad001a2b7abdc8d14f08a.1416211616.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Monday, November 17, 2014 01:38:00 PM Viresh Kumar wrote: > Giving a warning in case we add duplicate OPPs doesn't workout that great. For > example just playing with cpufreq-dt driver as a module results in this: > > $ modprobe cpufreq-dt > $ modprobe -r cpufreq-dt > $ modprobe cpufreq-dt > > cpu cpu0: dev_pm_opp_add: duplicate OPPs detected. Existing: > freq: 261819000, volt: 1350000, enabled: 1. New: freq: 261819000, volt: 1350000, > enabled: 1 > cpu cpu0: dev_pm_opp_add: duplicate OPPs detected. Existing: > freq: 360000000, volt: 1350000, enabled: 1. New: freq: 360000000, volt: 1350000, > enabled: 1 > cpu cpu0: dev_pm_opp_add: duplicate OPPs detected. Existing: > freq: 392728000, volt: 1450000, enabled: 1. New: freq: 392728000, volt: 1450000, > enabled: 1 > cpu cpu0: dev_pm_opp_add: duplicate OPPs detected. Existing: > freq: 454737000, volt: 1550000, enabled: 1. New: freq: 454737000, volt: 1550000, > enabled: 1 > > This happens because we don't destroy OPPs (created during ->init()) while > unloading modules. > > Now the question is: Should we destroy these OPPs? > > Logically kernel drivers *must* free resources they acquired. But in this > particular case, the OPPs are created using a static list present in device > tree. Destroying and then allocating them again isn't of much benefit. The only > benefit of removing OPPs is to save some space if the driver isn't loaded again. > > This has its own complications. OPPs can be created either from DT (static) or > platform code (dynamic). Driver should only remove static OPPs and not the > dynamic ones as they are controlled from platform code. But there is no field in > 'struct dev_pm_opp' which has this information to distinguish between different > kind of OPPs. > > Because of all this, I wasn't sure if drivers should remove static OPPs during > their removal. And so just fixing the reported issue by issuing a dev_dbg() > instead of dev_warn(). > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/base/power/opp.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > index 89ced95..490e9db 100644 > --- a/drivers/base/power/opp.c > +++ b/drivers/base/power/opp.c > @@ -466,9 +466,9 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) > int ret = opp->available && new_opp->u_volt == opp->u_volt ? > 0 : -EEXIST; > > - dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", > - __func__, opp->rate, opp->u_volt, opp->available, > - new_opp->rate, new_opp->u_volt, new_opp->available); > + dev_dbg(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", > + __func__, opp->rate, opp->u_volt, opp->available, > + new_opp->rate, new_opp->u_volt, new_opp->available); > mutex_unlock(&dev_opp_list_lock); > kfree(new_opp); > return ret; Don't you think that this may hide real bugs? Rafael -- 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 18 November 2014 05:09, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Monday, November 17, 2014 01:38:00 PM Viresh Kumar wrote: >> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c >> index 89ced95..490e9db 100644 >> --- a/drivers/base/power/opp.c >> +++ b/drivers/base/power/opp.c >> @@ -466,9 +466,9 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) >> int ret = opp->available && new_opp->u_volt == opp->u_volt ? >> 0 : -EEXIST; >> >> - dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", >> - __func__, opp->rate, opp->u_volt, opp->available, >> - new_opp->rate, new_opp->u_volt, new_opp->available); >> + dev_dbg(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", >> + __func__, opp->rate, opp->u_volt, opp->available, >> + new_opp->rate, new_opp->u_volt, new_opp->available); >> mutex_unlock(&dev_opp_list_lock); >> kfree(new_opp); >> return ret; > > Don't you think that this may hide real bugs? What kind of bugs exactly? We are allowing addition of duplicate OPPs as a standard thing right now as cpufreq drivers don't get rid of the OPPs they create with DT. So, that shouldn't complain, isn't it ? For example, what Stefan was doing was absolutely normal procedure and that complained for him.. The only thing we don't allow is when the existing OPP isn't available and we are trying to add a duplicate one. In that case we do return -EEXIST and so we will get errors from the upper layer. Or do we want to destroy OPPs created with help of DT while the driver unloads ? -- 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
On Tuesday, November 18, 2014 08:38:14 AM Viresh Kumar wrote: > On 18 November 2014 05:09, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Monday, November 17, 2014 01:38:00 PM Viresh Kumar wrote: > >> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > >> index 89ced95..490e9db 100644 > >> --- a/drivers/base/power/opp.c > >> +++ b/drivers/base/power/opp.c > >> @@ -466,9 +466,9 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) > >> int ret = opp->available && new_opp->u_volt == opp->u_volt ? > >> 0 : -EEXIST; > >> > >> - dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", > >> - __func__, opp->rate, opp->u_volt, opp->available, > >> - new_opp->rate, new_opp->u_volt, new_opp->available); > >> + dev_dbg(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", > >> + __func__, opp->rate, opp->u_volt, opp->available, > >> + new_opp->rate, new_opp->u_volt, new_opp->available); > >> mutex_unlock(&dev_opp_list_lock); > >> kfree(new_opp); > >> return ret; > > > > Don't you think that this may hide real bugs? > > What kind of bugs exactly? > > We are allowing addition of duplicate OPPs as a standard thing right now > as cpufreq drivers don't get rid of the OPPs they create with DT. So, that > shouldn't complain, isn't it ? Is cpufreq the only user of OPP? I thought there were other users, so what about them? > For example, what Stefan was doing was absolutely normal procedure > and that complained for him.. > > The only thing we don't allow is when the existing OPP isn't available > and we are trying to add a duplicate one. In that case we do return > -EEXIST and so we will get errors from the upper layer. > > Or do we want to destroy OPPs created with help of DT while the > driver unloads ? I'm not sure about that. If they aren't useful for anything after that, what's the benefit of keeping them around?
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 89ced95..490e9db 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -466,9 +466,9 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt) int ret = opp->available && new_opp->u_volt == opp->u_volt ? 0 : -EEXIST; - dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", - __func__, opp->rate, opp->u_volt, opp->available, - new_opp->rate, new_opp->u_volt, new_opp->available); + dev_dbg(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n", + __func__, opp->rate, opp->u_volt, opp->available, + new_opp->rate, new_opp->u_volt, new_opp->available); mutex_unlock(&dev_opp_list_lock); kfree(new_opp); return ret;
Giving a warning in case we add duplicate OPPs doesn't workout that great. For example just playing with cpufreq-dt driver as a module results in this: $ modprobe cpufreq-dt $ modprobe -r cpufreq-dt $ modprobe cpufreq-dt cpu cpu0: dev_pm_opp_add: duplicate OPPs detected. Existing: freq: 261819000, volt: 1350000, enabled: 1. New: freq: 261819000, volt: 1350000, enabled: 1 cpu cpu0: dev_pm_opp_add: duplicate OPPs detected. Existing: freq: 360000000, volt: 1350000, enabled: 1. New: freq: 360000000, volt: 1350000, enabled: 1 cpu cpu0: dev_pm_opp_add: duplicate OPPs detected. Existing: freq: 392728000, volt: 1450000, enabled: 1. New: freq: 392728000, volt: 1450000, enabled: 1 cpu cpu0: dev_pm_opp_add: duplicate OPPs detected. Existing: freq: 454737000, volt: 1550000, enabled: 1. New: freq: 454737000, volt: 1550000, enabled: 1 This happens because we don't destroy OPPs (created during ->init()) while unloading modules. Now the question is: Should we destroy these OPPs? Logically kernel drivers *must* free resources they acquired. But in this particular case, the OPPs are created using a static list present in device tree. Destroying and then allocating them again isn't of much benefit. The only benefit of removing OPPs is to save some space if the driver isn't loaded again. This has its own complications. OPPs can be created either from DT (static) or platform code (dynamic). Driver should only remove static OPPs and not the dynamic ones as they are controlled from platform code. But there is no field in 'struct dev_pm_opp' which has this information to distinguish between different kind of OPPs. Because of all this, I wasn't sure if drivers should remove static OPPs during their removal. And so just fixing the reported issue by issuing a dev_dbg() instead of dev_warn(). Reported-by: Stefan Wahren <stefan.wahren@i2se.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/base/power/opp.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)