Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model

[Daniel Lezcano]

Hi Lukasz,


On 06/04/2020 15:29, Lukasz Luba wrote:
> Hi Daniel,
> 
> Thank you for the review.
> 
> On 4/3/20 5:05 PM, Daniel Lezcano wrote:
>>
>> Hi Lukasz,
>>
>>
>> On 18/03/2020 12:45, Lukasz Luba wrote:
>>> Add support of other devices into the Energy Model framework not only
>>> the
>>> CPUs. Change the interface to be more unified which can handle other
>>> devices as well.
>>
>> thanks for taking care of that. Overall I like the changes in this patch
>> but it hard to review in details because the patch is too big :/
>>
>> Could you split this patch into smaller ones?
>>
>> eg. (at your convenience)
>>
>> Â - One patch renaming s/cap/perf/
>>
>> Â - One patch adding a new function:
>>
>> ÂÂÂÂ em_dev_register_perf_domain(struct device *dev,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int nr_states,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct em_data_callback *cb);
>>
>> ÂÂÂ (+ EXPORT_SYMBOL_GPL)
>>
>> ÂÂÂÂ And em_register_perf_domain() using it.
>>
>> Â - One converting the em_register_perf_domain() user to
>> ÂÂÂÂem_dev_register_perf_domain
>>
>> Â - One adding the different new 'em' functions
>>
>> Â - And finally one removing em_register_perf_domain().
> 
> I agree and will do the split. I could also break the dependencies
> for future easier merge.
> 
>>
>>
>>> Acked-by: Quentin Perret <qperret@xxxxxxxxxx>
>>> Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
>>> ---
>>
>> [ ... ]
>>
>>> Â 2. Core APIs
>>> @@ -70,14 +72,16 @@ CONFIG_ENERGY_MODEL must be enabled to use the EM
>>> framework.
>>> Â Drivers are expected to register performance domains into the EM
>>> framework by
>>> Â calling the following API::
>>> Â -Â int em_register_perf_domain(cpumask_t *span, unsigned int
>>> nr_states,
>>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct em_data_callback *cb);
>>> +Â int em_register_perf_domain(struct device *dev, unsigned int
>>> nr_states,
>>> +ÂÂÂÂÂÂÂ struct em_data_callback *cb, cpumask_t *cpus);
>>
>> Isn't possible to get rid of this cpumask by using
>> cpufreq_cpu_get() which returns the cpufreq's policy and from their get
>> the related cpus ?
> 
> We had similar thoughts with Quentin and I've checked this.

Yeah, I suspected you already think about that :)

> Unfortunately, if the policy is a 'new policy' [1] it gets
> allocated and passed into cpufreq driver ->init(policy) [2].
> Then that policy is set into per_cpu pointer for each related_cpu [3]:
> 
> for_each_cpu(j, policy->related_cpus)
> ÂÂÂÂper_cpu(cpufreq_cpu_data, j) = policy;
> 
>  
> Thus, any calls of functions (i.e. cpufreq_cpu_get()) which try to
> take this ptr before [3] won't work.
> 
> We are trying to register EM from cpufreq_driver->init(policy) and the
> per_cpu policy is likely to be not populated at that phase.

What is the problem of registering at the end of the cpufreq_online ?


> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1328
> 
> [2]
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1350
> 
> [3]
> https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1374
> 
> 
> 


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