Skip to contentSkip to conversation

thermal: sysfs: Simplifications of trip point attribute callbacks

PATCH Superseded Rafael J. Wysocki 3 participants 6 messages Dec 4, 2023 Thermal

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
author
#1 lore raw

PATCH v3 1/2 thermal: sysfs: Rework the handling of trip point updates

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

Both trip_point_temp_store() and trip_point_hyst_store() use thermal_zone_set_trip() to update a given trip point, but none of them actually needs to change more than one field in struct thermal_trip representing it. However, each of them effectively calls __thermal_zone_get_trip() twice in a row for the same trip index value, once directly and once via thermal_zone_set_trip(), which is not particularly efficient, and the way in which thermal_zone_set_trip() carries out the update is not particularly straightforward.

Moreover, input processing need not be done under the thermal zone lock in any of these functions.

Rework trip_point_temp_store() and trip_point_hyst_store() to address the above, move the part of thermal_zone_set_trip() that is still useful to a new function called thermal_zone_trip_updated() and drop the rest of it.

While at it, make trip_point_hyst_store() reject negative hysteresis values.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

v2 -> v3: No changes

v1 -> v2: Still check device_is_registered() under the zone lock

---
 drivers/thermal/thermal_core.h  |    2 +
 drivers/thermal/thermal_sysfs.c |   75 ++++++++++++++++++++++++++++------------
 drivers/thermal/thermal_trip.c  |   45 ++++--------------------
 include/linux/thermal.h         |    4 --
 4 files changed, 64 insertions(+), 62 deletions(-)
drivers/thermal/thermal_sysfs.c +53 -22
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
@@ -78,6 +78,19 @@ mode_store(struct device *dev, struct de
 	return count;
 }
 
+static int check_thermal_zone_and_trip_id(struct device *dev,
+					  struct thermal_zone_device *tz,
+					  int trip_id)
+{
+	if (!device_is_registered(dev))
+		return -ENODEV;
+
+	if (trip_id < 0 || trip_id >= tz->num_trips)
+		return -EINVAL;
+
+	return 0;
+}
+
 static ssize_t
 trip_point_type_show(struct device *dev, struct device_attribute *attr,
 		     char *buf)

@@ -120,28 +133,37 @@ trip_point_temp_store(struct device *dev
 		      const char *buf, size_t count)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
-	struct thermal_trip trip;
+	struct thermal_trip *trip;
 	int trip_id, ret;
+	int temp;
+
+	ret = kstrtoint(buf, 10, &temp);
+	if (ret)
+		return -EINVAL;
 
 	if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
 		return -EINVAL;
 
 	mutex_lock(&tz->lock);
 
-	if (!device_is_registered(dev)) {
-		ret = -ENODEV;
-		goto unlock;
-	}
-
-	ret = __thermal_zone_get_trip(tz, trip_id, &trip);
+	ret = check_thermal_zone_and_trip_id(dev, tz, trip_id);
 	if (ret)
 		goto unlock;
 
-	ret = kstrtoint(buf, 10, &trip.temperature);
-	if (ret)
-		goto unlock;
+	trip = &tz->trips[trip_id];
+
+	if (temp != trip->temperature) {
+		if (tz->ops->set_trip_temp) {
+			ret = tz->ops->set_trip_temp(tz, trip_id, temp);
+			if (ret)
+				goto unlock;
+		}
+
+		trip->temperature = temp;
+
+		thermal_zone_trip_updated(tz, trip);
+	}
 
-	ret = thermal_zone_set_trip(tz, trip_id, &trip);
 unlock:
 	mutex_unlock(&tz->lock);
 	

@@ -179,28 +201,37 @@ trip_point_hyst_store(struct device *dev
 		      const char *buf, size_t count)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
-	struct thermal_trip trip;
+	struct thermal_trip *trip;
 	int trip_id, ret;
+	int hyst;
+
+	ret = kstrtoint(buf, 10, &hyst);
+	if (ret || hyst < 0)
+		return -EINVAL;
 
 	if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip_id) != 1)
 		return -EINVAL;
 
 	mutex_lock(&tz->lock);
 
-	if (!device_is_registered(dev)) {
-		ret = -ENODEV;
-		goto unlock;
-	}
-
-	ret = __thermal_zone_get_trip(tz, trip_id, &trip);
+	ret = check_thermal_zone_and_trip_id(dev, tz, trip_id);
 	if (ret)
 		goto unlock;
 
-	ret = kstrtoint(buf, 10, &trip.hysteresis);
-	if (ret)
-		goto unlock;
+	trip = &tz->trips[trip_id];
+
+	if (hyst != trip->hysteresis) {
+		if (tz->ops->set_trip_hyst) {
+			ret = tz->ops->set_trip_hyst(tz, trip_id, hyst);
+			if (ret)
+				goto unlock;
+		}
+
+		trip->hysteresis = hyst;
+
+		thermal_zone_trip_updated(tz, trip);
+	}
 
-	ret = thermal_zone_set_trip(tz, trip_id, &trip);
 unlock:
 	mutex_unlock(&tz->lock);
drivers/thermal/thermal_core.h +2 -0
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
@@ -124,6 +124,8 @@ int __thermal_zone_get_trip(struct therm
 			    struct thermal_trip *trip);
 int thermal_zone_trip_id(struct thermal_zone_device *tz,
 			 const struct thermal_trip *trip);
+void thermal_zone_trip_updated(struct thermal_zone_device *tz,
+			       const struct thermal_trip *trip);
 int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
 
 /* sysfs I/F */
drivers/thermal/thermal_trip.c +9 -36

54 lines not on this page · raw message

include/linux/thermal.h +0 -4
Index: linux-pm/include/linux/thermal.h
===================================================================
@@ -282,10 +282,6 @@ int __thermal_zone_get_trip(struct therm
 			    struct thermal_trip *trip);
 int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
 			  struct thermal_trip *trip);
-
-int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
-			  const struct thermal_trip *trip);
-
 int for_each_thermal_trip(struct thermal_zone_device *tz,
 			  int (*cb)(struct thermal_trip *, void *),
 			  void *data);
#2 lore raw

Re: [PATCH v3 1/2] thermal: sysfs: Rework the handling of trip point updates

Hi Rafael,

Quoted 43 lines · Rafael J. Wysocki

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

Both trip_point_temp_store() and trip_point_hyst_store() use thermal_zone_set_trip() to update a given trip point, but none of them actually needs to change more than one field in struct thermal_trip representing it. However, each of them effectively calls __thermal_zone_get_trip() twice in a row for the same trip index value, once directly and once via thermal_zone_set_trip(), which is not particularly efficient, and the way in which thermal_zone_set_trip() carries out the update is not particularly straightforward.

Moreover, input processing need not be done under the thermal zone lock in any of these functions.

Rework trip_point_temp_store() and trip_point_hyst_store() to address the above, move the part of thermal_zone_set_trip() that is still useful to a new function called thermal_zone_trip_updated() and drop the rest of it.

While at it, make trip_point_hyst_store() reject negative hysteresis values.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

v2 -> v3: No changes

v1 -> v2: Still check device_is_registered() under the zone lock

---
  drivers/thermal/thermal_core.h  |    2 +
  drivers/thermal/thermal_sysfs.c |   75 ++++++++++++++++++++++++++++------------
  drivers/thermal/thermal_trip.c  |   45 ++++--------------------
  include/linux/thermal.h         |    4 --
  4 files changed, 64 insertions(+), 62 deletions(-)

Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================

drivers/thermal/thermal_sysfs.c +0 -0
@@ -78,6 +78,19 @@ mode_store(struct device *dev, struct de
  	return count;
  }

drivers/thermal/thermal_sysfs.c mode_store(struct device *dev, struct de · line 89

Quoted 10 lines
drivers/thermal/thermal_sysfs.c +9 -0
 
+static int check_thermal_zone_and_trip_id(struct device *dev,
+					  struct thermal_zone_device *tz,
+					  int trip_id)
+{
+	if (!device_is_registered(dev))
+		return -ENODEV;
+
+	if (trip_id < 0 || trip_id >= tz->num_trips)
+		return -EINVAL;

I'm not sure if this check is useful. The function is called from trip_point_*_store() which is providing the trip id from the file name parsing which is in turn built from an existing trip id. There is no reason the trip id is going to be wrong.

[ ... ]

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
maintainer
#3 lore raw

Re: [PATCH v3 1/2] thermal: sysfs: Rework the handling of trip point updates

Hi Daniel,

Quoted 49 lines · Daniel Lezcano

Hi Rafael,

Quoted 44 lines · Rafael J. Wysocki

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

Both trip_point_temp_store() and trip_point_hyst_store() use thermal_zone_set_trip() to update a given trip point, but none of them actually needs to change more than one field in struct thermal_trip representing it. However, each of them effectively calls __thermal_zone_get_trip() twice in a row for the same trip index value, once directly and once via thermal_zone_set_trip(), which is not particularly efficient, and the way in which thermal_zone_set_trip() carries out the update is not particularly straightforward.

Moreover, input processing need not be done under the thermal zone lock in any of these functions.

Rework trip_point_temp_store() and trip_point_hyst_store() to address the above, move the part of thermal_zone_set_trip() that is still useful to a new function called thermal_zone_trip_updated() and drop the rest of it.

While at it, make trip_point_hyst_store() reject negative hysteresis values.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

v2 -> v3: No changes

v1 -> v2: Still check device_is_registered() under the zone lock

---
  drivers/thermal/thermal_core.h  |    2 +
  drivers/thermal/thermal_sysfs.c |   75 ++++++++++++++++++++++++++++------------
  drivers/thermal/thermal_trip.c  |   45 ++++--------------------
  include/linux/thermal.h         |    4 --
  4 files changed, 64 insertions(+), 62 deletions(-)

Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================

drivers/thermal/thermal_sysfs.c +0 -0
@@ -78,6 +78,19 @@ mode_store(struct device *dev, struct de
      return count;
  }
Quoted 14 lines

drivers/thermal/thermal_sysfs.c mode_store(struct device *dev, struct de · line 89

Quoted 9 lines
drivers/thermal/thermal_sysfs.c +9 -0
+static int check_thermal_zone_and_trip_id(struct device *dev,
+                                       struct thermal_zone_device *tz,
+                                       int trip_id)
+{
+     if (!device_is_registered(dev))
+             return -ENODEV;
+
+     if (trip_id < 0 || trip_id >= tz->num_trips)
+             return -EINVAL;

I'm not sure if this check is useful. The function is called from trip_point_*_store() which is providing the trip id from the file name parsing which is in turn built from an existing trip id. There is no reason the trip id is going to be wrong.

I can drop the check just fine.

Do you have any other comments on this patch?

#4 lore raw

Re: [PATCH v3 1/2] thermal: sysfs: Rework the handling of trip point updates

On 04/12/2023 18:47, Rafael J. Wysocki wrote:

[ ... ]

Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
drivers/thermal/thermal_sysfs.c +6 -0
+static int check_thermal_zone_and_trip_id(struct device *dev,
+                                       struct thermal_zone_device *tz,
+                                       int trip_id)
+{
+     if (!device_is_registered(dev))
+             return -ENODEV;
Quoted 8 lines

drivers/thermal/thermal_sysfs.c mode_store(struct device *dev, struct de · line 89

Quoted 3 lines
drivers/thermal/thermal_sysfs.c +3 -0
+
+     if (trip_id < 0 || trip_id >= tz->num_trips)
+             return -EINVAL;

I'm not sure if this check is useful. The function is called from trip_point_*_store() which is providing the trip id from the file name parsing which is in turn built from an existing trip id. There is no reason the trip id is going to be wrong.

Quoted 3 lines

I can drop the check just fine.

Do you have any other comments on this patch?

No, the rest LGTM

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
author
#5 lore raw

PATCH v3 2/2 thermal: sysfs: Rework the reading of trip point attributes

In reply to Rafael J. Wysocki

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

Rework the _show() callback functions for the trip point temperature, hysteresis and type attributes to avoid copying the values of struct thermal_trip fields that they do not use and to make them carry out validation checks with the help of check_thermal_zone_and_trip_id(), like the corresponding _store() callback functions.

No intentional functional impact.

Signed-off-by Rafael J. Wysocki <[email protected]>

---

v2 -> v3: Drop a redundant 'ret' check at the end of trip_point_hyst_show.

v1 -> v2: Do not drop thermal zone locking from the _store() callback functions.

---
 drivers/thermal/thermal_sysfs.c |   55 ++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 27 deletions(-)
drivers/thermal/thermal_sysfs.c +28 -27
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
@@ -96,25 +96,25 @@ trip_point_type_show(struct device *dev,
 		     char *buf)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
-	struct thermal_trip trip;
-	int trip_id, result;
+	enum thermal_trip_type type;
+	int trip_id, ret;
 
 	if (sscanf(attr->attr.name, "trip_point_%d_type", &trip_id) != 1)
 		return -EINVAL;
 
 	mutex_lock(&tz->lock);
 
-	if (device_is_registered(dev))
-		result = __thermal_zone_get_trip(tz, trip_id, &trip);
-	else
-		result = -ENODEV;
+	ret = check_thermal_zone_and_trip_id(dev, tz, trip_id);
+	if (ret) {
+		mutex_unlock(&tz->lock);
+		return ret;
+	}
 
-	mutex_unlock(&tz->lock);
+	type = tz->trips[trip_id].type;
 
-	if (result)
-		return result;
+	mutex_unlock(&tz->lock);
 
-	switch (trip.type) {
+	switch (type) {
 	case THERMAL_TRIP_CRITICAL:
 		return sprintf(buf, "critical\n");
 	case THERMAL_TRIP_HOT:

@@ -175,25 +175,24 @@ trip_point_temp_show(struct device *dev,
 		     char *buf)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
-	struct thermal_trip trip;
-	int trip_id, ret;
+	int trip_id, ret, temp;
 
 	if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
 		return -EINVAL;
 
 	mutex_lock(&tz->lock);
 
-	if (device_is_registered(dev))
-		ret = __thermal_zone_get_trip(tz, trip_id, &trip);
-	else
-		ret = -ENODEV;
+	ret = check_thermal_zone_and_trip_id(dev, tz, trip_id);
+	if (ret) {
+		mutex_unlock(&tz->lock);
+		return ret;
+	}
 
-	mutex_unlock(&tz->lock);
+	temp = tz->trips[trip_id].temperature;
 
-	if (ret)
-		return ret;
+	mutex_unlock(&tz->lock);
 
-	return sprintf(buf, "%d\n", trip.temperature);
+	return sprintf(buf, "%d\n", temp);
 }
 
 static ssize_t

@@ -243,22 +242,24 @@ trip_point_hyst_show(struct device *dev,
 		     char *buf)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
-	struct thermal_trip trip;
-	int trip_id, ret;
+	int trip_id, ret, hyst;
 
 	if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip_id) != 1)
 		return -EINVAL;
 
 	mutex_lock(&tz->lock);
 
-	if (device_is_registered(dev))
-		ret = __thermal_zone_get_trip(tz, trip_id, &trip);
-	else
-		ret = -ENODEV;
+	ret = check_thermal_zone_and_trip_id(dev, tz, trip_id);
+	if (ret) {
+		mutex_unlock(&tz->lock);
+		return ret;
+	}
+
+	hyst = tz->trips[trip_id].hysteresis;
 
 	mutex_unlock(&tz->lock);
 
-	return ret ? ret : sprintf(buf, "%d\n", trip.hysteresis);
+	return sprintf(buf, "%d\n", hyst);
 }
 
 static ssize_t