diff mbox

cpufreq: fix current freq check on policy update

Message ID 5301C379.9020909@linux.vnet.ibm.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Srivatsa S. Bhat Feb. 17, 2014, 8:08 a.m. UTC 
On 02/14/2014 08:04 PM, Pierre Ossman wrote:
> On Fri, 14 Feb 2014 17:14:34 +0530
> "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> 
>>
>> So with that, I don't see how the KHz value can turn out to be zero!
>>
> 
> Very interesting. But I can tell you that it is indeed happening. With
> my earlier patch (that checked the return value), I did have this in my
> dmesg:
> 
> [  188.044175] cpufreq: updating policy for CPU 1
> [  188.044177] cpufreq: Driver did not initialize current freq
> [  188.044177] cpufreq: setting new policy for CPU 0: 1000000 - 2800000 kHz
> 
> I can sprinkle some more pr_debug:s in there if you want to trace it
> further?
>

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.

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.

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.




Regards,
Srivatsa S. Bhat

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Srivatsa S. Bhat Feb. 17, 2014, 8:21 a.m. UTC | #1 
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


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Feb. 17, 2014, 8:23 a.m. UTC | #2 
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!!

> 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.

Please send a patch for it, that is the problem for sure.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 
 	return 0;