| Message ID | 5754079.DvuYhMxLoT@kreacher |
|---|---|
| Headers | show |
| Series | thermal: sysfs: Simplifications of trip point attribute callbacks | expand |
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 > > > >
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().
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 > > > >
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.