Re: [PATCH] thermal/thresholds: Fix boundaries and detection routine

[Daniel Lezcano <daniel.lezcano@linaro.org>]

Hi Manaf,

On 12/15/24 20:02, Manaf Meethalavalappu Pallikunhi wrote:
> 
> Hi Daniel,

[ ... ]

>> -static bool thermal_thresholds_handle_raising(struct list_head 
>> *thresholds, int temperature,
>> -                          int last_temperature, int *low, int *high)
>> +static bool thermal_thresholds_handle_dropping(struct list_head 
>> *thresholds, int temperature,
>> +                           int last_temperature)
>>   {
>>       struct user_threshold *t;
>> -    list_for_each_entry(t, thresholds, list_node) {
>> -        if (__thermal_threshold_is_crossed(t, temperature, 
>> last_temperature,
>> -                           THERMAL_THRESHOLD_WAY_UP, low, high))
>> +    list_for_each_entry_reverse(t, thresholds, list_node) {
>> +
>> +        if (!(t->direction & THERMAL_THRESHOLD_WAY_DOWN))
>> +            continue;
>> +
>> +        if (temperature < t->temperature &&
>> +            last_temperature >= t->temperature)
>>               return true;
> 
> Currently WAY_UP notification triggers if temperature is greater than or 
> equal to t-> temperature, but for WAY_DOWN
> 
> it is only checking temperature is less than t->temperature. Why don't 
> we include temperature = t->temperature
> 
> for WAY_DOWN threshold ? In that case it will be consistent for both 
> WAY_UP and WAY_DOWN notification for userspace.
> 
> If we are not considering 'equal to' for WAY_DOWN, there is a 
> possibility of missing WAY_DOWN notification if a sensor
> 
> is violated with same WAY_DOWN threshold temperature and only interrupt 
> mode is enabled for that sensor.

You are right, we should detect when the temperature is lesser or equal 
to the threshold to be crossed the way down.

I'll send a V2 with this condition fixed.

Thanks for reporting this

>>       }
>>       return false;
>>   }
>> -static bool thermal_thresholds_handle_dropping(struct list_head 
>> *thresholds, int temperature,
>> -                           int last_temperature, int *low, int *high)
>> +static void thermal_threshold_find_boundaries(struct list_head 
>> *thresholds, int temperature,
>> +                          int *low, int *high)
>>   {
>>       struct user_threshold *t;
>> -    list_for_each_entry_reverse(t, thresholds, list_node) {
>> -        if (__thermal_threshold_is_crossed(t, temperature, 
>> last_temperature,
>> -                           THERMAL_THRESHOLD_WAY_DOWN, low, high))
>> -            return true;
>> +    list_for_each_entry(t, thresholds, list_node) {
>> +        if (temperature < t->temperature &&
>> +            (t->direction & THERMAL_THRESHOLD_WAY_UP) &&
>> +            *high > t->temperature)
>> +            *high = t->temperature;
>>       }
>> -    return false;
>> +    list_for_each_entry_reverse(t, thresholds, list_node) {
>> +        if (temperature > t->temperature &&
>> +            (t->direction & THERMAL_THRESHOLD_WAY_DOWN) &&
>> +            *low < t->temperature)
>> +            *low = t->temperature;
>> +    }
>>   }
>>   void thermal_thresholds_handle(struct thermal_zone_device *tz, int 
>> *low, int *high)
>> @@ -132,6 +134,8 @@ void thermal_thresholds_handle(struct 
>> thermal_zone_device *tz, int *low, int *hi
>>       lockdep_assert_held(&tz->lock);
>> +    thermal_threshold_find_boundaries(thresholds, temperature, low, 
>> high);
>> +
>>       /*
>>        * We need a second update in order to detect a threshold being 
>> crossed
>>        */
>> @@ -151,12 +155,12 @@ void thermal_thresholds_handle(struct 
>> thermal_zone_device *tz, int *low, int *hi
>>        * - decreased : thresholds are crossed the way down
>>        */
>>       if (temperature > last_temperature) {
>> -        if (thermal_thresholds_handle_raising(thresholds, temperature,
>> -                              last_temperature, low, high))
>> +        if (thermal_thresholds_handle_raising(thresholds,
>> +                              temperature, last_temperature))
>>               thermal_notify_threshold_up(tz);
>>       } else {
>> -        if (thermal_thresholds_handle_dropping(thresholds, temperature,
>> -                               last_temperature, low, high))
>> +        if (thermal_thresholds_handle_dropping(thresholds,
>> +                               temperature, last_temperature))
>>               thermal_notify_threshold_down(tz);
>>       }
>>   }


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog