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