[PATCH v1 0/3] thermal: intel: int340x: Use generic trip points table

[PATCH v1 0/3] thermal: intel: int340x: Use generic trip points table

[Rafael J. Wysocki]

Hi All,

This series replaces the following patch:

https://patchwork.kernel.org/project/linux-pm/patch/2147918.irdbgypaU6@kreacher/

but it has been almost completely rewritten, so I've dropped all tags from it.

The most significant difference is that firmware-induced trip point updates are
now handled in a less controversial manner (no renumbering, just temperature
updates if applicable).

Please refer to the individual patch changelogs for details.

The series is on top of this patch:

https://patchwork.kernel.org/project/linux-pm/patch/2688799.mvXUDI8C0e@kreacher/

which applies on top of the linux-next branch in linux-pm.git from today.

Thanks!

[PATCH v1 1/3] thermal: intel: int340x: Rework updating trip points

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It is generally invalid to change the trip point indices after they have
been exposed via sysfs.

Moreover, the thermal objects in the ACPI namespace cannot go away and
appear on the fly.  In practice, the only thing that can happen when the
INT3403_PERF_TRIP_POINT_CHANGED notification is sent by the platform
firmware is a change of the return values of those thermal objects.

For this reason, add a special function for updating the trip point
temperatures after re-evaluating the respective ACPI thermal objects
and change int3403_notify() to invoke it instead of
int340x_thermal_read_trips() that would change the trip point indices
on errors.  Also remove the locking from the latter, because it is only
called before registering the thermal zone and it cannot race with the
zone's callbacks.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/int340x_thermal/int3403_thermal.c      |    2 
 drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c |   51 +++++++++--
 drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h |    2 
 3 files changed, 47 insertions(+), 8 deletions(-)

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
@@ -168,13 +168,11 @@ static int int340x_thermal_get_trip_conf
 	return 0;
 }
 
-int int340x_thermal_read_trips(struct int34x_thermal_zone *int34x_zone)
+static int int340x_thermal_read_trips(struct int34x_thermal_zone *int34x_zone)
 {
 	int trip_cnt = int34x_zone->aux_trip_nr;
 	int i;
 
-	mutex_lock(&int34x_zone->trip_mutex);
-
 	int34x_zone->crt_trip_id = -1;
 	if (!int340x_thermal_get_trip_config(int34x_zone->adev->handle, "_CRT",
 					     &int34x_zone->crt_temp))
@@ -202,11 +200,8 @@ int int340x_thermal_read_trips(struct in
 		int34x_zone->act_trips[i].valid = true;
 	}
 
-	mutex_unlock(&int34x_zone->trip_mutex);
-
 	return trip_cnt;
 }
-EXPORT_SYMBOL_GPL(int340x_thermal_read_trips);
 
 static struct thermal_zone_params int340x_thermal_params = {
 	.governor_name = "user_space",
@@ -309,6 +304,50 @@ void int340x_thermal_zone_remove(struct
 }
 EXPORT_SYMBOL_GPL(int340x_thermal_zone_remove);
 
+void int340x_thermal_update_trips(struct int34x_thermal_zone *int34x_zone)
+{
+	acpi_handle zone_handle = int34x_zone->adev->handle;
+	int i, err;
+
+	mutex_lock(&int34x_zone->trip_mutex);
+
+	if (int34x_zone->crt_trip_id > 0) {
+		err = int340x_thermal_get_trip_config(zone_handle, "_CRT",
+						      &int34x_zone->crt_temp);
+		if (err)
+			int34x_zone->crt_temp = THERMAL_TEMP_INVALID;
+	}
+
+	if (int34x_zone->hot_trip_id > 0) {
+		err = int340x_thermal_get_trip_config(zone_handle, "_HOT",
+						      &int34x_zone->hot_temp);
+		if (err)
+			int34x_zone->hot_temp = THERMAL_TEMP_INVALID;
+	}
+
+	if (int34x_zone->psv_trip_id > 0) {
+		err = int340x_thermal_get_trip_config(zone_handle, "_PSV",
+						      &int34x_zone->psv_temp);
+		if (err)
+			int34x_zone->psv_temp = THERMAL_TEMP_INVALID;
+	}
+
+	for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) {
+		char name[5] = { '_', 'A', 'C', '0' + i, '\0' };
+
+		if (!int34x_zone->act_trips[i].valid)
+			break;
+
+		err = int340x_thermal_get_trip_config(zone_handle, name,
+						      &int34x_zone->act_trips[i].temp);
+		if (err)
+			int34x_zone->act_trips[i].temp = THERMAL_TEMP_INVALID;
+	}
+
+	mutex_unlock(&int34x_zone->trip_mutex);
+}
+EXPORT_SYMBOL_GPL(int340x_thermal_update_trips);
+
 MODULE_AUTHOR("Aaron Lu <aaron.lu@intel.com>");
 MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>");
 MODULE_DESCRIPTION("Intel INT340x common thermal zone handler");
Index: linux-pm/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
+++ linux-pm/drivers/thermal/intel/int340x_thermal/int3403_thermal.c
@@ -69,7 +69,7 @@ static void int3403_notify(acpi_handle h
 						   THERMAL_TRIP_VIOLATED);
 		break;
 	case INT3403_PERF_TRIP_POINT_CHANGED:
-		int340x_thermal_read_trips(obj->int340x_zone);
+		int340x_thermal_update_trips(obj->int340x_zone);
 		int340x_thermal_zone_device_update(obj->int340x_zone,
 						   THERMAL_TRIP_CHANGED);
 		break;
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
@@ -38,7 +38,7 @@ struct int34x_thermal_zone {
 struct int34x_thermal_zone *int340x_thermal_zone_add(struct acpi_device *,
 				int (*get_temp) (struct thermal_zone_device *, int *));
 void int340x_thermal_zone_remove(struct int34x_thermal_zone *);
-int int340x_thermal_read_trips(struct int34x_thermal_zone *int34x_zone);
+void int340x_thermal_update_trips(struct int34x_thermal_zone *int34x_zone);
 
 static inline void int340x_thermal_zone_set_priv_data(
 			struct int34x_thermal_zone *tzone, void *priv_data)

[PATCH v1 2/3] thermal: intel: int340x: Use zone lock for synchronization

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because the ->get_trip_temp() and ->get_trip_type() thermal zone
callbacks are only invoked from __thermal_zone_get_trip() which is
always called by the thermal core under the zone lock, it is sufficient
for int340x_thermal_update_trips() to acquire the zone lock for mutual
exclusion with those callbacks.

Accordingly, modify int340x_thermal_update_trips() to use the zone lock
instead of the internal trip_mutex and drop the latter which is not
necessary any more.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c |   28 +++--------
 drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h |    1 
 2 files changed, 8 insertions(+), 21 deletions(-)

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
@@ -41,9 +41,7 @@ static int int340x_thermal_get_trip_temp
 					 int trip, int *temp)
 {
 	struct int34x_thermal_zone *d = zone->devdata;
-	int i, ret = 0;
-
-	mutex_lock(&d->trip_mutex);
+	int i;
 
 	if (trip < d->aux_trip_nr)
 		*temp = d->aux_trips[trip];
@@ -62,12 +60,10 @@ static int int340x_thermal_get_trip_temp
 			}
 		}
 		if (i == INT340X_THERMAL_MAX_ACT_TRIP_COUNT)
-			ret = -EINVAL;
+			return -EINVAL;
 	}
 
-	mutex_unlock(&d->trip_mutex);
-
-	return ret;
+	return 0;
 }
 
 static int int340x_thermal_get_trip_type(struct thermal_zone_device *zone,
@@ -75,9 +71,7 @@ static int int340x_thermal_get_trip_type
 					 enum thermal_trip_type *type)
 {
 	struct int34x_thermal_zone *d = zone->devdata;
-	int i, ret = 0;
-
-	mutex_lock(&d->trip_mutex);
+	int i;
 
 	if (trip < d->aux_trip_nr)
 		*type = THERMAL_TRIP_PASSIVE;
@@ -96,12 +90,10 @@ static int int340x_thermal_get_trip_type
 			}
 		}
 		if (i == INT340X_THERMAL_MAX_ACT_TRIP_COUNT)
-			ret = -EINVAL;
+			return -EINVAL;
 	}
 
-	mutex_unlock(&d->trip_mutex);
-
-	return ret;
+	return 0;
 }
 
 static int int340x_thermal_set_trip_temp(struct thermal_zone_device *zone,
@@ -222,8 +214,6 @@ struct int34x_thermal_zone *int340x_ther
 	if (!int34x_thermal_zone)
 		return ERR_PTR(-ENOMEM);
 
-	mutex_init(&int34x_thermal_zone->trip_mutex);
-
 	int34x_thermal_zone->adev = adev;
 
 	int34x_thermal_zone->ops = kmemdup(&int340x_thermal_zone_ops,
@@ -286,7 +276,6 @@ err_thermal_zone:
 err_trip_alloc:
 	kfree(int34x_thermal_zone->ops);
 err_ops_alloc:
-	mutex_destroy(&int34x_thermal_zone->trip_mutex);
 	kfree(int34x_thermal_zone);
 	return ERR_PTR(ret);
 }
@@ -299,7 +288,6 @@ void int340x_thermal_zone_remove(struct
 	acpi_lpat_free_conversion_table(int34x_thermal_zone->lpat_table);
 	kfree(int34x_thermal_zone->aux_trips);
 	kfree(int34x_thermal_zone->ops);
-	mutex_destroy(&int34x_thermal_zone->trip_mutex);
 	kfree(int34x_thermal_zone);
 }
 EXPORT_SYMBOL_GPL(int340x_thermal_zone_remove);
@@ -309,7 +297,7 @@ void int340x_thermal_update_trips(struct
 	acpi_handle zone_handle = int34x_zone->adev->handle;
 	int i, err;
 
-	mutex_lock(&int34x_zone->trip_mutex);
+	mutex_lock(&int34x_zone->zone->lock);
 
 	if (int34x_zone->crt_trip_id > 0) {
 		err = int340x_thermal_get_trip_config(zone_handle, "_CRT",
@@ -344,7 +332,7 @@ void int340x_thermal_update_trips(struct
 			int34x_zone->act_trips[i].temp = THERMAL_TEMP_INVALID;
 	}
 
-	mutex_unlock(&int34x_zone->trip_mutex);
+	mutex_unlock(&int34x_zone->zone->lock);
 }
 EXPORT_SYMBOL_GPL(int340x_thermal_update_trips);
 
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
@@ -32,7 +32,6 @@ struct int34x_thermal_zone {
 	struct thermal_zone_device_ops *ops;
 	void *priv_data;
 	struct acpi_lpat_conversion_table *lpat_table;
-	struct mutex trip_mutex;
 };
 
 struct int34x_thermal_zone *int340x_thermal_zone_add(struct acpi_device *,

[PATCH v1 3/3] thermal: intel: int340x: Use generic trip points table

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify int340x_thermal_zone_add() to register the thermal zone along
with a trip points table, which allows the trip-related zone callbacks
to be dropped, because they are not needed any more.

In order to consolidate the code, use ACPI trip library functions to
populate generic trip points in int340x_thermal_read_trips() and to
update them in int340x_thermal_update_trips().

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Co-developed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/int340x_thermal/Kconfig                |    1 
 drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c |  265 +++--------
 drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.h |   10 
 3 files changed, 90 insertions(+), 186 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,25 +50,6 @@ 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)
-{
-	struct int34x_thermal_zone *d = zone->devdata;
-	acpi_status status;
-	unsigned long long hyst;
-
-	status = acpi_evaluate_integer(d->adev->handle, "GTSH", NULL, &hyst);
-	if (ACPI_FAILURE(status))
-		*temp = 0;
-	else
-		*temp = hyst * 100;
-
 	return 0;
 }
 
@@ -138,58 +60,35 @@ 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;
-}
-
-static int int340x_thermal_read_trips(struct int34x_thermal_zone *int34x_zone)
-{
-	int trip_cnt = int34x_zone->aux_trip_nr;
-	int i;
-
-	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++;
+static int int340x_thermal_read_trips(struct acpi_device *zone_adev,
+				      struct thermal_trip *zone_trips,
+				      int trip_cnt)
+{
+	int i, ret;
+
+	ret = thermal_acpi_trip_critical(zone_adev, &zone_trips[trip_cnt]);
+	if (!ret)
+		trip_cnt++;
+
+	ret = thermal_acpi_trip_hot(zone_adev, &zone_trips[trip_cnt]);
+	if (!ret)
+		trip_cnt++;
+
+	ret = thermal_acpi_trip_passive(zone_adev, &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(zone_adev, i, &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;
@@ -204,10 +103,12 @@ struct int34x_thermal_zone *int340x_ther
 						     int (*get_temp) (struct thermal_zone_device *, int *))
 {
 	struct int34x_thermal_zone *int34x_thermal_zone;
-	acpi_status status;
-	unsigned long long trip_cnt;
+	struct thermal_trip *zone_trips;
+	unsigned long long trip_cnt = 0;
+	unsigned long long hyst;
 	int trip_mask = 0;
-	int ret;
+	acpi_status status;
+	int i, ret;
 
 	int34x_thermal_zone = kzalloc(sizeof(*int34x_thermal_zone),
 				      GFP_KERNEL);
@@ -227,33 +128,42 @@ 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;
+	}
+
+	zone_trips = kzalloc(sizeof(*zone_trips) * (trip_cnt + INT340X_THERMAL_MAX_TRIP_COUNT),
+			     GFP_KERNEL);
+	if (!zone_trips) {
+		ret = -ENOMEM;
+		goto err_trips_alloc;
+	}
+
+	for (i = 0; i < trip_cnt; i++) {
+		zone_trips[i].type = THERMAL_TRIP_PASSIVE;
+		zone_trips[i].temperature = THERMAL_TEMP_INVALID;
 	}
 
-	trip_cnt = int340x_thermal_read_trips(int34x_thermal_zone);
+	trip_cnt = int340x_thermal_read_trips(adev, zone_trips, trip_cnt);
+
+	status = acpi_evaluate_integer(adev->handle, "GTSH", NULL, &hyst);
+	if (ACPI_SUCCESS(status))
+		hyst *= 100;
+	else
+		hyst = 0;
+
+	for (i = 0; i < trip_cnt; ++i)
+		zone_trips[i].hysteresis = hyst;
+
+	int34x_thermal_zone->trips = zone_trips;
 
 	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),
-						trip_cnt,
+						zone_trips, trip_cnt,
 						trip_mask, int34x_thermal_zone,
 						int34x_thermal_zone->ops,
 						&int340x_thermal_params,
@@ -271,9 +181,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);
@@ -286,7 +196,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);
 }
@@ -294,42 +204,41 @@ EXPORT_SYMBOL_GPL(int340x_thermal_zone_r
 
 void int340x_thermal_update_trips(struct int34x_thermal_zone *int34x_zone)
 {
-	acpi_handle zone_handle = int34x_zone->adev->handle;
-	int i, err;
+	struct acpi_device *zone_adev = int34x_zone->adev;
+	struct thermal_trip *zone_trips = int34x_zone->trips;
+	int trip_cnt = int34x_zone->zone->num_trips;
+	int act_trip_nr = 0;
+	int i;
 
 	mutex_lock(&int34x_zone->zone->lock);
 
-	if (int34x_zone->crt_trip_id > 0) {
-		err = int340x_thermal_get_trip_config(zone_handle, "_CRT",
-						      &int34x_zone->crt_temp);
-		if (err)
-			int34x_zone->crt_temp = THERMAL_TEMP_INVALID;
-	}
-
-	if (int34x_zone->hot_trip_id > 0) {
-		err = int340x_thermal_get_trip_config(zone_handle, "_HOT",
-						      &int34x_zone->hot_temp);
-		if (err)
-			int34x_zone->hot_temp = THERMAL_TEMP_INVALID;
-	}
-
-	if (int34x_zone->psv_trip_id > 0) {
-		err = int340x_thermal_get_trip_config(zone_handle, "_PSV",
-						      &int34x_zone->psv_temp);
-		if (err)
-			int34x_zone->psv_temp = THERMAL_TEMP_INVALID;
-	}
-
-	for (i = 0; i < INT340X_THERMAL_MAX_ACT_TRIP_COUNT; i++) {
-		char name[5] = { '_', 'A', 'C', '0' + i, '\0' };
-
-		if (!int34x_zone->act_trips[i].valid)
+	for (i = int34x_zone->aux_trip_nr; i < trip_cnt; i++) {
+		struct thermal_trip trip;
+		int err;
+
+		switch (zone_trips[i].type) {
+		case THERMAL_TRIP_CRITICAL:
+			err = thermal_acpi_trip_critical(zone_adev, &trip);
 			break;
-
-		err = int340x_thermal_get_trip_config(zone_handle, name,
-						      &int34x_zone->act_trips[i].temp);
-		if (err)
-			int34x_zone->act_trips[i].temp = THERMAL_TEMP_INVALID;
+		case THERMAL_TRIP_HOT:
+			err = thermal_acpi_trip_hot(zone_adev, &trip);
+			break;
+		case THERMAL_TRIP_PASSIVE:
+			err = thermal_acpi_trip_passive(zone_adev, &trip);
+			break;
+		case THERMAL_TRIP_ACTIVE:
+			err = thermal_acpi_trip_active(zone_adev, act_trip_nr++,
+						       &trip);
+			break;
+		default:
+			err = -ENODEV;
+		}
+		if (err) {
+			zone_trips[i].temperature = THERMAL_TEMP_INVALID;
+			continue;
+		}
+		
+		zone_trips[i].temperature = trip.temperature;
 	}
 
 	mutex_unlock(&int34x_zone->zone->lock);
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;

Re: [PATCH v1 0/3] thermal: intel: int340x: Use generic trip points table

[Rafael J. Wysocki]

Hi Srinivas,

On Wed, Jan 25, 2023 at 3:55 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi All,
>
> This series replaces the following patch:
>
> https://patchwork.kernel.org/project/linux-pm/patch/2147918.irdbgypaU6@kreacher/
>
> but it has been almost completely rewritten, so I've dropped all tags from it.
>
> The most significant difference is that firmware-induced trip point updates are
> now handled in a less controversial manner (no renumbering, just temperature
> updates if applicable).
>
> Please refer to the individual patch changelogs for details.
>
> The series is on top of this patch:
>
> https://patchwork.kernel.org/project/linux-pm/patch/2688799.mvXUDI8C0e@kreacher/
>
> which applies on top of the linux-next branch in linux-pm.git from today.

There are two additional branches in linux-pm.git:

thermal-intel-fixes
thermal-intel-testing

The former is just fixes to go on top of 6.2-rc5 and the latter - this
series on top of those and the current thermal-intel branch I have
locally with the Intel thermal drivers changes for 6.3.

I would appreciate giving each of them a go in your test setup.

Cheers!

Re: [PATCH v1 0/3] thermal: intel: int340x: Use generic trip points table

[srinivas pandruvada]

Hi Rafael,


On Wed, 2023-01-25 at 16:20 +0100, Rafael J. Wysocki wrote:
> Hi Srinivas,
> 
> On Wed, Jan 25, 2023 at 3:55 PM Rafael J. Wysocki <rjw@rjwysocki.net>
> wrote:
> > 
> > Hi All,
> > 
> > This series replaces the following patch:
> > 
> > https://patchwork.kernel.org/project/linux-pm/patch/2147918.irdbgypaU6@kreacher/
> > 
> > but it has been almost completely rewritten, so I've dropped all
> > tags from it.
> > 
> > 

[...]

> > The series is on top of this patch:
> > 
> > https://patchwork.kernel.org/project/linux-pm/patch/2688799.mvXUDI8C0e@kreacher/
> > 
> > which applies on top of the linux-next branch in linux-pm.git from
> > today.
> 
> There are two additional branches in linux-pm.git:
> 
> thermal-intel-fixes
On two systems test, no issues are observed.

> thermal-intel-testing
branch: thermal-intel-test

No issues, but number of trips are not same as invalid trips are not
registered.
Not sure if this is correct. At boot up they may be invalid, but
firmware may update later (Not aware of such scenario).

For example, the hot is not registered.

Current:

thermal_zone9/trip_point_0_type:critical
thermal_zone9/trip_point_0_temp:125050
thermal_zone9/trip_point_0_hyst:0

thermal_zone9/trip_point_1_type:hot
thermal_zone9/trip_point_1_temp:-273250
thermal_zone9/trip_point_1_hyst:0

thermal_zone9/trip_point_2_type:passive
thermal_zone9/trip_point_2_temp:103050
thermal_zone9/trip_point_2_hyst:0

thermal_zone9/trip_point_3_type:active
thermal_zone9/trip_point_3_temp:103050
thermal_zone9/trip_point_3_hyst:0

thermal_zone9/trip_point_4_type:active
thermal_zone9/trip_point_4_temp:101050
thermal_zone9/trip_point_4_hyst:0

thermal_zone9/trip_point_5_type:active
thermal_zone9/trip_point_5_temp:100050
thermal_zone9/trip_point_5_hyst:0


thermal_zone9/trip_point_6_type:active
thermal_zone9/trip_point_6_temp:98550
thermal_zone9/trip_point_6_hyst:0

thermal_zone9/trip_point_7_type:active
thermal_zone9/trip_point_7_temp:97050
thermal_zone9/trip_point_7_hyst:0


with 6.3-rc1 changes

thermal_zone9/trip_point_0_type:critical
thermal_zone9/trip_point_0_temp:125050
thermal_zone9/trip_point_0_hyst:0

thermal_zone9/trip_point_1_type:passive
thermal_zone9/trip_point_1_temp:103050
thermal_zone9/trip_point_1_hyst:0

thermal_zone9/trip_point_2_type:active
thermal_zone9/trip_point_2_temp:103050
thermal_zone9/trip_point_2_hyst:0

thermal_zone9/trip_point_3_type:active
thermal_zone9/trip_point_3_temp:101050
thermal_zone9/trip_point_3_hyst:0

thermal_zone9/trip_point_4_type:active
thermal_zone9/trip_point_4_temp:100050
thermal_zone9/trip_point_4_hyst:0

thermal_zone9/trip_point_5_type:active
thermal_zone9/trip_point_5_temp:98550
thermal_zone9/trip_point_5_hyst:0


thermal_zone9/trip_point_6_hyst:0
thermal_zone9/trip_point_6_temp:97050
thermal_zone9/trip_point_6_type:active

Thanks,
Srinivas


> 
> The former is just fixes to go on top of 6.2-rc5 and the latter -
> this
> series on top of those and the current thermal-intel branch I have
> locally with the Intel thermal drivers changes for 6.3.
> 
> I would appreciate giving each of them a go in your test setup.
> 
> Cheers!

Re: [PATCH v1 0/3] thermal: intel: int340x: Use generic trip points table

[Rafael J. Wysocki]

On Thursday, January 26, 2023 1:02:59 AM CET srinivas pandruvada wrote:
> Hi Rafael,
> 
> 
> On Wed, 2023-01-25 at 16:20 +0100, Rafael J. Wysocki wrote:
> > Hi Srinivas,
> > 
> > On Wed, Jan 25, 2023 at 3:55 PM Rafael J. Wysocki <rjw@rjwysocki.net>
> > wrote:
> > > 
> > > Hi All,
> > > 
> > > This series replaces the following patch:
> > > 
> > > https://patchwork.kernel.org/project/linux-pm/patch/2147918.irdbgypaU6@kreacher/
> > > 
> > > but it has been almost completely rewritten, so I've dropped all
> > > tags from it.
> > > 
> > > 
> 
> [...]
> 
> > > The series is on top of this patch:
> > > 
> > > https://patchwork.kernel.org/project/linux-pm/patch/2688799.mvXUDI8C0e@kreacher/
> > > 
> > > which applies on top of the linux-next branch in linux-pm.git from
> > > today.
> > 
> > There are two additional branches in linux-pm.git:
> > 
> > thermal-intel-fixes
> On two systems test, no issues are observed.

Great!  I'll move this to linux-next then.

> > thermal-intel-testing
> branch: thermal-intel-test
> 
> No issues, but number of trips are not same as invalid trips are not
> registered.
> Not sure if this is correct.

It may not be.  At least it is a change in behavior that is not expected to
happen after these changes.

> At boot up they may be invalid, but
> firmware may update later (Not aware of such scenario).
> 
> For example, the hot is not registered.
> 
> Current:
> 
> thermal_zone9/trip_point_0_type:critical
> thermal_zone9/trip_point_0_temp:125050
> thermal_zone9/trip_point_0_hyst:0
> 
> thermal_zone9/trip_point_1_type:hot
> thermal_zone9/trip_point_1_temp:-273250
> thermal_zone9/trip_point_1_hyst:0

So this means that _HOT is evaluated successfully (or the trip point index
would be negative), but it probably returned an invalid temperature (likely 0)
that has been turned into an error by the temperature range check in the
new ACPI helper introduced by the change.

OK, thanks for testing!

I've added the appended patch to the thermal-intel-test branch.  Can you please
check if it makes that difference in behavior go away?

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] thermal: ACPI: Initialize trips if temperature is out of range

In some cases it is still useful to register a trip point if the
temperature returned by the corresponding ACPI thermal object (for
example, _HOT) is invalid to start with, because the same ACPI
thermal object may start to return a valid temperature after a
system configuration change (for example, from an AC power source
to battery an vice versa).

For this reason, if the ACPI thermal object evaluated by
thermal_acpi_trip_init() successfully returns a temperature value that
is out of the range of values taken into account, initialize the trip
point using THERMAL_TEMP_INVALID as the temperature value instead of
returning an error to allow the user of the trip point to decide what
to do with it.

Also update pch_wpt_add_acpi_psv_trip() to reject trip points with
invalid temperature values.

Fixes: 7a0e39748861 ("thermal: ACPI: Add ACPI trip point routines")
Reported-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/intel_pch_thermal.c |    2 +-
 drivers/thermal/thermal_acpi.c            |    7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/thermal/thermal_acpi.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_acpi.c
+++ linux-pm/drivers/thermal/thermal_acpi.c
@@ -64,13 +64,14 @@ static int thermal_acpi_trip_init(struct
 		return -ENODATA;
 	}
 
-	if (temp < TEMP_MIN_DECIK || temp >= TEMP_MAX_DECIK) {
+	if (temp >= TEMP_MIN_DECIK && temp <= TEMP_MAX_DECIK) {
+		trip->temperature = deci_kelvin_to_millicelsius(temp);
+	} else {
 		acpi_handle_debug(adev->handle, "%s result %llu out of range\n",
 				  obj_name, temp);
-		return -ENODATA;
+		trip->temperature = THERMAL_TEMP_INVALID;
 	}
 
-	trip->temperature = deci_kelvin_to_millicelsius(temp);
 	trip->hysteresis = 0;
 	trip->type = type;
 
Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
+++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
@@ -107,7 +107,7 @@ static void pch_wpt_add_acpi_psv_trip(st
 		return;
 
 	ret = thermal_acpi_trip_passive(adev, &ptd->trips[*nr_trips]);
-	if (ret)
+	if (ret || ptd->trips[*nr_trips].temperature <= 0)
 		return;
 
 	++(*nr_trips);

Re: [PATCH v1 0/3] thermal: intel: int340x: Use generic trip points table

[srinivas pandruvada]

Hi Rafael,

On Thu, 2023-01-26 at 14:13 +0100, Rafael J. Wysocki wrote:
> On Thursday, January 26, 2023 1:02:59 AM CET srinivas pandruvada
> wrote:
> > Hi Rafael,
> > 
> > 
> 

[...]

> I've added the appended patch to the thermal-intel-test branch.  Can
> you please
> check if it makes that difference in behavior go away?
I synced the tree again and your patch in thermal-intel-test fixes the
issue.

Thanks,
Srinivas
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] thermal: ACPI: Initialize trips if temperature is
> out of range
> 
> In some cases it is still useful to register a trip point if the
> temperature returned by the corresponding ACPI thermal object (for
> example, _HOT) is invalid to start with, because the same ACPI
> thermal object may start to return a valid temperature after a
> system configuration change (for example, from an AC power source
> to battery an vice versa).
> 
> For this reason, if the ACPI thermal object evaluated by
> thermal_acpi_trip_init() successfully returns a temperature value
> that
> is out of the range of values taken into account, initialize the trip
> point using THERMAL_TEMP_INVALID as the temperature value instead of
> returning an error to allow the user of the trip point to decide what
> to do with it.
> 
> Also update pch_wpt_add_acpi_psv_trip() to reject trip points with
> invalid temperature values.
> 
> Fixes: 7a0e39748861 ("thermal: ACPI: Add ACPI trip point routines")
> Reported-by: Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/thermal/intel/intel_pch_thermal.c |    2 +-
>  drivers/thermal/thermal_acpi.c            |    7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_acpi.c
> +++ linux-pm/drivers/thermal/thermal_acpi.c
> @@ -64,13 +64,14 @@ static int thermal_acpi_trip_init(struct
>                 return -ENODATA;
>         }
>  
> -       if (temp < TEMP_MIN_DECIK || temp >= TEMP_MAX_DECIK) {
> +       if (temp >= TEMP_MIN_DECIK && temp <= TEMP_MAX_DECIK) {
> +               trip->temperature =
> deci_kelvin_to_millicelsius(temp);
> +       } else {
>                 acpi_handle_debug(adev->handle, "%s result %llu out
> of range\n",
>                                   obj_name, temp);
> -               return -ENODATA;
> +               trip->temperature = THERMAL_TEMP_INVALID;
>         }
>  
> -       trip->temperature = deci_kelvin_to_millicelsius(temp);
>         trip->hysteresis = 0;
>         trip->type = type;
>  
> Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
> +++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> @@ -107,7 +107,7 @@ static void pch_wpt_add_acpi_psv_trip(st
>                 return;
>  
>         ret = thermal_acpi_trip_passive(adev, &ptd-
> >trips[*nr_trips]);
> -       if (ret)
> +       if (ret || ptd->trips[*nr_trips].temperature <= 0)
>                 return;
>  
>         ++(*nr_trips);
> 
> 
> 

Re: [PATCH v1 0/3] thermal: intel: int340x: Use generic trip points table

[Rafael J. Wysocki]

Hi Srinivas,

On Thu, Jan 26, 2023 at 6:17 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Thu, 2023-01-26 at 14:13 +0100, Rafael J. Wysocki wrote:
> > On Thursday, January 26, 2023 1:02:59 AM CET srinivas pandruvada
> > wrote:
> > > Hi Rafael,
> > >
> > >
> >
>
> [...]
>
> > I've added the appended patch to the thermal-intel-test branch.  Can
> > you please
> > check if it makes that difference in behavior go away?
> I synced the tree again and your patch in thermal-intel-test fixes the
> issue.

Thanks a lot for testing and the confirmation!

In the meantime, I've merged the thermal-intel-test into the
bleeding-edge branch and if 0-day reports success with building it,
I'll move the patches to linux-next.

Cheers!