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(-)
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);
__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;
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;
'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)
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); }
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;
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; };
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; };
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:
'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 */
'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)
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);
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; }
'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;
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++) {
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
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);
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 ?
linaro-kernel@lists.linaro.org