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

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

[Pierre Ossman]

[-- Attachment #1.1: Type: text/plain, Size: 442 bytes --]

See attached patch. Discussed in bugzilla:

https://bugzilla.kernel.org/show_bug.cgi?id=70311

Not clear why this cpufreq bug causes that breakage to r8169 and to the
pci bus. Input welcome. :)

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by FRA, a
  Swedish intelligence agency. Make sure your server uses
  encryption for SMTP traffic and consider using PGP for
  end-to-end encryption.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-cpufreq-fix-current-freq-check-on-policy-update.patch --]
[-- Type: text/x-patch, Size: 1407 bytes --]

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.

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 230 bytes --]

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

[Srivatsa S. Bhat]

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

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

[Rafael J. Wysocki]

On Friday, February 14, 2014 05:14:34 PM Srivatsa S. Bhat wrote:
> 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!

Due to a race condition of some sort?

Rafael

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

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

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?

Note that this is with 3.12.9, not master.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by FRA, a
  Swedish intelligence agency. Make sure your server uses
  encryption for SMTP traffic and consider using PGP for
  end-to-end encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 230 bytes --]

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

[Viresh Kumar]

On 14 February 2014 20:04, Pierre Ossman <pierre-list@ossman.eu> 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!

I think he is hitting some bug where he might be getting to this:

if (!data)
    return 0;

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

Please get some prints and see what's going wrong in powernowk8_get().
In case you are getting !data as null, get some messages in
powernowk8_cpu_exit() as well..

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

[Srivatsa S. Bhat]

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.


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;
 


Regards,
Srivatsa S. Bhat

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

[Srivatsa S. Bhat]

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

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

[Viresh Kumar]

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.