Hi Daniel,
On 11/23/20 9:42 PM, Daniel Lezcano wrote:
With the powercap dtpm controller, we are able to plug devices with
power limitation features in the tree.
[snip]
+
+static void pd_release(struct dtpm *dtpm)
+{
+ struct dtpm_cpu *dtpm_cpu = dtpm->private;
+
Maybe it's worth to add:
------------------->8----------------
if (freq_qos_request_active(&dtpm_cpu->qos_req))
freq_qos_remove_request(&dtpm_cpu->qos_req);
-------------------8<---------------
If we are trying to unregister dtpm in error path due to freq_qos
registration failure, a warning would be emitted from freq_qos.
+ freq_qos_remove_request(&dtpm_cpu->qos_req);
+ kfree(dtpm_cpu);
+}
[snip]
+
+static int cpuhp_dtpm_cpu_online(unsigned int cpu)
+{
+ struct dtpm *dtpm;
+ struct dtpm_cpu *dtpm_cpu;
+ struct cpufreq_policy *policy;
+ struct em_perf_domain *pd;
+ char name[CPUFREQ_NAME_LEN];
+ int ret;
+
+ policy = cpufreq_cpu_get(cpu);
+
+ if (!policy)
+ return 0;
+
+ pd = em_cpu_get(cpu);
+ if (!pd)
+ return -EINVAL;
+
+ dtpm = per_cpu(dtpm_per_cpu, cpu);
+ if (dtpm)
+ return power_add(dtpm, pd);
+
+ dtpm = dtpm_alloc(&dtpm_ops);
+ if (!dtpm)
+ return -EINVAL;
+
+ dtpm_cpu = kzalloc(sizeof(dtpm_cpu), GFP_KERNEL);
+ if (!dtpm_cpu) {
+ kfree(dtpm);
+ return -ENOMEM;
+ }
+
+ dtpm->private = dtpm_cpu;
+ dtpm_cpu->cpu = cpu;
+
+ for_each_cpu(cpu, policy->related_cpus)
+ per_cpu(dtpm_per_cpu, cpu) = dtpm;
+
+ sprintf(name, "cpu%d", dtpm_cpu->cpu);
+
+ ret = dtpm_register(name, dtpm, __parent);
+ if (ret)
+ goto out_kfree_dtpm_cpu;
+
+ ret = power_add(dtpm, pd);
+ if (ret)
+ goto out_power_sub;
Shouldn't we call dtpm_unregister() instead?
The dtpm_unregister() would remove the zone, which IIUC we
are currently missing.
+
+ ret = freq_qos_add_request(&policy->constraints,
+ &dtpm_cpu->qos_req, FREQ_QOS_MAX,
+ pd->table[pd->nr_perf_states - 1].frequency);
+ if (ret)
+ goto out_dtpm_unregister;
Could this trigger different steps, starting from out_power_sub_v2
below?
+
+ return 0;
+
+out_dtpm_unregister:
+ dtpm_unregister(dtpm);
+ dtpm_cpu = NULL; /* Already freed by the release ops */
+out_power_sub:
+ power_sub(dtpm, pd);
I would change the order of these two above into something like:
out_power_sub_v2:
power_sub(dtpm, pd);
out_dtpm_unregister_v2:
dtpm_unregister(dtpm);
dtpm_cpu = NULL;
+out_kfree_dtpm_cpu:
+ for_each_cpu(cpu, policy->related_cpus)
+ per_cpu(dtpm_per_cpu, cpu) = NULL;
+ kfree(dtpm_cpu);
+
+ return ret;
+}
IIUC power_sub() would decrement the power and set it to 0 for that
dtmp, then the dtpm_unregister() would also try to decrement the power,
but by the value of 0. So it should be safe.
Regards,
Lukasz
