acpi-cpufreq: replace per_cpu with driver_data of policy
PATCH 1/1 acpi-cpufreq: replace per_cpu with driver_data of policy
Now policy has field of driver_data, we can use it to get rid of per_cpu acpi_cpufreq_data.
we have benefits after this replacing. 1) memory saving. 2) policy is shared by several cpus, per_cpu seems not correct. useing *driver_data* is more reasonable. 3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy->cpu might change after cpu hotplug. So sometimes we cant't free *data*, use *driver_data* to fix it. 4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu(policy->cpu) is set to *data*, if we try to get cpufreq on other cpus, we get zero instead of correct values. Use *driver_data* to fix it.
--- drivers/cpufreq/acpi-cpufreq.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)
drivers/cpufreq/acpi-cpufreq.c +23 -15
@@ -72,8 +72,6 @@ struct acpi_cpufreq_data {cpumask_var_t freqdomain_cpus; }; -static DEFINE_PER_CPU(struct acpi_cpufreq_data *, acfreq_data); -/* acpi_perf_data is a pointer to percpu data. */ static struct acpi_processor_performance __percpu *acpi_perf_data;
@@ -144,7 +142,7 @@ static int _store_boost(int val)static ssize_t show_freqdomain_cpus (struct cpufreq_policy *policy,char *buf) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;return cpufreq_show_cpus (data->freqdomain_cpus, buf); }
@@ -327,7 +325,8 @@ static void drv_write(struct drv_cmd *cmd)put_cpu (); } -static u32 get_cur_val (const struct cpumask *mask) +static u32 get_cur_val (const struct cpumask *mask, +struct acpi_cpufreq_data *data) {struct acpi_processor_performance *perf;struct drv_cmd cmd;
@@ -335,7 +334,7 @@ static u32 get_cur_val(const struct cpumask *mask)if (unlikely (cpumask_empty (mask)))return 0 ; -switch (per_cpu (acfreq_data,cpumask_first (mask))->cpu_feature) { +switch (data->cpu_feature) {case SYSTEM_INTEL_MSR_CAPABLE : cmd.type =SYSTEM_INTEL_MSR_CAPABLE ; cmd.addr.msr.reg =MSR_IA32_PERF_CTL ;
@@ -346,7 +345,7 @@ static u32 get_cur_val(const struct cpumask *mask)break ;case SYSTEM_IO_CAPABLE : cmd.type =SYSTEM_IO_CAPABLE ; - perf =per_cpu (acfreq_data,cpumask_first (mask))->acpi_data; + perf = data->acpi_data; cmd.addr.io.port = perf->control_register.address; cmd.addr.io.bit_width = perf->control_register.bit_width;break ;
@@ -364,19 +363,26 @@ static u32 get_cur_val(const struct cpumask *mask)static unsigned int get_cur_freq_on_cpu (unsigned int cpu) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, cpu); +struct acpi_cpufreq_data *data; +struct cpufreq_policy *policy;unsigned int freq;unsigned int cached_freq;pr_debug ("get_cur_freq_on_cpu (%d)\n" , cpu); + policy =cpufreq_cpu_get (cpu); +if (unlikely (!policy)) +return 0 ; + + data = policy->driver_data;if (unlikely (data ==NULL || data->acpi_data ==NULL || data->freq_table ==NULL )) { +cpufreq_cpu_put (policy);return 0 ; } cached_freq = data->freq_table[data->acpi_data->state].frequency; - freq =extract_freq (get_cur_val (cpumask_of (cpu)), data); + freq =extract_freq (get_cur_val (cpumask_of (cpu), data), data);if (freq != cached_freq) {/* * The dreaded BIOS frequency change behind our back.
@@ -385,6 +391,8 @@ static unsigned int get_cur_freq_on_cpu(unsigned int cpu) data->resume =1 ; } +cpufreq_cpu_put (policy); +pr_debug ("cur freq = %u\n" , freq);return freq;
@@ -397,7 +405,7 @@ static unsigned int check_freqs(const struct cpumask *mask, unsigned int freq,unsigned int i;for (i =0 ; i <100 ; i++) { - cur_freq =extract_freq (get_cur_val (mask), data); + cur_freq =extract_freq (get_cur_val (mask, data), data);if (cur_freq == freq)return 1 ;udelay (10 );
@@ -408,7 +416,7 @@ static unsigned int check_freqs(const struct cpumask *mask, unsigned int freq,static int acpi_cpufreq_target (struct cpufreq_policy *policy,unsigned int index) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;struct acpi_processor_performance *perf;struct drv_cmd cmd;unsigned int next_perf_state =0 ;/* Index into perf table */
@@ -673,7 +681,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) } data->acpi_data =per_cpu_ptr (acpi_perf_data, cpu); -per_cpu (acfreq_data, cpu) = data; + policy->driver_data = data;if (cpu_has (c,X86_FEATURE_CONSTANT_TSC )) acpi_cpufreq_driver.flags |=CPUFREQ_CONST_LOOPS ;
@@ -843,19 +851,19 @@ err_free_mask:free_cpumask_var (data->freqdomain_cpus); err_free:kfree (data); -per_cpu (acfreq_data, cpu) =NULL ; + policy->driver_data =NULL ;return result; }static int acpi_cpufreq_cpu_exit (struct cpufreq_policy *policy) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;pr_debug ("acpi_cpufreq_cpu_exit\n" );if (data) { -per_cpu (acfreq_data, policy->cpu) =NULL ; + policy->driver_data =NULL ;acpi_processor_unregister_performance (data->acpi_data, policy->cpu);free_cpumask_var (data->freqdomain_cpus);
@@ -868,7 +876,7 @@ static int acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy)static int acpi_cpufreq_resume (struct cpufreq_policy *policy) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;pr_debug ("acpi_cpufreq_resume\n" );
Signature · 1 line
1.9.1
Viresh Kumar
maintainerQuoted 3 lines · Pan Xinhui
Now policy has field of
driver_data, we can use it to get rid ofper_cpuacpi_cpufreq_data.
Instead:
"Drivers can store their internal per-policy information in policy->driver_data, lets use it."
Quoted 1 line
we have benefits after this replacing. 1) memory saving. 2) policy is
s/we/We
Also these points can be kept in separate lines :)
Quoted 1 line
shared by several cpus,
per_cpuseems not correct. useing *driver_data*
using
Quoted 1 line
is more reasonable. 3) fix a memory leak in
acpi_cpufreq_cpu_exit.
Not just that. You are also fixing NULL pointer dereferences if wrong or uninitialized per-cpu data is used, because of a recent hotplug of policy->cpu.
Quoted 35 lines
as
policy->cpumight change after cpu hotplug. So sometimes we cant't free *data*, use *driver_data* to fix it. 4) fix a zero return value ofget_cur_freq_on_cpu. Onlyper_cpu(policy->cpu)is set to *data*, if we try to get cpufreq on other cpus, we get zero instead of correct values. Use *driver_data* to fix it.Signed-off-by Pan Xinhui <[email protected]>--- drivers/cpufreq/acpi-cpufreq.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)drivers/cpufreq/acpi-cpufreq.c +1
-3@@ -72,8 +72,6 @@ struct acpi_cpufreq_data {cpumask_var_t freqdomain_cpus; }; -static DEFINE_PER_CPU(struct acpi_cpufreq_data *, acfreq_data); -/* acpi_perf_data is a pointer to percpu data. */ static struct acpi_processor_performance __percpu *acpi_perf_data;@@ -144,7 +142,7 @@ static int _store_boost(int val)static ssize_t show_freqdomain_cpus (struct cpufreq_policy *policy,char *buf) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;return cpufreq_show_cpus (data->freqdomain_cpus, buf); }
drivers/cpufreq/acpi-cpufreq.c static void drv_write(struct drv_cmd *cmd) · line 329
Quoted 6 lines
drivers/cpufreq/acpi-cpufreq.c +2
-1@@ -327,7 +325,8 @@ static void drv_write(struct drv_cmd *cmd)put_cpu (); } -static u32 get_cur_val (const struct cpumask *mask) +static u32 get_cur_val (const struct cpumask *mask, +struct acpi_cpufreq_data *data)
Run checkpatch --strict to see some alignment warnings here..
Quoted 28 lines
drivers/cpufreq/acpi-cpufreq.c +4
-3{struct acpi_processor_performance *perf;struct drv_cmd cmd;@@ -335,7 +334,7 @@ static u32 get_cur_val(const struct cpumask *mask)if (unlikely (cpumask_empty (mask)))return 0 ; -switch (per_cpu (acfreq_data,cpumask_first (mask))->cpu_feature) { +switch (data->cpu_feature) {case SYSTEM_INTEL_MSR_CAPABLE : cmd.type =SYSTEM_INTEL_MSR_CAPABLE ; cmd.addr.msr.reg =MSR_IA32_PERF_CTL ;@@ -346,7 +345,7 @@ static u32 get_cur_val(const struct cpumask *mask)break ;case SYSTEM_IO_CAPABLE : cmd.type =SYSTEM_IO_CAPABLE ; - perf =per_cpu (acfreq_data,cpumask_first (mask))->acpi_data; + perf = data->acpi_data; cmd.addr.io.port = perf->control_register.address; cmd.addr.io.bit_width = perf->control_register.bit_width;break ;@@ -364,19 +363,26 @@ static u32 get_cur_val(const struct cpumask *mask)static unsigned int get_cur_freq_on_cpu (unsigned int cpu) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, cpu); +struct acpi_cpufreq_data *data; +struct cpufreq_policy *policy;
drivers/cpufreq/acpi-cpufreq.c static u32 get_cur_val(const struct cpumask *mask) · line 377
Quoted 10 lines
drivers/cpufreq/acpi-cpufreq.c +5
-0unsigned int freq;unsigned int cached_freq;pr_debug ("get_cur_freq_on_cpu (%d)\n" , cpu); + policy =cpufreq_cpu_get (cpu); +if (unlikely (!policy)) +return 0 ; + + data = policy->driver_data;
Do
+ cpufreq_cpu_put(policy);
right here. No need of doing this towards the end.
drivers/cpufreq/acpi-cpufreq.c static u32 get_cur_val(const struct cpumask *mask) · line 379
Quoted 2 lines
drivers/cpufreq/acpi-cpufreq.c +0
-0if (unlikely (data ==NULL || data->acpi_data ==NULL || data->freq_table ==NULL )) {
Maybe this if you like:
if (unlikely(! data || !data->acpi_data || !data->freq_table))
return 0;drivers/cpufreq/acpi-cpufreq.c static u32 get_cur_val(const struct cpumask *mask) · line 384
Quoted 5 lines
drivers/cpufreq/acpi-cpufreq.c +1
-0+cpufreq_cpu_put (policy);return 0 ; } cached_freq = data->freq_table[data->acpi_data->state].frequency;
And this freq_table thing gives you opportunity to write another patch :)
policy already have: policy->freq_table :)
Quoted 65 lines
drivers/cpufreq/acpi-cpufreq.c +9
-7- freq =extract_freq (get_cur_val (cpumask_of (cpu)), data); + freq =extract_freq (get_cur_val (cpumask_of (cpu), data), data);if (freq != cached_freq) {/* * The dreaded BIOS frequency change behind our back. @@ -385,6 +391,8 @@ static unsigned int get_cur_freq_on_cpu(unsigned int cpu) data->resume =1 ; } +cpufreq_cpu_put (policy); +pr_debug ("cur freq = %u\n" , freq);return freq;@@ -397,7 +405,7 @@ static unsigned int check_freqs(const struct cpumask *mask, unsigned int freq,unsigned int i;for (i =0 ; i <100 ; i++) { - cur_freq =extract_freq (get_cur_val (mask), data); + cur_freq =extract_freq (get_cur_val (mask, data), data);if (cur_freq == freq)return 1 ;udelay (10 );@@ -408,7 +416,7 @@ static unsigned int check_freqs(const struct cpumask *mask, unsigned int freq,static int acpi_cpufreq_target (struct cpufreq_policy *policy,unsigned int index) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;struct acpi_processor_performance *perf;struct drv_cmd cmd;unsigned int next_perf_state =0 ;/* Index into perf table */ @@ -673,7 +681,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) } data->acpi_data =per_cpu_ptr (acpi_perf_data, cpu); -per_cpu (acfreq_data, cpu) = data; + policy->driver_data = data;if (cpu_has (c,X86_FEATURE_CONSTANT_TSC )) acpi_cpufreq_driver.flags |=CPUFREQ_CONST_LOOPS ;@@ -843,19 +851,19 @@ err_free_mask:free_cpumask_var (data->freqdomain_cpus); err_free:kfree (data); -per_cpu (acfreq_data, cpu) =NULL ; + policy->driver_data =NULL ;return result; }static int acpi_cpufreq_cpu_exit (struct cpufreq_policy *policy) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;pr_debug ("acpi_cpufreq_cpu_exit\n" );if (data) { -per_cpu (acfreq_data, policy->cpu) =NULL ; + policy->driver_data =NULL ;acpi_processor_unregister_performance (data->acpi_data, policy->cpu);free_cpumask_var (data->freqdomain_cpus);
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy) · line 881
Quoted 7 lines
drivers/cpufreq/acpi-cpufreq.c +1
-1@@ -868,7 +876,7 @@ static int acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy)static int acpi_cpufreq_resume (struct cpufreq_policy *policy) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;pr_debug ("acpi_cpufreq_resume\n" );
Rest looks fine.
Signature · 1 line
viresh
hi, Viresh
thanks for your reply. :)
Quoted 9 lines · Viresh Kumar
Quoted 3 lines · Pan Xinhui
Now policy has field of
driver_data, we can use it to get rid ofper_cpuacpi_cpufreq_data.Instead:
"Drivers can store their internal per-policy information inpolicy->driver_data, lets use it."
thanks.
Quoted 6 lines
Quoted 1 line
we have benefits after this replacing. 1) memory saving. 2) policy is
s/we/We
Also these points can be kept in separate lines :)
agree, thanks for pointing out it.
Quoted 10 lines
Quoted 1 line
shared by several cpus,
per_cpuseems not correct. useing *driver_data*using
Quoted 1 line
is more reasonable. 3) fix a memory leak in
acpi_cpufreq_cpu_exit.Not just that. You are also fixing NULL pointer dereferences if wrong or uninitialized per-cpu data is used, because of a recent hotplug of
policy->cpu.
acpi_cpufreq_cpu_exit has if (data) check.
In past days, I believed in that all per_cpu is initialized to NULL by default, like global variables.
Quoted 38 lines
Quoted 38 lines
as
policy->cpumight change after cpu hotplug. So sometimes we cant't free *data*, use *driver_data* to fix it. 4) fix a zero return value ofget_cur_freq_on_cpu. Onlyper_cpu(policy->cpu)is set to *data*, if we try to get cpufreq on other cpus, we get zero instead of correct values. Use *driver_data* to fix it.Signed-off-by Pan Xinhui <[email protected]>--- drivers/cpufreq/acpi-cpufreq.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)drivers/cpufreq/acpi-cpufreq.c +1
-3@@ -72,8 +72,6 @@ struct acpi_cpufreq_data {cpumask_var_t freqdomain_cpus; }; -static DEFINE_PER_CPU(struct acpi_cpufreq_data *, acfreq_data); -/* acpi_perf_data is a pointer to percpu data. */ static struct acpi_processor_performance __percpu *acpi_perf_data;@@ -144,7 +142,7 @@ static int _store_boost(int val)static ssize_t show_freqdomain_cpus (struct cpufreq_policy *policy,char *buf) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;return cpufreq_show_cpus (data->freqdomain_cpus, buf); }@@ -327,7 +325,8 @@ static void drv_write(struct drv_cmd *cmd)put_cpu (); }
Quoted 6 lines
drivers/cpufreq/acpi-cpufreq.c static void drv_write(struct drv_cmd *cmd) · line 329
Quoted 3 lines
drivers/cpufreq/acpi-cpufreq.c +2
-1-static u32 get_cur_val (const struct cpumask *mask) +static u32 get_cur_val (const struct cpumask *mask, +struct acpi_cpufreq_data *data)Run checkpatch --strict to see some alignment warnings here..
yes, confused..
Let me have a check.
Quoted 43 lines
Quoted 28 lines
drivers/cpufreq/acpi-cpufreq.c +4
-3{struct acpi_processor_performance *perf;struct drv_cmd cmd;@@ -335,7 +334,7 @@ static u32 get_cur_val(const struct cpumask *mask)if (unlikely (cpumask_empty (mask)))return 0 ; -switch (per_cpu (acfreq_data,cpumask_first (mask))->cpu_feature) { +switch (data->cpu_feature) {case SYSTEM_INTEL_MSR_CAPABLE : cmd.type =SYSTEM_INTEL_MSR_CAPABLE ; cmd.addr.msr.reg =MSR_IA32_PERF_CTL ;@@ -346,7 +345,7 @@ static u32 get_cur_val(const struct cpumask *mask)break ;case SYSTEM_IO_CAPABLE : cmd.type =SYSTEM_IO_CAPABLE ; - perf =per_cpu (acfreq_data,cpumask_first (mask))->acpi_data; + perf = data->acpi_data; cmd.addr.io.port = perf->control_register.address; cmd.addr.io.bit_width = perf->control_register.bit_width;break ;@@ -364,19 +363,26 @@ static u32 get_cur_val(const struct cpumask *mask)static unsigned int get_cur_freq_on_cpu (unsigned int cpu) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, cpu); +struct acpi_cpufreq_data *data; +struct cpufreq_policy *policy;drivers/cpufreq/acpi-cpufreq.c static u32 get_cur_val(const struct cpumask *mask) · line 377
Quoted 10 lines
drivers/cpufreq/acpi-cpufreq.c +5
-0unsigned int freq;unsigned int cached_freq;pr_debug ("get_cur_freq_on_cpu (%d)\n" , cpu); + policy =cpufreq_cpu_get (cpu); +if (unlikely (!policy)) +return 0 ; + + data = policy->driver_data;Do
(quoted diff) +1
-0+ cpufreq_cpu_put(policy);
Quoted 2 lines
right here. No need of doing this towards the end.
agree
thanks
Quoted 8 lines
drivers/cpufreq/acpi-cpufreq.c static u32 get_cur_val(const struct cpumask *mask) · line 379
Quoted 2 lines
drivers/cpufreq/acpi-cpufreq.c +0
-0if (unlikely (data ==NULL || data->acpi_data ==NULL || data->freq_table ==NULL )) {Maybe this if you like:
if (unlikely(! data || !data->acpi_data || !data->freq_table)) return 0;
Looks better
thanks
Quoted 11 lines
drivers/cpufreq/acpi-cpufreq.c static u32 get_cur_val(const struct cpumask *mask) · line 384
Quoted 5 lines
drivers/cpufreq/acpi-cpufreq.c +1
-0+cpufreq_cpu_put (policy);return 0 ; } cached_freq = data->freq_table[data->acpi_data->state].frequency;And this
freq_tablething gives you opportunity to write another patch :)policy already have:
policy->freq_table:)
Yes, upstream codes changes much.
These changes really help us. thanks for your hard work. :)
Quoted 66 lines
Quoted 66 lines
drivers/cpufreq/acpi-cpufreq.c +9
-7- freq =extract_freq (get_cur_val (cpumask_of (cpu)), data); + freq =extract_freq (get_cur_val (cpumask_of (cpu), data), data);if (freq != cached_freq) {/* * The dreaded BIOS frequency change behind our back. @@ -385,6 +391,8 @@ static unsigned int get_cur_freq_on_cpu(unsigned int cpu) data->resume =1 ; } +cpufreq_cpu_put (policy); +pr_debug ("cur freq = %u\n" , freq);return freq;@@ -397,7 +405,7 @@ static unsigned int check_freqs(const struct cpumask *mask, unsigned int freq,unsigned int i;for (i =0 ; i <100 ; i++) { - cur_freq =extract_freq (get_cur_val (mask), data); + cur_freq =extract_freq (get_cur_val (mask, data), data);if (cur_freq == freq)return 1 ;udelay (10 );@@ -408,7 +416,7 @@ static unsigned int check_freqs(const struct cpumask *mask, unsigned int freq,static int acpi_cpufreq_target (struct cpufreq_policy *policy,unsigned int index) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;struct acpi_processor_performance *perf;struct drv_cmd cmd;unsigned int next_perf_state =0 ;/* Index into perf table */ @@ -673,7 +681,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) } data->acpi_data =per_cpu_ptr (acpi_perf_data, cpu); -per_cpu (acfreq_data, cpu) = data; + policy->driver_data = data;if (cpu_has (c,X86_FEATURE_CONSTANT_TSC )) acpi_cpufreq_driver.flags |=CPUFREQ_CONST_LOOPS ;@@ -843,19 +851,19 @@ err_free_mask:free_cpumask_var (data->freqdomain_cpus); err_free:kfree (data); -per_cpu (acfreq_data, cpu) =NULL ; + policy->driver_data =NULL ;return result; }static int acpi_cpufreq_cpu_exit (struct cpufreq_policy *policy) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;pr_debug ("acpi_cpufreq_cpu_exit\n" );if (data) { -per_cpu (acfreq_data, policy->cpu) =NULL ; + policy->driver_data =NULL ;acpi_processor_unregister_performance (data->acpi_data, policy->cpu);free_cpumask_var (data->freqdomain_cpus);@@ -868,7 +876,7 @@ static int acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy)
Quoted 9 lines
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy) · line 881
Quoted 6 lines
drivers/cpufreq/acpi-cpufreq.c +1
-1static int acpi_cpufreq_resume (struct cpufreq_policy *policy) { -struct acpi_cpufreq_data *data =per_cpu (acfreq_data, policy->cpu); +struct acpi_cpufreq_data *data = policy->driver_data;pr_debug ("acpi_cpufreq_resume\n" );Rest looks fine.
thanks
xinhui