linux-kernel - Re: [PATCH v3 3/4] thermal/core: Build ascending ordered indexes for the trip points

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c0d3e523-d75a-d837-313f-bf46f3fab3b4@linaro.org>
Date:   Tue, 19 Jul 2022 09:22:02 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Zhang Rui <rui.zhang@...el.com>, rafael@...nel.org
Cc:     quic_manafm@...cinc.com, amitk@...nel.org, lukasz.luba@....com,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/4] thermal/core: Build ascending ordered indexes for
 the trip points

On 19/07/2022 03:14, Zhang Rui wrote:
> On Mon, 2022-07-18 at 15:21 +0200, Daniel Lezcano wrote:
>>
>> Hi Zhang,
>>
>> thanks for the review
>>
>> On 18/07/2022 07:28, Zhang Rui wrote:
>>> On Fri, 2022-07-15 at 23:09 +0200, Daniel Lezcano wrote:
>>
>> [ ... ]
>>
>>>> Instead of taking the risk of breaking the existing platforms,
>>>> use an
>>>> array of temperature ordered trip identifiers and make it
>>>> available
>>>> for the code needing to browse the trip points in an ordered way.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
>>>> ---
>>
>> [ ... ]
>>
>>>> +static void sort_trips_indexes(struct thermal_zone_device *tz)
>>>> +{
>>>> +       int i, j;
>>>> +
>>>> +       for (i = 0; i < tz->trips; i++)
>>>> +               tz->trips_indexes[i] = i;
>>>> +
>>>> +       for (i = 0; i < tz->trips; i++) {
>>>> +               for (j = i + 1; j < tz->trips; j++) {
>>>> +                       int t1, t2;
>>>> +
>>>> +                       tz->ops->get_trip_temp(tz, tz-
>>>>> trips_indexes[i], &t1);
>>>
>>> This line can be moved to the upper loop.
>>
>> Right, thanks!
>>
>>>> +                       tz->ops->get_trip_temp(tz, tz-
>>>>> trips_indexes[j], &t2);
>>>> +
>>>
>>> what about the disabled trip points?
>>>
>>> we should ignore those trip points and check the return value to
>>> make
>>> sure we're comparing the valid trip_temp values.
>>
>> We don't have to care about, whatever the position, the corresponding
>> trip id will be disabled by the trip init function before calling
>> this
>> one and ignored in the handle_thermal_trip() function
> 
> hah, I missed this one and replied to your latest reply directly.
> 
> The thing I'm concerning is that if we don't check the return value,
> for a disabled trip point, the trip_temp (t1/t2) returned is some
> random value, it all depends on the previous value set by last
> successful .get_trip_temp(), and this may screw up the sorting.

The indexes array is the same size as the trip array, that makes the 
code much less prone to errors.

To have the same number of trip points, the index of the disabled trip 
must be inserted also in the array. We don't care about its position in 
the indexes array because it is discarded in the handle_trip_point() 
function anyway. For this reason, the random temperature of the disabled 
trip point and the resulting position in the sorting is harmless.

It is made on purpose to ignore the return value, so we have a simpler code.

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