Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH] thermal/core: Emit a warning if the thermal zone is updated without ops | From | Daniel Lezcano <> | Date | Wed, 9 Dec 2020 13:20:24 +0100 |
| |
On 09/12/2020 11:41, Lukasz Luba wrote: > > > 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?
They can be called directly by the driver when there is no get_temp callback instead of calling thermal_zone_device_update, and by the usual code path via handle_critical_trip function.
Also that can solve the issue [1] when registering a device which is already too hot [1] by adding the ops in the thermal zone.
[1] https://lkml.org/lkml/2020/11/28/166
>> - 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.
-- <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
| ![\](/images/icornerr.gif) |