Re: [PATCH v3 1/5] powercap/drivers/dtpm: Encapsulate even more the code

[Daniel Lezcano]

On 11/03/2021 11:15, Lukasz Luba wrote:
> Hi Daniel,
> 
> On 3/10/21 11:02 AM, Daniel Lezcano wrote:
>> In order to increase the self-encapsulation of the dtpm generic code,
>> the following changes are adding a power update ops to the dtpm
>> ops. That allows the generic code to call directly the dtpm backend
>> function to update the power values.
>>
>> The power update function does compute the power characteristics when
>> the function is invoked. In the case of the CPUs, the power
>> consumption depends on the number of online CPUs. The online CPUs mask
>> is not up to date at CPUHP_AP_ONLINE_DYN state in the tear down
>> callback. That is the reason why the online / offline are at separate
>> state. As there is already an existing state for DTPM, this one is
>> only moved to the DEAD state, so there is no addition of new state
>> with these changes. The dtpm node is not removed when the cpu is
>> unplugged.
>>
>> That simplifies the code for the next changes and results in a more
>> self-encapsulated code.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
>> ---
>> V2:
>>   - Updated the changelog with the CPU node not being removed
>>   - Commented the cpu hotplug callbacks to explain why there are two
>> callbacks
>>   - Changed 'upt_power_uw' to 'update_power_uw'
>>   - Removed unused cpumask variable
>> ---
>>   drivers/powercap/dtpm.c     |  54 ++++++-------
>>   drivers/powercap/dtpm_cpu.c | 148 ++++++++++++++++--------------------
>>   include/linux/cpuhotplug.h  |   2 +-
>>   include/linux/dtpm.h        |   3 +-
>>   4 files changed, 97 insertions(+), 110 deletions(-)
>>
> 
> [snip]
> 
>> @@ -210,27 +175,20 @@ static int cpuhp_dtpm_cpu_online(unsigned int cpu)
>>       for_each_cpu(cpu, policy->related_cpus)
>>           per_cpu(dtpm_per_cpu, cpu) = dtpm;
>>   -    sprintf(name, "cpu%d", dtpm_cpu->cpu);
>> +    sprintf(name, "cpu%d-cpufreq", dtpm_cpu->cpu);
> 
> We should be safe in normal platforms, since there is less than
> < 300 cores. although, I would use 2x CPUFREQ_NAME_LEN array.

I'm counting 9999 cores.

We have:
#define CPUFREQ_NAME_LEN 16

"cpu-cpufreq\0" counts 12 characters.

There are 4 characters left, 9999 max then, no ?

The code is designed for cpufreq with the energy model which targets
ARM64 architecture and AFAICT we are far away of having so many cores on
our phones.

Except I'm wrong, I would prefer to keep the current CPUFREQ_NAME_LEN to
not introduce subtle bugs (like stack overflow) if the length is
increased in cpufreq.h.

What do you think ?

> Reviewed-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> 
> Regards,
> Lukasz
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog