Skip to contentSkip to conversation

thermal: core: Drop trips_disabled bitmask

PATCH In review Rafael J. Wysocki 4 participants 4 messages Sep 19, 2023 – Sep 20, 2023 Thermal

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 thermal: core: Drop trips_disabled bitmask

From: Rafael J. Wysocki <[email protected]>

After recent changes, thermal_zone_get_trip() cannot fail, as invoked from thermal_zone_device_register_with_trips(), so the only role of the trips_disabled bitmask is struct thermal_zone_device is to make handle_thermal_trip() skip trip points whose temperature was initially zero. However, since the unit of temperature in the thermal core is millicelsius, zero may very well be a valid temperature value at least in some usage scenarios and the trip temperature may as well change later. Thus there is no reason to permanently disable trip points with initial temperature equal to zero.

Accordingly, drop the trips_disabled bitmask along with the code related to it.

---
 drivers/thermal/thermal_core.c |   13 -------------
 include/linux/thermal.h        |    2 --
 2 files changed, 15 deletions(-)
drivers/thermal/thermal_core.c +0 -13
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
@@ -347,10 +347,6 @@ static void handle_thermal_trip(struct t
 {
 	struct thermal_trip trip;
 
-	/* Ignore disabled trip points */
-	if (test_bit(trip_id, &tz->trips_disabled))
-		return;
-
 	__thermal_zone_get_trip(tz, trip_id, &trip);
 
 	if (trip.temperature == THERMAL_TEMP_INVALID)
@@ -1231,7 +1227,6 @@ thermal_zone_device_register_with_trips(
 	struct thermal_zone_device *tz;
 	int id;
 	int result;
-	int count;
 	struct thermal_governor *governor;
 
 	if (!type || strlen(type) == 0) {
@@ -1328,14 +1323,6 @@ thermal_zone_device_register_with_trips(
 	if (result)
 		goto release_device;
 
-	for (count = 0; count < num_trips; count++) {
-		struct thermal_trip trip;
-
-		result = thermal_zone_get_trip(tz, count, &trip);
-		if (result || !trip.temperature)
-			set_bit(count, &tz->trips_disabled);
-	}
-
 	/* Update 'this' zone's governor information */
 	mutex_lock(&thermal_governor_lock);
 
include/linux/thermal.h +0 -2
Index: linux-pm/include/linux/thermal.h
===================================================================
@@ -122,7 +122,6 @@ struct thermal_cooling_device {
  * @devdata:	private pointer for device private data
  * @trips:	an array of struct thermal_trip
  * @num_trips:	number of trip points the thermal zone supports
- * @trips_disabled;	bitmap for disabled trips
  * @passive_delay_jiffies: number of jiffies to wait between polls when
  *			performing passive cooling.
  * @polling_delay_jiffies: number of jiffies to wait between polls when
@@ -163,7 +162,6 @@ struct thermal_zone_device {
 	void *devdata;
 	struct thermal_trip *trips;
 	int num_trips;
-	unsigned long trips_disabled;	/* bitmap for disabled trips */
 	unsigned long passive_delay_jiffies;
 	unsigned long polling_delay_jiffies;
 	int temperature;
Tested-by on v1 1/1
#1 lore raw
Quoted 12 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

After recent changes, thermal_zone_get_trip() cannot fail, as invoked from thermal_zone_device_register_with_trips(), so the only role of the trips_disabled bitmask is struct thermal_zone_device is to make handle_thermal_trip() skip trip points whose temperature was initially zero. However, since the unit of temperature in the thermal core is millicelsius, zero may very well be a valid temperature value at least in some usage scenarios and the trip temperature may as well change later. Thus there is no reason to permanently disable trip points with initial temperature equal to zero.

Quoted 4 lines

Accordingly, drop the trips_disabled bitmask along with the code related to it.

I guess I was copied because of commit f1b80a3878b2 ("thermal: core:
Restore behavior regarding invalid trip points"). Since then we stopped relying on this behavior with commit 5601ef91fba8 ("mlxsw: core_thermal:
Use static trip points for transceiver modules").

Tested your patch and didn't see any regressions:

Thanks

#2 lore raw
Quoted 14 lines · Ido Schimmel
Quoted 12 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

After recent changes, thermal_zone_get_trip() cannot fail, as invoked from thermal_zone_device_register_with_trips(), so the only role of the trips_disabled bitmask is struct thermal_zone_device is to make handle_thermal_trip() skip trip points whose temperature was initially zero. However, since the unit of temperature in the thermal core is millicelsius, zero may very well be a valid temperature value at least in some usage scenarios and the trip temperature may as well change later. Thus there is no reason to permanently disable trip points with initial temperature equal to zero.

Quoted 7 lines
Quoted 4 lines

Accordingly, drop the trips_disabled bitmask along with the code related to it.

I guess I was copied because of commit f1b80a3878b2 ("thermal: core:
Restore behavior regarding invalid trip points").

That's correct.

Quoted 7 lines

Since then we stopped
relying on this behavior with commit 5601ef91fba8 ("mlxsw: core_thermal:
Use static trip points for transceiver modules").

Tested your patch and didn't see any regressions:

Thank you!

Acked-by on v1 1/1
#3 lore raw
In reply to Rafael J. Wysocki
Quoted 12 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

After recent changes, thermal_zone_get_trip() cannot fail, as invoked from thermal_zone_device_register_with_trips(), so the only role of the trips_disabled bitmask is struct thermal_zone_device is to make handle_thermal_trip() skip trip points whose temperature was initially zero. However, since the unit of temperature in the thermal core is millicelsius, zero may very well be a valid temperature value at least in some usage scenarios and the trip temperature may as well change later. Thus there is no reason to permanently disable trip points with initial temperature equal to zero.

Quoted 4 lines

Accordingly, drop the trips_disabled bitmask along with the code related to it.

The logical change after changing how the trip point are handled ;)

Signature · 5 lines
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog