diff mbox series

[v2,19/48] opp: Fix adding OPP entries in a wrong order if rate is unavailable

Message ID 20201217180638.22748-20-digetx@gmail.com
State New
Headers show
Series [v2,01/48] dt-bindings: memory: tegra20: emc: Replace core regulator with power domain | expand

Commit Message

Dmitry Osipenko Dec. 17, 2020, 6:06 p.m. UTC
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(-)

Comments

Viresh Kumar Dec. 22, 2020, 9:12 a.m. UTC | #1
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
Dmitry Osipenko Dec. 22, 2020, 7:19 p.m. UTC | #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.
Viresh Kumar Dec. 23, 2020, 4:34 a.m. UTC | #3
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.
Dmitry Osipenko Dec. 23, 2020, 8:36 p.m. UTC | #4
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
Viresh Kumar Dec. 24, 2020, 6:28 a.m. UTC | #5
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..
Dmitry Osipenko Dec. 24, 2020, 12:14 p.m. UTC | #6
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 mbox series

Patch

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);