[02/34] cpufreq: acpi: remove calls to cpufreq_notify_transition()

Message ID 333bf5868dfbfb6de474f060dc216fa6d33848e2.1376619363.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar Aug. 16, 2013, 2:24 a.m.
Most of the drivers do following in their ->target_index() routines:

	struct cpufreq_freqs freqs;
	freqs.old = old freq...
	freqs.new = new freq...

	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);

	/* Change rate here */

	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);

This is replicated over all cpufreq drivers today and there doesn't exists a
good enough reason why this shouldn't be moved to cpufreq core instead.

Earlier patches have added support in cpufreq core to do cpufreq notification on
frequency change, this one removes it from this driver.

Some related minor cleanups are also done along with it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/acpi-cpufreq.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

Comments

蓝天宇 Aug. 16, 2013, 7:42 a.m. | #1
2013/8/16 Viresh Kumar <viresh.kumar@linaro.org>

> Most of the drivers do following in their ->target_index() routines:
>
>         struct cpufreq_freqs freqs;
>         freqs.old = old freq...
>         freqs.new = new freq...
>
>         cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
>
>         /* Change rate here */
>
>         cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
>
> This is replicated over all cpufreq drivers today and there doesn't exists
> a
> good enough reason why this shouldn't be moved to cpufreq core instead.
>
> Earlier patches have added support in cpufreq core to do cpufreq
> notification on
> frequency change, this one removes it from this driver.
>
> Some related minor cleanups are also done along with it.
>
Hi Viresh:




>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c
> b/drivers/cpufreq/acpi-cpufreq.c
> index 7536e7d..6b00cd8 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -428,14 +428,10 @@ static int acpi_cpufreq_target(struct cpufreq_policy
> *policy,
>  {
>         struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu);
>         struct acpi_processor_performance *perf;
> -       struct cpufreq_freqs freqs;
>         struct drv_cmd cmd;
>         unsigned int next_perf_state = 0; /* Index into perf table */
>         int result = 0;
>
> -       pr_debug("acpi_cpufreq_target %d (%d)\n",
> -                       data->freq_table[index].frequency, policy->cpu);
> -
>         if (unlikely(data == NULL ||
>              data->acpi_data == NULL || data->freq_table == NULL)) {
>                 return -ENODEV;
> @@ -483,23 +479,17 @@ static int acpi_cpufreq_target(struct cpufreq_policy
> *policy,
>         else
>                 cmd.mask = cpumask_of(policy->cpu);
>
> -       freqs.old = perf->states[perf->state].core_frequency * 1000;
> -       freqs.new = data->freq_table[index].frequency;
> -       cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
> -
>         drv_write(&cmd);
>
>         if (acpi_pstate_strict) {
> -               if (!check_freqs(cmd.mask, freqs.new, data)) {
> +               if (!check_freqs(cmd.mask,
> data->freq_table[index].frequency,
> +                                       data)) {
>                         pr_debug("acpi_cpufreq_target failed (%d)\n",
>                                 policy->cpu);
>                         result = -EAGAIN;
> -                       freqs.new = freqs.old;
>                 }
>         }
>
> -       cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
> -
>         if (!result)
>                 perf->state = next_perf_state;
>
> --
> 1.7.12.rc2.18.g61b472e
>
> --
> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
蓝天宇 Aug. 16, 2013, 7:54 a.m. | #2
2013/8/16 Lan Tianyu <lantianyu1986@gmail.com>

>
>
>
> 2013/8/16 Viresh Kumar <viresh.kumar@linaro.org>
>
>> Most of the drivers do following in their ->target_index() routines:
>>
>>         struct cpufreq_freqs freqs;
>>         freqs.old = old freq...
>>         freqs.new = new freq...
>>
>>         cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
>>
>>         /* Change rate here */
>>
>>         cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
>>
>> This is replicated over all cpufreq drivers today and there doesn't
>> exists a
>> good enough reason why this shouldn't be moved to cpufreq core instead.
>>
>> Earlier patches have added support in cpufreq core to do cpufreq
>> notification on
>> frequency change, this one removes it from this driver.
>>
>> Some related minor cleanups are also done along with it.
>>
>
Sorry for misoperation.

Hi Viresh:
>
>
Hi Viresh:

                One concern. Target() callback may return before changing
cpufreq actually due to some check failures. After this change, prechange
event will be triggered when these check failures take place. I am not sure
whether this should be took into account.


>
>
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>  drivers/cpufreq/acpi-cpufreq.c | 14 ++------------
>>  1 file changed, 2 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/cpufreq/acpi-cpufreq.c
>> b/drivers/cpufreq/acpi-cpufreq.c
>> index 7536e7d..6b00cd8 100644
>> --- a/drivers/cpufreq/acpi-cpufreq.c
>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>> @@ -428,14 +428,10 @@ static int acpi_cpufreq_target(struct
>> cpufreq_policy *policy,
>>  {
>>         struct acpi_cpufreq_data *data = per_cpu(acfreq_data,
>> policy->cpu);
>>         struct acpi_processor_performance *perf;
>> -       struct cpufreq_freqs freqs;
>>         struct drv_cmd cmd;
>>         unsigned int next_perf_state = 0; /* Index into perf table */
>>         int result = 0;
>>
>> -       pr_debug("acpi_cpufreq_target %d (%d)\n",
>> -                       data->freq_table[index].frequency, policy->cpu);
>> -
>>         if (unlikely(data == NULL ||
>>              data->acpi_data == NULL || data->freq_table == NULL)) {
>>                 return -ENODEV;
>> @@ -483,23 +479,17 @@ static int acpi_cpufreq_target(struct
>> cpufreq_policy *policy,
>>         else
>>                 cmd.mask = cpumask_of(policy->cpu);
>>
>> -       freqs.old = perf->states[perf->state].core_frequency * 1000;
>> -       freqs.new = data->freq_table[index].frequency;
>> -       cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
>> -
>>         drv_write(&cmd);
>>
>>         if (acpi_pstate_strict) {
>> -               if (!check_freqs(cmd.mask, freqs.new, data)) {
>> +               if (!check_freqs(cmd.mask,
>> data->freq_table[index].frequency,
>> +                                       data)) {
>>                         pr_debug("acpi_cpufreq_target failed (%d)\n",
>>                                 policy->cpu);
>>                         result = -EAGAIN;
>> -                       freqs.new = freqs.old;
>>                 }
>>         }
>>
>> -       cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
>> -
>>         if (!result)
>>                 perf->state = next_perf_state;
>>
>> --
>> 1.7.12.rc2.18.g61b472e
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>
>
> --
> Best regards
> Tianyu Lan
>
Viresh Kumar Aug. 16, 2013, 7:59 a.m. | #3
On 16 August 2013 13:24, Lan Tianyu <lantianyu1986@gmail.com> wrote:
> Sorry for misoperation.

No Problem...

>                 One concern. Target() callback may return before changing
> cpufreq actually due to some check failures. After this change, prechange
> event will be triggered when these check failures take place. I am not sure
> whether this should be took into account.

Yes, if you see the first patch of this series, it takes this into
account.. In case
target() failed and returned an error, we simply notify the POST CHANGE
notification with old frequencies instead of new ones. I believe that would be
enough..

This is exactly what acpi-cpufreq and others are doing currently.

Hope I answered your question well?

--
viresh
蓝天宇 Aug. 16, 2013, 3:30 p.m. | #4
2013/8/16 Viresh Kumar <viresh.kumar@linaro.org>:
> On 16 August 2013 13:24, Lan Tianyu <lantianyu1986@gmail.com> wrote:
>> Sorry for misoperation.
>
> No Problem...
>
>>                 One concern. Target() callback may return before changing
>> cpufreq actually due to some check failures. After this change, prechange
>> event will be triggered when these check failures take place. I am not sure
>> whether this should be took into account.
>
> Yes, if you see the first patch of this series, it takes this into
> account.. In case
> target() failed and returned an error, we simply notify the POST CHANGE
> notification with old frequencies instead of new ones. I believe that would be
> enough..

Yes, I have seen it but I missed the following two patches because they
are not in the linux-pm tree. You moved the cpufreq_frequency_table_target()
to cpufreq core and before notifying PRE CHANGE notification. The major
check has been done. Now I think it's ok. Thanks for explanation.

http://www.spinics.net/lists/cpufreq/msg06970.html
http://www.spinics.net/lists/cpufreq/msg06896.html

Reviewed-by: Lan Tianyu <tianyu.lan@intel.com>

>
> This is exactly what acpi-cpufreq and others are doing currently.
>
> Hope I answered your question well?
>
> --
> viresh

Patch

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 7536e7d..6b00cd8 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -428,14 +428,10 @@  static int acpi_cpufreq_target(struct cpufreq_policy *policy,
 {
 	struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu);
 	struct acpi_processor_performance *perf;
-	struct cpufreq_freqs freqs;
 	struct drv_cmd cmd;
 	unsigned int next_perf_state = 0; /* Index into perf table */
 	int result = 0;
 
-	pr_debug("acpi_cpufreq_target %d (%d)\n",
-			data->freq_table[index].frequency, policy->cpu);
-
 	if (unlikely(data == NULL ||
 	     data->acpi_data == NULL || data->freq_table == NULL)) {
 		return -ENODEV;
@@ -483,23 +479,17 @@  static int acpi_cpufreq_target(struct cpufreq_policy *policy,
 	else
 		cmd.mask = cpumask_of(policy->cpu);
 
-	freqs.old = perf->states[perf->state].core_frequency * 1000;
-	freqs.new = data->freq_table[index].frequency;
-	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
-
 	drv_write(&cmd);
 
 	if (acpi_pstate_strict) {
-		if (!check_freqs(cmd.mask, freqs.new, data)) {
+		if (!check_freqs(cmd.mask, data->freq_table[index].frequency,
+					data)) {
 			pr_debug("acpi_cpufreq_target failed (%d)\n",
 				policy->cpu);
 			result = -EAGAIN;
-			freqs.new = freqs.old;
 		}
 	}
 
-	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
-
 	if (!result)
 		perf->state = next_perf_state;