Re: [PATCH v2 2/7] thermal/core: Add thresholds support

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

On 22/08/2024 13:30, Rafael J. Wysocki wrote:
> On Wed, Aug 21, 2024 at 10:05 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Fri, Aug 16, 2024 at 10:12 AM Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
> 
> [cut]
> 
>>> --- /dev/null
>>> +++ b/drivers/thermal/thermal_thresholds.c
>>> @@ -0,0 +1,241 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright 2024 Linaro Limited
>>> + *
>>> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> + *
>>> + * Thermal thresholds
>>> + */
>>> +#include <linux/list.h>
>>> +#include <linux/list_sort.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#include "thermal_core.h"
>>
>> +#include "thermal_thresholds.h"
>>
>>> +
>>> +struct thresholds {
>>> +       struct list_head list;
>>> +};
>>
>> This duplicates the definition in the header file.
>>
>> Besides, why is the wrapper struct needed?
> 
> On second thought, it can hold a pointer to the first threshold that
> was strictly above the zone temperature when
> thermal_thresholds_handle() ran last time, so something like:
> 
> struct thresholds {
>        struct list_head list;
>        struct user_threhold *first_above;
> };
> 
> and first_above == NULL would mean that the zone temperature was above
> all of the threshold or at the highest one.
> 
> Then thermal_thresholds_handle() could do something like:
> 
> tr = tz->user_thresholds.first_above;
> 
> if (tr && tz->temperature >= tr->temperature) {
>      do {
>          if (tr->direction & THERMAL_THRESHOLD_WAY_UP)
>              notify = true;
> 
>          if (tr->list_node.next != &tz->user_thresholds.list) {
>              tr = list_next_entry(tr, list_node);
>          else
>              tr = NULL;
>      } while (tr && tz->temperature >= tr->temperature);
> } else {
>      if (!tr)
>          tr = list_last_entry(&tz->user_thresholds.list,
>                       struct user_threshold, list_node);
> 
>      while (tz->temperature < tr->temperature)
>          if (tr->direction & THERMAL_THRESHOLD_WAY_DOWN)
>              notify = true;
> 
>          if (tr->list_node.prev != &tz->user_thresholds.list) {
>              tr = list_prev_entry(tr, list_node);
>          else
>              break;
>      }
>      if (tz->temperature >= tr->temperature)
>          tr = NULL;
> }
> tz->user_thresholds.first_above = tr;
> 
> which is a bit simpler than the code in the current patch.

TBH, it is probably a matter of taste but I don't see how it is simpler, 
especially if someone wants to to understand or add something in the 
code. I would prefer to keep the initial routine which was already tested.

[ ... ]

> 
> Also, I'm not sure why "high" and "low" are needed at all.
> 
> The current and last zone temperature could be included in the
> notification just fine and user space should be able to figure out
> which thresholds are affected.

I'm not sure to get the question. high and low are needed because the 
thermal core will call set_trips(). The trips and the thresholds are 
combined to find out the high and low temperature to program the driver 
to trigger an interrupt.


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