linux-kernel - Re: [PATCH 11/12] cpufreq: arm-big-little: clarify frequency units

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:	Thu, 03 Dec 2015 15:37:44 +0100
From:	Ben Gamari <ben@...rt-cactus.org>
To:	"Jon Medhurst \(Tixy\)" <tixy@...aro.org>
Cc:	Thomas Abraham <thomas.ab@...sung.com>,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	Michael Turquette <mturquette@...libre.com>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Kukjin Kim <kgene@...nel.org>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Lukasz Majewski <l.majewski@...sung.com>,
	Kevin Hilman <khilman@...aro.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Tobias Jakobi <tjakobi@...h.uni-bielefeld.de>,
	Anand Moon <linux.amoon@...il.com>,
	Javier Martinez Canillas <javier@....samsung.com>,
	linux-pm@...r.kernel.org, Tomasz Figa <tomasz.figa@...il.com>,
	linux-kernel@...r.kernel.org, Chanwoo Choi <cw00.choi@...sung.com>,
	b.zolnierkie@...sung.com, linux-samsung-soc@...r.kernel.org,
	Javier Martinez Canillas <javier@...hile0.org>,
	linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 11/12] cpufreq: arm-big-little: clarify frequency units

"Jon Medhurst (Tixy)" <tixy@...aro.org> writes:

> On Wed, 2015-12-02 at 22:19 +0100, Ben Gamari wrote:
>> The frequency units are very confusing in this area as OPPs use Hz
>> whereas cpufreq uses kHz. Be explicit about this in variable naming.
>> 
>> Cc: Javier Martinez Canillas <javier@....samsung.com>
>> Signed-off-by: Ben Gamari <ben@...rt-cactus.org>
>> ---
>>  drivers/cpufreq/arm_big_little.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
>> index 855599b..2d5761c 100644
>> --- a/drivers/cpufreq/arm_big_little.c
>> +++ b/drivers/cpufreq/arm_big_little.c
>> @@ -130,14 +130,14 @@ static unsigned int bL_cpufreq_get_rate(unsigned int cpu)
>>  }
>>  
>>  static int
>> -bL_cpufreq_set_rate_cluster(u32 cpu, u32 cluster, u32 new_rate)
>> +bL_cpufreq_set_rate_cluster(u32 cpu, u32 cluster, u32 new_rate_kHz)
>>  {
>>  	unsigned long volt = 0, volt_old = 0;
>>  	long freq_Hz;
>>  	u32 old_rate;
>
> IMO variable renaming doesn't seem necessary, if cpufreq uses kHz then
> in a cpufreq driver adding 'kHz' to variable seems redundant, especially
> if Hz values like freq_Hz above are named especially to signal their
> different units.
> 
Correct; it isn't strictly necessary but it would have saved me half an
hour of poking around trying work out the intent of this code.

> However, if renaming is going to happen it should at
> least be consistent within the same function i.e. also rename the old
> old_rate variable above.
>
That's a reasonable objection. I'd be happy to do that.

snip
>>  static unsigned int
>> -bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>> +bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate_kHz)
>>  {
>>  	u32 new_rate, prev_rate;
>
> Ditto. Rename these too to add '_kHz' ?
>
Sure.
>>  	int ret;
>> @@ -209,13 +209,13 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>>  
>>  	if (bLs) {
>>  		prev_rate = per_cpu(cpu_last_req_freq, cpu);
>> -		per_cpu(cpu_last_req_freq, cpu) = rate;
>> +		per_cpu(cpu_last_req_freq, cpu) = rate_kHz;
>>  		per_cpu(physical_cluster, cpu) = new_cluster;
>>  
>>  		new_rate = find_cluster_maxfreq(new_cluster);
>>  		new_rate = ACTUAL_FREQ(new_cluster, new_rate);
>>  	} else {
>> -		new_rate = rate;
>> +		new_rate = rate_kHz;
>>  	}
>>  
>>  	pr_debug("%s: cpu: %d, old cluster: %d, new cluster: %d, freq: %d\n",
>> @@ -236,7 +236,7 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
>>  	} else if (ret && bLs) {
>>  		per_cpu(cpu_last_req_freq, cpu) = prev_rate;
>>  		per_cpu(physical_cluster, cpu) = old_cluster;
>> -	} 
>> +	}
>
> There's a spurious whitespace change here. I know the space you deleted
> shouldn't have been there, but doing tidyups like that generally isn't
> done in patches that don't otherwise affect the code in question.
>
Alright, I can drop that change.

Cheers,

- Ben


Download attachment "signature.asc" of type "application/pgp-signature" (473 bytes)