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

From: Lukasz Luba
Date: Wed Dec 09 2020 - 05:42:38 EST




On 12/8/20 3:19 PM, Daniel Lezcano wrote:
On 08/12/2020 15:37, Lukasz Luba wrote:


On 12/8/20 1:51 PM, Daniel Lezcano wrote:

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.

Yes, I agree that we can bail out when there is no .get_temp() callback
and even not schedule next polling in such case.
But I am just not sure if we can bail out and not schedule the next
polling, when there is .get_temp() populated and the driver returned
an error only at that moment, e.g. indicating some internal temporary,
issue like send queue full, so such as -EBUSY, or -EAGAIN, etc.
The thermal_zone_get_temp() would pass the error to update_temperature()
but we return, losing the next try. We would not check the temperature
again.

Hmm, right. I agree with your point.

What about the following changes:

- Add the new APIs:

thermal_zone_device_critical(struct thermal_zone_device *tz);
=> emergency poweroff

thermal_zone_device_hot(struct thermal_zone_device *tz);
=> userspace notification

They look promising, I have to look into the existing code.
When they would be called?


- Add a big fat WARN when thermal_zone_device_update is called with
.get_temp == NULL because that must not happen.

Good idea


If the .get_temp is NULL it is because we only have a HOT/CRITICAL
thermal trip points where we don't care about the temperature and
governor decision, right ?


That is a good question. Let me dig into the code. I would say yes - we
don't have to hassle with governor in this circumstances.