Messages in this thread | | | Subject | Re: [PATCH] thermal/core: Emit a warning if the thermal zone is updated without ops | From | Lukasz Luba <> | Date | Wed, 9 Dec 2020 10:41:44 +0000 |
| |
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.
| |