On Mon, Jan 23, 2023 at 7:41 PM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote: > > From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> > > The thermal framework gives the possibility to register the trip > points along with the thermal zone. When that is done, no get_trip_* > callbacks are needed and they can be removed. > > Convert the existing callbacks content logic into generic trip points > initialization code and register them along with the thermal zone. > > In order to consolidate the code, use ACPI trip library functions to > populate generic trip points. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> > Reviewed-by: Zhang Rui <rui.zhang@xxxxxxxxx> > [ rjw: Subject and changelog edits, rebase ] > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> This fix from Srinivas: https://lore.kernel.org/linux-pm/20230123172110.376549-1-srinivas.pandruvada@xxxxxxxxxxxxxxx/ clearly shows that there are problems with this patch. > --- > drivers/thermal/intel/int340x_thermal/Kconfig | 1 > drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c | 172 ++--------- > drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h | 10 > 3 files changed, 48 insertions(+), 135 deletions(-) > > Index: linux-pm/drivers/thermal/intel/int340x_thermal/Kconfig > =================================================================== > --- linux-pm.orig/drivers/thermal/intel/int340x_thermal/Kconfig > +++ linux-pm/drivers/thermal/intel/int340x_thermal/Kconfig > @@ -9,6 +9,7 @@ config INT340X_THERMAL > select THERMAL_GOV_USER_SPACE > select ACPI_THERMAL_REL > select ACPI_FAN > + select THERMAL_ACPI > select INTEL_SOC_DTS_IOSF_CORE > select INTEL_TCC > select PROC_THERMAL_MMIO_RAPL if POWERCAP > 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 > @@ -37,65 +37,6 @@ static int int340x_thermal_get_zone_temp > return 0; > } > > -static int int340x_thermal_get_trip_temp(struct thermal_zone_device *zone, > - int trip, int *temp) > -{ > - struct int34x_thermal_zone *d = zone->devdata; > - int i; > - > - if (trip < d->aux_trip_nr) > - *temp = d->aux_trips[trip]; > - else if (trip == d->crt_trip_id) > - *temp = d->crt_temp; > - else if (trip == d->psv_trip_id) > - *temp = d->psv_temp; > - else if (trip == d->hot_trip_id) > - *temp = d->hot_temp; > - else { > - for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) { > - if (d->act_trips[i].valid && > - d->act_trips[i].id == trip) { > - *temp = d->act_trips[i].temp; > - break; > - } > - } > - if (i == INT340X_THERMAL_MAX_ACT_TRIP_COUNT) > - return -EINVAL; > - } > - > - return 0; > -} > - > -static int int340x_thermal_get_trip_type(struct thermal_zone_device *zone, > - int trip, > - enum thermal_trip_type *type) > -{ > - struct int34x_thermal_zone *d = zone->devdata; > - int i; > - > - if (trip < d->aux_trip_nr) > - *type = THERMAL_TRIP_PASSIVE; > - else if (trip == d->crt_trip_id) > - *type = THERMAL_TRIP_CRITICAL; > - else if (trip == d->hot_trip_id) > - *type = THERMAL_TRIP_HOT; > - else if (trip == d->psv_trip_id) > - *type = THERMAL_TRIP_PASSIVE; > - else { > - for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) { > - if (d->act_trips[i].valid && > - d->act_trips[i].id == trip) { > - *type = THERMAL_TRIP_ACTIVE; > - break; > - } > - } > - if (i == INT340X_THERMAL_MAX_ACT_TRIP_COUNT) > - return -EINVAL; > - } > - > - return 0; > -} > - > static int int340x_thermal_set_trip_temp(struct thermal_zone_device *zone, > int trip, int temp) > { > @@ -109,20 +50,15 @@ static int int340x_thermal_set_trip_temp > if (ACPI_FAILURE(status)) > return -EIO; > > - d->aux_trips[trip] = temp; > - > return 0; > } > > - > -static int int340x_thermal_get_trip_hyst(struct thermal_zone_device *zone, > - int trip, int *temp) > +static int int340x_thermal_get_global_hyst(struct acpi_device *adev, int *temp) > { > - struct int34x_thermal_zone *d = zone->devdata; > acpi_status status; > unsigned long long hyst; > > - status = acpi_evaluate_integer(d->adev->handle, "GTSH", NULL, &hyst); > + status = acpi_evaluate_integer(adev->handle, "GTSH", NULL, &hyst); > if (ACPI_FAILURE(status)) > *temp = 0; > else > @@ -131,6 +67,7 @@ static int int340x_thermal_get_trip_hyst > return 0; > } > > + > static void int340x_thermal_critical(struct thermal_zone_device *zone) > { > dev_dbg(&zone->device, "%s: critical temperature reached\n", zone->type); > @@ -138,58 +75,36 @@ static void int340x_thermal_critical(str > > static struct thermal_zone_device_ops int340x_thermal_zone_ops = { > .get_temp = int340x_thermal_get_zone_temp, > - .get_trip_temp = int340x_thermal_get_trip_temp, > - .get_trip_type = int340x_thermal_get_trip_type, > .set_trip_temp = int340x_thermal_set_trip_temp, > - .get_trip_hyst = int340x_thermal_get_trip_hyst, > .critical = int340x_thermal_critical, > }; > > -static int int340x_thermal_get_trip_config(acpi_handle handle, char *name, > - int *temp) > -{ > - unsigned long long r; > - acpi_status status; > - > - status = acpi_evaluate_integer(handle, name, NULL, &r); > - if (ACPI_FAILURE(status)) > - return -EIO; > - > - *temp = deci_kelvin_to_millicelsius(r); > - > - return 0; > -} > - > int int340x_thermal_read_trips(struct int34x_thermal_zone *int34x_zone) First of all, this function can be invoked from int3403_notify() to update the trip points in the case of a firmware notification. This, of course, can be racing with the thermal core's use of the trip points and I don't think that there is a way to synchronize that right now. Second, the hysteresis value set by int340x_thermal_zone_add() will be overwritten with zero by the new code below. I'm dropping this one for now (and the fix on top of it) and we'll need to revisit it. > { > - int trip_cnt = int34x_zone->aux_trip_nr; > - int i; > + int trip_cnt; > + int i, ret; > > - int34x_zone->crt_trip_id = -1; > - if (!int340x_thermal_get_trip_config(int34x_zone->adev->handle, "_CRT", > - &int34x_zone->crt_temp)) > - int34x_zone->crt_trip_id = trip_cnt++; > - > - int34x_zone->hot_trip_id = -1; > - if (!int340x_thermal_get_trip_config(int34x_zone->adev->handle, "_HOT", > - &int34x_zone->hot_temp)) > - int34x_zone->hot_trip_id = trip_cnt++; > - > - int34x_zone->psv_trip_id = -1; > - if (!int340x_thermal_get_trip_config(int34x_zone->adev->handle, "_PSV", > - &int34x_zone->psv_temp)) > - int34x_zone->psv_trip_id = trip_cnt++; > + trip_cnt = int34x_zone->aux_trip_nr; > + > + ret = thermal_acpi_trip_critical(int34x_zone->adev, &int34x_zone->trips[trip_cnt]); > + if (!ret) > + trip_cnt++; > + > + ret = thermal_acpi_trip_hot(int34x_zone->adev, &int34x_zone->trips[trip_cnt]); > + if (!ret) > + trip_cnt++; > + > + ret = thermal_acpi_trip_passive(int34x_zone->adev, &int34x_zone->trips[trip_cnt]); > + if (!ret) > + trip_cnt++; > > for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) { > - char name[5] = { '_', 'A', 'C', '0' + i, '\0' }; > > - if (int340x_thermal_get_trip_config(int34x_zone->adev->handle, > - name, > - &int34x_zone->act_trips[i].temp)) > + ret = thermal_acpi_trip_active(int34x_zone->adev, i, &int34x_zone->trips[trip_cnt]); > + if (ret) > break; > > - int34x_zone->act_trips[i].id = trip_cnt++; > - int34x_zone->act_trips[i].valid = true; > + trip_cnt++; > } > > return trip_cnt; > @@ -208,7 +123,7 @@ struct int34x_thermal_zone *int340x_ther > acpi_status status; > unsigned long long trip_cnt; > int trip_mask = 0; > - int ret; > + int i, ret; > > int34x_thermal_zone = kzalloc(sizeof(*int34x_thermal_zone), > GFP_KERNEL); > @@ -228,32 +143,35 @@ struct int34x_thermal_zone *int340x_ther > int34x_thermal_zone->ops->get_temp = get_temp; > > status = acpi_evaluate_integer(adev->handle, "PATC", NULL, &trip_cnt); > - if (ACPI_FAILURE(status)) > - trip_cnt = 0; > - else { > - int i; > - > - int34x_thermal_zone->aux_trips = > - kcalloc(trip_cnt, > - sizeof(*int34x_thermal_zone->aux_trips), > - GFP_KERNEL); > - if (!int34x_thermal_zone->aux_trips) { > - ret = -ENOMEM; > - goto err_trip_alloc; > - } > - trip_mask = BIT(trip_cnt) - 1; > + if (!ACPI_FAILURE(status)) { > int34x_thermal_zone->aux_trip_nr = trip_cnt; > - for (i = 0; i < trip_cnt; ++i) > - int34x_thermal_zone->aux_trips[i] = THERMAL_TEMP_INVALID; > + trip_mask = BIT(trip_cnt) - 1; > + } > + > + int34x_thermal_zone->trips = kzalloc(sizeof(*int34x_thermal_zone->trips) * > + (INT340X_THERMAL_MAX_TRIP_COUNT + trip_cnt), > + GFP_KERNEL); > + if (!int34x_thermal_zone->trips) { > + ret = -ENOMEM; > + goto err_trips_alloc; > } > > trip_cnt = int340x_thermal_read_trips(int34x_thermal_zone); > > + for (i = 0; i < trip_cnt; ++i) > + int340x_thermal_get_global_hyst(adev, &int34x_thermal_zone->trips[i].hysteresis); > + > + for (i = 0; i < int34x_thermal_zone->aux_trip_nr; i++) { > + int34x_thermal_zone->trips[i].type = THERMAL_TRIP_PASSIVE; > + int34x_thermal_zone->trips[i].temperature = THERMAL_TEMP_INVALID; > + } > + > int34x_thermal_zone->lpat_table = acpi_lpat_get_conversion_table( > adev->handle); > > - int34x_thermal_zone->zone = thermal_zone_device_register( > + int34x_thermal_zone->zone = thermal_zone_device_register_with_trips( > acpi_device_bid(adev), > + int34x_thermal_zone->trips, > trip_cnt, > trip_mask, int34x_thermal_zone, > int34x_thermal_zone->ops, > @@ -272,9 +190,9 @@ struct int34x_thermal_zone *int340x_ther > err_enable: > thermal_zone_device_unregister(int34x_thermal_zone->zone); > err_thermal_zone: > + kfree(int34x_thermal_zone->trips); > acpi_lpat_free_conversion_table(int34x_thermal_zone->lpat_table); > - kfree(int34x_thermal_zone->aux_trips); > -err_trip_alloc: > +err_trips_alloc: > kfree(int34x_thermal_zone->ops); > err_ops_alloc: > kfree(int34x_thermal_zone); > @@ -287,7 +205,7 @@ void int340x_thermal_zone_remove(struct > { > thermal_zone_device_unregister(int34x_thermal_zone->zone); > acpi_lpat_free_conversion_table(int34x_thermal_zone->lpat_table); > - kfree(int34x_thermal_zone->aux_trips); > + kfree(int34x_thermal_zone->trips); > kfree(int34x_thermal_zone->ops); > kfree(int34x_thermal_zone); > } > Index: linux-pm/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h > =================================================================== > --- linux-pm.orig/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h > +++ linux-pm/drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h > @@ -10,6 +10,7 @@ > #include <acpi/acpi_lpat.h> > > #define INT340X_THERMAL_MAX_ACT_TRIP_COUNT 10 > +#define INT340X_THERMAL_MAX_TRIP_COUNT INT340X_THERMAL_MAX_ACT_TRIP_COUNT + 3 > > struct active_trip { > int temp; > @@ -19,15 +20,8 @@ struct active_trip { > > struct int34x_thermal_zone { > struct acpi_device *adev; > - struct active_trip act_trips[INT340X_THERMAL_MAX_ACT_TRIP_COUNT]; > - unsigned long *aux_trips; > + struct thermal_trip *trips; > int aux_trip_nr; > - int psv_temp; > - int psv_trip_id; > - int crt_temp; > - int crt_trip_id; > - int hot_temp; > - int hot_trip_id; > struct thermal_zone_device *zone; > struct thermal_zone_device_ops *ops; > void *priv_data; > > >
