Re: [RFC PATCH 1/2] thermal/cpufreq_cooling: remove unused cpu_idx in get_load()

[Lukasz Luba <lukasz.luba@arm.com>]

Hi Viresh,

On 3/23/26 05:34, Viresh Kumar wrote:
> On 20-03-26, 12:32, Lukasz Luba wrote:
>> Hi Xuewen,
>>
>> On 3/20/26 11:31, Xuewen Yan wrote:
>>> From: Di Shen <di.shen@unisoc.com>
>>>
>>> The cpu_idx variable in the get_load function is now
>>> unused and can be safely removed.
>>>
>>> No code logic is affected.
>>>
>>> Signed-off-by: Di Shen <di.shen@unisoc.com>
>>> ---
>>>    drivers/thermal/cpufreq_cooling.c | 13 +++++--------
>>>    1 file changed, 5 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
>>> index 32bf5ab44f4a..d030dbeb2973 100644
>>> --- a/drivers/thermal/cpufreq_cooling.c
>>> +++ b/drivers/thermal/cpufreq_cooling.c
>>> @@ -151,26 +151,23 @@ static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev,
>>>     * get_load() - get load for a cpu
>>>     * @cpufreq_cdev: struct cpufreq_cooling_device for the cpu
>>>     * @cpu: cpu number
>>> - * @cpu_idx: index of the cpu in time_in_idle array
>>>     *
>>>     * Return: The average load of cpu @cpu in percentage since this
>>>     * function was last called.
>>>     */
>>>    #ifdef CONFIG_SMP
>>> -static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
>>> -		    int cpu_idx)
>>> +static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu)
>>>    {
>>>    	unsigned long util = sched_cpu_util(cpu);
>>>    	return (util * 100) / arch_scale_cpu_capacity(cpu);
>>>    }
>>>    #else /* !CONFIG_SMP */
>>> -static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
>>> -		    int cpu_idx)
>>> +static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu)
>>>    {
>>>    	u32 load;
>>>    	u64 now, now_idle, delta_time, delta_idle;
>>> -	struct time_in_idle *idle_time = &cpufreq_cdev->idle_time[cpu_idx];
>>> +	struct time_in_idle *idle_time = &cpufreq_cdev->idle_time[cpu];
>>
>> This is a bug. We allocate 'num_cpus' size of array based on
>> number of CPU in the cpumask for a given cpufreq policy.
>> If there are 4 cpus in the CPU cluster but CPUs have ids:
>> CPU4-7 then accessing it with this change would explode.
> 
> I think following commit introduced a bug by removing `i++`.
> 
> commit 3f7ced7ac9af ("drivers/thermal/cpufreq_cooling : Refactor thermal_power_cpu_get_power tracing")
> 

Thanks for monitoring the development (it's always good
to have extra engineer opinion)!

I've checked the commit that you referred to and the 'i++' there.
It's safe. That commit removed the heavy operation for only
tracing purpose, namely:
- allocate buffer for N CPUs for 'load_cpu' pointer
- populate CPUs' load from the idle fwk
- put that info into the trace
- free the 'load_cpu' buffer

That has been redesigned since it was just for tracing
and introducing extra time spent for code run in the
throttling phase.

The code in get_load() is OK with the commit that you
mentioned.

Regards,
Lukasz