| Message ID | 5737811.DvuYhMxLoT@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] thermal: trip: Send trip change notifications on all trip updates | expand |
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 would make sense to have the thermal framework to be notified about this change and check if there is a trip violation, no ? > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > Depends on https://lore.kernel.org/linux-pm/12337662.O9o76ZdvQC@kreacher/ > > --- > drivers/acpi/thermal.c | 7 +++-- > drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c | 8 +++--- > drivers/thermal/thermal_sysfs.c | 4 +-- > drivers/thermal/thermal_trip.c | 14 ++++++++++- > include/linux/thermal.h | 2 + > 5 files changed, 27 insertions(+), 8 deletions(-) > > 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); > > > >
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.
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);