[v1,2/3] thermal: Drop redundant and confusing device_is_registered() checks
Commit Message
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Multiple places in the thermal subsystem (most importantly, sysfs
attribute callback functions) check if the given thermal zone device is
still registered in order to return early in case the device_del() in
thermal_zone_device_unregister() has run already.
However, after thermal_zone_device_unregister() has been made wait for
all of the zone-related activity to complete before returning, it is
not necessary to do that any more, because all of the code holding a
reference to the thermal zone device object will be waited for even if
it does not do anything special to enforce this.
Accordingly, drop all of the device_is_registered() checks that are now
redundant and get rid of the zone locking that is not necessary any more
after dropping them.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/thermal/thermal_core.c | 9 -----
drivers/thermal/thermal_helpers.c | 5 ---
drivers/thermal/thermal_hwmon.c | 5 ---
drivers/thermal/thermal_sysfs.c | 60 +++-----------------------------------
4 files changed, 7 insertions(+), 72 deletions(-)
Comments
On 08/12/2023 20:19, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Multiple places in the thermal subsystem (most importantly, sysfs
> attribute callback functions) check if the given thermal zone device is
> still registered in order to return early in case the device_del() in
> thermal_zone_device_unregister() has run already.
>
> However, after thermal_zone_device_unregister() has been made wait for
> all of the zone-related activity to complete before returning, it is
> not necessary to do that any more, because all of the code holding a
> reference to the thermal zone device object will be waited for even if
> it does not do anything special to enforce this.
>
> Accordingly, drop all of the device_is_registered() checks that are now
> redundant and get rid of the zone locking that is not necessary any more
> after dropping them.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
[ ... ]
> @@ -132,11 +120,6 @@ trip_point_temp_store(struct device *dev
>
> mutex_lock(&tz->lock);
>
> - if (!device_is_registered(dev)) {
> - ret = -ENODEV;
> - goto unlock;
> - }
> -
> trip = &tz->trips[trip_id];
>
> if (temp != trip->temperature) {
> @@ -162,23 +145,12 @@ trip_point_temp_show(struct device *dev,
> char *buf)
> {
> struct thermal_zone_device *tz = to_thermal_zone(dev);
> - int trip_id, temp;
> + int trip_id;
>
> if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
> return -EINVAL;
>
> - mutex_lock(&tz->lock);
> -
> - if (!device_is_registered(dev)) {
> - mutex_unlock(&tz->lock);
> - return -ENODEV;
> - }
> -
> - temp = tz->trips[trip_id].temperature;
> -
> - mutex_unlock(&tz->lock);
> -
> - return sprintf(buf, "%d\n", temp);
> + return sprintf(buf, "%d\n", tz->trips[trip_id].temperature);
Without the lock, could the trip_temp_store() make the value change
while we read it?
[ ... ]
On Mon, Dec 11, 2023 at 6:39 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 08/12/2023 20:19, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Multiple places in the thermal subsystem (most importantly, sysfs
> > attribute callback functions) check if the given thermal zone device is
> > still registered in order to return early in case the device_del() in
> > thermal_zone_device_unregister() has run already.
> >
> > However, after thermal_zone_device_unregister() has been made wait for
> > all of the zone-related activity to complete before returning, it is
> > not necessary to do that any more, because all of the code holding a
> > reference to the thermal zone device object will be waited for even if
> > it does not do anything special to enforce this.
> >
> > Accordingly, drop all of the device_is_registered() checks that are now
> > redundant and get rid of the zone locking that is not necessary any more
> > after dropping them.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
>
> [ ... ]
>
> > @@ -132,11 +120,6 @@ trip_point_temp_store(struct device *dev
> >
> > mutex_lock(&tz->lock);
> >
> > - if (!device_is_registered(dev)) {
> > - ret = -ENODEV;
> > - goto unlock;
> > - }
> > -
> > trip = &tz->trips[trip_id];
> >
> > if (temp != trip->temperature) {
> > @@ -162,23 +145,12 @@ trip_point_temp_show(struct device *dev,
> > char *buf)
> > {
> > struct thermal_zone_device *tz = to_thermal_zone(dev);
> > - int trip_id, temp;
> > + int trip_id;
> >
> > if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
> > return -EINVAL;
> >
> > - mutex_lock(&tz->lock);
> > -
> > - if (!device_is_registered(dev)) {
> > - mutex_unlock(&tz->lock);
> > - return -ENODEV;
> > - }
> > -
> > - temp = tz->trips[trip_id].temperature;
> > -
> > - mutex_unlock(&tz->lock);
> > -
> > - return sprintf(buf, "%d\n", temp);
> > + return sprintf(buf, "%d\n", tz->trips[trip_id].temperature);
>
> Without the lock, could the trip_temp_store() make the value change
> while we read it?
The lock doesn't change that, because the write can occur before
dropping the lock and the printf() and reading an int is atomic on all
architectures supported by Linux.
On 08/12/2023 20:19, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Multiple places in the thermal subsystem (most importantly, sysfs
> attribute callback functions) check if the given thermal zone device is
> still registered in order to return early in case the device_del() in
> thermal_zone_device_unregister() has run already.
>
> However, after thermal_zone_device_unregister() has been made wait for
> all of the zone-related activity to complete before returning, it is
> not necessary to do that any more, because all of the code holding a
> reference to the thermal zone device object will be waited for even if
> it does not do anything special to enforce this.
>
> Accordingly, drop all of the device_is_registered() checks that are now
> redundant and get rid of the zone locking that is not necessary any more
> after dropping them.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
===================================================================
@@ -83,24 +83,12 @@ trip_point_type_show(struct device *dev,
char *buf)
{
struct thermal_zone_device *tz = to_thermal_zone(dev);
- enum thermal_trip_type type;
int trip_id;
if (sscanf(attr->attr.name, "trip_point_%d_type", &trip_id) != 1)
return -EINVAL;
- mutex_lock(&tz->lock);
-
- if (!device_is_registered(dev)) {
- mutex_unlock(&tz->lock);
- return -ENODEV;
- }
-
- type = tz->trips[trip_id].type;
-
- mutex_unlock(&tz->lock);
-
- switch (type) {
+ switch (tz->trips[trip_id].type) {
case THERMAL_TRIP_CRITICAL:
return sprintf(buf, "critical\n");
case THERMAL_TRIP_HOT:
@@ -132,11 +120,6 @@ trip_point_temp_store(struct device *dev
mutex_lock(&tz->lock);
- if (!device_is_registered(dev)) {
- ret = -ENODEV;
- goto unlock;
- }
-
trip = &tz->trips[trip_id];
if (temp != trip->temperature) {
@@ -162,23 +145,12 @@ trip_point_temp_show(struct device *dev,
char *buf)
{
struct thermal_zone_device *tz = to_thermal_zone(dev);
- int trip_id, temp;
+ int trip_id;
if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip_id) != 1)
return -EINVAL;
- mutex_lock(&tz->lock);
-
- if (!device_is_registered(dev)) {
- mutex_unlock(&tz->lock);
- return -ENODEV;
- }
-
- temp = tz->trips[trip_id].temperature;
-
- mutex_unlock(&tz->lock);
-
- return sprintf(buf, "%d\n", temp);
+ return sprintf(buf, "%d\n", tz->trips[trip_id].temperature);
}
static ssize_t
@@ -199,11 +171,6 @@ trip_point_hyst_store(struct device *dev
mutex_lock(&tz->lock);
- if (!device_is_registered(dev)) {
- ret = -ENODEV;
- goto unlock;
- }
-
trip = &tz->trips[trip_id];
if (hyst != trip->hysteresis) {
@@ -229,23 +196,12 @@ trip_point_hyst_show(struct device *dev,
char *buf)
{
struct thermal_zone_device *tz = to_thermal_zone(dev);
- int trip_id, hyst;
+ int trip_id;
if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip_id) != 1)
return -EINVAL;
- mutex_lock(&tz->lock);
-
- if (!device_is_registered(dev)) {
- mutex_unlock(&tz->lock);
- return -ENODEV;
- }
-
- hyst = tz->trips[trip_id].hysteresis;
-
- mutex_unlock(&tz->lock);
-
- return sprintf(buf, "%d\n", hyst);
+ return sprintf(buf, "%d\n", tz->trips[trip_id].hysteresis);
}
static ssize_t
@@ -294,11 +250,6 @@ emul_temp_store(struct device *dev, stru
mutex_lock(&tz->lock);
- if (!device_is_registered(dev)) {
- ret = -ENODEV;
- goto unlock;
- }
-
if (!tz->ops->set_emul_temp)
tz->emul_temperature = temperature;
else
@@ -307,7 +258,6 @@ emul_temp_store(struct device *dev, stru
if (!ret)
__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
-unlock:
mutex_unlock(&tz->lock);
return ret ? ret : count;
===================================================================
@@ -203,9 +203,6 @@ int thermal_zone_device_set_policy(struc
mutex_lock(&thermal_governor_lock);
mutex_lock(&tz->lock);
- if (!device_is_registered(&tz->device))
- goto exit;
-
gov = __find_governor(strim(policy));
if (!gov)
goto exit;
@@ -471,12 +468,6 @@ static int thermal_zone_device_set_mode(
return ret;
}
- if (!device_is_registered(&tz->device)) {
- mutex_unlock(&tz->lock);
-
- return -ENODEV;
- }
-
if (tz->ops->change_mode)
ret = tz->ops->change_mode(tz, mode);
===================================================================
@@ -137,10 +137,7 @@ int thermal_zone_get_temp(struct thermal
goto unlock;
}
- if (device_is_registered(&tz->device))
- ret = __thermal_zone_get_temp(tz, temp);
- else
- ret = -ENODEV;
+ ret = __thermal_zone_get_temp(tz, temp);
unlock:
mutex_unlock(&tz->lock);
===================================================================
@@ -80,10 +80,7 @@ temp_crit_show(struct device *dev, struc
mutex_lock(&tz->lock);
- if (device_is_registered(&tz->device))
- ret = tz->ops->get_crit_temp(tz, &temperature);
- else
- ret = -ENODEV;
+ ret = tz->ops->get_crit_temp(tz, &temperature);
mutex_unlock(&tz->lock);