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

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

On 02/17/2014 01:53 PM, Viresh Kumar wrote:
> On 17 February 2014 13:38, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> Hmm, that would be good. However, I did find something odd while browsing
>> through the powernow-k8 code.
>>
>> It maintains a per-cpu data-structure called powernow_data, but it is
>> initialized only for the policy->cpu.
> 
> I thought about it earlier, but thought it will be called only with policy->cpu
> by core.
> 
>> In powernowk8_cpu_init():
>>
>>         per_cpu(powernow_data, pol->cpu) = data;
>>
>> Everywhere in the code, this per-cpu data-structure is accessed always with
>> pol->cpu as the argument, *except* in powernowk8_get():
>>
>> 1210 static unsigned int powernowk8_get(unsigned int cpu)
>> 1211 {
>> 1212         struct powernow_k8_data *data = per_cpu(powernow_data, cpu);
>> 1213         unsigned int khz = 0;
>> 1214         int err;
>> 1215
>> 1216         if (!data)
>> 1217                 return 0;
>> 1218
>> 1219         smp_call_function_single(cpu, query_values_on_cpu, &err, true);
>> 1220         if (err)
>> 1221                 goto out;
>> 1222
>> 1223         khz = find_khz_freq_from_fid(data->currfid);
>> 1224
>>
>> So, obviously it finds data to be uninitialized if cpu != pol->cpu, and hence
>> it would return 0 due to the "if (!data)" check. In other words, the init
>> routine initializes the per-cpu memory only for the pol->cpu, whereas the
>> ->get() routine tries to access it for some other cpu, and fails.
> 
> bingo!!
> 

:-) You guessed it right! :-)

>> So I think the following patch should fix your problem. Basically, this
>> initializes the per-cpu data-structures of all the CPUs to the correct memory,
>> and not just for the pol->cpu.
>>
>>
>> diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
>> index e10b646..bd7bc39 100644
>> --- a/drivers/cpufreq/powernow-k8.c
>> +++ b/drivers/cpufreq/powernow-k8.c
>> @@ -1076,7 +1076,7 @@ static int powernowk8_cpu_init(struct cpufreq_policy *pol)
>>  {
>>         struct powernow_k8_data *data;
>>         struct init_on_cpu init_on_cpu;
>> -       int rc;
>> +       int rc, cpu;
>>
>>         smp_call_function_single(pol->cpu, check_supported_cpu, &rc, 1);
>>         if (rc)
>> @@ -1140,7 +1140,9 @@ static int powernowk8_cpu_init(struct cpufreq_policy *pol)
>>         pr_debug("cpu_init done, current fid 0x%x, vid 0x%x\n",
>>                  data->currfid, data->currvid);
>>
>> -       per_cpu(powernow_data, pol->cpu) = data;
>> +       /* Point all the CPUs in this policy to the same data */
>> +       for_each_cpu(cpu, pol->cpus)
>> +               per_cpu(powernow_data, cpu) = data;
> 
> You need to do something similar on: powernowk8_cpu_exit() as well
> which sets it to zero.
>

Ah, yes. I missed that part.
 
> Please send a patch for it, that is the problem for sure.

Ok, will do. Thanks!

Regards,
Srivatsa S. Bhat