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 - Add a big fat WARN when thermal_zone_device_update is called with .get_temp == NULL because that must not happen. 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 ? -- <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