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

[Lukasz Luba <lukasz.luba@arm.com>]

On 10/22/24 08:49, Daniel Lezcano wrote:
> 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

Yes, please add some comment e.g. that size=0 will be then
first modified by the 1st 'if()' so 'size++' will happen
and there is no way that the 2nd 'if()' will trigger before that.
Those 2 'if()' are kind of independent in the code and it's
not obvious from that part of code, why the 2nd 'if()' won't
run at the beginning. The dangerous situation would be:
'__tt[0 - 1].direction = ' assignment, which is due to
'size=0' init value.

> 
>>> +    }
>>> +
>>> +    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.

So we cannot receive that we have new thresholds?
I thought we will get that information, even in runtime, so the old
memory should be just freed.

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

Thanks, I see.

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

We're welcome.