Skip to content

Commit d0b7306

Browse files
mdigiorgiozhang-rui
mdigiorgio
authored and
zhang-rui
committed
thermal: fix race condition when updating cooling device
When multiple thermal zones are bound to the same cooling device, multiple kernel threads may want to update the cooling device state by calling thermal_cdev_update(). Having cdev not protected by a mutex can lead to a race condition. Consider the following situation with two kernel threads k1 and k2: Thread k1 Thread k2 || || call thermal_cdev_update() || ... || set_cur_state(cdev, target); call power_actor_set_power() || ... || instance->target = state; || cdev->updated = false; || || cdev->updated = true; || // completes execution call thermal_cdev_update() || // cdev->updated == true || return; || \/ time k2 has already looped through the thermal instances looking for the deepest cooling device state and is preempted right before setting cdev->updated to true. Now, k1 runs, modifies the thermal instance state and sets cdev->updated to false. Then, k1 is preempted and k2 continues the execution by setting cdev->updated to true, therefore preventing k1 from performing the update. Notice that this is not an issue if k2 looks at the instance->target modified by k1 "after" it is assigned by k1. In fact, in this case the update will happen anyway and k1 can safely return immediately from thermal_cdev_update(). This may lead to a situation where a thermal governor never updates the cooling device. For example, this is the case for the step_wise governor: when calling the function thermal_zone_trip_update(), the governor may always get a new state equal to the old one (which, however, wasn't notified to the cooling device) and will therefore skip the update. CC: Zhang Rui <rui.zhang@intel.com> CC: Eduardo Valentin <edubezval@gmail.com> CC: Peter Feuerer <peter@piie.net> Reported-by: Toby Huang <toby.huang@arm.com> Signed-off-by: Michele Di Giorgio <michele.digiorgio@arm.com> Reviewed-by: Javi Merino <javi.merino@arm.com> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
1 parent 29b4817 commit d0b7306

File tree

5 files changed

+15
-3
lines changed

5 files changed

+15
-3
lines changed

‎drivers/thermal/fair_share.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,9 @@ static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
116116
instance->target = get_target_state(tz, cdev, percentage,
117117
cur_trip_level);
118118

119+
mutex_lock(&instance->cdev->lock);
119120
instance->cdev->updated = false;
121+
mutex_unlock(&instance->cdev->lock);
120122
thermal_cdev_update(cdev);
121123
}
122124
return 0;

‎drivers/thermal/gov_bang_bang.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,9 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
7171
dev_dbg(&instance->cdev->device, "target=%d\n",
7272
(int)instance->target);
7373

74+
mutex_lock(&instance->cdev->lock);
7475
instance->cdev->updated = false; /* cdev needs update */
76+
mutex_unlock(&instance->cdev->lock);
7577
}
7678

7779
mutex_unlock(&tz->lock);

‎drivers/thermal/power_allocator.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,9 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
529529
continue;
530530

531531
instance->target = 0;
532+
mutex_lock(&instance->cdev->lock);
532533
instance->cdev->updated = false;
534+
mutex_unlock(&instance->cdev->lock);
533535
thermal_cdev_update(instance->cdev);
534536
}
535537
}

‎drivers/thermal/step_wise.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,9 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
175175
update_passive_instance(tz, trip_type, -1);
176176

177177
instance->initialized = true;
178+
mutex_lock(&instance->cdev->lock);
178179
instance->cdev->updated = false; /* cdev needs update */
180+
mutex_unlock(&instance->cdev->lock);
179181
}
180182

181183
mutex_unlock(&tz->lock);

‎drivers/thermal/thermal_core.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1093,7 +1093,9 @@ int power_actor_set_power(struct thermal_cooling_device *cdev,
10931093
return ret;
10941094

10951095
instance->target = state;
1096+
mutex_lock(&cdev->lock);
10961097
cdev->updated = false;
1098+
mutex_unlock(&cdev->lock);
10971099
thermal_cdev_update(cdev);
10981100

10991101
return 0;
@@ -1623,11 +1625,13 @@ void thermal_cdev_update(struct thermal_cooling_device *cdev)
16231625
struct thermal_instance *instance;
16241626
unsigned long target = 0;
16251627

1628+
mutex_lock(&cdev->lock);
16261629
/* cooling device is updated*/
1627-
if (cdev->updated)
1630+
if (cdev->updated) {
1631+
mutex_unlock(&cdev->lock);
16281632
return;
1633+
}
16291634

1630-
mutex_lock(&cdev->lock);
16311635
/* Make sure cdev enters the deepest cooling state */
16321636
list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
16331637
dev_dbg(&cdev->device, "zone%d->target=%lu\n",
@@ -1637,9 +1641,9 @@ void thermal_cdev_update(struct thermal_cooling_device *cdev)
16371641
if (instance->target > target)
16381642
target = instance->target;
16391643
}
1640-
mutex_unlock(&cdev->lock);
16411644
cdev->ops->set_cur_state(cdev, target);
16421645
cdev->updated = true;
1646+
mutex_unlock(&cdev->lock);
16431647
trace_cdev_update(cdev, target);
16441648
dev_dbg(&cdev->device, "set to state %lu\n", target);
16451649
}

0 commit comments

Comments
 (0)