Messages in this thread | | | Subject | Re: [PATCH v5 1/5] PM / EM: add devices to Energy Model | From | Lukasz Luba <> | Date | Mon, 6 Apr 2020 14:29:56 +0100 |
| |
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@google.com> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> --- > > [ ... ] > >> 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
| |