| Message ID | 5737811.DvuYhMxLoT@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] thermal: trip: Send trip change notifications on all trip updates | expand |
On Wed, Dec 6, 2023 at 5:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > Hi Daniel, > > On Wed, Dec 6, 2023 at 3:38 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > > > > > Hi Rafael, > > > > On 05/12/2023 20:18, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > The _store callbacks of the trip point temperature and hysteresis sysfs > > > attributes invoke thermal_notify_tz_trip_change() to send a notification > > > regarding the trip point change, but when trip points are updated by the > > > platform firmware, trip point change notifications are not sent. > > > > > > To make the behavior after a trip point change more consistent, > > > modify all of the 3 places where trip point temperature is updated > > > to use a new function called thermal_zone_set_trip_temp() for this > > > purpose and make that function call thermal_notify_tz_trip_change(). > > > > > > Note that trip point hysteresis can only be updated via sysfs and > > > trip_point_hyst_store() calls thermal_notify_tz_trip_change() already, > > > so this code path need not be changed. > > > > Why the ACPI driver is not calling thermal_zone_device_update() after > > changing the trip point like the other drivers? > > It calls that function, but because it may update multiple trips in > one go, it does that after all of the updates are done, via > acpi_thermal_check_fn(). > > > It would make sense to have the thermal framework to be notified about > > this change and check if there is a trip violation, no ? > > It is notified as noted above, but not synchronously. I believe that the question above has been answered, so are there any other comments or concerns regarding this patch?
On 12/12/2023 20:04, Rafael J. Wysocki wrote: > On Wed, Dec 6, 2023 at 5:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >> >> Hi Daniel, >> >> On Wed, Dec 6, 2023 at 3:38 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >>> >>> >>> Hi Rafael, >>> >>> On 05/12/2023 20:18, Rafael J. Wysocki wrote: >>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>> >>>> The _store callbacks of the trip point temperature and hysteresis sysfs >>>> attributes invoke thermal_notify_tz_trip_change() to send a notification >>>> regarding the trip point change, but when trip points are updated by the >>>> platform firmware, trip point change notifications are not sent. >>>> >>>> To make the behavior after a trip point change more consistent, >>>> modify all of the 3 places where trip point temperature is updated >>>> to use a new function called thermal_zone_set_trip_temp() for this >>>> purpose and make that function call thermal_notify_tz_trip_change(). >>>> >>>> Note that trip point hysteresis can only be updated via sysfs and >>>> trip_point_hyst_store() calls thermal_notify_tz_trip_change() already, >>>> so this code path need not be changed. >>> >>> Why the ACPI driver is not calling thermal_zone_device_update() after >>> changing the trip point like the other drivers? >> >> It calls that function, but because it may update multiple trips in >> one go, it does that after all of the updates are done, via >> acpi_thermal_check_fn(). >> >>> It would make sense to have the thermal framework to be notified about >>> this change and check if there is a trip violation, no ? >> >> It is notified as noted above, but not synchronously. > > I believe that the question above has been answered, so are there any > other comments or concerns regarding this patch? No, it is fine. Thanks for the clarification Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Index: linux-pm/drivers/thermal/thermal_sysfs.c =================================================================== --- linux-pm.orig/drivers/thermal/thermal_sysfs.c +++ linux-pm/drivers/thermal/thermal_sysfs.c @@ -146,9 +146,9 @@ trip_point_temp_store(struct device *dev goto unlock; } - trip->temperature = temp; + thermal_zone_set_trip_temp(tz, trip, temp); - thermal_zone_trip_updated(tz, trip); + __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); } unlock: Index: linux-pm/drivers/thermal/thermal_trip.c =================================================================== --- linux-pm.orig/drivers/thermal/thermal_trip.c +++ linux-pm/drivers/thermal/thermal_trip.c @@ -152,7 +152,6 @@ int thermal_zone_trip_id(struct thermal_ */ return trip - tz->trips; } - void thermal_zone_trip_updated(struct thermal_zone_device *tz, const struct thermal_trip *trip) { @@ -161,3 +160,16 @@ void thermal_zone_trip_updated(struct th trip->hysteresis); __thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED); } + +void thermal_zone_set_trip_temp(struct thermal_zone_device *tz, + struct thermal_trip *trip, int temp) +{ + if (trip->temperature == temp) + return; + + trip->temperature = temp; + thermal_notify_tz_trip_change(tz->id, thermal_zone_trip_id(tz, trip), + trip->type, trip->temperature, + trip->hysteresis); +} +EXPORT_SYMBOL_GPL(thermal_zone_set_trip_temp); Index: linux-pm/include/linux/thermal.h =================================================================== --- linux-pm.orig/include/linux/thermal.h +++ linux-pm/include/linux/thermal.h @@ -289,6 +289,8 @@ int thermal_zone_for_each_trip(struct th int (*cb)(struct thermal_trip *, void *), void *data); int thermal_zone_get_num_trips(struct thermal_zone_device *tz); +void thermal_zone_set_trip_temp(struct thermal_zone_device *tz, + struct thermal_trip *trip, int temp); int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp); Index: linux-pm/drivers/acpi/thermal.c =================================================================== --- linux-pm.orig/drivers/acpi/thermal.c +++ linux-pm/drivers/acpi/thermal.c @@ -294,6 +294,7 @@ static int acpi_thermal_adjust_trip(stru struct acpi_thermal_trip *acpi_trip = trip->priv; struct adjust_trip_data *atd = data; struct acpi_thermal *tz = atd->tz; + int temp; if (!acpi_trip || !acpi_thermal_trip_valid(acpi_trip)) return 0; @@ -304,9 +305,11 @@ static int acpi_thermal_adjust_trip(stru acpi_thermal_update_trip_devices(tz, trip); if (acpi_thermal_trip_valid(acpi_trip)) - trip->temperature = acpi_thermal_temp(tz, acpi_trip->temp_dk); + temp = acpi_thermal_temp(tz, acpi_trip->temp_dk); else - trip->temperature = THERMAL_TEMP_INVALID; + temp = THERMAL_TEMP_INVALID; + + thermal_zone_set_trip_temp(tz->thermal_zone, trip, temp); return 0; } Index: linux-pm/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c =================================================================== --- linux-pm.orig/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c +++ linux-pm/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c @@ -225,7 +225,8 @@ EXPORT_SYMBOL_GPL(int340x_thermal_zone_r static int int340x_update_one_trip(struct thermal_trip *trip, void *arg) { - struct acpi_device *zone_adev = arg; + struct int34x_thermal_zone *int34x_zone = arg; + struct acpi_device *zone_adev = int34x_zone->adev; int temp, err; switch (trip->type) { @@ -249,14 +250,15 @@ static int int340x_update_one_trip(struc if (err) temp = THERMAL_TEMP_INVALID; - trip->temperature = temp; + thermal_zone_set_trip_temp(int34x_zone->zone, trip, temp); + return 0; } void int340x_thermal_update_trips(struct int34x_thermal_zone *int34x_zone) { thermal_zone_for_each_trip(int34x_zone->zone, int340x_update_one_trip, - int34x_zone->adev); + int34x_zone); } EXPORT_SYMBOL_GPL(int340x_thermal_update_trips);