Hi Lukasz,
On Monday 09 Mar 2020 at 13:41:14 (+0000), Lukasz Luba wrote: <snip>
diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 9cd8f0adacae..0efd6cf6d023 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -1047,9 +1047,8 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_get_of_node);
- calculation failed because of missing parameters, 0 otherwise.
*/ static int __maybe_unused _get_cpu_power(unsigned long *mW, unsigned long *kHz,
int cpu)
struct device *cpu_dev){
- struct device *cpu_dev; struct dev_pm_opp *opp; struct device_node *np; unsigned long mV, Hz;
@@ -1057,10 +1056,6 @@ static int __maybe_unused _get_cpu_power(unsigned long *mW, unsigned long *kHz, u64 tmp; int ret;
- cpu_dev = get_cpu_device(cpu);
- if (!cpu_dev)
return -ENODEV;- np = of_node_get(cpu_dev->of_node); if (!np) return -EINVAL;
@@ -1128,6 +1123,6 @@ void dev_pm_opp_of_register_em(struct cpumask *cpus) if (ret || !cap) return;
- em_register_perf_domain(cpus, nr_opp, &em_cb);
- em_register_perf_domain(cpu_dev, nr_opp, &em_cb, cpus);
Any reason for not checking the return value here ? You added a nice check in scmi_get_cpu_power(), perhaps do the same thing here ?
} EXPORT_SYMBOL_GPL(dev_pm_opp_of_register_em); diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c index fe83d7a210d4..fcf2dab1b3b8 100644 --- a/drivers/thermal/cpufreq_cooling.c +++ b/drivers/thermal/cpufreq_cooling.c @@ -333,18 +333,18 @@ static inline bool em_is_sane(struct cpufreq_cooling_device *cpufreq_cdev, return false;
policy = cpufreq_cdev->policy;
- if (!cpumask_equal(policy->related_cpus, to_cpumask(em->cpus))) {
- if (!cpumask_equal(policy->related_cpus, em_span_cpus(em))) { pr_err("The span of pd %*pbl is misaligned with cpufreq policy %*pbl\n",
cpumask_pr_args(to_cpumask(em->cpus)),
cpumask_pr_args(em_span_cpus(em)), cpumask_pr_args(policy->related_cpus));return false; }
nr_levels = cpufreq_cdev->max_level + 1;
- if (em->nr_cap_states != nr_levels) {
- if (em->nr_perf_states != nr_levels) { pr_err("The number of cap states in pd %*pbl (%u) doesn't match the number of cooling levels (%u)\n",
s/cap states/performance states
cpumask_pr_args(to_cpumask(em->cpus)),em->nr_cap_states, nr_levels);
cpumask_pr_args(em_span_cpus(em)),em->nr_perf_states, nr_levels);
<snip>
+static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
int nr_states, struct em_data_callback *cb){ unsigned long opp_eff, prev_opp_eff = ULONG_MAX; unsigned long power, freq, prev_freq = 0;
- int i, ret, cpu = cpumask_first(span);
- struct em_cap_state *table;
- struct em_perf_domain *pd;
- struct em_perf_state *table;
- int i, ret; u64 fmax;
- if (!cb->active_power)
return NULL;- pd = kzalloc(sizeof(*pd) + cpumask_size(), GFP_KERNEL);
- if (!pd)
return NULL;- table = kcalloc(nr_states, sizeof(*table), GFP_KERNEL); if (!table)
goto free_pd;
return -ENOMEM;
- /* Build the list of capacity states for this performance domain */
- /* Build the list of performance states for this performance domain */ for (i = 0, freq = 0; i < nr_states; i++, freq++) { /*
- active_power() is a driver callback which ceils 'freq' to
* lowest capacity state of 'cpu' above 'freq' and updates
* lowest performance state of 'dev' above 'freq' and updates*/
- 'power' and 'freq' accordingly.
ret = cb->active_power(&power, &freq, cpu);
ret = cb->active_power(&power, &freq, dev);
pr_err("pd%d: invalid cap. state: %d\n", cpu, ret);
dev_err(dev, "EM: invalid perf. state: %d\n",ret);
Not easy to figure out which device has a problem with this. I'm guessing you went that way since this is called before ida_simple_get() ? Could that be refactored to make the error message more useful ?
goto free_cs_table; }
<snip>
+/**
- em_unregister_perf_domain() - Unregister Energy Model (EM) for the device
- @dev : Device for which the EM is registered
- Try to unregister the EM for the specified device (it checks current
- reference counter). The EM for CPUs will not be freed.
- */
+void em_unregister_perf_domain(struct device *dev) +{
- struct em_device *em_dev, *tmp;
- if (IS_ERR_OR_NULL(dev))
return;- /* We don't support freeing CPU structures in hotplug */
- if (_is_cpu_device(dev))
return;
Can we WARN() here ?
- mutex_lock(&em_pd_mutex);
- if (list_empty(&em_pd_dev_list)) {
mutex_unlock(&em_pd_mutex);return;- }
- list_for_each_entry_safe(em_dev, tmp, &em_pd_dev_list, em_dev_list) {
if (em_dev->dev == dev) {kref_put(&em_dev->kref, _em_release);break;}- }
- mutex_unlock(&em_pd_mutex);
+} +EXPORT_SYMBOL_GPL(em_unregister_perf_domain);
Otherwise this looks pretty good to me. So, with these small nits addressed:
Acked-by: Quentin Perret qperret@google.com
Thanks! Quentin