thermal: core: Remove thermal zones during unregistration

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

Make thermal_zone_device_unregister() wait until all of the references to the given thermal zone object have been dropped and free it before returning.

This guarantees that when thermal_zone_device_unregister() returns, there is no leftover activity regarding the thermal zone in question which is required by some of its callers (for instance, modular driver code that wants to know when it is safe to let the module go away).

Subsequently, this will allow some confusing device_is_registered() checks to be dropped from the thermal sysfs and core code.

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

---

  drivers/thermal/thermal_core.c |    6 +++++-
  include/linux/thermal.h        |    2 ++
  2 files changed, 7 insertions(+), 1 deletion(-)

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

drivers/thermal/thermal_core.c +5 -1

@@ -822,7 +822,7 @@ static void thermal_release(struct devic
  		tz = to_thermal_zone(dev);
  		thermal_zone_destroy_device_groups(tz);
  		mutex_destroy(&tz->lock);
-		kfree(tz);
+		complete(&tz->removal);
  	} else if (!strncmp(dev_name(dev), "cooling_device",
  			    sizeof("cooling_device") - 1)) {
  		cdev = to_cooling_device(dev);

@@ -1315,6 +1315,7 @@ thermal_zone_device_register_with_trips(
  	INIT_LIST_HEAD(&tz->thermal_instances);
  	ida_init(&tz->ida);
  	mutex_init(&tz->lock);
+	init_completion(&tz->removal);
  	id = ida_alloc(&thermal_tz_ida, GFP_KERNEL);
  	if (id < 0) {
  		result = id;

@@ -1494,6 +1495,9 @@ void thermal_zone_device_unregister(stru
  	put_device(&tz->device);
 
  	thermal_notify_tz_delete(tz_id);
+
+	wait_for_completion(&tz->removal);
+	kfree(tz);
  }
  EXPORT_SYMBOL_GPL(thermal_zone_device_unregister);
 

Index: linux-pm/include/linux/thermal.h
===================================================================

include/linux/thermal.h +2 -0

@@ -117,6 +117,7 @@ struct thermal_cooling_device {
   * @id:		unique id number for each thermal zone
   * @type:	the thermal zone device type
   * @device:	&struct device for this thermal zone
+ * @removal:	removal completion
   * @trip_temp_attrs:	attributes for trip points for sysfs: trip temperature
   * @trip_type_attrs:	attributes for trip points for sysfs: trip type
   * @trip_hyst_attrs:	attributes for trip points for sysfs: trip hysteresis

@@ -156,6 +157,7 @@ struct thermal_zone_device {
  	int id;
  	char type[THERMAL_NAME_LENGTH];
  	struct device device;
+	struct completion removal;
  	struct attribute_group trips_attribute_group;
  	struct thermal_attr *trip_temp_attrs;
  	struct thermal_attr *trip_type_attrs;

Quoted 14 lines · Rafael J. Wysocki

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

Make thermal_zone_device_unregister() wait until all of the references to the given thermal zone object have been dropped and free it before returning.

This guarantees that when thermal_zone_device_unregister() returns, there is no leftover activity regarding the thermal zone in question which is required by some of its callers (for instance, modular driver code that wants to know when it is safe to let the module go away).

Subsequently, this will allow some confusing device_is_registered() checks to be dropped from the thermal sysfs and core code.

Quoted 2 lines

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

---

Definitively agree on the change

Acked-by Daniel Lezcano <[email protected]>

Would it make sense to use kref_get/put ?

Quoted 21 lines · Daniel Lezcano

Quoted 11 lines · Rafael J. Wysocki

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

Make thermal_zone_device_unregister() wait until all of the references to the given thermal zone object have been dropped and free it before returning.

This guarantees that when thermal_zone_device_unregister() returns, there is no leftover activity regarding the thermal zone in question which is required by some of its callers (for instance, modular driver code that wants to know when it is safe to let the module go away).

Quoted 5 lines

Subsequently, this will allow some confusing device_is_registered() checks to be dropped from the thermal sysfs and core code.

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

---

Definitively agree on the change

Quoted 1 line

Acked-by Daniel Lezcano <[email protected]>

Thanks!

Quoted 1 line

Would it make sense to use kref_get/put ?

Why and where?

Quoted 25 lines · Rafael J. Wysocki

Quoted 22 lines · Daniel Lezcano

Quoted 11 lines · Rafael J. Wysocki

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

Make thermal_zone_device_unregister() wait until all of the references to the given thermal zone object have been dropped and free it before returning.

This guarantees that when thermal_zone_device_unregister() returns, there is no leftover activity regarding the thermal zone in question which is required by some of its callers (for instance, modular driver code that wants to know when it is safe to let the module go away).

Quoted 5 lines

Subsequently, this will allow some confusing device_is_registered() checks to be dropped from the thermal sysfs and core code.

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

---

Definitively agree on the change

Acked-by Daniel Lezcano <[email protected]>

Quoted 5 lines

Thanks!

Quoted 1 line

Would it make sense to use kref_get/put ?

Why and where?

Well it is a general question. Usually this kind of removal is tied with a refcount

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

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 <[email protected]>

---

drivers/thermal/thermal_sysfs.c +1 -9

@@ -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)) {

Quoted 19 lines · Rafael J. Wysocki

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

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 <[email protected]>

---

[ ... ]

Quoted 22 lines

drivers/thermal/thermal_sysfs.c +1 -6

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

drivers/thermal/thermal_sysfs.c trip_point_temp_show(struct device *dev, · line 153

Quoted 13 lines

drivers/thermal/thermal_sysfs.c +1 -12

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

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

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 <[email protected]>

---

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

In order to avoid running __thermal_zone_device_update() for thermal zones going away, the thermal zone lock is held around device_del() in thermal_zone_device_unregister() and thermal_zone_device_update() passes the given thermal zone device to device_is_registered(). This allows thermal_zone_device_update() to skip the __thermal_zone_device_update() if device_del() has already run for the thermal zone at hand.

However, instead of looking at driver core internals, the thermal subsystem may as well rely on its own data structures for this purpose. Namely, if the thermal zone is not present in thermal_tz_list, it can be regarded as unavailable, which in fact is already the case in thermal_zone_device_unregister(). Accordingly, the device_is_registered() check in thermal_zone_device_update() can be replaced with checking whether or not the node list_head in struct thermal_zone_device is empty, in which case it is not there in thermal_tz_list.

To make this work, though, it is necessary to initialize tz->node in thermal_zone_device_register_with_trips() before registering the thermal zone device and it needs to be added to thermal_tz_list and deleted from it under its zone lock.

After the above modifications, the zone lock does not need to be held around device_del() in thermal_zone_device_unregister() any more.

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

---

Hi All,

This patch series adds a mechanism to guarantee that thermal_zone_device_unregister() will not return until all of the active references to the thermal zone device object in question have been dropped and it has been deleted (patch [1/3]).

This supersedes the approach used so far in which all thermal zone sysfs attribute callbacks check if the zone device is still registered under the zone lock, so as to return early if that is not the case, as it means that device_del() has been called for the thermal zone in question (and returned). It is not necessary to do that any more after patch [1/3], so patch [2/3] removes those checks from the code and drops zone locking that is not necessary any more either.

Patch [3/3] uses the observation that the thermal subsystem does not need to check if a thermal zone device is registered at all, because it can use its own data to determine whether or not the thermal zone is going away and so it may not be worth updating it, for example.

Please refer to the patch changelogs for details.

The series depends on new thermal material in linux-next, but it should not substantially depend on any changes that have not made it into linux-next yet.

Thanks!

Hi Rafael,

Quoted 20 lines · Rafael J. Wysocki

Hi All,

This patch series adds a mechanism to guarantee that thermal_zone_device_unregister() will not return until all of the active references to the thermal zone device object in question have been dropped and it has been deleted (patch [1/3]).

This supersedes the approach used so far in which all thermal zone sysfs attribute callbacks check if the zone device is still registered under the zone lock, so as to return early if that is not the case, as it means that device_del() has been called for the thermal zone in question (and returned). It is not necessary to do that any more after patch [1/3], so patch [2/3] removes those checks from the code and drops zone locking that is not necessary any more either.

Patch [3/3] uses the observation that the thermal subsystem does not need to check if a thermal zone device is registered at all, because it can use its own data to determine whether or not the thermal zone is going away and so it may not be worth updating it, for example.

Quoted 9 lines

Please refer to the patch changelogs for details.

The series depends on new thermal material in linux-next, but it should not substantially depend on any changes that have not made it into linux-next yet.

Thanks!

I like the concept with completion thing for this. I have tired to stress test these patches with my mock thermal zone module load/unload and it works good.

The test was doing the these bits:
for i in $(seq 1 1000000) ; do cat /sys/class/thermal/thermal_zone2/trip_point_0_temp > /dev/null 2>&1 ; done & for i in $(seq 1 10000) ; do insmod /data/selftest_ipa.ko ; rmmod selftest_ipa ; done &

I couldn't trigger any issues in reading from this
trip temp file in background, which should go now w/o the locking. I thought it would be nice test, since we have direct call to trips array 'tz->trips[trip_id].temperature'. Let me know if you think about other scenario for stress testing it. (I have also checked the 'temp' sysfs read, where the mutex for tz is used - also no issues).

Feel free to add to all patches:

Reviewed-and-tested-by Lukasz Luba <[email protected]>