The loops which are used to perform lookups in CPU frequency tables in
cpu_cooling and the Exynos thermal driver do not update the loop counter
if they encounter an invalid table entry, leading to an infinite loop in
that case.
Signed-off-by: Andrew Bresticker <[email protected]>
---
drivers/thermal/cpu_cooling.c | 19 ++++++++++---------
drivers/thermal/exynos_thermal.c | 8 ++++----
2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 836828e..e6db441 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -124,14 +124,14 @@ static int is_cpufreq_valid(int cpu)
static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level)
{
int ret = 0, i = 0;
- unsigned long level_index;
+ unsigned long level_index = 0;
bool descend = false;
struct cpufreq_frequency_table *table =
cpufreq_frequency_get_table(cpu);
if (!table)
return ret;
- while (table[i].frequency != CPUFREQ_TABLE_END) {
+ for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) {
if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
continue;
@@ -143,24 +143,25 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level)
}
/*return if level matched and table in descending order*/
- if (descend && i == level)
+ if (descend && level_index == level)
return table[i].frequency;
- i++;
+ level_index++;
}
i--;
+ level_index--;
- if (level > i || descend)
+ if (level > level_index || descend)
return ret;
- level_index = i - level;
+ level = level_index - level;
/*Scan the table in reverse order and match the level*/
- while (i >= 0) {
+ for (; i >= 0; i--) {
if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
continue;
/*return if level matched*/
- if (i == level_index)
+ if (level_index == level)
return table[i].frequency;
- i--;
+ level_index--;
}
return ret;
}
diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c
index d5e6267..524b2a0 100644
--- a/drivers/thermal/exynos_thermal.c
+++ b/drivers/thermal/exynos_thermal.c
@@ -237,7 +237,7 @@ static int exynos_get_crit_temp(struct thermal_zone_device *thermal,
static int exynos_get_frequency_level(unsigned int cpu, unsigned int freq)
{
- int i = 0, ret = -EINVAL;
+ int i, level = 0, ret = -EINVAL;
struct cpufreq_frequency_table *table = NULL;
#ifdef CONFIG_CPU_FREQ
table = cpufreq_frequency_get_table(cpu);
@@ -245,12 +245,12 @@ static int exynos_get_frequency_level(unsigned int cpu, unsigned int freq)
if (!table)
return ret;
- while (table[i].frequency != CPUFREQ_TABLE_END) {
+ for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) {
if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
continue;
if (table[i].frequency == freq)
- return i;
- i++;
+ return level;
+ level++;
}
return ret;
}
--
1.8.1.3
Hi Andrew,
On 08-04-2013 19:54, Andrew Bresticker wrote:
> The loops which are used to perform lookups in CPU frequency tables in
> cpu_cooling and the Exynos thermal driver do not update the loop counter
> if they encounter an invalid table entry, leading to an infinite loop in
> that case.
>
> Signed-off-by: Andrew Bresticker <[email protected]>
> ---
> drivers/thermal/cpu_cooling.c | 19 ++++++++++---------
> drivers/thermal/exynos_thermal.c | 8 ++++----
> 2 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index 836828e..e6db441 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -124,14 +124,14 @@ static int is_cpufreq_valid(int cpu)
> static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level)
> {
> int ret = 0, i = 0;
> - unsigned long level_index;
> + unsigned long level_index = 0;
> bool descend = false;
> struct cpufreq_frequency_table *table =
> cpufreq_frequency_get_table(cpu);
> if (!table)
> return ret;
>
> - while (table[i].frequency != CPUFREQ_TABLE_END) {
> + for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) {
> if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
> continue;
Wouldn't be easier to just increase the index i before doing a continue?
>
> @@ -143,24 +143,25 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level)
> }
>
> /*return if level matched and table in descending order*/
> - if (descend && i == level)
> + if (descend && level_index == level)
> return table[i].frequency;
What this has to do with the patch description? Besides why would you be
comparing level against 0 all the time (you have initialized level_index
to 0 at this point).
> - i++;
> + level_index++;
level_index wont be updated in case of INVALID entry.
> }
> i--;
> + level_index--;
>
> - if (level > i || descend)
> + if (level > level_index || descend)
> return ret;
> - level_index = i - level;
> + level = level_index - level;
>
> /*Scan the table in reverse order and match the level*/
> - while (i >= 0) {
> + for (; i >= 0; i--) {
> if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
> continue;
> /*return if level matched*/
> - if (i == level_index)
> + if (level_index == level)
> return table[i].frequency;
> - i--;
> + level_index--;
> }
I believe you do more than what you have described in your intention
under you patch description
Can you please split your patch into smaller changes?
> return ret;
> }
> diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c
> index d5e6267..524b2a0 100644
> --- a/drivers/thermal/exynos_thermal.c
> +++ b/drivers/thermal/exynos_thermal.c
> @@ -237,7 +237,7 @@ static int exynos_get_crit_temp(struct thermal_zone_device *thermal,
>
> static int exynos_get_frequency_level(unsigned int cpu, unsigned int freq)
> {
> - int i = 0, ret = -EINVAL;
> + int i, level = 0, ret = -EINVAL;
> struct cpufreq_frequency_table *table = NULL;
> #ifdef CONFIG_CPU_FREQ
> table = cpufreq_frequency_get_table(cpu);
> @@ -245,12 +245,12 @@ static int exynos_get_frequency_level(unsigned int cpu, unsigned int freq)
> if (!table)
> return ret;
>
> - while (table[i].frequency != CPUFREQ_TABLE_END) {
> + for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) {
> if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
> continue;
> if (table[i].frequency == freq)
> - return i;
> - i++;
> + return level;
> + level++;
Can you please send a separate patch on this driver instead?
> }
> return ret;
> }
>
Hi Eduardo,
On Tue, Apr 9, 2013 at 7:55 AM, Eduardo Valentin
<[email protected]> wrote:
> Hi Andrew,
>
>
> On 08-04-2013 19:54, Andrew Bresticker wrote:
>>
>> The loops which are used to perform lookups in CPU frequency tables in
>> cpu_cooling and the Exynos thermal driver do not update the loop counter
>> if they encounter an invalid table entry, leading to an infinite loop in
>> that case.
>>
>> Signed-off-by: Andrew Bresticker <[email protected]>
>> ---
>> drivers/thermal/cpu_cooling.c | 19 ++++++++++---------
>> drivers/thermal/exynos_thermal.c | 8 ++++----
>> 2 files changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>> index 836828e..e6db441 100644
>> --- a/drivers/thermal/cpu_cooling.c
>> +++ b/drivers/thermal/cpu_cooling.c
>> @@ -124,14 +124,14 @@ static int is_cpufreq_valid(int cpu)
>> static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long
>> level)
>> {
>> int ret = 0, i = 0;
>> - unsigned long level_index;
>> + unsigned long level_index = 0;
>> bool descend = false;
>> struct cpufreq_frequency_table *table =
>> cpufreq_frequency_get_table(cpu);
>> if (!table)
>> return ret;
>>
>> - while (table[i].frequency != CPUFREQ_TABLE_END) {
>> + for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) {
>> if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
>> continue;
>
> Wouldn't be easier to just increase the index i before doing a continue?
I think this is cleaner. The code is iterating through an array -- it
should be a for loop.
>
>
>>
>> @@ -143,24 +143,25 @@ static unsigned int get_cpu_frequency(unsigned int
>> cpu, unsigned long level)
>> }
>>
>> /*return if level matched and table in descending order*/
>> - if (descend && i == level)
>> + if (descend && level_index == level)
>> return table[i].frequency;
>
>
> What this has to do with the patch description?
I'm using level_index as the counter of valid frequencies, where as i
is the index into the array. If there are invalid entries, they are
not necessarily equal. The point of this function is to find the
level-th *valid* frequency in the table.
> Besides why would you be comparing level against 0 all the time (you have
> initialized level_index to 0 at this point).
Huh? level_index is clearly incremented below...
>
>> - i++;
>> + level_index++;
>
>
> level_index wont be updated in case of INVALID entry.
That's the point.
>
>
>> }
>> i--;
>> + level_index--;
>>
>> - if (level > i || descend)
>> + if (level > level_index || descend)
>> return ret;
>> - level_index = i - level;
>> + level = level_index - level;
>>
>> /*Scan the table in reverse order and match the level*/
>> - while (i >= 0) {
>> + for (; i >= 0; i--) {
>> if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
>> continue;
>> /*return if level matched*/
>> - if (i == level_index)
>> + if (level_index == level)
>> return table[i].frequency;
>> - i--;
>> + level_index--;
>> }
>
>
> I believe you do more than what you have described in your intention under
> you patch description
I disagree. I'm fixing the loop so that it properly handles invalid
entries and thus the infinite loop problem I mention in the commit
message.
> Can you please split your patch into smaller changes?
I don't think there is a need for separate patches to cpu_cooling.c.
>
>> return ret;
>> }
>> diff --git a/drivers/thermal/exynos_thermal.c
>> b/drivers/thermal/exynos_thermal.c
>> index d5e6267..524b2a0 100644
>> --- a/drivers/thermal/exynos_thermal.c
>> +++ b/drivers/thermal/exynos_thermal.c
>> @@ -237,7 +237,7 @@ static int exynos_get_crit_temp(struct
>> thermal_zone_device *thermal,
>>
>> static int exynos_get_frequency_level(unsigned int cpu, unsigned int
>> freq)
>> {
>> - int i = 0, ret = -EINVAL;
>> + int i, level = 0, ret = -EINVAL;
>> struct cpufreq_frequency_table *table = NULL;
>> #ifdef CONFIG_CPU_FREQ
>> table = cpufreq_frequency_get_table(cpu);
>> @@ -245,12 +245,12 @@ static int exynos_get_frequency_level(unsigned int
>> cpu, unsigned int freq)
>> if (!table)
>> return ret;
>>
>> - while (table[i].frequency != CPUFREQ_TABLE_END) {
>> + for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) {
>> if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
>> continue;
>> if (table[i].frequency == freq)
>> - return i;
>> - i++;
>> + return level;
>> + level++;
>
>
> Can you please send a separate patch on this driver instead?
Sure.
>
>
>
>> }
>> return ret;
>> }
>>
>
Thanks,
Andrew
On 09-04-2013 13:02, Andrew Bresticker wrote:
> Hi Eduardo,
>
> On Tue, Apr 9, 2013 at 7:55 AM, Eduardo Valentin
> <[email protected]> wrote:
>> Hi Andrew,
>>
>>
>> On 08-04-2013 19:54, Andrew Bresticker wrote:
>>>
>>> The loops which are used to perform lookups in CPU frequency tables in
>>> cpu_cooling and the Exynos thermal driver do not update the loop counter
>>> if they encounter an invalid table entry, leading to an infinite loop in
>>> that case.
>>>
>>> Signed-off-by: Andrew Bresticker <[email protected]>
>>> ---
>>> drivers/thermal/cpu_cooling.c | 19 ++++++++++---------
>>> drivers/thermal/exynos_thermal.c | 8 ++++----
>>> 2 files changed, 14 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
>>> index 836828e..e6db441 100644
>>> --- a/drivers/thermal/cpu_cooling.c
>>> +++ b/drivers/thermal/cpu_cooling.c
>>> @@ -124,14 +124,14 @@ static int is_cpufreq_valid(int cpu)
>>> static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long
>>> level)
>>> {
>>> int ret = 0, i = 0;
>>> - unsigned long level_index;
>>> + unsigned long level_index = 0;
>>> bool descend = false;
>>> struct cpufreq_frequency_table *table =
>>> cpufreq_frequency_get_table(cpu);
>>> if (!table)
>>> return ret;
>>>
>>> - while (table[i].frequency != CPUFREQ_TABLE_END) {
>>> + for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) {
>>> if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
>>> continue;
>>
>> Wouldn't be easier to just increase the index i before doing a continue?
>
> I think this is cleaner. The code is iterating through an array -- it
> should be a for loop.
>
>>
>>
>>>
>>> @@ -143,24 +143,25 @@ static unsigned int get_cpu_frequency(unsigned int
>>> cpu, unsigned long level)
>>> }
>>>
>>> /*return if level matched and table in descending order*/
>>> - if (descend && i == level)
>>> + if (descend && level_index == level)
>>> return table[i].frequency;
>>
>>
>> What this has to do with the patch description?
>
> I'm using level_index as the counter of valid frequencies, where as i
> is the index into the array. If there are invalid entries, they are
> not necessarily equal. The point of this function is to find the
> level-th *valid* frequency in the table.
>
>> Besides why would you be comparing level against 0 all the time (you have
>> initialized level_index to 0 at this point).
>
> Huh? level_index is clearly incremented below...
>
>>
>>> - i++;
>>> + level_index++;
>>
>>
>> level_index wont be updated in case of INVALID entry.
>
> That's the point.
>
>>
>>
>>> }
>>> i--;
>>> + level_index--;
>>>
>>> - if (level > i || descend)
>>> + if (level > level_index || descend)
>>> return ret;
>>> - level_index = i - level;
>>> + level = level_index - level;
>>>
>>> /*Scan the table in reverse order and match the level*/
>>> - while (i >= 0) {
>>> + for (; i >= 0; i--) {
>>> if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
>>> continue;
>>> /*return if level matched*/
>>> - if (i == level_index)
>>> + if (level_index == level)
>>> return table[i].frequency;
>>> - i--;
>>> + level_index--;
>>> }
>>
>>
>> I believe you do more than what you have described in your intention under
>> you patch description
>
> I disagree. I'm fixing the loop so that it properly handles invalid
> entries and thus the infinite loop problem I mention in the commit
> message.
>
In this case, I believe you should also rephrase your patch description,
explaining that you are also fixing a role for each index.
>> Can you please split your patch into smaller changes?
>
> I don't think there is a need for separate patches to cpu_cooling.c.
>
You do two things in this change on cpu_cooling.c: (1) fix the case
where the loop is kept running indefinitely. (2) Reserve a specific role
for each index in this function.
For this reason, I suggested doing one thing per patch and splitting
this change into two for better review process. Having that split with a
good description for each change makes everyone life easier, don t you
think?
>>
>>> return ret;
>>> }
>>> diff --git a/drivers/thermal/exynos_thermal.c
>>> b/drivers/thermal/exynos_thermal.c
>>> index d5e6267..524b2a0 100644
>>> --- a/drivers/thermal/exynos_thermal.c
>>> +++ b/drivers/thermal/exynos_thermal.c
>>> @@ -237,7 +237,7 @@ static int exynos_get_crit_temp(struct
>>> thermal_zone_device *thermal,
>>>
>>> static int exynos_get_frequency_level(unsigned int cpu, unsigned int
>>> freq)
>>> {
>>> - int i = 0, ret = -EINVAL;
>>> + int i, level = 0, ret = -EINVAL;
>>> struct cpufreq_frequency_table *table = NULL;
>>> #ifdef CONFIG_CPU_FREQ
>>> table = cpufreq_frequency_get_table(cpu);
>>> @@ -245,12 +245,12 @@ static int exynos_get_frequency_level(unsigned int
>>> cpu, unsigned int freq)
>>> if (!table)
>>> return ret;
>>>
>>> - while (table[i].frequency != CPUFREQ_TABLE_END) {
>>> + for (i = 0; table[i].frequency != CPUFREQ_TABLE_END; i++) {
>>> if (table[i].frequency == CPUFREQ_ENTRY_INVALID)
>>> continue;
>>> if (table[i].frequency == freq)
>>> - return i;
>>> - i++;
>>> + return level;
>>> + level++;
>>
>>
>> Can you please send a separate patch on this driver instead?
>
> Sure.
>
>>
>>
>>
>>> }
>>> return ret;
>>> }
>>>
>>
>
> Thanks,
> Andrew
>
>
> You do two things in this change on cpu_cooling.c: (1) fix the case where
> the loop is kept running indefinitely. (2) Reserve a specific role for each
> index in this function.
So the issue is that the changes are not independent. With just the
fix for the infinite loop, get_cpu_frequency() is still completely
broken because it is not interpreting the level correctly and will
return the wrong frequency (and thus not throttling correctly, which
is bad). Perhaps the commit should be more general, like "fix
handling of invalid frequency table entries"? What do you think?
Thanks,
Andrew
On 09-04-2013 14:27, Andrew Bresticker wrote:
>> You do two things in this change on cpu_cooling.c: (1) fix the case where
>> the loop is kept running indefinitely. (2) Reserve a specific role for each
>> index in this function.
>
> So the issue is that the changes are not independent. With just the
> fix for the infinite loop, get_cpu_frequency() is still completely
> broken because it is not interpreting the level correctly and will
> return the wrong frequency (and thus not throttling correctly, which
> is bad). Perhaps the commit should be more general, like "fix
> handling of invalid frequency table entries"? What do you think?
It fits better. I am OK if we improve the commit title and description,
providing better explanation of what is the issue (you have done
already) and how what needed to be done to fix it.
>
> Thanks,
> Andrew
>
>