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>
This commit is contained in:
parent
29b4817d40
commit
d0b7306d20
|
@ -116,7 +116,9 @@ static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
|
||||||
instance->target = get_target_state(tz, cdev, percentage,
|
instance->target = get_target_state(tz, cdev, percentage,
|
||||||
cur_trip_level);
|
cur_trip_level);
|
||||||
|
|
||||||
|
mutex_lock(&instance->cdev->lock);
|
||||||
instance->cdev->updated = false;
|
instance->cdev->updated = false;
|
||||||
|
mutex_unlock(&instance->cdev->lock);
|
||||||
thermal_cdev_update(cdev);
|
thermal_cdev_update(cdev);
|
||||||
}
|
}
|
||||||
return 0;
|
return 0;
|
||||||
|
|
|
@ -71,7 +71,9 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
|
||||||
dev_dbg(&instance->cdev->device, "target=%d\n",
|
dev_dbg(&instance->cdev->device, "target=%d\n",
|
||||||
(int)instance->target);
|
(int)instance->target);
|
||||||
|
|
||||||
|
mutex_lock(&instance->cdev->lock);
|
||||||
instance->cdev->updated = false; /* cdev needs update */
|
instance->cdev->updated = false; /* cdev needs update */
|
||||||
|
mutex_unlock(&instance->cdev->lock);
|
||||||
}
|
}
|
||||||
|
|
||||||
mutex_unlock(&tz->lock);
|
mutex_unlock(&tz->lock);
|
||||||
|
|
|
@ -529,7 +529,9 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
|
||||||
continue;
|
continue;
|
||||||
|
|
||||||
instance->target = 0;
|
instance->target = 0;
|
||||||
|
mutex_lock(&instance->cdev->lock);
|
||||||
instance->cdev->updated = false;
|
instance->cdev->updated = false;
|
||||||
|
mutex_unlock(&instance->cdev->lock);
|
||||||
thermal_cdev_update(instance->cdev);
|
thermal_cdev_update(instance->cdev);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
@ -175,7 +175,9 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
|
||||||
update_passive_instance(tz, trip_type, -1);
|
update_passive_instance(tz, trip_type, -1);
|
||||||
|
|
||||||
instance->initialized = true;
|
instance->initialized = true;
|
||||||
|
mutex_lock(&instance->cdev->lock);
|
||||||
instance->cdev->updated = false; /* cdev needs update */
|
instance->cdev->updated = false; /* cdev needs update */
|
||||||
|
mutex_unlock(&instance->cdev->lock);
|
||||||
}
|
}
|
||||||
|
|
||||||
mutex_unlock(&tz->lock);
|
mutex_unlock(&tz->lock);
|
||||||
|
|
|
@ -1093,7 +1093,9 @@ int power_actor_set_power(struct thermal_cooling_device *cdev,
|
||||||
return ret;
|
return ret;
|
||||||
|
|
||||||
instance->target = state;
|
instance->target = state;
|
||||||
|
mutex_lock(&cdev->lock);
|
||||||
cdev->updated = false;
|
cdev->updated = false;
|
||||||
|
mutex_unlock(&cdev->lock);
|
||||||
thermal_cdev_update(cdev);
|
thermal_cdev_update(cdev);
|
||||||
|
|
||||||
return 0;
|
return 0;
|
||||||
|
@ -1623,11 +1625,13 @@ void thermal_cdev_update(struct thermal_cooling_device *cdev)
|
||||||
struct thermal_instance *instance;
|
struct thermal_instance *instance;
|
||||||
unsigned long target = 0;
|
unsigned long target = 0;
|
||||||
|
|
||||||
/* cooling device is updated*/
|
|
||||||
if (cdev->updated)
|
|
||||||
return;
|
|
||||||
|
|
||||||
mutex_lock(&cdev->lock);
|
mutex_lock(&cdev->lock);
|
||||||
|
/* cooling device is updated*/
|
||||||
|
if (cdev->updated) {
|
||||||
|
mutex_unlock(&cdev->lock);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
/* Make sure cdev enters the deepest cooling state */
|
/* Make sure cdev enters the deepest cooling state */
|
||||||
list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
|
list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
|
||||||
dev_dbg(&cdev->device, "zone%d->target=%lu\n",
|
dev_dbg(&cdev->device, "zone%d->target=%lu\n",
|
||||||
|
@ -1637,9 +1641,9 @@ void thermal_cdev_update(struct thermal_cooling_device *cdev)
|
||||||
if (instance->target > target)
|
if (instance->target > target)
|
||||||
target = instance->target;
|
target = instance->target;
|
||||||
}
|
}
|
||||||
mutex_unlock(&cdev->lock);
|
|
||||||
cdev->ops->set_cur_state(cdev, target);
|
cdev->ops->set_cur_state(cdev, target);
|
||||||
cdev->updated = true;
|
cdev->updated = true;
|
||||||
|
mutex_unlock(&cdev->lock);
|
||||||
trace_cdev_update(cdev, target);
|
trace_cdev_update(cdev, target);
|
||||||
dev_dbg(&cdev->device, "set to state %lu\n", target);
|
dev_dbg(&cdev->device, "set to state %lu\n", target);
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in New Issue