Re: [PATCH v3 4/4] powercap/drivers/dtpm: Add CPU energy model based support

[Lukasz Luba]

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