[PATCH V3 00/16] cpufreq: stats: cleanups

[PATCH V3 00/16] cpufreq: stats: cleanups

[Viresh Kumar]

Hi Rafael,

This is V3 of the stats cleanup I sent earlier. Few things are improved based on
the feedback received (mostly from you). Please see if it looks any better.

Pushed here:
git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/stats/cleanups

V1->V2:
- New patches:
  cpufreq: stats: remove unused cpufreq_stats_attribute
  cpufreq: stats: initialize 'cur_time' on its definition
  cpufreq: stats: rename 'struct cpufreq_stats' objects as 'stats'
  cpufreq: stats: drop unnecessary locking

- Patches dropped:
  cpufreq: stats: replace spinlock with mutex
  cpufreq: stats: Fix locking
  cpufreq: stats: call cpufreq_stats_update() with locks held

- Logs improved
- don't return error values notifiers on missing policy/stats
- rename stats_data as stats and make it cpufreq_stats pointer instead of void *

Viresh Kumar (16):
  cpufreq: stats: Improve module description string
  cpufreq: stats: return -EEXIST when stats are already allocated
  cpufreq: stats: remove unused cpufreq_stats_attribute
  cpufreq: stats: initialize 'cur_time' on its definition
  cpufreq: stats: don't check for freq table while freeing stats
  cpufreq: stats: pass 'stat' to cpufreq_stats_update()
  cpufreq: stats: get rid of per-cpu cpufreq_stats_table
  cpufreq: stats: rename 'struct cpufreq_stats' objects as 'stats'
  cpufreq: Remove (now) unused 'last_cpu' from struct cpufreq_policy
  cpufreq: stats: drop 'cpu' field of struct cpufreq_stats
  cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications
  cpufreq: stats: create sysfs group once we are ready
  cpufreq: stats: time_in_state can't be NULL in cpufreq_stats_update()
  cpufreq: stats: don't update stats from show_trans_table()
  cpufreq: stats: don't update stats on false notifiers
  cpufreq: stats: drop unnecessary locking

 drivers/cpufreq/cpufreq.c       |   6 --
 drivers/cpufreq/cpufreq_stats.c | 211 ++++++++++++++++++----------------------
 include/linux/cpufreq.h         |  10 +-
 3 files changed, 98 insertions(+), 129 deletions(-)

-- 
2.2.0

[PATCH V3 01/16] cpufreq: stats: Improve module description string

[Viresh Kumar]

The MODULE_DESCRIPTION() string is just too long and then is broken into
multiple lines just to make checkpatch happy.

Rewrite it to make it more precise.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 0cd9b4dcef99..80801f880dd8 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -374,8 +374,7 @@ static void __exit cpufreq_stats_exit(void)
 }
 
 MODULE_AUTHOR("Zou Nan hai <nanhai.zou@intel.com>");
-MODULE_DESCRIPTION("'cpufreq_stats' - A driver to export cpufreq stats "
-				"through sysfs filesystem");
+MODULE_DESCRIPTION("Export cpufreq stats via sysfs");
 MODULE_LICENSE("GPL");
 
 module_init(cpufreq_stats_init);
-- 
2.2.0

[PATCH V3 02/16] cpufreq: stats: return -EEXIST when stats are already allocated

[Viresh Kumar]

__cpufreq_stats_create_table() is called from:

- cpufreq notifier on creation of a new policy. Stats will always be NULL here.
- cpufreq_stats_init() for all CPUs as cpufreq-stats might have been initialized
  after cpufreq driver. For any policy, 'stats' will be NULL for the first cpu
  only and will be valid for all other CPUs managed by the same policy.

While we return for other CPUs, we don't return the right error value. Its not
that we would fail with -EBUSY. But generally, this is what these return values
mean:
- EBUSY: we are busy right now, try again. And the retry attempt might be
  immediate.
- EEXIST: We already have what you are trying to create and there is no need to
  create it again, and so no more tries are required.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 80801f880dd8..d2299ca2fc2c 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -192,8 +192,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	if (unlikely(!table))
 		return 0;
 
+	/* stats already initialized */
 	if (per_cpu(cpufreq_stats_table, cpu))
-		return -EBUSY;
+		return -EEXIST;
+
 	stat = kzalloc(sizeof(*stat), GFP_KERNEL);
 	if ((stat) == NULL)
 		return -ENOMEM;
-- 
2.2.0

[PATCH V3 03/16] cpufreq: stats: remove unused cpufreq_stats_attribute

[Viresh Kumar]

It was never used, but is there since the first commit. Remove it.

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

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index d2299ca2fc2c..14f8d858a63d 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -33,11 +33,6 @@ struct cpufreq_stats {
 
 static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table);
 
-struct cpufreq_stats_attribute {
-	struct attribute attr;
-	ssize_t(*show) (struct cpufreq_stats *, char *);
-};
-
 static int cpufreq_stats_update(unsigned int cpu)
 {
 	struct cpufreq_stats *stat;
-- 
2.2.0

[PATCH V3 04/16] cpufreq: stats: initialize 'cur_time' on its definition

[Viresh Kumar]

'cur_time' is defined in the first line and is then assigned a value in the next
line. Initialize it while defining it.

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

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 14f8d858a63d..21bec770569d 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -36,9 +36,8 @@ static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table);
 static int cpufreq_stats_update(unsigned int cpu)
 {
 	struct cpufreq_stats *stat;
-	unsigned long long cur_time;
+	unsigned long long cur_time = get_jiffies_64();
 
-	cur_time = get_jiffies_64();
 	spin_lock(&cpufreq_stats_lock);
 	stat = per_cpu(cpufreq_stats_table, cpu);
 	if (stat->time_in_state)
-- 
2.2.0

[PATCH V3 05/16] cpufreq: stats: don't check for freq table while freeing stats

[Viresh Kumar]

While we allocate stats, we do need to check if freq-table is present or not as
we need to use it then. But while freeing stats, all we need to know is if stats
holds a valid pointer value. There is no use of testing if cpufreq table is
present or not.

Don't check it.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 21bec770569d..e9d68420b876 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -168,8 +168,7 @@ static void cpufreq_stats_free_table(unsigned int cpu)
 	if (!policy)
 		return;
 
-	if (cpufreq_frequency_get_table(policy->cpu))
-		__cpufreq_stats_free_table(policy);
+	__cpufreq_stats_free_table(policy);
 
 	cpufreq_cpu_put(policy);
 }
-- 
2.2.0

[PATCH V3 06/16] cpufreq: stats: pass 'stat' to cpufreq_stats_update()

[Viresh Kumar]

It is better to pass a struct cpufreq_stats pointer to cpufreq_stats_update()
instead of a CPU number, because that's all it needs.

Even if we pass a cpu number to cpufreq_stats_update(), it reads the per-cpu
variable to get 'stats' out of it. So we are doing these operations
unnecessarily:
- First getting the cpu number to pass to cpufreq_stats_update(), stat->cpu.
- And then getting stats from the cpu, per_cpu(cpufreq_stats_table, cpu).

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index e9d68420b876..6c234f548601 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -33,13 +33,11 @@ struct cpufreq_stats {
 
 static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table);
 
-static int cpufreq_stats_update(unsigned int cpu)
+static int cpufreq_stats_update(struct cpufreq_stats *stat)
 {
-	struct cpufreq_stats *stat;
 	unsigned long long cur_time = get_jiffies_64();
 
 	spin_lock(&cpufreq_stats_lock);
-	stat = per_cpu(cpufreq_stats_table, cpu);
 	if (stat->time_in_state)
 		stat->time_in_state[stat->last_index] +=
 			cur_time - stat->last_time;
@@ -64,7 +62,7 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
 	if (!stat)
 		return 0;
-	cpufreq_stats_update(stat->cpu);
+	cpufreq_stats_update(stat);
 	for (i = 0; i < stat->state_num; i++) {
 		len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
 			(unsigned long long)
@@ -82,7 +80,7 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
 	if (!stat)
 		return 0;
-	cpufreq_stats_update(stat->cpu);
+	cpufreq_stats_update(stat);
 	len += snprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
 	len += snprintf(buf + len, PAGE_SIZE - len, "         : ");
 	for (i = 0; i < stat->state_num; i++) {
@@ -307,7 +305,7 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 	if (old_index == -1 || new_index == -1)
 		return 0;
 
-	cpufreq_stats_update(freq->cpu);
+	cpufreq_stats_update(stat);
 
 	if (old_index == new_index)
 		return 0;
-- 
2.2.0

[PATCH V3 07/16] cpufreq: stats: get rid of per-cpu cpufreq_stats_table

[Viresh Kumar]

All CPUs sharing a cpufreq policy share stats too. For this reason, add a stats
pointer to struct cpufreq_policy and drop per-CPU variable cpufreq_stats_table
used for accessing cpufreq stats so as to reduce code complexity.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 54 +++++++++++++++++++----------------------
 include/linux/cpufreq.h         |  3 +++
 2 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 6c234f548601..d6f5053d8ffb 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -31,8 +31,6 @@ struct cpufreq_stats {
 #endif
 };
 
-static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table);
-
 static int cpufreq_stats_update(struct cpufreq_stats *stat)
 {
 	unsigned long long cur_time = get_jiffies_64();
@@ -48,20 +46,15 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat)
 
 static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
 {
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
-	if (!stat)
-		return 0;
-	return sprintf(buf, "%d\n",
-			per_cpu(cpufreq_stats_table, stat->cpu)->total_trans);
+	return sprintf(buf, "%d\n", policy->stats->total_trans);
 }
 
 static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 {
+	struct cpufreq_stats *stat = policy->stats;
 	ssize_t len = 0;
 	int i;
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
-	if (!stat)
-		return 0;
+
 	cpufreq_stats_update(stat);
 	for (i = 0; i < stat->state_num; i++) {
 		len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
@@ -74,12 +67,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 {
+	struct cpufreq_stats *stat = policy->stats;
 	ssize_t len = 0;
 	int i, j;
 
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
-	if (!stat)
-		return 0;
 	cpufreq_stats_update(stat);
 	len += snprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
 	len += snprintf(buf + len, PAGE_SIZE - len, "         : ");
@@ -145,8 +136,9 @@ static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)
 
 static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
 {
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
+	struct cpufreq_stats *stat = policy->stats;
 
+	/* Already freed */
 	if (!stat)
 		return;
 
@@ -155,7 +147,7 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
 	sysfs_remove_group(&policy->kobj, &stats_attr_group);
 	kfree(stat->time_in_state);
 	kfree(stat);
-	per_cpu(cpufreq_stats_table, policy->cpu) = NULL;
+	policy->stats = NULL;
 }
 
 static void cpufreq_stats_free_table(unsigned int cpu)
@@ -184,7 +176,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 		return 0;
 
 	/* stats already initialized */
-	if (per_cpu(cpufreq_stats_table, cpu))
+	if (policy->stats)
 		return -EEXIST;
 
 	stat = kzalloc(sizeof(*stat), GFP_KERNEL);
@@ -196,7 +188,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 		goto error_out;
 
 	stat->cpu = cpu;
-	per_cpu(cpufreq_stats_table, cpu) = stat;
+	policy->stats = stat;
 
 	cpufreq_for_each_valid_entry(pos, table)
 		count++;
@@ -231,7 +223,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	sysfs_remove_group(&policy->kobj, &stats_attr_group);
 error_out:
 	kfree(stat);
-	per_cpu(cpufreq_stats_table, cpu) = NULL;
+	policy->stats = NULL;
 	return ret;
 }
 
@@ -254,15 +246,7 @@ static void cpufreq_stats_create_table(unsigned int cpu)
 
 static void cpufreq_stats_update_policy_cpu(struct cpufreq_policy *policy)
 {
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table,
-			policy->last_cpu);
-
-	pr_debug("Updating stats_table for new_cpu %u from last_cpu %u\n",
-			policy->cpu, policy->last_cpu);
-	per_cpu(cpufreq_stats_table, policy->cpu) = per_cpu(cpufreq_stats_table,
-			policy->last_cpu);
-	per_cpu(cpufreq_stats_table, policy->last_cpu) = NULL;
-	stat->cpu = policy->cpu;
+	policy->stats->cpu = policy->cpu;
 }
 
 static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
@@ -288,15 +272,24 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 		unsigned long val, void *data)
 {
 	struct cpufreq_freqs *freq = data;
+	struct cpufreq_policy *policy = cpufreq_cpu_get(freq->cpu);
 	struct cpufreq_stats *stat;
 	int old_index, new_index;
 
 	if (val != CPUFREQ_POSTCHANGE)
 		return 0;
 
-	stat = per_cpu(cpufreq_stats_table, freq->cpu);
-	if (!stat)
+	if (!policy) {
+		pr_err("%s: No policy found\n", __func__);
 		return 0;
+	}
+
+	if (!policy->stats) {
+		pr_debug("%s: No stats found\n", __func__);
+		goto put_policy;
+	}
+
+	stat = policy->stats;
 
 	old_index = stat->last_index;
 	new_index = freq_table_get_index(stat, freq->new);
@@ -317,6 +310,9 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 #endif
 	stat->total_trans++;
 	spin_unlock(&cpufreq_stats_lock);
+
+put_policy:
+	cpufreq_cpu_put(policy);
 	return 0;
 }
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 4d078cebafd2..60b7b496565d 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -113,6 +113,9 @@ struct cpufreq_policy {
 	wait_queue_head_t	transition_wait;
 	struct task_struct	*transition_task; /* Task which is doing the transition */
 
+	/* cpufreq-stats */
+	struct cpufreq_stats	*stats;
+
 	/* For cpufreq driver's internal use */
 	void			*driver_data;
 };
-- 
2.2.0

[PATCH V3 Resend 07/16] cpufreq: stats: get rid of per-cpu cpufreq_stats_table

[Viresh Kumar]

All CPUs sharing a cpufreq policy share stats too. For this reason, add a stats
pointer to struct cpufreq_policy and drop per-CPU variable cpufreq_stats_table
used for accessing cpufreq stats so as to reduce code complexity.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Resending only a single patch as found a issue with it while testing with my
newly written cpufreq-test suite. The problem was that we haven't done
cpufreq_cpu_put() for few failure cases and that restricts the cpufreq driver
module to unload.

Here is the diff to V3:

@@ -265,14 +265,14 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
        struct cpufreq_stats *stats;
        int old_index, new_index;
 
-       if (val != CPUFREQ_POSTCHANGE)
-               return 0;
-
        if (!policy) {
                pr_err("%s: No policy found\n", __func__);
                return 0;
        }
 
+       if (val != CPUFREQ_POSTCHANGE)
+               goto put_policy;
+
        if (!policy->stats) {
                pr_debug("%s: No stats found\n", __func__);
                goto put_policy;
@@ -285,10 +285,10 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 
        /* We can't do stats->time_in_state[-1]= .. */
        if (old_index == -1 || new_index == -1)
-               return 0;
+               goto put_policy;
 
        if (old_index == new_index)
-               return 0;
+               goto put_policy;
 
        cpufreq_stats_update(stats);

 drivers/cpufreq/cpufreq_stats.c | 62 +++++++++++++++++++----------------------
 include/linux/cpufreq.h         |  3 ++
 2 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 6c234f548601..3792b2e2f4a8 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -31,8 +31,6 @@ struct cpufreq_stats {
 #endif
 };
 
-static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table);
-
 static int cpufreq_stats_update(struct cpufreq_stats *stat)
 {
 	unsigned long long cur_time = get_jiffies_64();
@@ -48,20 +46,15 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat)
 
 static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
 {
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
-	if (!stat)
-		return 0;
-	return sprintf(buf, "%d\n",
-			per_cpu(cpufreq_stats_table, stat->cpu)->total_trans);
+	return sprintf(buf, "%d\n", policy->stats->total_trans);
 }
 
 static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 {
+	struct cpufreq_stats *stat = policy->stats;
 	ssize_t len = 0;
 	int i;
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
-	if (!stat)
-		return 0;
+
 	cpufreq_stats_update(stat);
 	for (i = 0; i < stat->state_num; i++) {
 		len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
@@ -74,12 +67,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 {
+	struct cpufreq_stats *stat = policy->stats;
 	ssize_t len = 0;
 	int i, j;
 
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
-	if (!stat)
-		return 0;
 	cpufreq_stats_update(stat);
 	len += snprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
 	len += snprintf(buf + len, PAGE_SIZE - len, "         : ");
@@ -145,8 +136,9 @@ static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)
 
 static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
 {
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
+	struct cpufreq_stats *stat = policy->stats;
 
+	/* Already freed */
 	if (!stat)
 		return;
 
@@ -155,7 +147,7 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
 	sysfs_remove_group(&policy->kobj, &stats_attr_group);
 	kfree(stat->time_in_state);
 	kfree(stat);
-	per_cpu(cpufreq_stats_table, policy->cpu) = NULL;
+	policy->stats = NULL;
 }
 
 static void cpufreq_stats_free_table(unsigned int cpu)
@@ -184,7 +176,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 		return 0;
 
 	/* stats already initialized */
-	if (per_cpu(cpufreq_stats_table, cpu))
+	if (policy->stats)
 		return -EEXIST;
 
 	stat = kzalloc(sizeof(*stat), GFP_KERNEL);
@@ -196,7 +188,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 		goto error_out;
 
 	stat->cpu = cpu;
-	per_cpu(cpufreq_stats_table, cpu) = stat;
+	policy->stats = stat;
 
 	cpufreq_for_each_valid_entry(pos, table)
 		count++;
@@ -231,7 +223,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	sysfs_remove_group(&policy->kobj, &stats_attr_group);
 error_out:
 	kfree(stat);
-	per_cpu(cpufreq_stats_table, cpu) = NULL;
+	policy->stats = NULL;
 	return ret;
 }
 
@@ -254,15 +246,7 @@ static void cpufreq_stats_create_table(unsigned int cpu)
 
 static void cpufreq_stats_update_policy_cpu(struct cpufreq_policy *policy)
 {
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table,
-			policy->last_cpu);
-
-	pr_debug("Updating stats_table for new_cpu %u from last_cpu %u\n",
-			policy->cpu, policy->last_cpu);
-	per_cpu(cpufreq_stats_table, policy->cpu) = per_cpu(cpufreq_stats_table,
-			policy->last_cpu);
-	per_cpu(cpufreq_stats_table, policy->last_cpu) = NULL;
-	stat->cpu = policy->cpu;
+	policy->stats->cpu = policy->cpu;
 }
 
 static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
@@ -288,27 +272,36 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 		unsigned long val, void *data)
 {
 	struct cpufreq_freqs *freq = data;
+	struct cpufreq_policy *policy = cpufreq_cpu_get(freq->cpu);
 	struct cpufreq_stats *stat;
 	int old_index, new_index;
 
-	if (val != CPUFREQ_POSTCHANGE)
+	if (!policy) {
+		pr_err("%s: No policy found\n", __func__);
 		return 0;
+	}
 
-	stat = per_cpu(cpufreq_stats_table, freq->cpu);
-	if (!stat)
-		return 0;
+	if (val != CPUFREQ_POSTCHANGE)
+		goto put_policy;
+
+	if (!policy->stats) {
+		pr_debug("%s: No stats found\n", __func__);
+		goto put_policy;
+	}
+
+	stat = policy->stats;
 
 	old_index = stat->last_index;
 	new_index = freq_table_get_index(stat, freq->new);
 
 	/* We can't do stat->time_in_state[-1]= .. */
 	if (old_index == -1 || new_index == -1)
-		return 0;
+		goto put_policy;
 
 	cpufreq_stats_update(stat);
 
 	if (old_index == new_index)
-		return 0;
+		goto put_policy;
 
 	spin_lock(&cpufreq_stats_lock);
 	stat->last_index = new_index;
@@ -317,6 +310,9 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 #endif
 	stat->total_trans++;
 	spin_unlock(&cpufreq_stats_lock);
+
+put_policy:
+	cpufreq_cpu_put(policy);
 	return 0;
 }
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 4d078cebafd2..60b7b496565d 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -113,6 +113,9 @@ struct cpufreq_policy {
 	wait_queue_head_t	transition_wait;
 	struct task_struct	*transition_task; /* Task which is doing the transition */
 
+	/* cpufreq-stats */
+	struct cpufreq_stats	*stats;
+
 	/* For cpufreq driver's internal use */
 	void			*driver_data;
 };
-- 
2.2.0

[PATCH V3 08/16] cpufreq: stats: rename 'struct cpufreq_stats' objects as 'stats'

[Viresh Kumar]

Currently we name objects of 'struct cpufreq_stats' as 'stat' and 'stats'. Use
'stats' to make it consistent.

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

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index d6f5053d8ffb..ca3d5b71e828 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -31,15 +31,15 @@ struct cpufreq_stats {
 #endif
 };
 
-static int cpufreq_stats_update(struct cpufreq_stats *stat)
+static int cpufreq_stats_update(struct cpufreq_stats *stats)
 {
 	unsigned long long cur_time = get_jiffies_64();
 
 	spin_lock(&cpufreq_stats_lock);
-	if (stat->time_in_state)
-		stat->time_in_state[stat->last_index] +=
-			cur_time - stat->last_time;
-	stat->last_time = cur_time;
+	if (stats->time_in_state)
+		stats->time_in_state[stats->last_index] +=
+			cur_time - stats->last_time;
+	stats->last_time = cur_time;
 	spin_unlock(&cpufreq_stats_lock);
 	return 0;
 }
@@ -51,15 +51,15 @@ static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
 
 static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 {
-	struct cpufreq_stats *stat = policy->stats;
+	struct cpufreq_stats *stats = policy->stats;
 	ssize_t len = 0;
 	int i;
 
-	cpufreq_stats_update(stat);
-	for (i = 0; i < stat->state_num; i++) {
-		len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
+	cpufreq_stats_update(stats);
+	for (i = 0; i < stats->state_num; i++) {
+		len += sprintf(buf + len, "%u %llu\n", stats->freq_table[i],
 			(unsigned long long)
-			jiffies_64_to_clock_t(stat->time_in_state[i]));
+			jiffies_64_to_clock_t(stats->time_in_state[i]));
 	}
 	return len;
 }
@@ -67,36 +67,36 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 {
-	struct cpufreq_stats *stat = policy->stats;
+	struct cpufreq_stats *stats = policy->stats;
 	ssize_t len = 0;
 	int i, j;
 
-	cpufreq_stats_update(stat);
+	cpufreq_stats_update(stats);
 	len += snprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
 	len += snprintf(buf + len, PAGE_SIZE - len, "         : ");
-	for (i = 0; i < stat->state_num; i++) {
+	for (i = 0; i < stats->state_num; i++) {
 		if (len >= PAGE_SIZE)
 			break;
 		len += snprintf(buf + len, PAGE_SIZE - len, "%9u ",
-				stat->freq_table[i]);
+				stats->freq_table[i]);
 	}
 	if (len >= PAGE_SIZE)
 		return PAGE_SIZE;
 
 	len += snprintf(buf + len, PAGE_SIZE - len, "\n");
 
-	for (i = 0; i < stat->state_num; i++) {
+	for (i = 0; i < stats->state_num; i++) {
 		if (len >= PAGE_SIZE)
 			break;
 
 		len += snprintf(buf + len, PAGE_SIZE - len, "%9u: ",
-				stat->freq_table[i]);
+				stats->freq_table[i]);
 
-		for (j = 0; j < stat->state_num; j++) {
+		for (j = 0; j < stats->state_num; j++) {
 			if (len >= PAGE_SIZE)
 				break;
 			len += snprintf(buf + len, PAGE_SIZE - len, "%9u ",
-					stat->trans_table[i*stat->max_state+j]);
+					stats->trans_table[i*stats->max_state+j]);
 		}
 		if (len >= PAGE_SIZE)
 			break;
@@ -125,28 +125,28 @@ static struct attribute_group stats_attr_group = {
 	.name = "stats"
 };
 
-static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)
+static int freq_table_get_index(struct cpufreq_stats *stats, unsigned int freq)
 {
 	int index;
-	for (index = 0; index < stat->max_state; index++)
-		if (stat->freq_table[index] == freq)
+	for (index = 0; index < stats->max_state; index++)
+		if (stats->freq_table[index] == freq)
 			return index;
 	return -1;
 }
 
 static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
 {
-	struct cpufreq_stats *stat = policy->stats;
+	struct cpufreq_stats *stats = policy->stats;
 
 	/* Already freed */
-	if (!stat)
+	if (!stats)
 		return;
 
-	pr_debug("%s: Free stat table\n", __func__);
+	pr_debug("%s: Free stats table\n", __func__);
 
 	sysfs_remove_group(&policy->kobj, &stats_attr_group);
-	kfree(stat->time_in_state);
-	kfree(stat);
+	kfree(stats->time_in_state);
+	kfree(stats);
 	policy->stats = NULL;
 }
 
@@ -166,7 +166,7 @@ static void cpufreq_stats_free_table(unsigned int cpu)
 static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 {
 	unsigned int i, count = 0, ret = 0;
-	struct cpufreq_stats *stat;
+	struct cpufreq_stats *stats;
 	unsigned int alloc_size;
 	unsigned int cpu = policy->cpu;
 	struct cpufreq_frequency_table *pos, *table;
@@ -179,16 +179,16 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	if (policy->stats)
 		return -EEXIST;
 
-	stat = kzalloc(sizeof(*stat), GFP_KERNEL);
-	if ((stat) == NULL)
+	stats = kzalloc(sizeof(*stats), GFP_KERNEL);
+	if ((stats) == NULL)
 		return -ENOMEM;
 
 	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
 	if (ret)
 		goto error_out;
 
-	stat->cpu = cpu;
-	policy->stats = stat;
+	stats->cpu = cpu;
+	policy->stats = stats;
 
 	cpufreq_for_each_valid_entry(pos, table)
 		count++;
@@ -198,31 +198,31 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 	alloc_size += count * count * sizeof(int);
 #endif
-	stat->max_state = count;
-	stat->time_in_state = kzalloc(alloc_size, GFP_KERNEL);
-	if (!stat->time_in_state) {
+	stats->max_state = count;
+	stats->time_in_state = kzalloc(alloc_size, GFP_KERNEL);
+	if (!stats->time_in_state) {
 		ret = -ENOMEM;
 		goto error_alloc;
 	}
-	stat->freq_table = (unsigned int *)(stat->time_in_state + count);
+	stats->freq_table = (unsigned int *)(stats->time_in_state + count);
 
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
-	stat->trans_table = stat->freq_table + count;
+	stats->trans_table = stats->freq_table + count;
 #endif
 	i = 0;
 	cpufreq_for_each_valid_entry(pos, table)
-		if (freq_table_get_index(stat, pos->frequency) == -1)
-			stat->freq_table[i++] = pos->frequency;
-	stat->state_num = i;
+		if (freq_table_get_index(stats, pos->frequency) == -1)
+			stats->freq_table[i++] = pos->frequency;
+	stats->state_num = i;
 	spin_lock(&cpufreq_stats_lock);
-	stat->last_time = get_jiffies_64();
-	stat->last_index = freq_table_get_index(stat, policy->cur);
+	stats->last_time = get_jiffies_64();
+	stats->last_index = freq_table_get_index(stats, policy->cur);
 	spin_unlock(&cpufreq_stats_lock);
 	return 0;
 error_alloc:
 	sysfs_remove_group(&policy->kobj, &stats_attr_group);
 error_out:
-	kfree(stat);
+	kfree(stats);
 	policy->stats = NULL;
 	return ret;
 }
@@ -273,7 +273,7 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 {
 	struct cpufreq_freqs *freq = data;
 	struct cpufreq_policy *policy = cpufreq_cpu_get(freq->cpu);
-	struct cpufreq_stats *stat;
+	struct cpufreq_stats *stats;
 	int old_index, new_index;
 
 	if (val != CPUFREQ_POSTCHANGE)
@@ -289,26 +289,26 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 		goto put_policy;
 	}
 
-	stat = policy->stats;
+	stats = policy->stats;
 
-	old_index = stat->last_index;
-	new_index = freq_table_get_index(stat, freq->new);
+	old_index = stats->last_index;
+	new_index = freq_table_get_index(stats, freq->new);
 
-	/* We can't do stat->time_in_state[-1]= .. */
+	/* We can't do stats->time_in_state[-1]= .. */
 	if (old_index == -1 || new_index == -1)
 		return 0;
 
-	cpufreq_stats_update(stat);
+	cpufreq_stats_update(stats);
 
 	if (old_index == new_index)
 		return 0;
 
 	spin_lock(&cpufreq_stats_lock);
-	stat->last_index = new_index;
+	stats->last_index = new_index;
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
-	stat->trans_table[old_index * stat->max_state + new_index]++;
+	stats->trans_table[old_index * stats->max_state + new_index]++;
 #endif
-	stat->total_trans++;
+	stats->total_trans++;
 	spin_unlock(&cpufreq_stats_lock);
 
 put_policy:
-- 
2.2.0

[PATCH V3 09/16] cpufreq: Remove (now) unused 'last_cpu' from struct cpufreq_policy

[Viresh Kumar]

'last_cpu' was used only from cpufreq-stats and isn't used anymore. Get rid of
it.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 3 ---
 include/linux/cpufreq.h   | 2 --
 2 files changed, 5 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a09a29c312a9..b88894578fd1 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1091,10 +1091,7 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
 	}
 
 	down_write(&policy->rwsem);
-
-	policy->last_cpu = policy->cpu;
 	policy->cpu = cpu;
-
 	up_write(&policy->rwsem);
 
 	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 60b7b496565d..7e1a389b4e92 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -66,8 +66,6 @@ struct cpufreq_policy {
 	unsigned int		shared_type; /* ACPI: ANY or ALL affected CPUs
 						should set cpufreq */
 	unsigned int		cpu;    /* cpu nr of CPU managing this policy */
-	unsigned int		last_cpu; /* cpu nr of previous CPU that managed
-					   * this policy */
 	struct clk		*clk;
 	struct cpufreq_cpuinfo	cpuinfo;/* see above */
 
-- 
2.2.0

[PATCH V3 10/16] cpufreq: stats: drop 'cpu' field of struct cpufreq_stats

[Viresh Kumar]

'cpu' field of struct cpufreq_stats isn't used anymore and so can be dropped.
This change makes cpufreq_stats_update_policy_cpu() empty and so that is removed
as well.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index ca3d5b71e828..1ed703a3e21e 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -18,7 +18,6 @@
 static spinlock_t cpufreq_stats_lock;
 
 struct cpufreq_stats {
-	unsigned int cpu;
 	unsigned int total_trans;
 	unsigned long long last_time;
 	unsigned int max_state;
@@ -187,7 +186,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	if (ret)
 		goto error_out;
 
-	stats->cpu = cpu;
 	policy->stats = stats;
 
 	cpufreq_for_each_valid_entry(pos, table)
@@ -244,22 +242,12 @@ static void cpufreq_stats_create_table(unsigned int cpu)
 	cpufreq_cpu_put(policy);
 }
 
-static void cpufreq_stats_update_policy_cpu(struct cpufreq_policy *policy)
-{
-	policy->stats->cpu = policy->cpu;
-}
-
 static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
 		unsigned long val, void *data)
 {
 	int ret = 0;
 	struct cpufreq_policy *policy = data;
 
-	if (val == CPUFREQ_UPDATE_POLICY_CPU) {
-		cpufreq_stats_update_policy_cpu(policy);
-		return 0;
-	}
-
 	if (val == CPUFREQ_CREATE_POLICY)
 		ret = __cpufreq_stats_create_table(policy);
 	else if (val == CPUFREQ_REMOVE_POLICY)
-- 
2.2.0

[PATCH V3 11/16] cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications

[Viresh Kumar]

CPUFREQ_UPDATE_POLICY_CPU notifications were used only from cpufreq-stats which
doesn't use it anymore. Remove them.

This also decrements values of other notification macros defined after
CPUFREQ_UPDATE_POLICY_CPU by 1 to remove gaps. Hopefully all users are using
macro's instead of direct numbers and so they wouldn't break as macro values are
changed now.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 3 ---
 include/linux/cpufreq.h   | 5 ++---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b88894578fd1..6d97dffeaaf7 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1094,9 +1094,6 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
 	policy->cpu = cpu;
 	up_write(&policy->rwsem);
 
-	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
-			CPUFREQ_UPDATE_POLICY_CPU, policy);
-
 	return 0;
 }
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 7e1a389b4e92..2ee4888c1f47 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -368,9 +368,8 @@ static inline void cpufreq_resume(void) {}
 #define CPUFREQ_INCOMPATIBLE		(1)
 #define CPUFREQ_NOTIFY			(2)
 #define CPUFREQ_START			(3)
-#define CPUFREQ_UPDATE_POLICY_CPU	(4)
-#define CPUFREQ_CREATE_POLICY		(5)
-#define CPUFREQ_REMOVE_POLICY		(6)
+#define CPUFREQ_CREATE_POLICY		(4)
+#define CPUFREQ_REMOVE_POLICY		(5)
 
 #ifdef CONFIG_CPU_FREQ
 int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list);
-- 
2.2.0

[PATCH V3 12/16] cpufreq: stats: create sysfs group once we are ready

[Viresh Kumar]

Userspace is free to read value of any file from cpufreq/stats directory once
they are created. __cpufreq_stats_create_table() is creating the sysfs files
first and then allocating resources for them. Though it would be quite difficult
to trigger the racy situation here, but for the sake of keeping sensible code
lets create sysfs entries only after we are ready to go.

This also does some makeup to the routine to make it look better.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 44 +++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 1ed703a3e21e..17df8c507594 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -164,12 +164,13 @@ static void cpufreq_stats_free_table(unsigned int cpu)
 
 static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 {
-	unsigned int i, count = 0, ret = 0;
+	unsigned int i = 0, count = 0, ret = -ENOMEM;
 	struct cpufreq_stats *stats;
 	unsigned int alloc_size;
 	unsigned int cpu = policy->cpu;
 	struct cpufreq_frequency_table *pos, *table;
 
+	/* We need cpufreq table for creating stats table */
 	table = cpufreq_frequency_get_table(cpu);
 	if (unlikely(!table))
 		return 0;
@@ -179,15 +180,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 		return -EEXIST;
 
 	stats = kzalloc(sizeof(*stats), GFP_KERNEL);
-	if ((stats) == NULL)
+	if (!stats)
 		return -ENOMEM;
 
-	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
-	if (ret)
-		goto error_out;
-
-	policy->stats = stats;
-
+	/* Find total allocation size */
 	cpufreq_for_each_valid_entry(pos, table)
 		count++;
 
@@ -196,32 +192,42 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 	alloc_size += count * count * sizeof(int);
 #endif
-	stats->max_state = count;
+
+	/* Allocate memory for time_in_state/freq_table/trans_table in one go */
 	stats->time_in_state = kzalloc(alloc_size, GFP_KERNEL);
-	if (!stats->time_in_state) {
-		ret = -ENOMEM;
-		goto error_alloc;
-	}
+	if (!stats->time_in_state)
+		goto free_stat;
+
 	stats->freq_table = (unsigned int *)(stats->time_in_state + count);
 
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 	stats->trans_table = stats->freq_table + count;
 #endif
-	i = 0;
+
+	stats->max_state = count;
+
+	/* Find valid-unique entries */
 	cpufreq_for_each_valid_entry(pos, table)
 		if (freq_table_get_index(stats, pos->frequency) == -1)
 			stats->freq_table[i++] = pos->frequency;
 	stats->state_num = i;
+
 	spin_lock(&cpufreq_stats_lock);
 	stats->last_time = get_jiffies_64();
 	stats->last_index = freq_table_get_index(stats, policy->cur);
 	spin_unlock(&cpufreq_stats_lock);
-	return 0;
-error_alloc:
-	sysfs_remove_group(&policy->kobj, &stats_attr_group);
-error_out:
-	kfree(stats);
+
+	policy->stats = stats;
+	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
+	if (!ret)
+		return 0;
+
+	/* We failed, release resources */
 	policy->stats = NULL;
+	kfree(stats->time_in_state);
+free_stat:
+	kfree(stats);
+
 	return ret;
 }
 
-- 
2.2.0

[PATCH V3 13/16] cpufreq: stats: time_in_state can't be NULL in cpufreq_stats_update()

[Viresh Kumar]

'time_in_state' can't be NULL if 'stats' is valid. These are allocated together
and only if time_in_state is allocated successfully, we update policy->stats.

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

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 17df8c507594..66ebdc4c1762 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -35,9 +35,7 @@ static int cpufreq_stats_update(struct cpufreq_stats *stats)
 	unsigned long long cur_time = get_jiffies_64();
 
 	spin_lock(&cpufreq_stats_lock);
-	if (stats->time_in_state)
-		stats->time_in_state[stats->last_index] +=
-			cur_time - stats->last_time;
+	stats->time_in_state[stats->last_index] += cur_time - stats->last_time;
 	stats->last_time = cur_time;
 	spin_unlock(&cpufreq_stats_lock);
 	return 0;
-- 
2.2.0

[PATCH V3 14/16] cpufreq: stats: don't update stats from show_trans_table()

[Viresh Kumar]

cpufreq_stats_update() updates time_in_state and nothing else. It should ideally
be updated only in two cases:
- User requested for the current value of time_in_state.
- We have switched states and so need to update time for the last state.

Currently, we are also doing this while user asks for the transition table of
frequencies. It wouldn't do any harm, but no good as well. Its useless here.

Remove it.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 66ebdc4c1762..6e87dd9c4083 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -68,7 +68,6 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 	ssize_t len = 0;
 	int i, j;
 
-	cpufreq_stats_update(stats);
 	len += snprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
 	len += snprintf(buf + len, PAGE_SIZE - len, "         : ");
 	for (i = 0; i < stats->state_num; i++) {
-- 
2.2.0

[PATCH V3 15/16] cpufreq: stats: don't update stats on false notifiers

[Viresh Kumar]

We need to call cpufreq_stats_update() to update 'time_in_state' for the last
frequency. This is achieved by calling it from cpufreq_stat_notifier_trans(),
which is called after frequency transition.

But if we detect that the cpu's frequency haven't really changed and its a false
POSTCHANGE notification, we don't really need to update time_in_state.

It wouldn't cause any harm in calling cpufreq_stats_update() but we can avoid
calling it here and call it when the frequency really changes. The result will
be the same but more efficient.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 6e87dd9c4083..71fcc7c198a1 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -289,11 +289,11 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 	if (old_index == -1 || new_index == -1)
 		return 0;
 
-	cpufreq_stats_update(stats);
-
 	if (old_index == new_index)
 		return 0;
 
+	cpufreq_stats_update(stats);
+
 	spin_lock(&cpufreq_stats_lock);
 	stats->last_index = new_index;
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
-- 
2.2.0

[PATCH V3 16/16] cpufreq: stats: drop unnecessary locking

[Viresh Kumar]

There is no possibility of any race on updating last_index, trans_table or
total_trans as these are updated only by cpufreq_stat_notifier_trans() which
will be called sequentially.

The only place where locking is still relevant is: cpufreq_stats_update(), which
updates time_in_state and last_time. This can be called by two thread in
parallel, that may result in races.

The two threads being:
- sysfs read of time_in_state
- and frequency transition that calls cpufreq_stat_notifier_trans().

Remove locking from the first case mentioned above.

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

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 71fcc7c198a1..bce255568123 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -207,12 +207,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	cpufreq_for_each_valid_entry(pos, table)
 		if (freq_table_get_index(stats, pos->frequency) == -1)
 			stats->freq_table[i++] = pos->frequency;
-	stats->state_num = i;
 
-	spin_lock(&cpufreq_stats_lock);
+	stats->state_num = i;
 	stats->last_time = get_jiffies_64();
 	stats->last_index = freq_table_get_index(stats, policy->cur);
-	spin_unlock(&cpufreq_stats_lock);
 
 	policy->stats = stats;
 	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
@@ -294,13 +292,11 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 
 	cpufreq_stats_update(stats);
 
-	spin_lock(&cpufreq_stats_lock);
 	stats->last_index = new_index;
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 	stats->trans_table[old_index * stats->max_state + new_index]++;
 #endif
 	stats->total_trans++;
-	spin_unlock(&cpufreq_stats_lock);
 
 put_policy:
 	cpufreq_cpu_put(policy);
-- 
2.2.0

Re: [PATCH V3 00/16] cpufreq: stats: cleanups

[Viresh Kumar]

On 6 January 2015 at 21:08, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi Rafael,
>
> This is V3 of the stats cleanup I sent earlier. Few things are improved based on
> the feedback received (mostly from you). Please see if it looks any better.

Any more inputs ?