[PATCH 0/3] Cpufreq frequency serialization fixes

[PATCH 0/3] Cpufreq frequency serialization fixes

[Srivatsa S. Bhat]

Hi,

Meelis Roos reported hangs during boot in the longhaul cpufreq driver, after
commit 12478cf0c55 (cpufreq: Make sure frequency transitions are serialized).
The root-cause of this issue is the extra invocation of the
cpufreq_freq_transition_begin() and cpufreq_freq_transition_end() APIs in the
longhaul driver. I found similar issues in the powernow-k6 and powernow-k7
drivers as well. This patchset fixes the issue in all the 3 drivers.

 Srivatsa S. Bhat (3):
      cpufreq, powernow-k7: Fix double invocation of cpufreq_freq_transition_begin/end
      cpufreq, powernow-k6: Fix double invocation of cpufreq_freq_transition_begin/end
      cpufreq, longhaul: Fix double invocation of cpufreq_freq_transition_begin/end


 drivers/cpufreq/longhaul.c    |   12 ++++++++----
 drivers/cpufreq/powernow-k6.c |   20 +++++++++++---------
 drivers/cpufreq/powernow-k7.c |    4 ----
 3 files changed, 19 insertions(+), 17 deletions(-)


Thanks,
Srivatsa S. Bhat
IBM Linux Technology Center

[PATCH 1/3] cpufreq, longhaul: Fix double invocation of cpufreq_freq_transition_begin/end

[Srivatsa S. Bhat]

During frequency transitions, the cpufreq core takes the responsibility of
invoking cpufreq_freq_transition_begin() and cpufreq_freq_transition_end()
for those cpufreq drivers that define the ->target_index callback but don't
set the ASYNC_NOTIFICATION flag.

The longhaul cpufreq driver falls under this category, but this driver was
invoking the _begin() and _end() APIs itself around frequency transitions,
which led to double invocation of the _begin() API. The _begin API makes
contending callers wait until the previous invocation is complete. Hence,
the longhaul driver ended up waiting on itself, leading to system hangs
during boot.

Fix this by removing the calls to the _begin() and _end() APIs from the
longhaul driver, since they rightly belong to the cpufreq core.

(Note that during module_exit(), the longhaul driver sets the frequency
 without any help from the cpufreq core. So add explicit calls to the
 _begin() and _end() APIs around that frequency transition alone, to take
 care of that special case.)

Reported-by: Meelis Roos <mroos@linux.ee>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 drivers/cpufreq/longhaul.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/longhaul.c b/drivers/cpufreq/longhaul.c
index d00e5d1..9b6dd9a 100644
--- a/drivers/cpufreq/longhaul.c
+++ b/drivers/cpufreq/longhaul.c
@@ -269,8 +269,6 @@ static void longhaul_setstate(struct cpufreq_policy *policy,
 	freqs.old = calc_speed(longhaul_get_cpu_mult());
 	freqs.new = speed;
 
-	cpufreq_freq_transition_begin(policy, &freqs);
-
 	pr_debug("Setting to FSB:%dMHz Mult:%d.%dx (%s)\n",
 			fsb, mult/10, mult%10, print_speed(speed/1000));
 retry_loop:
@@ -385,8 +383,6 @@ retry_loop:
 			goto retry_loop;
 		}
 	}
-	/* Report true CPU frequency */
-	cpufreq_freq_transition_end(policy, &freqs, 0);
 
 	if (!bm_timeout)
 		printk(KERN_INFO PFX "Warning: Timeout while waiting for "
@@ -968,7 +964,15 @@ static void __exit longhaul_exit(void)
 
 	for (i = 0; i < numscales; i++) {
 		if (mults[i] == maxmult) {
+			struct cpufreq_freqs freqs;
+
+			freqs.old = policy->cur;
+			freqs.new = longhaul_table[i].frequency;
+			freqs.flags = 0;
+
+			cpufreq_freq_transition_begin(policy, &freqs);
 			longhaul_setstate(policy, i);
+			cpufreq_freq_transition_end(policy, &freqs, 0);
 			break;
 		}
 	}

Re: [PATCH 1/3] cpufreq, longhaul: Fix double invocation of cpufreq_freq_transition_begin/end

[Viresh Kumar]

On 25 April 2014 13:48, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:

> diff --git a/drivers/cpufreq/longhaul.c b/drivers/cpufreq/longhaul.c

> @@ -269,8 +269,6 @@ static void longhaul_setstate(struct cpufreq_policy *policy,

This routine has this code as well:

mult = mults[mults_index & 0x1f];
if (mult == -1)
        return;
speed = calc_speed(mult);
if ((speed > highest_speed) || (speed < lowest_speed))
        return;

And so it might return without changing frequency and that's why I
left this driver earlier for this change.. So, please return -EINVAL
from here..

Re: [PATCH 1/3] cpufreq, longhaul: Fix double invocation of cpufreq_freq_transition_begin/end

[Srivatsa S. Bhat]

On 04/25/2014 02:07 PM, Viresh Kumar wrote:
> On 25 April 2014 13:48, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> 
>> diff --git a/drivers/cpufreq/longhaul.c b/drivers/cpufreq/longhaul.c
> 
>> @@ -269,8 +269,6 @@ static void longhaul_setstate(struct cpufreq_policy *policy,
> 
> This routine has this code as well:
> 
> mult = mults[mults_index & 0x1f];
> if (mult == -1)
>         return;
> speed = calc_speed(mult);
> if ((speed > highest_speed) || (speed < lowest_speed))
>         return;
> 
> And so it might return without changing frequency and that's why I
> left this driver earlier for this change.. So, please return -EINVAL
> from here..
> 

Ok, I'll address this in the next version. Thanks for pointing
this out!

Regards,
Srivatsa S. Bhat

[PATCH 2/3] cpufreq, powernow-k6: Fix double invocation of cpufreq_freq_transition_begin/end

[Srivatsa S. Bhat]

During frequency transitions, the cpufreq core takes the responsibility of
invoking cpufreq_freq_transition_begin() and cpufreq_freq_transition_end()
for those cpufreq drivers that define the ->target_index callback but don't
set the ASYNC_NOTIFICATION flag.

The powernow-k6 cpufreq driver falls under this category, but this driver was
invoking the _begin() and _end() APIs itself around frequency transitions,
which led to double invocation of the _begin() API. The _begin API makes
contending callers wait until the previous invocation is complete. Hence,
the powernow-k6 driver ended up waiting on itself, leading to system hangs
during boot.

Fix this by removing the calls to the _begin() and _end() APIs from the
powernow-k6 driver, since they rightly belong to the cpufreq core.

(Note that during ->exit(), the powernow-k6 driver sets the frequency
 without any help from the cpufreq core. So add explicit calls to the
 _begin() and _end() APIs around that frequency transition alone, to take
 care of that special case. Also, add a missing 'break' statement there.)

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 drivers/cpufreq/powernow-k6.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/cpufreq/powernow-k6.c b/drivers/cpufreq/powernow-k6.c
index 49f120e..6a4c34e 100644
--- a/drivers/cpufreq/powernow-k6.c
+++ b/drivers/cpufreq/powernow-k6.c
@@ -138,22 +138,14 @@ static void powernow_k6_set_cpu_multiplier(unsigned int best_i)
 static int powernow_k6_target(struct cpufreq_policy *policy,
 		unsigned int best_i)
 {
-	struct cpufreq_freqs freqs;
 
 	if (clock_ratio[best_i].driver_data > max_multiplier) {
 		printk(KERN_ERR PFX "invalid target frequency\n");
 		return -EINVAL;
 	}
 
-	freqs.old = busfreq * powernow_k6_get_cpu_multiplier();
-	freqs.new = busfreq * clock_ratio[best_i].driver_data;
-
-	cpufreq_freq_transition_begin(policy, &freqs);
-
 	powernow_k6_set_cpu_multiplier(best_i);
 
-	cpufreq_freq_transition_end(policy, &freqs, 0);
-
 	return 0;
 }
 
@@ -228,8 +220,18 @@ static int powernow_k6_cpu_exit(struct cpufreq_policy *policy)
 {
 	unsigned int i;
 	for (i = 0; i < 8; i++) {
-		if (i == max_multiplier)
+		if (i == max_multiplier) {
+			struct cpufreq_freqs freqs;
+
+			freqs.old = policy->cur;
+			freqs.new = clock_ratio[i].frequency;
+			freqs.flags = 0;
+
+			cpufreq_freq_transition_begin(policy, &freqs);
 			powernow_k6_target(policy, i);
+			cpufreq_freq_transition_end(policy, &freqs, 0);
+			break;
+		}
 	}
 	return 0;
 }

Re: [PATCH 2/3] cpufreq, powernow-k6: Fix double invocation of cpufreq_freq_transition_begin/end

[Viresh Kumar]

On 25 April 2014 13:48, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> During frequency transitions, the cpufreq core takes the responsibility of
> invoking cpufreq_freq_transition_begin() and cpufreq_freq_transition_end()
> for those cpufreq drivers that define the ->target_index callback but don't
> set the ASYNC_NOTIFICATION flag.
>
> The powernow-k6 cpufreq driver falls under this category, but this driver was
> invoking the _begin() and _end() APIs itself around frequency transitions,
> which led to double invocation of the _begin() API. The _begin API makes
> contending callers wait until the previous invocation is complete. Hence,
> the powernow-k6 driver ended up waiting on itself, leading to system hangs
> during boot.
>
> Fix this by removing the calls to the _begin() and _end() APIs from the
> powernow-k6 driver, since they rightly belong to the cpufreq core.
>
> (Note that during ->exit(), the powernow-k6 driver sets the frequency
>  without any help from the cpufreq core. So add explicit calls to the
>  _begin() and _end() APIs around that frequency transition alone, to take
>  care of that special case. Also, add a missing 'break' statement there.)
>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
>
>  drivers/cpufreq/powernow-k6.c |   20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cpufreq/powernow-k6.c b/drivers/cpufreq/powernow-k6.c
> index 49f120e..6a4c34e 100644
> --- a/drivers/cpufreq/powernow-k6.c
> +++ b/drivers/cpufreq/powernow-k6.c
> @@ -138,22 +138,14 @@ static void powernow_k6_set_cpu_multiplier(unsigned int best_i)
>  static int powernow_k6_target(struct cpufreq_policy *policy,
>                 unsigned int best_i)
>  {
> -       struct cpufreq_freqs freqs;
>
>         if (clock_ratio[best_i].driver_data > max_multiplier) {
>                 printk(KERN_ERR PFX "invalid target frequency\n");
>                 return -EINVAL;
>         }
>
> -       freqs.old = busfreq * powernow_k6_get_cpu_multiplier();
> -       freqs.new = busfreq * clock_ratio[best_i].driver_data;
> -
> -       cpufreq_freq_transition_begin(policy, &freqs);
> -
>         powernow_k6_set_cpu_multiplier(best_i);
>
> -       cpufreq_freq_transition_end(policy, &freqs, 0);
> -
>         return 0;
>  }
>
> @@ -228,8 +220,18 @@ static int powernow_k6_cpu_exit(struct cpufreq_policy *policy)
>  {
>         unsigned int i;
>         for (i = 0; i < 8; i++) {
> -               if (i == max_multiplier)
> +               if (i == max_multiplier) {

This wouldn't work, it has to be: (clock_ratio[i].driver_data == max_multiplier)

> +                       struct cpufreq_freqs freqs;
> +
> +                       freqs.old = policy->cur;
> +                       freqs.new = clock_ratio[i].frequency;
> +                       freqs.flags = 0;
> +
> +                       cpufreq_freq_transition_begin(policy, &freqs);
>                         powernow_k6_target(policy, i);
> +                       cpufreq_freq_transition_end(policy, &freqs, 0);
> +                       break;
> +               }
>         }
>         return 0;
>  }

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 2/3] cpufreq, powernow-k6: Fix double invocation of cpufreq_freq_transition_begin/end

[Srivatsa S. Bhat]

On 04/25/2014 01:58 PM, Viresh Kumar wrote:
> On 25 April 2014 13:48, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> During frequency transitions, the cpufreq core takes the responsibility of
>> invoking cpufreq_freq_transition_begin() and cpufreq_freq_transition_end()
>> for those cpufreq drivers that define the ->target_index callback but don't
>> set the ASYNC_NOTIFICATION flag.
>>
>> The powernow-k6 cpufreq driver falls under this category, but this driver was
>> invoking the _begin() and _end() APIs itself around frequency transitions,
>> which led to double invocation of the _begin() API. The _begin API makes
>> contending callers wait until the previous invocation is complete. Hence,
>> the powernow-k6 driver ended up waiting on itself, leading to system hangs
>> during boot.
>>
>> Fix this by removing the calls to the _begin() and _end() APIs from the
>> powernow-k6 driver, since they rightly belong to the cpufreq core.
>>
>> (Note that during ->exit(), the powernow-k6 driver sets the frequency
>>  without any help from the cpufreq core. So add explicit calls to the
>>  _begin() and _end() APIs around that frequency transition alone, to take
>>  care of that special case. Also, add a missing 'break' statement there.)
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>>
>>  drivers/cpufreq/powernow-k6.c |   20 +++++++++++---------
>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/cpufreq/powernow-k6.c b/drivers/cpufreq/powernow-k6.c
>> index 49f120e..6a4c34e 100644
>> --- a/drivers/cpufreq/powernow-k6.c
>> +++ b/drivers/cpufreq/powernow-k6.c
>> @@ -138,22 +138,14 @@ static void powernow_k6_set_cpu_multiplier(unsigned int best_i)
>>  static int powernow_k6_target(struct cpufreq_policy *policy,
>>                 unsigned int best_i)
>>  {
>> -       struct cpufreq_freqs freqs;
>>
>>         if (clock_ratio[best_i].driver_data > max_multiplier) {
>>                 printk(KERN_ERR PFX "invalid target frequency\n");
>>                 return -EINVAL;
>>         }
>>
>> -       freqs.old = busfreq * powernow_k6_get_cpu_multiplier();
>> -       freqs.new = busfreq * clock_ratio[best_i].driver_data;
>> -
>> -       cpufreq_freq_transition_begin(policy, &freqs);
>> -
>>         powernow_k6_set_cpu_multiplier(best_i);
>>
>> -       cpufreq_freq_transition_end(policy, &freqs, 0);
>> -
>>         return 0;
>>  }
>>
>> @@ -228,8 +220,18 @@ static int powernow_k6_cpu_exit(struct cpufreq_policy *policy)
>>  {
>>         unsigned int i;
>>         for (i = 0; i < 8; i++) {
>> -               if (i == max_multiplier)
>> +               if (i == max_multiplier) {
> 
> This wouldn't work, it has to be: (clock_ratio[i].driver_data == max_multiplier)
>

Hmm, looks like an existing bug in the driver. Will fix this in the next
version.

Regards,
Srivatsa S. Bhat
 
>> +                       struct cpufreq_freqs freqs;
>> +
>> +                       freqs.old = policy->cur;
>> +                       freqs.new = clock_ratio[i].frequency;
>> +                       freqs.flags = 0;
>> +
>> +                       cpufreq_freq_transition_begin(policy, &freqs);
>>                         powernow_k6_target(policy, i);
>> +                       cpufreq_freq_transition_end(policy, &freqs, 0);
>> +                       break;
>> +               }
>>         }
>>         return 0;
>>  }
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 

[PATCH 3/3] cpufreq, powernow-k7: Fix double invocation of cpufreq_freq_transition_begin/end

[Srivatsa S. Bhat]

During frequency transitions, the cpufreq core takes the responsibility of
invoking cpufreq_freq_transition_begin() and cpufreq_freq_transition_end()
for those cpufreq drivers that define the ->target_index callback but don't
set the ASYNC_NOTIFICATION flag.

The powernow-k7 cpufreq driver falls under this category, but this driver was
invoking the _begin() and _end() APIs itself around frequency transitions,
which led to double invocation of the _begin() API. The _begin API makes
contending callers wait until the previous invocation is complete. Hence,
the powernow-k7 driver ended up waiting on itself, leading to system hangs
during boot.

Fix this by removing the calls to the _begin() and _end() APIs from the
powernow-k7 driver, since they rightly belong to the cpufreq core.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 drivers/cpufreq/powernow-k7.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/cpufreq/powernow-k7.c b/drivers/cpufreq/powernow-k7.c
index f911645..e61e224 100644
--- a/drivers/cpufreq/powernow-k7.c
+++ b/drivers/cpufreq/powernow-k7.c
@@ -269,8 +269,6 @@ static int powernow_target(struct cpufreq_policy *policy, unsigned int index)
 
 	freqs.new = powernow_table[index].frequency;
 
-	cpufreq_freq_transition_begin(policy, &freqs);
-
 	/* Now do the magic poking into the MSRs.  */
 
 	if (have_a0 == 1)	/* A0 errata 5 */
@@ -290,8 +288,6 @@ static int powernow_target(struct cpufreq_policy *policy, unsigned int index)
 	if (have_a0 == 1)
 		local_irq_enable();
 
-	cpufreq_freq_transition_end(policy, &freqs, 0);
-
 	return 0;
 }
 

Re: [PATCH 3/3] cpufreq, powernow-k7: Fix double invocation of cpufreq_freq_transition_begin/end

[Viresh Kumar]

On 25 April 2014 13:48, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> During frequency transitions, the cpufreq core takes the responsibility of
> invoking cpufreq_freq_transition_begin() and cpufreq_freq_transition_end()
> for those cpufreq drivers that define the ->target_index callback but don't
> set the ASYNC_NOTIFICATION flag.
>
> The powernow-k7 cpufreq driver falls under this category, but this driver was
> invoking the _begin() and _end() APIs itself around frequency transitions,
> which led to double invocation of the _begin() API. The _begin API makes
> contending callers wait until the previous invocation is complete. Hence,
> the powernow-k7 driver ended up waiting on itself, leading to system hangs
> during boot.
>
> Fix this by removing the calls to the _begin() and _end() APIs from the
> powernow-k7 driver, since they rightly belong to the cpufreq core.
>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
>
>  drivers/cpufreq/powernow-k7.c |    4 ----
>  1 file changed, 4 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 0/3] Cpufreq frequency serialization fixes

[Meelis Roos]

> Meelis Roos reported hangs during boot in the longhaul cpufreq driver, after
> commit 12478cf0c55 (cpufreq: Make sure frequency transitions are serialized).
> The root-cause of this issue is the extra invocation of the
> cpufreq_freq_transition_begin() and cpufreq_freq_transition_end() APIs in the
> longhaul driver. I found similar issues in the powernow-k6 and powernow-k7
> drivers as well. This patchset fixes the issue in all the 3 drivers.
> 
>  Srivatsa S. Bhat (3):
>       cpufreq, powernow-k7: Fix double invocation of cpufreq_freq_transition_begin/end
>       cpufreq, powernow-k6: Fix double invocation of cpufreq_freq_transition_begin/end
>       cpufreq, longhaul: Fix double invocation of cpufreq_freq_transition_begin/end

This set fixes it for me on VIA EPIA with longhaul.

-- 
Meelis Roos (mroos@linux.ee)

Re: [PATCH 0/3] Cpufreq frequency serialization fixes

[Srivatsa S. Bhat]

On 04/25/2014 10:59 PM, Meelis Roos wrote:
>> Meelis Roos reported hangs during boot in the longhaul cpufreq driver, after
>> commit 12478cf0c55 (cpufreq: Make sure frequency transitions are serialized).
>> The root-cause of this issue is the extra invocation of the
>> cpufreq_freq_transition_begin() and cpufreq_freq_transition_end() APIs in the
>> longhaul driver. I found similar issues in the powernow-k6 and powernow-k7
>> drivers as well. This patchset fixes the issue in all the 3 drivers.
>>
>>  Srivatsa S. Bhat (3):
>>       cpufreq, powernow-k7: Fix double invocation of cpufreq_freq_transition_begin/end
>>       cpufreq, powernow-k6: Fix double invocation of cpufreq_freq_transition_begin/end
>>       cpufreq, longhaul: Fix double invocation of cpufreq_freq_transition_begin/end
> 
> This set fixes it for me on VIA EPIA with longhaul.
> 

Thanks a lot for testing this Meelis! I'll send out the v2
soon.

Regards,
Srivatsa S. Bhat