[PATCH 0/3] Thermal governors improvements and a fix

[PATCH 0/3] Thermal governors improvements and a fix

[Lukasz Luba]

Hi all,

The patch set introduces one fix (patch 1/3) and two improvements, which
are possible thanks to the new helper function [1].
The patch 1/3 with a fix for fair share thermal governor is also sent
CC'ed stable, but it's hard to point a particular commit, which back
then was for fair_share.c.

The patch set should apply on top of [1].

Regards,
Lukasz

[1] https://lore.kernel.org/linux-pm/20210422114308.29684-2-lukasz.luba@arm.com/

Lukasz Luba (3):
  thermal: fair share: lock thermal zone while looping over instances
  thermal: fair share: use __thermal_cdev_update()
  thermal: power allocator: use __thermal_cdev_update()

 drivers/thermal/gov_fair_share.c      | 11 +++++++----
 drivers/thermal/gov_power_allocator.c |  3 +--
 2 files changed, 8 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/3] thermal: fair share: lock thermal zone while looping over instances

[Lukasz Luba]

The tz->lock must be hold during the looping over the instances in that
thermal zone. This lock was missing in the governor code since the
beginning, so it's hard to point into a particular commit.

CC: stable@vger.kernel.org # 4.4+
Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_fair_share.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c
index aaa07180ab48..645432ce6365 100644
--- a/drivers/thermal/gov_fair_share.c
+++ b/drivers/thermal/gov_fair_share.c
@@ -82,6 +82,8 @@ static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
 	int total_instance = 0;
 	int cur_trip_level = get_trip_level(tz);
 
+	mutex_lock(&tz->lock);
+
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		if (instance->trip != trip)
 			continue;
@@ -110,6 +112,8 @@ static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
 		mutex_unlock(&instance->cdev->lock);
 		thermal_cdev_update(cdev);
 	}
+
+	mutex_unlock(&tz->lock);
 	return 0;
 }
 
-- 
2.17.1

[PATCH 2/3] thermal: fair share: use __thermal_cdev_update()

[Lukasz Luba]

Use the new helper function and avoid unnecessery second lock/unlock,
which was present in old approach with thermal_cdev_update().

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_fair_share.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c
index 645432ce6365..1e5abf4822be 100644
--- a/drivers/thermal/gov_fair_share.c
+++ b/drivers/thermal/gov_fair_share.c
@@ -107,10 +107,9 @@ static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
 		instance->target = get_target_state(tz, cdev, percentage,
 						    cur_trip_level);
 
-		mutex_lock(&instance->cdev->lock);
-		instance->cdev->updated = false;
-		mutex_unlock(&instance->cdev->lock);
-		thermal_cdev_update(cdev);
+		mutex_lock(&cdev->lock);
+		__thermal_cdev_update(cdev);
+		mutex_unlock(&cdev->lock);
 	}
 
 	mutex_unlock(&tz->lock);
-- 
2.17.1

[PATCH 3/3] thermal: power allocator: use __thermal_cdev_update()

[Lukasz Luba]

Use the new helper function and avoid unnecessery second lock/unlock,
which was present in old approach with thermal_cdev_update().

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 drivers/thermal/gov_power_allocator.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index a6cdb2e892da..13e375751d22 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -301,9 +301,8 @@ power_actor_set_power(struct thermal_cooling_device *cdev,
 
 	instance->target = clamp_val(state, instance->lower, instance->upper);
 	mutex_lock(&cdev->lock);
-	cdev->updated = false;
+	__thermal_cdev_update(cdev);
 	mutex_unlock(&cdev->lock);
-	thermal_cdev_update(cdev);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH 0/3] Thermal governors improvements and a fix

[Daniel Lezcano]

On 22/04/2021 17:36, Lukasz Luba wrote:
> Hi all,
> 
> The patch set introduces one fix (patch 1/3) and two improvements, which
> are possible thanks to the new helper function [1].
> The patch 1/3 with a fix for fair share thermal governor is also sent
> CC'ed stable, but it's hard to point a particular commit, which back
> then was for fair_share.c.
> 
> The patch set should apply on top of [1].
> 
> Regards,
> Lukasz
> 
> [1] https://lore.kernel.org/linux-pm/20210422114308.29684-2-lukasz.luba@arm.com/
> 
> Lukasz Luba (3):
>   thermal: fair share: lock thermal zone while looping over instances
>   thermal: fair share: use __thermal_cdev_update()
>   thermal: power allocator: use __thermal_cdev_update()
> 
>  drivers/thermal/gov_fair_share.c      | 11 +++++++----
>  drivers/thermal/gov_power_allocator.c |  3 +--
>  2 files changed, 8 insertions(+), 6 deletions(-)

Applied, thanks

Two users left of thermal_cdev_update ;)


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/3] Thermal governors improvements and a fix

[Lukasz Luba]

On 4/22/21 6:31 PM, Daniel Lezcano wrote:
> On 22/04/2021 17:36, Lukasz Luba wrote:
>> Hi all,
>>
>> The patch set introduces one fix (patch 1/3) and two improvements, which
>> are possible thanks to the new helper function [1].
>> The patch 1/3 with a fix for fair share thermal governor is also sent
>> CC'ed stable, but it's hard to point a particular commit, which back
>> then was for fair_share.c.
>>
>> The patch set should apply on top of [1].
>>
>> Regards,
>> Lukasz
>>
>> [1] https://lore.kernel.org/linux-pm/20210422114308.29684-2-lukasz.luba@arm.com/
>>
>> Lukasz Luba (3):
>>    thermal: fair share: lock thermal zone while looping over instances
>>    thermal: fair share: use __thermal_cdev_update()
>>    thermal: power allocator: use __thermal_cdev_update()
>>
>>   drivers/thermal/gov_fair_share.c      | 11 +++++++----
>>   drivers/thermal/gov_power_allocator.c |  3 +--
>>   2 files changed, 8 insertions(+), 6 deletions(-)
> 
> Applied, thanks

thanks!

> 
> Two users left of thermal_cdev_update ;)
> 
> 

True, I didn't dare to touch them, since that would require more
work and understandings :)

Re: [PATCH 0/3] Thermal governors improvements and a fix

[Daniel Lezcano]

On 22/04/2021 19:33, Lukasz Luba wrote:
> 
> 
> On 4/22/21 6:31 PM, Daniel Lezcano wrote:
>> On 22/04/2021 17:36, Lukasz Luba wrote:
>>> Hi all,
>>>
>>> The patch set introduces one fix (patch 1/3) and two improvements, which
>>> are possible thanks to the new helper function [1].
>>> The patch 1/3 with a fix for fair share thermal governor is also sent
>>> CC'ed stable, but it's hard to point a particular commit, which back
>>> then was for fair_share.c.
>>>
>>> The patch set should apply on top of [1].
>>>
>>> Regards,
>>> Lukasz
>>>
>>> [1]
>>> https://lore.kernel.org/linux-pm/20210422114308.29684-2-lukasz.luba@arm.com/
>>>
>>>
>>> Lukasz Luba (3):
>>>    thermal: fair share: lock thermal zone while looping over instances
>>>    thermal: fair share: use __thermal_cdev_update()
>>>    thermal: power allocator: use __thermal_cdev_update()
>>>
>>>   drivers/thermal/gov_fair_share.c      | 11 +++++++----
>>>   drivers/thermal/gov_power_allocator.c |  3 +--
>>>   2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> Applied, thanks
> 
> thanks!
> 
>>
>> Two users left of thermal_cdev_update ;)
>>
>>
> 
> True, I didn't dare to touch them, since that would require more
> work and understandings :)

Yeah I agree. The changes are no trivial ...




-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[thermal: thermal/next] thermal/core/fair share: Use the lockless __thermal_cdev_update() function

[thermal-bot for Lukasz Luba]

The following commit has been merged into the thermal/next branch of thermal:

Commit-ID:     1a93369810660905f5e89b527cd709fa6832f7c8
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git//1a93369810660905f5e89b527cd709fa6832f7c8
Author:        Lukasz Luba <lukasz.luba@arm.com>
AuthorDate:    Thu, 22 Apr 2021 16:36:23 +01:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Thu, 22 Apr 2021 23:51:32 +02:00

thermal/core/fair share: Use the lockless __thermal_cdev_update() function

Use the new helper function and avoid unnecessery second lock/unlock,
which was present in old approach with thermal_cdev_update().

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20210422153624.6074-3-lukasz.luba@arm.com
---
 drivers/thermal/gov_fair_share.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c
index 645432c..1e5abf4 100644
--- a/drivers/thermal/gov_fair_share.c
+++ b/drivers/thermal/gov_fair_share.c
@@ -107,10 +107,9 @@ static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
 		instance->target = get_target_state(tz, cdev, percentage,
 						    cur_trip_level);
 
-		mutex_lock(&instance->cdev->lock);
-		instance->cdev->updated = false;
-		mutex_unlock(&instance->cdev->lock);
-		thermal_cdev_update(cdev);
+		mutex_lock(&cdev->lock);
+		__thermal_cdev_update(cdev);
+		mutex_unlock(&cdev->lock);
 	}
 
 	mutex_unlock(&tz->lock);

[thermal: thermal/next] thermal/core/power allocator: Use the lockless __thermal_cdev_update() function

[thermal-bot for Lukasz Luba]

The following commit has been merged into the thermal/next branch of thermal:

Commit-ID:     ab39c8853737158604e154ad3b03639e74082bd6
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git//ab39c8853737158604e154ad3b03639e74082bd6
Author:        Lukasz Luba <lukasz.luba@arm.com>
AuthorDate:    Thu, 22 Apr 2021 16:36:24 +01:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Thu, 22 Apr 2021 23:51:32 +02:00

thermal/core/power allocator: Use the lockless __thermal_cdev_update() function

Use the new helper function and avoid unnecessery second lock/unlock,
which was present in old approach with thermal_cdev_update().

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20210422153624.6074-4-lukasz.luba@arm.com
---
 drivers/thermal/gov_power_allocator.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index a6cdb2e..13e3757 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -301,9 +301,8 @@ power_actor_set_power(struct thermal_cooling_device *cdev,
 
 	instance->target = clamp_val(state, instance->lower, instance->upper);
 	mutex_lock(&cdev->lock);
-	cdev->updated = false;
+	__thermal_cdev_update(cdev);
 	mutex_unlock(&cdev->lock);
-	thermal_cdev_update(cdev);
 
 	return 0;
 }

[thermal: thermal/next] thermal/core/fair share: Lock the thermal zone while looping over instances

[thermal-bot for Lukasz Luba]

The following commit has been merged into the thermal/next branch of thermal:

Commit-ID:     fef05776eb02238dcad8d5514e666a42572c3f32
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git//fef05776eb02238dcad8d5514e666a42572c3f32
Author:        Lukasz Luba <lukasz.luba@arm.com>
AuthorDate:    Thu, 22 Apr 2021 16:36:22 +01:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Thu, 22 Apr 2021 23:51:32 +02:00

thermal/core/fair share: Lock the thermal zone while looping over instances

The tz->lock must be hold during the looping over the instances in that
thermal zone. This lock was missing in the governor code since the
beginning, so it's hard to point into a particular commit.

CC: stable@vger.kernel.org # 4.4+
Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20210422153624.6074-2-lukasz.luba@arm.com
---
 drivers/thermal/gov_fair_share.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c
index aaa0718..645432c 100644
--- a/drivers/thermal/gov_fair_share.c
+++ b/drivers/thermal/gov_fair_share.c
@@ -82,6 +82,8 @@ static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
 	int total_instance = 0;
 	int cur_trip_level = get_trip_level(tz);
 
+	mutex_lock(&tz->lock);
+
 	list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
 		if (instance->trip != trip)
 			continue;
@@ -110,6 +112,8 @@ static int fair_share_throttle(struct thermal_zone_device *tz, int trip)
 		mutex_unlock(&instance->cdev->lock);
 		thermal_cdev_update(cdev);
 	}
+
+	mutex_unlock(&tz->lock);
 	return 0;
 }