Hey Lukasz,
Still reading through this, but with small changes, this looks pretty good to me.
On Thursday 16 Jan 2020 at 15:20:29 (+0000), lukasz.luba@arm.com wrote:
+int em_register_perf_domain(struct device *dev, unsigned int nr_states,
struct em_data_callback *cb){ unsigned long cap, prev_cap = 0; struct em_perf_domain *pd;
- int cpu, ret = 0;
- struct em_device *em_dev;
- cpumask_t *span = NULL;
- int cpu, ret;
- if (!span || !nr_states || !cb)
- if (!dev || !nr_states || !cb || !cb->active_power)
Nit: you check !cb->active_power in em_create_pd() too I think, so only one of the two is needed.
return -EINVAL;
/*
* Use a mutex to serialize the registration of performance domains and* let the driver-defined callback functions sleep.*/mutex_lock(&em_pd_mutex);
for_each_cpu(cpu, span) {
/* Make sure we don't register again an existing domain. */if (READ_ONCE(per_cpu(em_data, cpu))) {
- if (_is_cpu_device(dev)) {
span = kzalloc(cpumask_size(), GFP_KERNEL);if (!span) {mutex_unlock(&em_pd_mutex);return -ENOMEM;}ret = dev_pm_opp_get_sharing_cpus(dev, span);if (ret)goto free_cpumask;
That I think should be changed. This creates some dependency on PM_OPP for the EM framework. And in fact, the reason we came up with PM_EM was precisely to not depend on PM_OPP which was deemed too Arm-specific.
Suggested alternative: have two registration functions like so:
int em_register_dev_pd(struct device *dev, unsigned int nr_states, struct em_data_callback *cb); int em_register_cpu_pd(cpumask_t *span, unsigned int nr_states, struct em_data_callback *cb);
where em_register_cpu_pd() does the CPU-specific work and then calls em_register_dev_pd() (instead of having that big if (_is_cpu_device(dev)) as you currently have). Would that work ?
Another possibility would be to query CPUFreq instead of PM_OPP to get the mask, but I'd need to look again at the driver registration path in CPUFreq to see if the policy masks have been populated when we enter PM_EM ... I am not sure if this is the case, but it's worth having a look too.
Thanks, Quentin