[PATCH 1/2] thermal: core: Move thermal_cdev_update next to updated=false

[PATCH 1/2] thermal: core: Move thermal_cdev_update next to updated=false

[Daniel Lezcano]

The call to the thermal_cdev_update() function is done after browsing
the thermal instances which sets the updated flag by browsing them
again.

Instead of doing this, let's move the call right after setting the
cooling device 'updated' flag as it is done in the other governors.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/gov_bang_bang.c | 10 +---------
 drivers/thermal/step_wise.c     | 10 +---------
 2 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c
index 991a1c54296d..c292a69845bb 100644
--- a/drivers/thermal/gov_bang_bang.c
+++ b/drivers/thermal/gov_bang_bang.c
@@ -64,6 +64,7 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 		mutex_lock(&instance->cdev->lock);
 		instance->cdev->updated = false; /* cdev needs update */
 		mutex_unlock(&instance->cdev->lock);
+		thermal_cdev_update(instance->cdev);
 	}
 
 	mutex_unlock(&tz->lock);
@@ -98,17 +99,8 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
  */
 static int bang_bang_control(struct thermal_zone_device *tz, int trip)
 {
-	struct thermal_instance *instance;
-
 	thermal_zone_trip_update(tz, trip);
 
-	mutex_lock(&tz->lock);
-
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-		thermal_cdev_update(instance->cdev);
-
-	mutex_unlock(&tz->lock);
-
 	return 0;
 }
 
diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index 2ae7198d3067..298eedac0293 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -167,6 +167,7 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 		mutex_lock(&instance->cdev->lock);
 		instance->cdev->updated = false; /* cdev needs update */
 		mutex_unlock(&instance->cdev->lock);
+		thermal_cdev_update(instance->cdev);
 	}
 
 	mutex_unlock(&tz->lock);
@@ -185,20 +186,11 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
  */
 static int step_wise_throttle(struct thermal_zone_device *tz, int trip)
 {
-	struct thermal_instance *instance;
-
 	thermal_zone_trip_update(tz, trip);
 
 	if (tz->forced_passive)
 		thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE);
 
-	mutex_lock(&tz->lock);
-
-	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
-		thermal_cdev_update(instance->cdev);
-
-	mutex_unlock(&tz->lock);
-
 	return 0;
 }
 
-- 
2.17.1

[PATCH 2/2] thermal: core: Remove pointless 'updated' boolean

[Daniel Lezcano]

The sequence to update the cooling state in the thermal instances is always:

    mutex_lock(&instance->cdev->lock);
    instance->cdev->updated = false;
    mutex_unlock(&instance->cdev->lock);
    thermal_cdev_update(instance->cdev);

So each call to thermal_cdev_update() is prefixed by resetting the updated
flag which turns on to be a pointless test in the function itself.

Remove the flag.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/fair_share.c      | 3 ---
 drivers/thermal/gov_bang_bang.c   | 3 ---
 drivers/thermal/power_allocator.c | 3 ---
 drivers/thermal/step_wise.c       | 3 ---
 drivers/thermal/thermal_core.c    | 4 ----
 drivers/thermal/thermal_helpers.c | 6 ------
 include/linux/thermal.h           | 1 -
 7 files changed, 23 deletions(-)

diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
index aaa07180ab48..718de1f96cb6 100644
--- a/drivers/thermal/fair_share.c
+++ b/drivers/thermal/fair_share.c
@@ -105,9 +105,6 @@ 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);
 	}
 	return 0;
diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c
index c292a69845bb..d678a2a0c4d4 100644
--- a/drivers/thermal/gov_bang_bang.c
+++ b/drivers/thermal/gov_bang_bang.c
@@ -61,9 +61,6 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 		dev_dbg(&instance->cdev->device, "target=%d\n",
 					(int)instance->target);
 
-		mutex_lock(&instance->cdev->lock);
-		instance->cdev->updated = false; /* cdev needs update */
-		mutex_unlock(&instance->cdev->lock);
 		thermal_cdev_update(instance->cdev);
 	}
 
diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
index 44636475b2a3..f8e4219cf5de 100644
--- a/drivers/thermal/power_allocator.c
+++ b/drivers/thermal/power_allocator.c
@@ -530,9 +530,6 @@ static void allow_maximum_power(struct thermal_zone_device *tz)
 			continue;
 
 		instance->target = 0;
-		mutex_lock(&instance->cdev->lock);
-		instance->cdev->updated = false;
-		mutex_unlock(&instance->cdev->lock);
 		thermal_cdev_update(instance->cdev);
 	}
 	mutex_unlock(&tz->lock);
diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
index 298eedac0293..9ddff715f3dd 100644
--- a/drivers/thermal/step_wise.c
+++ b/drivers/thermal/step_wise.c
@@ -164,9 +164,6 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip)
 			update_passive_instance(tz, trip_type, -1);
 
 		instance->initialized = true;
-		mutex_lock(&instance->cdev->lock);
-		instance->cdev->updated = false; /* cdev needs update */
-		mutex_unlock(&instance->cdev->lock);
 		thermal_cdev_update(instance->cdev);
 	}
 
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index c06550930979..da63899b9e6c 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -593,9 +593,6 @@ int power_actor_set_power(struct thermal_cooling_device *cdev,
 		return ret;
 
 	instance->target = state;
-	mutex_lock(&cdev->lock);
-	cdev->updated = false;
-	mutex_unlock(&cdev->lock);
 	thermal_cdev_update(cdev);
 
 	return 0;
@@ -969,7 +966,6 @@ __thermal_cooling_device_register(struct device_node *np,
 	INIT_LIST_HEAD(&cdev->thermal_instances);
 	cdev->np = np;
 	cdev->ops = ops;
-	cdev->updated = false;
 	cdev->device.class = &thermal_class;
 	cdev->devdata = devdata;
 	thermal_cooling_device_setup_sysfs(cdev);
diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
index 59eaf2d0fdb3..85cae31301aa 100644
--- a/drivers/thermal/thermal_helpers.c
+++ b/drivers/thermal/thermal_helpers.c
@@ -180,11 +180,6 @@ void thermal_cdev_update(struct thermal_cooling_device *cdev)
 	unsigned long target = 0;
 
 	mutex_lock(&cdev->lock);
-	/* cooling device is updated*/
-	if (cdev->updated) {
-		mutex_unlock(&cdev->lock);
-		return;
-	}
 
 	/* Make sure cdev enters the deepest cooling state */
 	list_for_each_entry(instance, &cdev->thermal_instances, cdev_node) {
@@ -199,7 +194,6 @@ void thermal_cdev_update(struct thermal_cooling_device *cdev)
 	if (!cdev->ops->set_cur_state(cdev, target))
 		thermal_cooling_device_stats_update(cdev, target);
 
-	cdev->updated = true;
 	mutex_unlock(&cdev->lock);
 	trace_cdev_update(cdev, target);
 	dev_dbg(&cdev->device, "set to state %lu\n", target);
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 216185bb3014..08969f0be6a0 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -114,7 +114,6 @@ struct thermal_cooling_device {
 	void *devdata;
 	void *stats;
 	const struct thermal_cooling_device_ops *ops;
-	bool updated; /* true if the cooling device does not need update */
 	struct mutex lock; /* protect thermal_instances list */
 	struct list_head thermal_instances;
 	struct list_head node;
-- 
2.17.1

Re: [PATCH 1/2] thermal: core: Move thermal_cdev_update next to updated=false

[Zhang Rui]

Hi, Daniel,

On Thu, 2020-04-09 at 17:15 +0200, Daniel Lezcano wrote:
> The call to the thermal_cdev_update() function is done after browsing
> the thermal instances which sets the updated flag by browsing them
> again.
> 
> Instead of doing this, let's move the call right after setting the
> cooling device 'updated' flag as it is done in the other governors.

The reason we do this in two steps is that we want to avoid redundant
cooling device state changes.

Further more, I think it is better to move the thermal_cdev_update out
of .throllte() callback, to thermal_zone_device_update(). So that we do
not need to update the cooling device for each trip point.

is there any specific reason we need to do thermal_cdev_update() for
every potential change?

thanks,
rui
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/gov_bang_bang.c | 10 +---------
>  drivers/thermal/step_wise.c     | 10 +---------
>  2 files changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/thermal/gov_bang_bang.c
> b/drivers/thermal/gov_bang_bang.c
> index 991a1c54296d..c292a69845bb 100644
> --- a/drivers/thermal/gov_bang_bang.c
> +++ b/drivers/thermal/gov_bang_bang.c
> @@ -64,6 +64,7 @@ static void thermal_zone_trip_update(struct
> thermal_zone_device *tz, int trip)
>  		mutex_lock(&instance->cdev->lock);
>  		instance->cdev->updated = false; /* cdev needs update
> */
>  		mutex_unlock(&instance->cdev->lock);
> +		thermal_cdev_update(instance->cdev);
>  	}
>  
>  	mutex_unlock(&tz->lock);
> @@ -98,17 +99,8 @@ static void thermal_zone_trip_update(struct
> thermal_zone_device *tz, int trip)
>   */
>  static int bang_bang_control(struct thermal_zone_device *tz, int
> trip)
>  {
> -	struct thermal_instance *instance;
> -
>  	thermal_zone_trip_update(tz, trip);
>  
> -	mutex_lock(&tz->lock);
> -
> -	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> -		thermal_cdev_update(instance->cdev);
> -
> -	mutex_unlock(&tz->lock);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/thermal/step_wise.c
> b/drivers/thermal/step_wise.c
> index 2ae7198d3067..298eedac0293 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -167,6 +167,7 @@ static void thermal_zone_trip_update(struct
> thermal_zone_device *tz, int trip)
>  		mutex_lock(&instance->cdev->lock);
>  		instance->cdev->updated = false; /* cdev needs update
> */
>  		mutex_unlock(&instance->cdev->lock);
> +		thermal_cdev_update(instance->cdev);
>  	}
>  
>  	mutex_unlock(&tz->lock);
> @@ -185,20 +186,11 @@ static void thermal_zone_trip_update(struct
> thermal_zone_device *tz, int trip)
>   */
>  static int step_wise_throttle(struct thermal_zone_device *tz, int
> trip)
>  {
> -	struct thermal_instance *instance;
> -
>  	thermal_zone_trip_update(tz, trip);
>  
>  	if (tz->forced_passive)
>  		thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE);
>  
> -	mutex_lock(&tz->lock);
> -
> -	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> -		thermal_cdev_update(instance->cdev);
> -
> -	mutex_unlock(&tz->lock);
> -
>  	return 0;
>  }
>  

Re: [PATCH 1/2] thermal: core: Move thermal_cdev_update next to updated=false

[Daniel Lezcano]

On 10/04/2020 12:14, Zhang Rui wrote:
> Hi, Daniel,
> 
> On Thu, 2020-04-09 at 17:15 +0200, Daniel Lezcano wrote:
>> The call to the thermal_cdev_update() function is done after browsing
>> the thermal instances which sets the updated flag by browsing them
>> again.
>>
>> Instead of doing this, let's move the call right after setting the
>> cooling device 'updated' flag as it is done in the other governors.
> 
> The reason we do this in two steps is that we want to avoid redundant
> cooling device state changes.
> 
> Further more, I think it is better to move the thermal_cdev_update out
> of .throllte() callback, to thermal_zone_device_update(). So that we do
> not need to update the cooling device for each trip point.
> 
> is there any specific reason we need to do thermal_cdev_update() for
> every potential change?

I agree we can go further and move the cooling device update in the
thermal_zone_device_update() by letting the throttle callback let us
know an update is needed with the return value.

Makes sense to provide more changes on top of those two patches ?

>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/thermal/gov_bang_bang.c | 10 +---------
>>  drivers/thermal/step_wise.c     | 10 +---------
>>  2 files changed, 2 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/thermal/gov_bang_bang.c
>> b/drivers/thermal/gov_bang_bang.c
>> index 991a1c54296d..c292a69845bb 100644
>> --- a/drivers/thermal/gov_bang_bang.c
>> +++ b/drivers/thermal/gov_bang_bang.c
>> @@ -64,6 +64,7 @@ static void thermal_zone_trip_update(struct
>> thermal_zone_device *tz, int trip)
>>  		mutex_lock(&instance->cdev->lock);
>>  		instance->cdev->updated = false; /* cdev needs update
>> */
>>  		mutex_unlock(&instance->cdev->lock);
>> +		thermal_cdev_update(instance->cdev);
>>  	}
>>  
>>  	mutex_unlock(&tz->lock);
>> @@ -98,17 +99,8 @@ static void thermal_zone_trip_update(struct
>> thermal_zone_device *tz, int trip)
>>   */
>>  static int bang_bang_control(struct thermal_zone_device *tz, int
>> trip)
>>  {
>> -	struct thermal_instance *instance;
>> -
>>  	thermal_zone_trip_update(tz, trip);
>>  
>> -	mutex_lock(&tz->lock);
>> -
>> -	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
>> -		thermal_cdev_update(instance->cdev);
>> -
>> -	mutex_unlock(&tz->lock);
>> -
>>  	return 0;
>>  }
>>  
>> diff --git a/drivers/thermal/step_wise.c
>> b/drivers/thermal/step_wise.c
>> index 2ae7198d3067..298eedac0293 100644
>> --- a/drivers/thermal/step_wise.c
>> +++ b/drivers/thermal/step_wise.c
>> @@ -167,6 +167,7 @@ static void thermal_zone_trip_update(struct
>> thermal_zone_device *tz, int trip)
>>  		mutex_lock(&instance->cdev->lock);
>>  		instance->cdev->updated = false; /* cdev needs update
>> */
>>  		mutex_unlock(&instance->cdev->lock);
>> +		thermal_cdev_update(instance->cdev);
>>  	}
>>  
>>  	mutex_unlock(&tz->lock);
>> @@ -185,20 +186,11 @@ static void thermal_zone_trip_update(struct
>> thermal_zone_device *tz, int trip)
>>   */
>>  static int step_wise_throttle(struct thermal_zone_device *tz, int
>> trip)
>>  {
>> -	struct thermal_instance *instance;
>> -
>>  	thermal_zone_trip_update(tz, trip);
>>  
>>  	if (tz->forced_passive)
>>  		thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE);
>>  
>> -	mutex_lock(&tz->lock);
>> -
>> -	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
>> -		thermal_cdev_update(instance->cdev);
>> -
>> -	mutex_unlock(&tz->lock);
>> -
>>  	return 0;
>>  }
>>  
> 


-- 
<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 1/2] thermal: core: Move thermal_cdev_update next to updated=false

[Zhang Rui]

On Fri, 2020-04-10 at 13:26 +0200, Daniel Lezcano wrote:
> On 10/04/2020 12:14, Zhang Rui wrote:
> > Hi, Daniel,
> > 
> > On Thu, 2020-04-09 at 17:15 +0200, Daniel Lezcano wrote:
> > > The call to the thermal_cdev_update() function is done after
> > > browsing
> > > the thermal instances which sets the updated flag by browsing
> > > them
> > > again.
> > > 
> > > Instead of doing this, let's move the call right after setting
> > > the
> > > cooling device 'updated' flag as it is done in the other
> > > governors.
> > 
> > The reason we do this in two steps is that we want to avoid
> > redundant
> > cooling device state changes.
> > 
> > Further more, I think it is better to move the thermal_cdev_update
> > out
> > of .throllte() callback, to thermal_zone_device_update(). So that
> > we do
> > not need to update the cooling device for each trip point.
> > 
> > is there any specific reason we need to do thermal_cdev_update()
> > for
> > every potential change?
> 
> I agree we can go further and move the cooling device update in the
> thermal_zone_device_update() by letting the throttle callback let us
> know an update is needed with the return value.
> 
> Makes sense to provide more changes on top of those two patches ?

Hmmm, without the update flag, we can only updating all the cooling
devices blindly. And this is time consuming for some cooling devices.

thanks,
rui

> 
> > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > ---
> > >  drivers/thermal/gov_bang_bang.c | 10 +---------
> > >  drivers/thermal/step_wise.c     | 10 +---------
> > >  2 files changed, 2 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/thermal/gov_bang_bang.c
> > > b/drivers/thermal/gov_bang_bang.c
> > > index 991a1c54296d..c292a69845bb 100644
> > > --- a/drivers/thermal/gov_bang_bang.c
> > > +++ b/drivers/thermal/gov_bang_bang.c
> > > @@ -64,6 +64,7 @@ static void thermal_zone_trip_update(struct
> > > thermal_zone_device *tz, int trip)
> > >  		mutex_lock(&instance->cdev->lock);
> > >  		instance->cdev->updated = false; /* cdev needs update
> > > */
> > >  		mutex_unlock(&instance->cdev->lock);
> > > +		thermal_cdev_update(instance->cdev);
> > >  	}
> > >  
> > >  	mutex_unlock(&tz->lock);
> > > @@ -98,17 +99,8 @@ static void thermal_zone_trip_update(struct
> > > thermal_zone_device *tz, int trip)
> > >   */
> > >  static int bang_bang_control(struct thermal_zone_device *tz, int
> > > trip)
> > >  {
> > > -	struct thermal_instance *instance;
> > > -
> > >  	thermal_zone_trip_update(tz, trip);
> > >  
> > > -	mutex_lock(&tz->lock);
> > > -
> > > -	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> > > -		thermal_cdev_update(instance->cdev);
> > > -
> > > -	mutex_unlock(&tz->lock);
> > > -
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/drivers/thermal/step_wise.c
> > > b/drivers/thermal/step_wise.c
> > > index 2ae7198d3067..298eedac0293 100644
> > > --- a/drivers/thermal/step_wise.c
> > > +++ b/drivers/thermal/step_wise.c
> > > @@ -167,6 +167,7 @@ static void thermal_zone_trip_update(struct
> > > thermal_zone_device *tz, int trip)
> > >  		mutex_lock(&instance->cdev->lock);
> > >  		instance->cdev->updated = false; /* cdev needs update
> > > */
> > >  		mutex_unlock(&instance->cdev->lock);
> > > +		thermal_cdev_update(instance->cdev);
> > >  	}
> > >  
> > >  	mutex_unlock(&tz->lock);
> > > @@ -185,20 +186,11 @@ static void thermal_zone_trip_update(struct
> > > thermal_zone_device *tz, int trip)
> > >   */
> > >  static int step_wise_throttle(struct thermal_zone_device *tz,
> > > int
> > > trip)
> > >  {
> > > -	struct thermal_instance *instance;
> > > -
> > >  	thermal_zone_trip_update(tz, trip);
> > >  
> > >  	if (tz->forced_passive)
> > >  		thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE);
> > >  
> > > -	mutex_lock(&tz->lock);
> > > -
> > > -	list_for_each_entry(instance, &tz->thermal_instances, tz_node)
> > > -		thermal_cdev_update(instance->cdev);
> > > -
> > > -	mutex_unlock(&tz->lock);
> > > -
> > >  	return 0;
> > >  }
> > >  
> 
>