linux-kernel - Re: [PATCH 1/2] thermal: core: Move thermal_cdev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8e4c2825d71e5bf5602b92937a49c04187c68e17.camel@intel.com>
Date:   Fri, 10 Apr 2020 18:14:08 +0800
From:   Zhang Rui <rui.zhang@...el.com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     linux-kernel@...r.kernel.org, amit.kucheria@...durent.com,
        "open list:THERMAL" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH 1/2] thermal: core: Move thermal_cdev_update next to
 updated=false

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@...aro.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;
>  }
>