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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <50db7265-3870-b977-6e41-b0a0ac3cdb94@arm.com>
Date:   Thu, 26 Nov 2020 10:06:52 +0000
From:   Lukasz Luba <lukasz.luba@....com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     rjw@...ysocki.net, corbet@....net, ulf.hansson@...aro.org,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        ilina@...eaurora.org, rkumbako@...eaurora.org, rui.zhang@...el.com,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 4/4] powercap/drivers/dtpm: Add CPU energy model based
 support

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