linux-kernel - Re: [PATCH] thermal/core: Emit a warning if the thermal zone is updated without ops

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: <81e25f27-344e-f6c2-5f08-68068348f7ba@linaro.org>
Date:   Tue, 8 Dec 2020 14:51:41 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Lukasz Luba <lukasz.luba@....com>
Cc:     rui.zhang@...el.com, Thara Gopinath <thara.gopinath@...aro.org>,
        Amit Kucheria <amitk@...nel.org>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] thermal/core: Emit a warning if the thermal zone is
 updated without ops


Hi Lukasz,

On 08/12/2020 10:36, Lukasz Luba wrote:
> Hi Daniel,

[ ... ]

>>     static void thermal_zone_device_init(struct thermal_zone_device *tz)
>> @@ -553,11 +555,9 @@ void thermal_zone_device_update(struct
>> thermal_zone_device *tz,
>>       if (atomic_read(&in_suspend))
>>           return;
>>   -    if (!tz->ops->get_temp)
>> +    if (update_temperature(tz))
>>           return;
>>   -    update_temperature(tz);
>> -
> 
> I think the patch does a bit more. Previously we continued running the
> code below even when the thermal_zone_get_temp() returned an error (due
> to various reasons). Now we stop and probably would not schedule next
> polling, not calling:
> handle_thermal_trip() and monitor_thermal_zone()

I agree there is a change in the behavior.

> I would left update_temperature(tz) as it was and not check the return.
> The function thermal_zone_get_temp() can protect itself from missing
> tz->ops->get_temp(), so we should be safe.
> 
> What do you think?

Does it make sense to handle the trip point if we are unable to read the
temperature?

The lines following the update_temperature() are:

 - thermal_zone_set_trips() which needs a correct tz->temperature

 - handle_thermal_trip() which needs a correct tz->temperature to
compare with

 - monitor_thermal_zone() which needs a consistent tz->passive. This one
is updated by the governor which is in an inconsistent state because the
temperature is not updated.

The problem I see here is how the interrupt mode and the polling mode
are existing in the same code path.

The interrupt mode can call thermal_notify_framework() for critical/hot
trip points without being followed by a monitoring. But for the other
trip points, the get_temp is needed.

IMHO, we should return if update_temperature() is failing.

Perhaps, it would make sense to simply prevent to register a thermal
zone if the get_temp ops is not defined.

AFAICS, if the interrupt mode without get_temp callback are for hot and
critical trip points which can be directly invoked from the sensor via a
specified callback, no thermal zone would be needed in this case.



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