[PATCH 0/2] fix some problems for cpufreq

[PATCH 0/2] fix some problems for cpufreq

[Wang Weidong]

Hi Rafael and Viresh

Sorry to trouble you again. As for:
"acpi-cpufreq: get the cur_freq from acpi_processor_performance states"
I do it again, and add the other patch.

patch #1: acpi-cpufreq: make the freq_table store the same freq value

I think it can work. The set of available states which come
from acpi won't change. Just like the power would be remove,
the acpi driver will do that:
call
 ->acpi_processor_ppc_has_changed
 ->cpufreq_update_policy
 ->acpi_ppc_notifier_block.notifier_call
   ->acpi_processor_ppc_notifier
   ->cpufreq_verify_within_limits
The progress will change the policy's min_freq and max_freq
while it won't change the set of states(freq_tables).

patch #2: cpufreq: show the real avail freqs with the freq_table

when the min_freq and max_freq change, we should sync the availble
freqs.

Regards,
Wang

Wang Weidong (2):
  acpi-cpufreq: make the freq_table store the same freq value
  cpufreq: show the real avail freqs with the freq_table

 drivers/cpufreq/acpi-cpufreq.c | 2 +-
 drivers/cpufreq/freq_table.c   | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
1.7.12

[PATCH 1/2] acpi-cpufreq: make the freq_table store the same freq value

[Wang Weidong]

As the initialized freq_tables maybe different from the p-states
values, so the array index is different as well.

Although in this case:
p-states value: [2400 2400 2000 ...], while the freq_tables:
[2400 2000 ... CPUFREQ_TABLE_END]. After setted the freqs 2000,
the perf->state is 3 while the freqs_table's index should be 2.
So when call the get_cur_freq_on_cpu, the freqs value we get
is 2400.

So, make the freq_table store the same value as well.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 drivers/cpufreq/acpi-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index b0c18ed..93634a4 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -779,7 +779,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 
 	/* table init */
 	for (i = 0; i < perf->state_count; i++) {
-		if (i > 0 && perf->states[i].core_frequency >=
+		if (i > 0 && perf->states[i].core_frequency >
 		    data->freq_table[valid_states-1].frequency / 1000)
 			continue;
 
-- 
1.7.12

[PATCH 2/2] cpufreq: show the real avail freqs with the freq_table

[Wang Weidong]

Some time, the policy's max_freq and min_freq will change, so
the avail freqs is not from the cpuinfo_min to cpuinfo_max.
Just add the check for the freq_table.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 drivers/cpufreq/freq_table.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index df14766..4f86010 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -237,7 +237,9 @@ static ssize_t show_available_freqs(struct cpufreq_policy *policy, char *buf,
 		 * show_boost = false and driver_data != BOOST freq
 		 * display NON BOOST freqs
 		 */
-		if (show_boost ^ (pos->flags & CPUFREQ_BOOST_FREQ))
+		if (pos->frequency < policy->min ||
+		    pos->frequency > policy->max ||
+		    show_boost ^ (pos->flags & CPUFREQ_BOOST_FREQ))
 			continue;
 
 		count += sprintf(&buf[count], "%d ", pos->frequency);
-- 
1.7.12

Re: [PATCH 0/2] fix some problems for cpufreq

[Rafael J. Wysocki]

On Friday, November 28, 2014 10:43:37 AM Wang Weidong wrote:
> Hi Rafael and Viresh
> 
> Sorry to trouble you again. As for:
> "acpi-cpufreq: get the cur_freq from acpi_processor_performance states"
> I do it again, and add the other patch.
> 
> patch #1: acpi-cpufreq: make the freq_table store the same freq value
> 
> I think it can work. The set of available states which come
> from acpi won't change. Just like the power would be remove,
> the acpi driver will do that:
> call
>  ->acpi_processor_ppc_has_changed
>  ->cpufreq_update_policy
>  ->acpi_ppc_notifier_block.notifier_call
>    ->acpi_processor_ppc_notifier
>    ->cpufreq_verify_within_limits
> The progress will change the policy's min_freq and max_freq
> while it won't change the set of states(freq_tables).

OK, so the above information needs to go into the changelog of patch [1/2].
Also, please clarify the problem description in that changelog, it is very
difficult to understand the way it is now.

> patch #2: cpufreq: show the real avail freqs with the freq_table
> 
> when the min_freq and max_freq change, we should sync the availble
> freqs.

Why?  Do any other cpufreq drivers do that?

Rafael

Re: [PATCH 0/2] fix some problems for cpufreq

[Wang Weidong]

On 2014/11/29 9:26, Rafael J. Wysocki wrote:
> On Friday, November 28, 2014 10:43:37 AM Wang Weidong wrote:
>> Hi Rafael and Viresh
>>
>> Sorry to trouble you again. As for:
>> "acpi-cpufreq: get the cur_freq from acpi_processor_performance states"
>> I do it again, and add the other patch.
>>
>> patch #1: acpi-cpufreq: make the freq_table store the same freq value
>>
>> I think it can work. The set of available states which come
>> from acpi won't change. Just like the power would be remove,
>> the acpi driver will do that:
>> call
>>  ->acpi_processor_ppc_has_changed
>>  ->cpufreq_update_policy
>>  ->acpi_ppc_notifier_block.notifier_call
>>    ->acpi_processor_ppc_notifier
>>    ->cpufreq_verify_within_limits
>> The progress will change the policy's min_freq and max_freq
>> while it won't change the set of states(freq_tables).
> 
> OK, so the above information needs to go into the changelog of patch [1/2].
> Also, please clarify the problem description in that changelog, it is very
> difficult to understand the way it is now.
> 

sure, I should do it.

>> patch #2: cpufreq: show the real avail freqs with the freq_table
>>
>> when the min_freq and max_freq change, we should sync the availble
>> freqs.
> 
> Why?  Do any other cpufreq drivers do that?
> 

If some cpufreq drivers support several freqs like this:
1.05 Ghz 1.30Ghz 1.70GHz 2.10GHz 2.3GHz
           |                |
          min              max
So what the available freqs is 1.30GHz 1.70GHz 2.10GHz

when we do cpufreq-info or cat scaling_available_frequencies,
I think the available freqs table show only show these 3 value,
not all the values.

Wang,
Regards

> Rafael
> 
> 
> .
> 

Re: [PATCH 0/2] fix some problems for cpufreq

[Rafael J. Wysocki]

On Saturday, November 29, 2014 09:40:02 AM Wang Weidong wrote:
> On 2014/11/29 9:26, Rafael J. Wysocki wrote:
> > On Friday, November 28, 2014 10:43:37 AM Wang Weidong wrote:
> >> Hi Rafael and Viresh
> >>
> >> Sorry to trouble you again. As for:
> >> "acpi-cpufreq: get the cur_freq from acpi_processor_performance states"
> >> I do it again, and add the other patch.
> >>
> >> patch #1: acpi-cpufreq: make the freq_table store the same freq value
> >>
> >> I think it can work. The set of available states which come
> >> from acpi won't change. Just like the power would be remove,
> >> the acpi driver will do that:
> >> call
> >>  ->acpi_processor_ppc_has_changed
> >>  ->cpufreq_update_policy
> >>  ->acpi_ppc_notifier_block.notifier_call
> >>    ->acpi_processor_ppc_notifier
> >>    ->cpufreq_verify_within_limits
> >> The progress will change the policy's min_freq and max_freq
> >> while it won't change the set of states(freq_tables).
> > 
> > OK, so the above information needs to go into the changelog of patch [1/2].
> > Also, please clarify the problem description in that changelog, it is very
> > difficult to understand the way it is now.
> > 
> 
> sure, I should do it.
> 
> >> patch #2: cpufreq: show the real avail freqs with the freq_table
> >>
> >> when the min_freq and max_freq change, we should sync the availble
> >> freqs.
> > 
> > Why?  Do any other cpufreq drivers do that?
> > 
> 
> If some cpufreq drivers support several freqs like this:
> 1.05 Ghz 1.30Ghz 1.70GHz 2.10GHz 2.3GHz
>            |                |
>           min              max
> So what the available freqs is 1.30GHz 1.70GHz 2.10GHz
> 
> when we do cpufreq-info or cat scaling_available_frequencies,
> I think the available freqs table show only show these 3 value,
> not all the values.

That changes an existing user space interface, however, and the
only reason I can figure out from what you're saying is your personal
opinion.  This isn't a good enough reason, however.

What if there are utilities and scripts out there relying on the
current behavior?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/2] fix some problems for cpufreq

[Wang Weidong]

On 2014/11/30 6:30, Rafael J. Wysocki wrote:
> On Saturday, November 29, 2014 09:40:02 AM Wang Weidong wrote:
>> On 2014/11/29 9:26, Rafael J. Wysocki wrote:
>>> On Friday, November 28, 2014 10:43:37 AM Wang Weidong wrote:
>>>> Hi Rafael and Viresh
>>>>
>>>> Sorry to trouble you again. As for:
>>>> "acpi-cpufreq: get the cur_freq from acpi_processor_performance states"
>>>> I do it again, and add the other patch.
>>>>
>>>> patch #1: acpi-cpufreq: make the freq_table store the same freq value
>>>>
>>>> I think it can work. The set of available states which come
>>>> from acpi won't change. Just like the power would be remove,
>>>> the acpi driver will do that:
>>>> call
>>>>  ->acpi_processor_ppc_has_changed
>>>>  ->cpufreq_update_policy
>>>>  ->acpi_ppc_notifier_block.notifier_call
>>>>    ->acpi_processor_ppc_notifier
>>>>    ->cpufreq_verify_within_limits
>>>> The progress will change the policy's min_freq and max_freq
>>>> while it won't change the set of states(freq_tables).
>>>
>>> OK, so the above information needs to go into the changelog of patch [1/2].
>>> Also, please clarify the problem description in that changelog, it is very
>>> difficult to understand the way it is now.
>>>
>>
>> sure, I should do it.
>>
>>>> patch #2: cpufreq: show the real avail freqs with the freq_table
>>>>
>>>> when the min_freq and max_freq change, we should sync the availble
>>>> freqs.
>>>
>>> Why?  Do any other cpufreq drivers do that?
>>>
>>
>> If some cpufreq drivers support several freqs like this:
>> 1.05 Ghz 1.30Ghz 1.70GHz 2.10GHz 2.3GHz
>>            |                |
>>           min              max
>> So what the available freqs is 1.30GHz 1.70GHz 2.10GHz
>>
>> when we do cpufreq-info or cat scaling_available_frequencies,
>> I think the available freqs table show only show these 3 value,
>> not all the values.
> 
> That changes an existing user space interface, however, and the
> only reason I can figure out from what you're saying is your personal
> opinion.  This isn't a good enough reason, however.
> 
> What if there are utilities and scripts out there relying on the
> current behavior?
> 

No, there are not utilities and scripts relying on it. 

I just confuse that:
If the policy->min and policy-max is changed while it shows all available freqs
though scaling_available_frequencies. I can't set all freq-steps, only [policy->min, policy->max].
why should it show all the available freqs.

Although, it doesn't impact on us. So just ignore the patch#2. :)

Wang,
Regards

> 

[PATCH v2] acpi-cpufreq: make the freq_table store the same freq value

[Wang Weidong]

ACPI's P-states will report the acpi_processor_px *states to acpi-cpufreq.
When the states likes these:

[index:freq, 0:2400 1:2400 2:2000 3:1600...],

we will initialize the freq_tables to those:

[index:driver_data:freq, 0:0:2400, 1:2:2000 2:3,1600 ... CPUFREQ_TABLE_END]

So when set the freqs to 2000, the data->acpi_data->state is 2
(data->freq_table[1].driver_data), So when call get_cur_freq_on_cpu, we get
the freqs is data->freq_table[2].frequency,the value is 1600.

we can make the freq_table store the same value to fix this case problem.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
Change note: 

v2: to check weather the freq_table will be changeable and clarify the problem.

The set of available states which come from acpi won't change. Just like
the power would be remove, the acpi driver will do that:

 ->acpi_processor_ppc_has_changed
 ->cpufreq_update_policy
 ->acpi_ppc_notifier_block.notifier_call
   ->acpi_processor_ppc_notifier
   ->cpufreq_verify_within_limits

The progress will change the policy's min_freq and max_freq
while it won't change the set of states(freq_tables).

---
 drivers/cpufreq/acpi-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index b0c18ed..93634a4 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -779,7 +779,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 
 	/* table init */
 	for (i = 0; i < perf->state_count; i++) {
-		if (i > 0 && perf->states[i].core_frequency >=
+		if (i > 0 && perf->states[i].core_frequency >
 		    data->freq_table[valid_states-1].frequency / 1000)
 			continue;
 
-- 
1.7.12

Re: [PATCH 0/2] fix some problems for cpufreq

[Viresh Kumar]

On 30 November 2014 at 13:53, Wang Weidong <wangweidong1@huawei.com> wrote:
> No, there are not utilities and scripts relying on it.

How can you be so sure about it ? There might be scripts/utils you
aren't aware of
and are depending on this..

> I just confuse that:
> If the policy->min and policy-max is changed while it shows all available freqs
> though scaling_available_frequencies. I can't set all freq-steps, only [policy->min, policy->max].
> why should it show all the available freqs.

That's not the only purpose of those frequencies there. It shows list
of all possible
frequencies. Now, there can be issues if those lists are updated.

Suppose somebody just played with the min/max frequency, now how would anybody
come to know about the frequencies available above/below the
user-max/min frequency?

So as you mentioned in your example above: User space would never know about

1.05 Ghz and 2.3GHz anymore.. Unless you remember it or save it somewhere.

> Although, it doesn't impact on us. So just ignore the patch#2. :)

Probably yes.

Re: [PATCH 0/2] fix some problems for cpufreq

[Wang Weidong]

On 2014/12/2 12:38, Viresh Kumar wrote:
> On 30 November 2014 at 13:53, Wang Weidong <wangweidong1@huawei.com> wrote:
>> No, there are not utilities and scripts relying on it.
> 
> How can you be so sure about it ? There might be scripts/utils you
> aren't aware of
> and are depending on this..
> 
>> I just confuse that:
>> If the policy->min and policy-max is changed while it shows all available freqs
>> though scaling_available_frequencies. I can't set all freq-steps, only [policy->min, policy->max].
>> why should it show all the available freqs.
> 
> That's not the only purpose of those frequencies there. It shows list
> of all possible
> frequencies. Now, there can be issues if those lists are updated.
> 
> Suppose somebody just played with the min/max frequency, now how would anybody
> come to know about the frequencies available above/below the
> user-max/min frequency?
> 
> So as you mentioned in your example above: User space would never know about
> 
> 1.05 Ghz and 2.3GHz anymore.. Unless you remember it or save it somewhere.
> 

Nice, Thanks for your reply.

Got it.

Wang,
Regards

>> Although, it doesn't impact on us. So just ignore the patch#2. :)
> 
> Probably yes.
> 
> 

[PATCH RESEND v2] acpi-cpufreq: make the freq_table store the same freq value

[Wang Weidong]

ACPI's P-states will report the acpi_processor_px *states to acpi-cpufreq.
When the states likes these:

[index:freq, 0:2400 1:2400 2:2000 3:1600...],

we will initialize the freq_tables to those:

[index:driver_data:freq, 0:0:2400, 1:2:2000 2:3,1600 ... CPUFREQ_TABLE_END]

So when set the freqs to 2000, the data->acpi_data->state is 2
(data->freq_table[1].driver_data), So when call get_cur_freq_on_cpu, we get
the freqs is data->freq_table[2].frequency,the value is 1600.

we can make the freq_table store the same value to fix this case problem.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
Change note: 

v2: to check weather the freq_table will be changeable and clarify the problem.

The set of available states which come from acpi won't change. Just like
the power would be remove, the acpi driver will do that:

 ->acpi_processor_ppc_has_changed
 ->cpufreq_update_policy
 ->acpi_ppc_notifier_block.notifier_call
   ->acpi_processor_ppc_notifier
   ->cpufreq_verify_within_limits

The progress will change the policy's min_freq and max_freq
while it won't change the set of states(freq_tables).

---
 drivers/cpufreq/acpi-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index b0c18ed..93634a4 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -779,7 +779,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 
 	/* table init */
 	for (i = 0; i < perf->state_count; i++) {
-		if (i > 0 && perf->states[i].core_frequency >=
+		if (i > 0 && perf->states[i].core_frequency >
 		    data->freq_table[valid_states-1].frequency / 1000)
 			continue;
 
-- 
1.7.12




.