| Message ID | 20241022155147.463475-2-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add thermal user thresholds support | expand |
On 10/22/24 16:51, Daniel Lezcano wrote: > Initialize, de-initialize and handle the threshold in the same place > than the trip points. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Link: https://patch.msgid.link/20240923100005.2532430-3-daniel.lezcano@linaro.org > [ rjw: Subject edit ] > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/thermal/thermal_core.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 8f03985f971c..1e87256e86be 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -585,6 +585,8 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz, > high = td->threshold; > } > > + thermal_thresholds_handle(tz, &low, &high); > + > thermal_zone_set_trips(tz, low, high); > > list_sort(NULL, &way_up_list, thermal_trip_notify_cmp); > @@ -1491,6 +1493,10 @@ thermal_zone_device_register_with_trips(const char *type, > goto unregister; > } > > + result = thermal_thresholds_init(tz); > + if (result) > + goto remove_hwmon; > + AFAICS the function thermal_thresholds_init() cannot fail. It always returns 0. So compiler+linker will remove that check anyway. Therefore, I would remove the check in this code. Or maybe you have some other plans in near future to that init function... > mutex_lock(&thermal_list_lock); > > mutex_lock(&tz->lock); > @@ -1514,6 +1520,8 @@ thermal_zone_device_register_with_trips(const char *type, > > return tz; > > +remove_hwmon: > + thermal_remove_hwmon_sysfs(tz); So remove this line as well, since it cannot happen. Although, it's a minor so up to you. > unregister: > device_del(&tz->device); > release_device: > @@ -1601,6 +1609,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz) > > thermal_set_governor(tz, NULL); > > + thermal_thresholds_exit(tz); > thermal_remove_hwmon_sysfs(tz); > ida_free(&thermal_tz_ida, tz->id); > ida_destroy(&tz->ida); Other than that, since no functional issues Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
On 22/10/2024 22:25, Lukasz Luba wrote: > > > On 10/22/24 16:51, Daniel Lezcano wrote: >> Initialize, de-initialize and handle the threshold in the same place >> than the trip points. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> Link: https://patch.msgid.link/20240923100005.2532430-3- >> daniel.lezcano@linaro.org >> [ rjw: Subject edit ] >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> --- >> drivers/thermal/thermal_core.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/ >> thermal_core.c >> index 8f03985f971c..1e87256e86be 100644 >> --- a/drivers/thermal/thermal_core.c >> +++ b/drivers/thermal/thermal_core.c >> @@ -585,6 +585,8 @@ void __thermal_zone_device_update(struct >> thermal_zone_device *tz, >> high = td->threshold; >> } >> + thermal_thresholds_handle(tz, &low, &high); >> + >> thermal_zone_set_trips(tz, low, high); >> list_sort(NULL, &way_up_list, thermal_trip_notify_cmp); >> @@ -1491,6 +1493,10 @@ thermal_zone_device_register_with_trips(const >> char *type, >> goto unregister; >> } >> + result = thermal_thresholds_init(tz); >> + if (result) >> + goto remove_hwmon; >> + > > AFAICS the function thermal_thresholds_init() cannot fail. > It always returns 0. So compiler+linker will remove that check anyway. > Therefore, I would remove the check in this code. > Or maybe you have some other plans in near future to that init > function... Yes, right. However, it is how I wrote the code. If the call site returns an error then the called functions must return an error. That way the code handling the error is already there and when the underlying functions change their logic with a possible error then the call site is not impacted. That makes the code more maintainable for external contributions and as you mentioned the compiler is smart enough to optimize the path so there is no performance penalty. >> mutex_lock(&thermal_list_lock); >> mutex_lock(&tz->lock); >> @@ -1514,6 +1520,8 @@ thermal_zone_device_register_with_trips(const >> char *type, >> return tz; >> +remove_hwmon: >> + thermal_remove_hwmon_sysfs(tz); > > So remove this line as well, since it cannot happen. > Although, it's a minor so up to you. > >> unregister: >> device_del(&tz->device); >> release_device: >> @@ -1601,6 +1609,7 @@ void thermal_zone_device_unregister(struct >> thermal_zone_device *tz) >> thermal_set_governor(tz, NULL); >> + thermal_thresholds_exit(tz); >> thermal_remove_hwmon_sysfs(tz); >> ida_free(&thermal_tz_ida, tz->id); >> ida_destroy(&tz->ida); > > Other than that, since no functional issues > > Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 8f03985f971c..1e87256e86be 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -585,6 +585,8 @@ void __thermal_zone_device_update(struct thermal_zone_device *tz, high = td->threshold; } + thermal_thresholds_handle(tz, &low, &high); + thermal_zone_set_trips(tz, low, high); list_sort(NULL, &way_up_list, thermal_trip_notify_cmp); @@ -1491,6 +1493,10 @@ thermal_zone_device_register_with_trips(const char *type, goto unregister; } + result = thermal_thresholds_init(tz); + if (result) + goto remove_hwmon; + mutex_lock(&thermal_list_lock); mutex_lock(&tz->lock); @@ -1514,6 +1520,8 @@ thermal_zone_device_register_with_trips(const char *type, return tz; +remove_hwmon: + thermal_remove_hwmon_sysfs(tz); unregister: device_del(&tz->device); release_device: @@ -1601,6 +1609,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz) thermal_set_governor(tz, NULL); + thermal_thresholds_exit(tz); thermal_remove_hwmon_sysfs(tz); ida_free(&thermal_tz_ida, tz->id); ida_destroy(&tz->ida);