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

[Daniel Lezcano]

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