Message-ID: <db539c1e-22d5-2261-1248-07883dac12ee@arm.com>
Date:   Fri, 10 Mar 2023 15:13:09 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH] thermal/core/power_allocator: avoid cdev->state can not
 be reset
Content-Language: en-US
To:     Di Shen <di.shen@unisoc.com>
Cc:     daniel.lezcano@linaro.org, rafael@kernel.org, amitk@kernel.org,
        rui.zhang@intel.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, xuewen.yan@unisoc.com
References: <20230309135515.1232-1-di.shen@unisoc.com>
From:   Lukasz Luba <lukasz.luba@arm.com>
In-Reply-To: <20230309135515.1232-1-di.shen@unisoc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Di,

On 3/9/23 13:55, Di Shen wrote:
> Commit 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
> add a update flag to update cooling device only once when temp is low.
> But when the switch_on_temp is set to be a higher value, the cooling device state
> may not be reset to max, because the last_temp is smaller than the switch_on_temp.
> 
> For example:
> First:
> swicth_on_temp=70 control_temp=85;
> 
> Then userspace change the trip_temp:
> swicth_on_temp=45 control_temp=55 cur_temp=54
> 
> Then userspace reset the trip_temp:
> swicth_on_temp=70 control_temp=85 cur_temp=57 last_temp=54
> 
> At this time, the cooling device state should be reset to be max.
> However, because cur_temp(57) < switch_on_temp(70)
> last_temp(54) < swicth_on_temp(70) --> update = false
> When update is false, the cooling device state can not be reset.

That's a tricky use case. How is that now possible,
> 
> So delete the update condition, so that the cooling device state
> could be reset.

IMO this is not the desired solution. Daniel reported the issue that
IPA triggers the event sent to user-space even when there is no need.
That's the motivation for the 0952177f2a1f change.

To address your scenario properly, we need an interface which allows
to respond properly for such situation when someone from user-space
writes a new value to something fundamental as trip point.

You also have a kernel config enabled:
CONFIG_THERMAL_WRITABLE_TRIPS
which IMO is only for debug kernels for system integrator (according
to the Kconfig description).

When you disable this config in your deploy/product kernel
than this issue would disappear.

> 
> Fixes: 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
> Signed-off-by: Di Shen <di.shen@unisoc.com>
> ---
>   drivers/thermal/gov_power_allocator.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 

That's why IMO this is not the solution.

Regards,
Lukasz