[PATCH v1 0/2] thermal: sysfs: Simplifications of trip point attribute callbacks

[PATCH v1 0/2] thermal: sysfs: Simplifications of trip point attribute callbacks

[Rafael J. Wysocki]

Hi,

These patches simplify trip point attribute callbacks in two ways.

The first patch, which is a direct replacement for

https://patchwork.kernel.org/project/linux-pm/patch/4869676.GXAFRqVoOG@kreacher/

gets rid of redundant steps from the _store() callbacks for the trip point
temperature and hysteresis and makes them use zone locking only as necessary.

The second patch eliminates zone locking from all of the _show() callbacks
for trip point temperature, hysteresis and type, because it is not needed
there.

Please refer to the individual patch changelogs for details.

Thanks!

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

[Rafael J. Wysocki]

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

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, some checks done by them both need not go under the thermal
zone lock.

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 <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_core.h  |    2 +
 drivers/thermal/thermal_sysfs.c |   70 ++++++++++++++++++++++++++--------------
 drivers/thermal/thermal_trip.c  |   45 +++++--------------------
 include/linux/thermal.h         |    3 -
 4 files changed, 57 insertions(+), 63 deletions(-)

Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ 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 */
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -120,28 +120,39 @@ 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 temperature;
+
+	if (!device_is_registered(dev))
+		return -ENODEV;
 
 	if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
 		return -EINVAL;
 
+	if (trip_id < 0 || trip_id >= tz->num_trips)
+		return -EINVAL;
+
+	ret = kstrtoint(buf, 10, &temperature);
+	if (ret)
+		return -EINVAL;
+
 	mutex_lock(&tz->lock);
 
-	if (!device_is_registered(dev)) {
-		ret = -ENODEV;
-		goto unlock;
-	}
+	trip = &tz->trips[trip_id];
 
-	ret = __thermal_zone_get_trip(tz, trip_id, &trip);
-	if (ret)
-		goto unlock;
+	if (temperature != trip->temperature) {
+		if (tz->ops->set_trip_temp) {
+			ret = tz->ops->set_trip_temp(tz, trip_id, temperature);
+			if (ret)
+				goto unlock;
+		}
 
-	ret = kstrtoint(buf, 10, &trip.temperature);
-	if (ret)
-		goto unlock;
+		trip->temperature = temperature;
+
+		thermal_zone_trip_updated(tz, trip);
+	}
 
-	ret = thermal_zone_set_trip(tz, trip_id, &trip);
 unlock:
 	mutex_unlock(&tz->lock);
 	
@@ -179,28 +190,39 @@ 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 hysteresis;
+
+	if (!device_is_registered(dev))
+		return -ENODEV;
 
 	if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip_id) != 1)
 		return -EINVAL;
 
+	if (trip_id < 0 || trip_id >= tz->num_trips)
+		return -EINVAL;
+
+	ret = kstrtoint(buf, 10, &hysteresis);
+	if (ret || hysteresis < 0)
+		return -EINVAL;
+
 	mutex_lock(&tz->lock);
 
-	if (!device_is_registered(dev)) {
-		ret = -ENODEV;
-		goto unlock;
-	}
+	trip = &tz->trips[trip_id];
 
-	ret = __thermal_zone_get_trip(tz, trip_id, &trip);
-	if (ret)
-		goto unlock;
+	if (hysteresis != trip->hysteresis) {
+		if (tz->ops->set_trip_hyst) {
+			ret = tz->ops->set_trip_hyst(tz, trip_id, hysteresis);
+			if (ret)
+				goto unlock;
+		}
 
-	ret = kstrtoint(buf, 10, &trip.hysteresis);
-	if (ret)
-		goto unlock;
+		trip->hysteresis = hysteresis;
+
+		thermal_zone_trip_updated(tz, trip);
+	}
 
-	ret = thermal_zone_set_trip(tz, trip_id, &trip);
 unlock:
 	mutex_unlock(&tz->lock);
 
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -147,42 +147,6 @@ int thermal_zone_get_trip(struct thermal
 }
 EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
 
-int thermal_zone_set_trip(struct thermal_zone_device *tz, int trip_id,
-			  const struct thermal_trip *trip)
-{
-	struct thermal_trip t;
-	int ret;
-
-	ret = __thermal_zone_get_trip(tz, trip_id, &t);
-	if (ret)
-		return ret;
-
-	if (t.type != trip->type)
-		return -EINVAL;
-
-	if (t.temperature != trip->temperature && tz->ops->set_trip_temp) {
-		ret = tz->ops->set_trip_temp(tz, trip_id, trip->temperature);
-		if (ret)
-			return ret;
-	}
-
-	if (t.hysteresis != trip->hysteresis && tz->ops->set_trip_hyst) {
-		ret = tz->ops->set_trip_hyst(tz, trip_id, trip->hysteresis);
-		if (ret)
-			return ret;
-	}
-
-	if (tz->trips && (t.temperature != trip->temperature || t.hysteresis != trip->hysteresis))
-		tz->trips[trip_id] = *trip;
-
-	thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
-				      trip->temperature, trip->hysteresis);
-
-	__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
-
-	return 0;
-}
-
 int thermal_zone_trip_id(struct thermal_zone_device *tz,
 			 const struct thermal_trip *trip)
 {
@@ -192,3 +156,12 @@ int thermal_zone_trip_id(struct thermal_
 	 */
 	return trip - tz->trips;
 }
+
+void thermal_zone_trip_updated(struct thermal_zone_device *tz,
+			       const struct thermal_trip *trip)
+{
+	thermal_notify_tz_trip_change(tz->id, thermal_zone_trip_id(tz, trip),
+				      trip->type, trip->temperature,
+				      trip->hysteresis);
+	__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
+}
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -283,9 +283,6 @@ int __thermal_zone_get_trip(struct therm
 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);

[PATCH v1 2/2] thermal: sysfs: Eliminate unnecessary zone locking

[Rafael J. Wysocki]

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

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 <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_sysfs.c |   60 ++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 39 deletions(-)

Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -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

Re: [PATCH v1 2/2] thermal: sysfs: Eliminate unnecessary zone locking

[Rafael J. Wysocki]

On Thu, Nov 30, 2023 at 7:38 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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 <rafael.j.wysocki@intel.com>
> ---
>  drivers/thermal/thermal_sysfs.c |   60 ++++++++++++++--------------------------
>  1 file changed, 21 insertions(+), 39 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -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)

An off-by-one here, it should be trip_id >= tz->num_trips (and
analogously below).

I'll send an update of this shortly.

> +               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
>
>
>
>

Re: [PATCH v1 2/2] thermal: sysfs: Eliminate unnecessary zone locking

[Daniel Lezcano]

On 30/11/2023 19:37, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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().

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


> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_sysfs.c |   60 ++++++++++++++--------------------------
>   1 file changed, 21 insertions(+), 39 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -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
> 
> 
> 
> 

-- 
<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

Re: [PATCH v1 2/2] thermal: sysfs: Eliminate unnecessary zone locking

[Rafael J. Wysocki]

On Fri, Dec 1, 2023 at 10:16 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 30/11/2023 19:37, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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().
>
> Isn't the lock used to protect against device_del() from
> thermal_zone_device_unregister() ?

Ah, I missed the zone locking around device_del() in there.

OK, please scratch this series for now.

[PATCH v1.1 2/2] thermal: sysfs: Simplifications of trip point attribute callbacks

[Rafael J. Wysocki]

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

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 <rafael.j.wysocki@intel.com>
---

v1 -> v1.1: Use >= instead of > (which was incorrect) in 3 places.

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

Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -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

Re: [PATCH v1.1 2/2] thermal: sysfs: Simplifications of trip point attribute callbacks

[Rafael J. Wysocki]

Wrong subject, sorry for the noise.  Will resend.

On Thu, Nov 30, 2023 at 8:42 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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().

[PATCH v1.1 2/2] thermal: sysfs: Eliminate unnecessary zone locking

[Rafael J. Wysocki]

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

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 <rafael.j.wysocki@intel.com>
---

v1 -> v1.1: Use >= instead of > (which was incorrect) in 3 places.

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

Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -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