[PATCH 1/2] cpufreq: stats: Declare freq-attr right after their callbacks

[PATCH 1/2] cpufreq: stats: Declare freq-attr right after their callbacks

[Viresh Kumar]

Freq attribute for "trans_table" is defined right after its callback
(without any blank line between them), but the others are defined
separately later on. Keep this consistent and define all attributes
right after their callbacks.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 1572129844a5..941e63e3e652 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -51,6 +51,7 @@ static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
 {
 	return sprintf(buf, "%d\n", policy->stats->total_trans);
 }
+cpufreq_freq_attr_ro(total_trans);
 
 static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 {
@@ -69,6 +70,7 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 	}
 	return len;
 }
+cpufreq_freq_attr_ro(time_in_state);
 
 static ssize_t store_reset(struct cpufreq_policy *policy, const char *buf,
 			   size_t count)
@@ -77,6 +79,7 @@ static ssize_t store_reset(struct cpufreq_policy *policy, const char *buf,
 	cpufreq_stats_clear_table(policy->stats);
 	return count;
 }
+cpufreq_freq_attr_wo(reset);
 
 static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 {
@@ -126,10 +129,6 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 }
 cpufreq_freq_attr_ro(trans_table);
 
-cpufreq_freq_attr_ro(total_trans);
-cpufreq_freq_attr_ro(time_in_state);
-cpufreq_freq_attr_wo(reset);
-
 static struct attribute *default_attrs[] = {
 	&total_trans.attr,
 	&time_in_state.attr,
-- 
2.20.1.321.g9e740568ce00

[PATCH 2/2] cpufreq: stats: Fix concurrency issues while resetting stats

[Viresh Kumar]

It is possible for cpufreq_stats_clear_table() and
cpufreq_stats_record_transition() to get called concurrently and they
will try to update same variables simultaneously and may lead to
corruption of data.

Prevent that with the help of existing spinlock.

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

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 941e63e3e652..e2db5581489a 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -31,20 +31,20 @@ static void cpufreq_stats_update(struct cpufreq_stats *stats)
 {
 	unsigned long long cur_time = get_jiffies_64();
 
-	spin_lock(&cpufreq_stats_lock);
 	stats->time_in_state[stats->last_index] += cur_time - stats->last_time;
 	stats->last_time = cur_time;
-	spin_unlock(&cpufreq_stats_lock);
 }
 
 static void cpufreq_stats_clear_table(struct cpufreq_stats *stats)
 {
 	unsigned int count = stats->max_state;
 
+	spin_lock(&cpufreq_stats_lock);
 	memset(stats->time_in_state, 0, count * sizeof(u64));
 	memset(stats->trans_table, 0, count * count * sizeof(int));
 	stats->last_time = get_jiffies_64();
 	stats->total_trans = 0;
+	spin_unlock(&cpufreq_stats_lock);
 }
 
 static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
@@ -62,7 +62,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 	if (policy->fast_switch_enabled)
 		return 0;
 
+	spin_lock(&cpufreq_stats_lock);
 	cpufreq_stats_update(stats);
+	spin_unlock(&cpufreq_stats_lock);
+
 	for (i = 0; i < stats->state_num; i++) {
 		len += sprintf(buf + len, "%u %llu\n", stats->freq_table[i],
 			(unsigned long long)
@@ -239,9 +242,11 @@ void cpufreq_stats_record_transition(struct cpufreq_policy *policy,
 	if (old_index == -1 || new_index == -1 || old_index == new_index)
 		return;
 
+	spin_lock(&cpufreq_stats_lock);
 	cpufreq_stats_update(stats);
 
 	stats->last_index = new_index;
 	stats->trans_table[old_index * stats->max_state + new_index]++;
 	stats->total_trans++;
+	spin_unlock(&cpufreq_stats_lock);
 }
-- 
2.20.1.321.g9e740568ce00

Re: [PATCH 2/2] cpufreq: stats: Fix concurrency issues while resetting stats

[Matthias Kaehlcke]

On Fri, Feb 01, 2019 at 11:45:45AM +0530, Viresh Kumar wrote:
> It is possible for cpufreq_stats_clear_table() and
> cpufreq_stats_record_transition() to get called concurrently and they
> will try to update same variables simultaneously and may lead to
> corruption of data.
> 
> Prevent that with the help of existing spinlock.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 941e63e3e652..e2db5581489a 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -31,20 +31,20 @@ static void cpufreq_stats_update(struct cpufreq_stats *stats)
>  {
>  	unsigned long long cur_time = get_jiffies_64();
>  
> -	spin_lock(&cpufreq_stats_lock);
>  	stats->time_in_state[stats->last_index] += cur_time - stats->last_time;
>  	stats->last_time = cur_time;
> -	spin_unlock(&cpufreq_stats_lock);
>  }
>  
>  static void cpufreq_stats_clear_table(struct cpufreq_stats *stats)
>  {
>  	unsigned int count = stats->max_state;
>  
> +	spin_lock(&cpufreq_stats_lock);
>  	memset(stats->time_in_state, 0, count * sizeof(u64));
>  	memset(stats->trans_table, 0, count * count * sizeof(int));
>  	stats->last_time = get_jiffies_64();
>  	stats->total_trans = 0;
> +	spin_unlock(&cpufreq_stats_lock);
>  }
>  
>  static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
> @@ -62,7 +62,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
>  	if (policy->fast_switch_enabled)
>  		return 0;
>  
> +	spin_lock(&cpufreq_stats_lock);
>  	cpufreq_stats_update(stats);
> +	spin_unlock(&cpufreq_stats_lock);
> +
>  	for (i = 0; i < stats->state_num; i++) {
>  		len += sprintf(buf + len, "%u %llu\n", stats->freq_table[i],
>  			(unsigned long long)
> @@ -239,9 +242,11 @@ void cpufreq_stats_record_transition(struct cpufreq_policy *policy,
>  	if (old_index == -1 || new_index == -1 || old_index == new_index)
>  		return;
>  
> +	spin_lock(&cpufreq_stats_lock);
>  	cpufreq_stats_update(stats);
>  
>  	stats->last_index = new_index;
>  	stats->trans_table[old_index * stats->max_state + new_index]++;
>  	stats->total_trans++;
> +	spin_unlock(&cpufreq_stats_lock);
>  }

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 1/2] cpufreq: stats: Declare freq-attr right after their callbacks

[Rafael J. Wysocki]

On Friday, February 1, 2019 7:15:44 AM CET Viresh Kumar wrote:
> Freq attribute for "trans_table" is defined right after its callback
> (without any blank line between them), but the others are defined
> separately later on. Keep this consistent and define all attributes
> right after their callbacks.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 1572129844a5..941e63e3e652 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -51,6 +51,7 @@ static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
>  {
>  	return sprintf(buf, "%d\n", policy->stats->total_trans);
>  }
> +cpufreq_freq_attr_ro(total_trans);
>  
>  static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
>  {
> @@ -69,6 +70,7 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
>  	}
>  	return len;
>  }
> +cpufreq_freq_attr_ro(time_in_state);
>  
>  static ssize_t store_reset(struct cpufreq_policy *policy, const char *buf,
>  			   size_t count)
> @@ -77,6 +79,7 @@ static ssize_t store_reset(struct cpufreq_policy *policy, const char *buf,
>  	cpufreq_stats_clear_table(policy->stats);
>  	return count;
>  }
> +cpufreq_freq_attr_wo(reset);
>  
>  static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
>  {
> @@ -126,10 +129,6 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
>  }
>  cpufreq_freq_attr_ro(trans_table);
>  
> -cpufreq_freq_attr_ro(total_trans);
> -cpufreq_freq_attr_ro(time_in_state);
> -cpufreq_freq_attr_wo(reset);
> -
>  static struct attribute *default_attrs[] = {
>  	&total_trans.attr,
>  	&time_in_state.attr,
> 

Applied, along with the [2/2], thanks!