linux-kernel - Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

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]

Date:	Mon, 25 Nov 2013 08:38:36 -0800
From:	Dirk Brandewie <dirk.brandewie@...il.com>
To:	Viresh Kumar <viresh.kumar@...aro.org>, rjw@...ysocki.net
CC:	linaro-kernel@...ts.linaro.org, patches@...aro.org,
	cpufreq@...r.kernel.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org, nm@...com, ceh@...com
Subject: Re: [PATCH V2] cpufreq: Make sure CPU is running on a freq from freq-table

On 11/24/2013 08:23 PM, Viresh Kumar wrote:
> Sometimes boot loaders set CPU frequency to a value outside of frequency table
> present with cpufreq core. In such cases CPU might be unstable if it has to run
> on that frequency for long duration of time and so its better to set it to a
> frequency which is specified in freq-table. This also makes cpufreq stats
> inconsistent as cpufreq-stats would fail to register because current frequency
> of CPU isn't found in freq-table.
>

IMHO this issue should be fixed in the scaling driver for the platform.

The scaling driver sets policy->cur and fills in the frequency table and has the
abilty to adjust the frequency of the CPU.  Letting this leak up into the core
is wrong IMHO and just widens the window where the CPU will be running at the
wrong frequency set by the bootloader.

> Because we don't want this change to effect boot process badly, we go for the
> next freq which is >= policy->cur ('cur' must be set by now, otherwise we will
> end up setting freq to lowest of the table as 'cur' is initialized to zero).
>
> In case where CPU is already running on one of the frequencies present in
> freq-table, this would turn into a dummy call as __cpufreq_driver_target() would
> return early.
>
> Reported-by: Carlos Hernandez <ceh@...com>
> Reported-and-tested-by: Nishanth Menon <nm@...com>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
> V1->V2
> - Set to (policy->cur - 1) instead of policy->cur.
> - return early in case __cpufreq_driver_target() fails.
>
>   drivers/cpufreq/cpufreq.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 02d534d..7be996c 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1038,6 +1038,38 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
>   		}
>   	}
>
> +	/*
> +	 * Sometimes boot loaders set CPU frequency to a value outside of
> +	 * frequency table present with cpufreq core. In such cases CPU might be
> +	 * unstable if it has to run on that frequency for long duration of time
> +	 * and so its better to set it to a frequency which is specified in
> +	 * freq-table. This also makes cpufreq stats inconsistent as
> +	 * cpufreq-stats would fail to register because current frequency of CPU
> +	 * isn't found in freq-table.
> +	 *
> +	 * Because we don't want this change to effect boot process badly, we go
> +	 * for the next freq which is >= policy->cur ('cur' must be set by now,
> +	 * otherwise we will end up setting freq to lowest of the table as 'cur'
> +	 * is initialized to zero).
> +	 *
> +	 * In case where CPU is already running on one of the frequencies
> +	 * present in freq-table, this would turn into a dummy call as
> +	 * __cpufreq_driver_target() would return early.
> +	 *
> +	 * We are passing target-freq as "policy->cur - 1" otherwise
> +	 * __cpufreq_driver_target() would simply fail, as policy->cur will be
> +	 * equal to target-freq.
> +	 */

Shouldn't there be a check to see if the problem exists at all?  Otherwise
the core is setting a policy for ALL platform even those where the issue does
not exist.

> +	if (has_target()) {
> +		ret = __cpufreq_driver_target(policy, policy->cur - 1,
> +				CPUFREQ_RELATION_L);
> +		if (ret) {
> +			pr_err("%s: Unable to set frequency from table: %d\n",
> +					__func__, ret);
> +			goto err_out_unregister;
> +		}
> +	}
> +
>   	/* related cpus should atleast have policy->cpus */
>   	cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/