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

[Lukasz Luba]

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

Regards,
Lukasz

[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