Re: [PATCH v5 3/4] tools/lib/thermal: Add the threshold netlink ABI

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

On 21/10/2024 22:47, Lukasz Luba wrote:
> 
> 
> On 10/14/24 10:43, Daniel Lezcano wrote:
>> The thermal framework supports the thresholds and allows the userspace
>> to create, delete, flush, get the list of the thresholds as well as
>> getting the list of the thresholds set for a specific thermal zone.
>>
>> Add the netlink abstraction in the thermal library to take full
>> advantage of thresholds for the userspace program.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>   tools/lib/thermal/commands.c        | 128 +++++++++++++++++++++++++++-
>>   tools/lib/thermal/events.c          |  55 +++++++++---
>>   tools/lib/thermal/include/thermal.h |  40 +++++++++
>>   tools/lib/thermal/libthermal.map    |   5 ++
>>   tools/lib/thermal/thermal.c         |  17 ++++
>>   tools/thermal/lib/Makefile          |   2 +-
>>   6 files changed, 232 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/lib/thermal/commands.c b/tools/lib/thermal/commands.c
>> index a9223df91dcf..9d5e3e891628 100644
>> --- a/tools/lib/thermal/commands.c
>> +++ b/tools/lib/thermal/commands.c
>> @@ -5,6 +5,7 @@
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   #include <unistd.h>
>> +#include <limits.h>
>>   #include <thermal.h>
>>   #include "thermal_nl.h"
>> @@ -33,6 +34,11 @@ static struct nla_policy 
>> thermal_genl_policy[THERMAL_GENL_ATTR_MAX + 1] = {
>>       [THERMAL_GENL_ATTR_CDEV_CUR_STATE]      = { .type = NLA_U32 },
>>       [THERMAL_GENL_ATTR_CDEV_MAX_STATE]      = { .type = NLA_U32 },
>>       [THERMAL_GENL_ATTR_CDEV_NAME]           = { .type = NLA_STRING },
>> +
>> +        /* Thresholds */
>> +        [THERMAL_GENL_ATTR_THRESHOLD]          = { .type = NLA_NESTED },
>> +        [THERMAL_GENL_ATTR_THRESHOLD_TEMP]      = { .type = NLA_U32 },
>> +        [THERMAL_GENL_ATTR_THRESHOLD_DIRECTION] = { .type = NLA_U32 },
>>   };
>>   static int parse_tz_get(struct genl_info *info, struct thermal_zone 
>> **tz)
>> @@ -182,6 +188,38 @@ static int parse_tz_get_gov(struct genl_info 
>> *info, struct thermal_zone *tz)
>>       return THERMAL_SUCCESS;
>>   }
>> +static int parse_threshold_get(struct genl_info *info, struct 
>> thermal_zone *tz)
>> +{
>> +    struct nlattr *attr;
>> +    struct thermal_threshold *__tt = NULL;
>> +    size_t size = 0;
> 
> why not simply 'int' ?

Because it is used with realloc where the function definition is:

void *realloc(void *_Nullable ptr, size_t size);

and sizeof() return a size_t type.

So in order to not mix the types, size_t is used which I believe is a 
good practice :)

> also: size=0 which has some impact mentioned below.
> 
>> +    int rem;
> 
> I would make them a descending order, those lines.
> 
>> +
>> +    nla_for_each_nested(attr, info- 
>> >attrs[THERMAL_GENL_ATTR_THRESHOLD], rem) {
>> +
>> +        if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_TEMP) {
>> +
>> +            size++;
>> +
>> +            __tt = realloc(__tt, sizeof(*__tt) * (size + 2));
>> +            if (!__tt)
>> +                return THERMAL_ERROR;
>> +
>> +            __tt[size - 1].temperature = nla_get_u32(attr);
>> +        }
>> +
>> +        if (nla_type(attr) == THERMAL_GENL_ATTR_THRESHOLD_DIRECTION)
>> +            __tt[size - 1].direction = nla_get_u32(attr);
> 
> We probably relay on some order here, because the 'size -1' needs to be
> done after first 'size++'.
> If that the case then maybe it's worth a comment. Or if it wasn't
> intended and there are no strong guarantees, then this needs a fix.

The size contains the size of the array and we want to access the last 
element, size - 1

I will add this sentence above as a comment if it is ok for you

>> +    }
>> +
>> +    if (__tt)
>> +        __tt[size].temperature = INT_MAX;
>> +
>> +    tz->thresholds = __tt;
> 
> I wonder what would happen to the previous 'tz->thresholds' when
> we just put new one here... I cannot find other place when it's set.
> 
> Since we have '*__tt = NULL' then one of the solutions would be
> to simply call:
>      free(tz->thresholds);
>      tz->thresholds = __tt;
> 
> Am I missing something, when it might be cleaned in different place?

The caller is supposed to pass a clean empty structure.

Usually, this function is to discover the current configuration, so it 
is a one shot call keeping the structure in memory for the libthermal 
lifecycle.

The events sends updates of the thermal zones. So with the events and 
the initial configuration from the discovery, the userspace is always 
up-to-date with the thermal setup.


>> +
>> +    return THERMAL_SUCCESS;
>> +}
>> +
>>   static int handle_netlink(struct nl_cache_ops *unused,
>>                 struct genl_cmd *cmd,
>>                 struct genl_info *info, void *arg)
>> @@ -210,6 +248,10 @@ static int handle_netlink(struct nl_cache_ops 
>> *unused,
>>           ret = parse_tz_get_gov(info, arg);
>>           break;
>> +    case THERMAL_GENL_CMD_THRESHOLD_GET:
>> +        ret = parse_threshold_get(info, arg);
>> +        break;
>> +
> 
> I can see in the kernel part in the funciton:
> thermal_genl_cmd_doit()
> that there add, delete, flush also send a response
> message. Shouldn't be handled here gently, otherwise

No, those are commands and the transaction is done at the netlink level 
to say if the command was successful or not.

>>       default:
>>           return THERMAL_ERROR;
> 
> that 'default' might capture them?

[ ... ]

>>   int for_each_thermal_cdev(struct thermal_cdev *cdev, cb_tc_t cb, 
>> void *arg)
>>   {
>>       int i, ret = 0;
>> @@ -80,6 +94,9 @@ static int __thermal_zone_discover(struct 
>> thermal_zone *tz, void *th)
>>       if (thermal_cmd_get_trip(th, tz) < 0)
>>           return -1;
>> +    if (thermal_cmd_threshold_get(th, tz) < 0)
> 
> There are only 2 definitions in the enum thermal_error_t.
> I would just simply checked if it's not 0:


Ok

Thanks for the review

>      if (thermal_cmd_threshold_get(th, tz))
>          return -1;
> 
> Although, it's a minor thing, not affecting the end result.
> 
>> +        return -1;
>> +
>>       if (thermal_cmd_get_governor(th, tz))
>>           return -1;
>> diff --git a/tools/thermal/lib/Makefile b/tools/thermal/lib/Makefile
>> index 82db451935c5..f2552f73a64c 100644
>> --- a/tools/thermal/lib/Makefile
>> +++ b/tools/thermal/lib/Makefile
>> @@ -3,7 +3,7 @@
>>   LIBTHERMAL_TOOLS_VERSION = 0
>>   LIBTHERMAL_TOOLS_PATCHLEVEL = 0
>> -LIBTHERMAL_TOOLS_EXTRAVERSION = 1
>> +LIBTHERMAL_TOOLS_EXTRAVERSION = 2
>>   MAKEFLAGS += --no-print-directory


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