linux-kernel - Re: [PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed

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: <78b14ec9-e107-95dc-03b6-194b00834d6a@arm.com>
Date:   Mon, 24 Apr 2017 17:53:10 +0100
From:   Lukasz Luba <lukasz.luba@....com>
To:     Viresh Kumar <viresh.kumar@...aro.org>,
        Javi Merino <javi.merino@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>,
        Eduardo Valentin <edubezval@...il.com>,
        Amit Daniel Kachhap <amit.kachhap@...il.com>
Cc:     linaro-kernel@...ts.linaro.org, Rafael Wysocki <rjw@...ysocki.net>,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Punit Agrawal <punit.agrawal@....com>
Subject: Re: [PATCH V3 11/17] thermal: cpu_cooling: get rid of 'allowed_cpus'

Hi Viresh,

I have been testing the patch set and found one of the issues.
Please see the comment below.

On 19/04/17 06:29, Viresh Kumar wrote:
> 'allowed_cpus' is a copy of policy->related_cpus and can be replaced by
> it directly. At some places we are only concerned about online CPUs and
> policy->cpus can be used there.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
>  drivers/thermal/cpu_cooling.c | 77 ++++++++++++-------------------------------
>  1 file changed, 21 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index ce387f62c93e..1097162f7f8a 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -74,7 +74,6 @@ struct power_table {
>   *	frequency.
>   * @max_level: maximum cooling level. One less than total number of valid
>   *	cpufreq frequencies.
> - * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
>   * @node: list_head to link all cpufreq_cooling_device together.
>   * @last_load: load measured by the latest call to cpufreq_get_requested_power()
>   * @time_in_idle: previous reading of the absolute time that this cpu was idle
> @@ -97,7 +96,6 @@ struct cpufreq_cooling_device {
>  	unsigned int clipped_freq;
>  	unsigned int max_level;
>  	unsigned int *freq_table;	/* In descending order */
> -	struct cpumask allowed_cpus;
>  	struct list_head node;
>  	u32 last_load;
>  	u64 *time_in_idle;
> @@ -161,7 +159,7 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
>
>  	mutex_lock(&cooling_list_lock);
>  	list_for_each_entry(cpufreq_cdev, &cpufreq_cdev_list, node) {
> -		if (!cpumask_test_cpu(policy->cpu, &cpufreq_cdev->allowed_cpus))
> +		if (policy != cpufreq_cdev->policy)
The policy pointer forwarded from cpufreq_update_policy()
is a local variable 'new_policy' so cannot be compared with pinned
policy pointer in the cooling device.
You should do the cpumask test like before:
	if (!cpumask_test_cpu(policy->cpu,
			      cpufreq_cdev->policy->related_cpus))


But there is something still in the patch set...
I will try to check it tomorrow.

Best regards,
Lukasz