Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: lEAAHwxNYLaVaWCPPSMq63WM/1ghUIAhBnoOJFhfyHCPMklvbFQ+311Ioh9yqqWLcHEIbOaQTa
 +X/iECOtrvgg==
IronPort-SDR: znwmLGqGF7fBrxbjOQHuDL9fIjHD9nBDwbGOgyLlWixbox6ihlAJtbZRPT8E9bvFR1EixwvFk2
 MGfV9tIeJbrA==
Message-ID: <8e4c2825d71e5bf5602b92937a49c04187c68e17.camel@intel.com>
Subject: Re: [PATCH 1/2] thermal: core: Move thermal_cdev_update next to
 updated=false
From:   Zhang Rui <rui.zhang@intel.com>
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     linux-kernel@vger.kernel.org, amit.kucheria@verdurent.com,
        "open list:THERMAL" <linux-pm@vger.kernel.org>
Date:   Fri, 10 Apr 2020 18:14:08 +0800
In-Reply-To: <20200409151515.6607-1-daniel.lezcano@linaro.org>
References: <20200409151515.6607-1-daniel.lezcano@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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;
>  }
>