thermal: sysfs: Simplifications of trip point attribute callbacks

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

The _show() callback functions of the trip point sysfs attributes, temperature, hysteresis and type, need not use thermal zone locking, because the layout of the data structures they access does not change after the thermal zone registration.

Namely, they all need to access a specific entry in the thermal zone's trips[] table that is always present for non-tripless thermal zones and its size cannot change after the thermal zone has been registered. Thus it is always safe to access the trips[] table of a registered thermal zone from each of the sysfs attributes in question.

Moreover, the type of a trip point does not change after registering its thermal zone, and while its temperature and hysteresis can change, for example due to a firmware-induced thermal zone update, holding the zone lock around reading them is pointless, because it does not prevent stale values from being returned to user space. For example, a trip point temperature can always change ater trip_point_temp_show() has read it and before the function's return statement is executed, regardless of whether or not zone locking is used.

For this reason, drop the zone locking from trip_point_type_show(), trip_point_temp_show(), and trip_point_hyst_show().

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

---
 drivers/thermal/thermal_sysfs.c |   60 ++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 39 deletions(-)

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

drivers/thermal/thermal_sysfs.c +4 -4

@@ -83,25 +83,18 @@ 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;
+       int trip_id;
+
+       if (!device_is_registered(dev))
+               return -ENODEV;
 
        if (sscanf(attr->attr.name, "trip_point_%d_type", &trip_id) != 1)
                return -EINVAL;
 
-       mutex_lock(&tz->lock);
-

drivers/thermal/thermal_sysfs.c +16 -26

+               return -EINVAL;
 
-       switch (trip.type) {
+       switch (tz->trips[trip_id].type) {
        case THERMAL_TRIP_CRITICAL:
                return sprintf(buf, "critical\n");
        case THERMAL_TRIP_HOT:

@@ -164,25 +157,18 @@ 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;
+
+       if (!device_is_registered(dev))
+               return -ENODEV;
 
        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;
-
-       mutex_unlock(&tz->lock);
-
-       if (ret)
-               return ret;
+       if (trip_id < 0 || trip_id > tz->num_trips)
+               return -EINVAL;
 
-       return sprintf(buf, "%d\n", trip.temperature);
+       return sprintf(buf, "%d\n", tz->trips[trip_id].temperature);
 }
 
 static ssize_t

@@ -234,22 +220,18 @@ 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;
+
+       if (!device_is_registered(dev))
+               return -ENODEV;
 
        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;
-
-       mutex_unlock(&tz->lock);
+       if (trip_id < 0 || trip_id > tz->num_trips)
+               return -EINVAL;
 
-       return ret ? ret : sprintf(buf, "%d\n", trip.hysteresis);
+       return sprintf(buf, "%d\n", tz->trips[trip_id].hysteresis);
 }
 
 static ssize_t

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

The _show() callback functions of the trip point sysfs attributes, temperature, hysteresis and type, need not use thermal zone locking, because the layout of the data structures they access does not change after the thermal zone registration.

Namely, they all need to access a specific entry in the thermal zone's trips[] table that is always present for non-tripless thermal zones and its size cannot change after the thermal zone has been registered. Thus it is always safe to access the trips[] table of a registered thermal zone from each of the sysfs attributes in question.

Moreover, the type of a trip point does not change after registering its thermal zone, and while its temperature and hysteresis can change, for example due to a firmware-induced thermal zone update, holding the zone lock around reading them is pointless, because it does not prevent stale values from being returned to user space. For example, a trip point temperature can always change ater trip_point_temp_show() has read it and before the function's return statement is executed, regardless of whether or not zone locking is used.

For this reason, drop the zone locking from trip_point_type_show(), trip_point_temp_show(), and trip_point_hyst_show().

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

---
  drivers/thermal/thermal_sysfs.c |   60 ++++++++++++++--------------------------
  1 file changed, 21 insertions(+), 39 deletions(-)

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

drivers/thermal/thermal_sysfs.c +21 -39

@@ -83,25 +83,18 @@ 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;
+	int trip_id;
+
+	if (!device_is_registered(dev))
+		return -ENODEV;
 
  	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;
-
-	mutex_unlock(&tz->lock);
-
-	if (result)
-		return result;
+	if (trip_id < 0 || trip_id > tz->num_trips)
+		return -EINVAL;
 
-	switch (trip.type) {
+	switch (tz->trips[trip_id].type) {
  	case THERMAL_TRIP_CRITICAL:
  		return sprintf(buf, "critical\n");
  	case THERMAL_TRIP_HOT:

@@ -164,25 +157,18 @@ 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;
+
+	if (!device_is_registered(dev))
+		return -ENODEV;
 
  	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;
-
-	mutex_unlock(&tz->lock);
-
-	if (ret)
-		return ret;
+	if (trip_id < 0 || trip_id > tz->num_trips)
+		return -EINVAL;
 
-	return sprintf(buf, "%d\n", trip.temperature);
+	return sprintf(buf, "%d\n", tz->trips[trip_id].temperature);
  }
 
  static ssize_t

@@ -234,22 +220,18 @@ 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;
+
+	if (!device_is_registered(dev))
+		return -ENODEV;
 
  	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;
-
-	mutex_unlock(&tz->lock);
+	if (trip_id < 0 || trip_id > tz->num_trips)
+		return -EINVAL;
 
-	return ret ? ret : sprintf(buf, "%d\n", trip.hysteresis);
+	return sprintf(buf, "%d\n", tz->trips[trip_id].hysteresis);
  }
 
  static ssize_t

Quoted 22 lines · Rafael J. Wysocki

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

The _show() callback functions of the trip point sysfs attributes, temperature, hysteresis and type, need not use thermal zone locking, because the layout of the data structures they access does not change after the thermal zone registration.

Namely, they all need to access a specific entry in the thermal zone's trips[] table that is always present for non-tripless thermal zones and its size cannot change after the thermal zone has been registered. Thus it is always safe to access the trips[] table of a registered thermal zone from each of the sysfs attributes in question.

Moreover, the type of a trip point does not change after registering its thermal zone, and while its temperature and hysteresis can change, for example due to a firmware-induced thermal zone update, holding the zone lock around reading them is pointless, because it does not prevent stale values from being returned to user space. For example, a trip point temperature can always change ater trip_point_temp_show() has read it and before the function's return statement is executed, regardless of whether or not zone locking is used.

Quoted 2 lines

For this reason, drop the zone locking from trip_point_type_show(), trip_point_temp_show(), and trip_point_hyst_show().

Isn't the lock used to protect against device_del() from thermal_zone_device_unregister() ?

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

The _show() callback functions of the trip point sysfs attributes, temperature, hysteresis and type, need not use thermal zone locking, because the layout of the data structures they access does not change after the thermal zone registration.

Namely, they all need to access a specific entry in the thermal zone's trips[] table that is always present for non-tripless thermal zones and its size cannot change after the thermal zone has been registered. Thus it is always safe to access the trips[] table of a registered thermal zone from each of the sysfs attributes in question.

Moreover, the type of a trip point does not change after registering its thermal zone, and while its temperature and hysteresis can change, for example due to a firmware-induced thermal zone update, holding the zone lock around reading them is pointless, because it does not prevent stale values from being returned to user space. For example, a trip point temperature can always change ater trip_point_temp_show() has read it and before the function's return statement is executed, regardless of whether or not zone locking is used.

For this reason, drop the zone locking from trip_point_type_show(), trip_point_temp_show(), and trip_point_hyst_show().