Re: [PATCH] cpufreq: fix current freq check on policy update

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 02/14/2014 11:56 AM, Pierre Ossman wrote:
> From de497de3fcd81e8340498cd0b34b3388fe75cc19 Mon Sep 17 00:00:00 2001
> From: Pierre Ossman <pierre@ossman.eu>
> Date: Fri, 14 Feb 2014 07:17:02 +0100
> Subject: [PATCH] cpufreq: fix current freq check on policy update
> 
> There was some variable confusion in cpufreq_update_policy()
> when we tried to get a current reading of the CPU frequency.
> If it failed to get the frequency, a current frequency of
> 0 kHz would be stored which in turn screwed up other parts
> of the kernel.
> 

You know what's interesting? I went through the powernow-k8 cpufreq
driver code (based on your system data in the bugzilla), and found
that (if I read it correctly), ->get() can simply never return 0 !

.get is mapped to powernowk8_get

powernowk8_get() in turn calls smp_call_function_single() on that cpu
and fetches the data into data->currfid.

Then we have this statement:
	khz = find_khz_freq_from_fid(data->currfid);

which is:

  66 /* Return a frequency in MHz, given an input fid */
  67 static u32 find_freq_from_fid(u32 fid)
  68 {
  69         return 800 + (fid * 100);
  70 }
  71 
  72 /* Return a frequency in KHz, given an input fid */
  73 static u32 find_khz_freq_from_fid(u32 fid)
  74 {
  75         return 1000 * find_freq_from_fid(fid);
  76 }


So with that, I don't see how the KHz value can turn out to be zero!

Regards,
Srivatsa S. Bhat

> In particular it somehow disoriented the r8169 driver and
> this entire issue was handled on this bug:
> 
>   https://bugzilla.kernel.org/show_bug.cgi?id=70311
> 
> Signed-off-by: Pierre Ossman <pierre@ossman.eu>
> ---
>  drivers/cpufreq/cpufreq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 08ca8c9..1b61310 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2151,9 +2151,9 @@ int cpufreq_update_policy(unsigned int cpu)
>  	 */
>  	if (cpufreq_driver->get) {
>  		new_policy.cur = cpufreq_driver->get(cpu);
> -		if (!policy->cur) {
> +		if (!new_policy.cur) {
>  			pr_debug("Driver did not initialize current freq");
> -			policy->cur = new_policy.cur;
> +			new_policy.cur = policy->cur;
>  		} else {
>  			if (policy->cur != new_policy.cur && has_target())
>  				cpufreq_out_of_sync(cpu, policy->cur,
> -- 1.8.5.3