| Message ID | 20201217180638.22748-20-digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs | expand |
On 17-12-20, 21:06, Dmitry Osipenko wrote: > Fix adding OPP entries in a wrong (opposite) order if OPP rate is > unavailable. The OPP comparison is erroneously skipped if OPP rate is > missing, thus OPPs are left unsorted. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/opp/core.c | 23 ++++++++++++----------- > drivers/opp/opp.h | 2 +- > 2 files changed, 13 insertions(+), 12 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 34f7e530d941..5c7f130a8de2 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, > return true; > } > > -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2) > +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, > + bool rate_not_available) > { > - if (opp1->rate != opp2->rate) > + if (!rate_not_available && opp1->rate != opp2->rate) rate will be 0 for both the OPPs here if rate_not_available is true and so this change shouldn't be required. > return opp1->rate < opp2->rate ? -1 : 1; > if (opp1->bandwidth && opp2->bandwidth && > opp1->bandwidth[0].peak != opp2->bandwidth[0].peak) > @@ -1545,7 +1546,8 @@ int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2) > > static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp, > struct opp_table *opp_table, > - struct list_head **head) > + struct list_head **head, > + bool rate_not_available) > { > struct dev_pm_opp *opp; > int opp_cmp; > @@ -1559,13 +1561,13 @@ static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp, > * loop. > */ > list_for_each_entry(opp, &opp_table->opp_list, node) { > - opp_cmp = _opp_compare_key(new_opp, opp); > + opp_cmp = _opp_compare_key(new_opp, opp, rate_not_available); > if (opp_cmp > 0) { > *head = &opp->node; > continue; > } > > - if (opp_cmp < 0) > + if (opp_cmp < 0 || rate_not_available) > return 0; This shouldn't be required as well, isn't it ? > > /* Duplicate OPPs */ > @@ -1601,12 +1603,11 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, > mutex_lock(&opp_table->lock); > head = &opp_table->opp_list; > > - if (likely(!rate_not_available)) { > - ret = _opp_is_duplicate(dev, new_opp, opp_table, &head); > - if (ret) { > - mutex_unlock(&opp_table->lock); > - return ret; > - } > + ret = _opp_is_duplicate(dev, new_opp, opp_table, &head, > + rate_not_available); This is the only thing we need to do here I believe. > + if (ret) { > + mutex_unlock(&opp_table->lock); > + return ret; > } > > list_add(&new_opp->node, head); > diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h > index 4ced7ffa8158..6f5be6c72f13 100644 > --- a/drivers/opp/opp.h > +++ b/drivers/opp/opp.h > @@ -219,7 +219,7 @@ struct opp_table *_find_opp_table(struct device *dev); > struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table); > struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table); > void _opp_free(struct dev_pm_opp *opp); > -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2); > +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, bool rate_not_available); > int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available); > int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic); > void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu); > -- > 2.29.2
22.12.2020 12:12, Viresh Kumar пишет: > On 17-12-20, 21:06, Dmitry Osipenko wrote: >> Fix adding OPP entries in a wrong (opposite) order if OPP rate is >> unavailable. The OPP comparison is erroneously skipped if OPP rate is >> missing, thus OPPs are left unsorted. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/opp/core.c | 23 ++++++++++++----------- >> drivers/opp/opp.h | 2 +- >> 2 files changed, 13 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >> index 34f7e530d941..5c7f130a8de2 100644 >> --- a/drivers/opp/core.c >> +++ b/drivers/opp/core.c >> @@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, >> return true; >> } >> >> -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2) >> +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, >> + bool rate_not_available) >> { >> - if (opp1->rate != opp2->rate) >> + if (!rate_not_available && opp1->rate != opp2->rate) > > rate will be 0 for both the OPPs here if rate_not_available is true and so this > change shouldn't be required. The rate_not_available is negated in the condition. This change is required because both rates are 0 and then we should proceed to the levels comparison. I guess it's not clear by looking at this patch, please see a full version of the function: int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, bool rate_not_available) { if (!rate_not_available && opp1->rate != opp2->rate) return opp1->rate < opp2->rate ? -1 : 1; if (opp1->bandwidth && opp2->bandwidth && opp1->bandwidth[0].peak != opp2->bandwidth[0].peak) return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1; if (opp1->level != opp2->level) return opp1->level < opp2->level ? -1 : 1; return 0; } Perhaps we could check whether opp1->rate=0, like it's done for the opp1->bandwidth. I'll consider this variant for v3, thanks.
On 22-12-20, 22:19, Dmitry Osipenko wrote: > 22.12.2020 12:12, Viresh Kumar пишет: > > On 17-12-20, 21:06, Dmitry Osipenko wrote: > >> Fix adding OPP entries in a wrong (opposite) order if OPP rate is > >> unavailable. The OPP comparison is erroneously skipped if OPP rate is > >> missing, thus OPPs are left unsorted. > >> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/opp/core.c | 23 ++++++++++++----------- > >> drivers/opp/opp.h | 2 +- > >> 2 files changed, 13 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c > >> index 34f7e530d941..5c7f130a8de2 100644 > >> --- a/drivers/opp/core.c > >> +++ b/drivers/opp/core.c > >> @@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, > >> return true; > >> } > >> > >> -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2) > >> +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, > >> + bool rate_not_available) > >> { > >> - if (opp1->rate != opp2->rate) > >> + if (!rate_not_available && opp1->rate != opp2->rate) > > > > rate will be 0 for both the OPPs here if rate_not_available is true and so this > > change shouldn't be required. > > The rate_not_available is negated in the condition. This change is > required because both rates are 0 and then we should proceed to the > levels comparison. Won't that happen without this patch ? > I guess it's not clear by looking at this patch, please see a full > version of the function: > > int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, > bool rate_not_available) > { > if (!rate_not_available && opp1->rate != opp2->rate) > return opp1->rate < opp2->rate ? -1 : 1; > if (opp1->bandwidth && opp2->bandwidth && > opp1->bandwidth[0].peak != opp2->bandwidth[0].peak) > return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1; > if (opp1->level != opp2->level) > return opp1->level < opp2->level ? -1 : 1; > return 0; > } > > Perhaps we could check whether opp1->rate=0, like it's done for the > opp1->bandwidth. I'll consider this variant for v3, thanks.
23.12.2020 07:34, Viresh Kumar пишет: > On 22-12-20, 22:19, Dmitry Osipenko wrote: >> 22.12.2020 12:12, Viresh Kumar пишет: >>> On 17-12-20, 21:06, Dmitry Osipenko wrote: >>>> Fix adding OPP entries in a wrong (opposite) order if OPP rate is >>>> unavailable. The OPP comparison is erroneously skipped if OPP rate is >>>> missing, thus OPPs are left unsorted. >>>> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> drivers/opp/core.c | 23 ++++++++++++----------- >>>> drivers/opp/opp.h | 2 +- >>>> 2 files changed, 13 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >>>> index 34f7e530d941..5c7f130a8de2 100644 >>>> --- a/drivers/opp/core.c >>>> +++ b/drivers/opp/core.c >>>> @@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, >>>> return true; >>>> } >>>> >>>> -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2) >>>> +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, >>>> + bool rate_not_available) >>>> { >>>> - if (opp1->rate != opp2->rate) >>>> + if (!rate_not_available && opp1->rate != opp2->rate) >>> >>> rate will be 0 for both the OPPs here if rate_not_available is true and so this >>> change shouldn't be required. >> >> The rate_not_available is negated in the condition. This change is >> required because both rates are 0 and then we should proceed to the >> levels comparison. > > Won't that happen without this patch ? No
On 23-12-20, 23:36, Dmitry Osipenko wrote: > 23.12.2020 07:34, Viresh Kumar пишет: > > On 22-12-20, 22:19, Dmitry Osipenko wrote: > >> 22.12.2020 12:12, Viresh Kumar пишет: > >>> rate will be 0 for both the OPPs here if rate_not_available is true and so this > >>> change shouldn't be required. > >> > >> The rate_not_available is negated in the condition. This change is > >> required because both rates are 0 and then we should proceed to the > >> levels comparison. > > > > Won't that happen without this patch ? > > No This is how the code looks like currently: int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2) { if (opp1->rate != opp2->rate) return opp1->rate < opp2->rate ? -1 : 1; if (opp1->bandwidth && opp2->bandwidth && opp1->bandwidth[0].peak != opp2->bandwidth[0].peak) return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1; if (opp1->level != opp2->level) return opp1->level < opp2->level ? -1 : 1; return 0; } Lets consider the case you are focussing on, where rate is 0 for both the OPPs, bandwidth isn't there and we want to run the level comparison here. Since both the rates are 0, (opp1->rate != opp2->rate) will fail and so we will move to bandwidth check which will fail too. And so we will get to the level comparison. What am I missing here ? I am sure there is something for sure as you won't have missed this..
24.12.2020 09:28, Viresh Kumar пишет: > On 23-12-20, 23:36, Dmitry Osipenko wrote: >> 23.12.2020 07:34, Viresh Kumar пишет: >>> On 22-12-20, 22:19, Dmitry Osipenko wrote: >>>> 22.12.2020 12:12, Viresh Kumar пишет: >>>>> rate will be 0 for both the OPPs here if rate_not_available is true and so this >>>>> change shouldn't be required. >>>> >>>> The rate_not_available is negated in the condition. This change is >>>> required because both rates are 0 and then we should proceed to the >>>> levels comparison. >>> >>> Won't that happen without this patch ? >> >> No > > This is how the code looks like currently: > > int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2) > { > if (opp1->rate != opp2->rate) > return opp1->rate < opp2->rate ? -1 : 1; > if (opp1->bandwidth && opp2->bandwidth && > opp1->bandwidth[0].peak != opp2->bandwidth[0].peak) > return opp1->bandwidth[0].peak < opp2->bandwidth[0].peak ? -1 : 1; > if (opp1->level != opp2->level) > return opp1->level < opp2->level ? -1 : 1; > return 0; > } > > Lets consider the case you are focussing on, where rate is 0 for both the OPPs, > bandwidth isn't there and we want to run the level comparison here. > > Since both the rates are 0, (opp1->rate != opp2->rate) will fail and so we will > move to bandwidth check which will fail too. And so we will get to the level > comparison. > > What am I missing here ? I am sure there is something for sure as you won't have > missed this.. > Ah, you're right. It was me who was missing something as I see now, after taking a closer look and trying to implement yours suggestion, my bad. I'll improve this patch in the next revision, thanks!
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 34f7e530d941..5c7f130a8de2 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1531,9 +1531,10 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp, return true; } -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2) +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, + bool rate_not_available) { - if (opp1->rate != opp2->rate) + if (!rate_not_available && opp1->rate != opp2->rate) return opp1->rate < opp2->rate ? -1 : 1; if (opp1->bandwidth && opp2->bandwidth && opp1->bandwidth[0].peak != opp2->bandwidth[0].peak) @@ -1545,7 +1546,8 @@ int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2) static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, - struct list_head **head) + struct list_head **head, + bool rate_not_available) { struct dev_pm_opp *opp; int opp_cmp; @@ -1559,13 +1561,13 @@ static int _opp_is_duplicate(struct device *dev, struct dev_pm_opp *new_opp, * loop. */ list_for_each_entry(opp, &opp_table->opp_list, node) { - opp_cmp = _opp_compare_key(new_opp, opp); + opp_cmp = _opp_compare_key(new_opp, opp, rate_not_available); if (opp_cmp > 0) { *head = &opp->node; continue; } - if (opp_cmp < 0) + if (opp_cmp < 0 || rate_not_available) return 0; /* Duplicate OPPs */ @@ -1601,12 +1603,11 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, mutex_lock(&opp_table->lock); head = &opp_table->opp_list; - if (likely(!rate_not_available)) { - ret = _opp_is_duplicate(dev, new_opp, opp_table, &head); - if (ret) { - mutex_unlock(&opp_table->lock); - return ret; - } + ret = _opp_is_duplicate(dev, new_opp, opp_table, &head, + rate_not_available); + if (ret) { + mutex_unlock(&opp_table->lock); + return ret; } list_add(&new_opp->node, head); diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h index 4ced7ffa8158..6f5be6c72f13 100644 --- a/drivers/opp/opp.h +++ b/drivers/opp/opp.h @@ -219,7 +219,7 @@ struct opp_table *_find_opp_table(struct device *dev); struct opp_device *_add_opp_dev(const struct device *dev, struct opp_table *opp_table); struct dev_pm_opp *_opp_allocate(struct opp_table *opp_table); void _opp_free(struct dev_pm_opp *opp); -int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2); +int _opp_compare_key(struct dev_pm_opp *opp1, struct dev_pm_opp *opp2, bool rate_not_available); int _opp_add(struct device *dev, struct dev_pm_opp *new_opp, struct opp_table *opp_table, bool rate_not_available); int _opp_add_v1(struct opp_table *opp_table, struct device *dev, unsigned long freq, long u_volt, bool dynamic); void _dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask, int last_cpu);
Fix adding OPP entries in a wrong (opposite) order if OPP rate is unavailable. The OPP comparison is erroneously skipped if OPP rate is missing, thus OPPs are left unsorted. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/opp/core.c | 23 ++++++++++++----------- drivers/opp/opp.h | 2 +- 2 files changed, 13 insertions(+), 12 deletions(-)