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

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

On 22/10/2024 11:50, Lukasz Luba wrote:
> 
> 
> 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>
>>>> ---

[ ... ]

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

Sorry may be I was unclear. I meant we will have the list of thresholds 
and we will then receive event when they are created, deleted, etc ...


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