Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <32ee5e63-5b0c-3869-c196-265444d008e9@arm.com>
Date:   Tue, 11 Oct 2022 08:51:43 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] thermal/governors: Fix cooling device setting cooling
 state failure
Content-Language: en-US
To:     Qibo Huang <huangqibo.tech@gmail.com>
Cc:     daniel.lezcano@linaro.org, amitk@kernel.org,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        rui.zhang@intel.com, rafael@kernel.org
References: <20221010144341.16738-1-huangqibo.tech@gmail.com>
From:   Lukasz Luba <lukasz.luba@arm.com>
In-Reply-To: <20221010144341.16738-1-huangqibo.tech@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Qibo,

Thank you for using IPA and sending a patch. Unfortunately,
this code cannot be added into the governor.

On 10/10/22 15:43, Qibo Huang wrote:
> Because the __thermal_cdev_update function traverses the
> cooling_device->thermal_instances list to obtain the maximum
> target state, and then the cooling device sets the maximum
> cooling state. However, the power_actor_set_power function
> only updates the target value of thermal_zone->thermal_instances
> to the target state, and does not update the target value of
> cooling_device->thermal_instances, resulting in the target
> being 0 all the time.
> 
> Signed-off-by: Qibo Huang <huangqibo.tech@gmail.com>
> ---
>   drivers/thermal/gov_power_allocator.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
> index 2d1aeaba38a8..8a6a08906dd4 100644
> --- a/drivers/thermal/gov_power_allocator.c
> +++ b/drivers/thermal/gov_power_allocator.c
> @@ -293,6 +293,7 @@ power_actor_set_power(struct thermal_cooling_device *cdev,
>   		      struct thermal_instance *instance, u32 power)
>   {
>   	unsigned long state;
> +	struct thermal_instance *cdev_instance;
>   	int ret;
>   
>   	ret = cdev->ops->power2state(cdev, power, &state);
> @@ -300,6 +301,10 @@ power_actor_set_power(struct thermal_cooling_device *cdev,
>   		return ret;
>   
>   	instance->target = clamp_val(state, instance->lower, instance->upper);

For our 'instance', which IPA was given be the framework, we set the new
'target' value above.

> +	list_for_each_entry(cdev_instance, &cdev->thermal_instances, cdev_node) {
> +		if (cdev_instance->tz->id == instance->tz->id)
> +			cdev_instance->target = state;

This is not the approach for the governor of a single thermal zone.
Please check the implementation of the function below
__thermal_cdev_update()
You will see there the 'voting' code. This belongs to the framework
code.

> +	}
>   	mutex_lock(&cdev->lock);
>   	__thermal_cdev_update(cdev);

This is the place where the right 'targe't is picked for cooling device
from different thermal zones (and maybe governors). When a new target
value is higher than other target values, the framework would throttle
the device. Then other thermal framework are kicked to make this change
propagated correctly.

Also important, please see that this work is done under the
cdev->lock. Not every code is safe to run, because e.g. this
list cdev->thermal_instances might change in the meantime.

>   	mutex_unlock(&cdev->lock);

Regards,
Lukasz