* Re: [PATCH] cpufreq: Allow accessing freq_table for offline CPUs
2015-07-08 7:38 [PATCH] cpufreq: Allow accessing freq_table for offline CPUs Viresh Kumar
@ 2015-07-08 8:05 ` Pi-Cheng Chen
2015-07-08 9:01 ` Javi Merino
2015-07-09 0:40 ` [PATCH] " Rafael J. Wysocki
2 siblings, 0 replies; 7+ messages in thread
From: Pi-Cheng Chen @ 2015-07-08 8:05 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Linaro Kernel Mailman List,
linux-pm@vger.kernel.org, Zhang Rui, Eduardo Valentin,
javi.merino, open list
On Wed, Jul 8, 2015 at 3:38 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Users of freq table may want to access it for any CPU from
> policy->related_cpus mask. One such user is cpu-cooling layer. It gets a
> list of 'clip_cpus' (equivalent to policy->related_cpus) during
> registration and tries to get freq_table for the first CPU of this mask.
>
> If the CPU, for which it tries to fetch freq_table, is offline,
> cpufreq_frequency_get_table() fails. This happens because it relies on
> cpufreq_cpu_get_raw() for its functioning which returns policy only for
> online CPUs.
>
> Fix this by considering offline CPUs, but with a restriction that the
> policy is active at that time.
>
> Because we will be using 'cpufreq_cpu_data' now, which is internal to
> cpufreq-core, lets also move cpufreq_frequency_get_table() to cpufreq.c
> file.
>
> Reported-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
Thanks.
> ---
> This was recently tried by Pi cheng but in a completely different way:
> http://marc.info/?l=linux-pm&m=143572367402758&w=2
>
> Its not broken by any stuff in 4.2, but would be good if we can push
> this for 4.2 as well.
>
> drivers/cpufreq/cpufreq.c | 9 +++++++++
> drivers/cpufreq/freq_table.c | 7 -------
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2c22e3902e72..26063afb3eba 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -169,6 +169,15 @@ struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy)
> }
> EXPORT_SYMBOL_GPL(get_governor_parent_kobj);
>
> +struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
> +{
> + struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
> +
> + return policy && !policy_is_inactive(policy) ?
> + policy->freq_table : NULL;
> +}
> +EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
> +
> static inline u64 get_cpu_idle_time_jiffy(unsigned int cpu, u64 *wall)
> {
> u64 idle_time;
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index df14766a8e06..7551f01e8536 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -299,13 +299,6 @@ EXPORT_SYMBOL_GPL(cpufreq_table_validate_and_show);
>
> struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu);
>
> -struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
> -{
> - struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
> - return policy ? policy->freq_table : NULL;
> -}
> -EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
> -
> MODULE_AUTHOR("Dominik Brodowski <linux@brodo.de>");
> MODULE_DESCRIPTION("CPUfreq frequency table helpers");
> MODULE_LICENSE("GPL");
> --
> 2.4.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] cpufreq: Allow accessing freq_table for offline CPUs
2015-07-08 7:38 [PATCH] cpufreq: Allow accessing freq_table for offline CPUs Viresh Kumar
2015-07-08 8:05 ` Pi-Cheng Chen
@ 2015-07-08 9:01 ` Javi Merino
2015-07-08 9:08 ` [PATCH V2] " Viresh Kumar
2015-07-09 0:40 ` [PATCH] " Rafael J. Wysocki
2 siblings, 1 reply; 7+ messages in thread
From: Javi Merino @ 2015-07-08 9:01 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, linaro-kernel@lists.linaro.org,
linux-pm@vger.kernel.org, rui.zhang@intel.com,
edubezval@gmail.com, Pi-Cheng Chen, open list
Hi Viresh,
one minor nit below
On Wed, Jul 08, 2015 at 08:38:15AM +0100, Viresh Kumar wrote:
> Users of freq table may want to access it for any CPU from
> policy->related_cpus mask. One such user is cpu-cooling layer. It gets a
> list of 'clip_cpus' (equivalent to policy->related_cpus) during
> registration and tries to get freq_table for the first CPU of this mask.
>
> If the CPU, for which it tries to fetch freq_table, is offline,
> cpufreq_frequency_get_table() fails. This happens because it relies on
> cpufreq_cpu_get_raw() for its functioning which returns policy only for
> online CPUs.
>
> Fix this by considering offline CPUs, but with a restriction that the
> policy is active at that time.
>
> Because we will be using 'cpufreq_cpu_data' now, which is internal to
> cpufreq-core, lets also move cpufreq_frequency_get_table() to cpufreq.c
> file.
>
> Reported-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> This was recently tried by Pi cheng but in a completely different way:
> http://marc.info/?l=linux-pm&m=143572367402758&w=2
>
> Its not broken by any stuff in 4.2, but would be good if we can push
> this for 4.2 as well.
>
> drivers/cpufreq/cpufreq.c | 9 +++++++++
> drivers/cpufreq/freq_table.c | 7 -------
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2c22e3902e72..26063afb3eba 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -169,6 +169,15 @@ struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy)
> }
> EXPORT_SYMBOL_GPL(get_governor_parent_kobj);
>
> +struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
> +{
> + struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
> +
> + return policy && !policy_is_inactive(policy) ?
> + policy->freq_table : NULL;
> +}
> +EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
> +
> static inline u64 get_cpu_idle_time_jiffy(unsigned int cpu, u64 *wall)
> {
> u64 idle_time;
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index df14766a8e06..7551f01e8536 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -299,13 +299,6 @@ EXPORT_SYMBOL_GPL(cpufreq_table_validate_and_show);
>
> struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu);
While you are at it, you can remove this prototype of
cpufreq_cpu_get_raw(), as it's no longer needed.
Cheers,
Javi
>
> -struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
> -{
> - struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
> - return policy ? policy->freq_table : NULL;
> -}
> -EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
> -
> MODULE_AUTHOR("Dominik Brodowski <linux@brodo.de>");
> MODULE_DESCRIPTION("CPUfreq frequency table helpers");
> MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH V2] cpufreq: Allow accessing freq_table for offline CPUs
2015-07-08 9:01 ` Javi Merino
@ 2015-07-08 9:08 ` Viresh Kumar
0 siblings, 0 replies; 7+ messages in thread
From: Viresh Kumar @ 2015-07-08 9:08 UTC (permalink / raw)
To: Rafael Wysocki
Cc: linaro-kernel, linux-pm, rui.zhang, edubezval, javi.merino,
pi-cheng.chen, Viresh Kumar, open list
Users of freq table may want to access it for any CPU from
policy->related_cpus mask. One such user is cpu-cooling layer. It gets a
list of 'clip_cpus' (equivalent to policy->related_cpus) during
registration and tries to get freq_table for the first CPU of this mask.
If the CPU, for which it tries to fetch freq_table, is offline,
cpufreq_frequency_get_table() fails. This happens because it relies on
cpufreq_cpu_get_raw() for its functioning which returns policy only for
online CPUs.
Fix this by considering offline CPUs, but with a restriction that the
policy is active at that time.
Because we will be using 'cpufreq_cpu_data' now, which is internal to
cpufreq-core, lets also move cpufreq_frequency_get_table() to cpufreq.c
file.
Reported-and-tested-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2: Get rid of cpufreq_cpu_data()'s declaration as well.. Thanks Javi.
drivers/cpufreq/cpufreq.c | 9 +++++++++
drivers/cpufreq/freq_table.c | 9 ---------
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2c22e3902e72..26063afb3eba 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -169,6 +169,15 @@ struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy)
}
EXPORT_SYMBOL_GPL(get_governor_parent_kobj);
+struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
+{
+ struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
+
+ return policy && !policy_is_inactive(policy) ?
+ policy->freq_table : NULL;
+}
+EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
+
static inline u64 get_cpu_idle_time_jiffy(unsigned int cpu, u64 *wall)
{
u64 idle_time;
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index df14766a8e06..dfbbf981ed56 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -297,15 +297,6 @@ int cpufreq_table_validate_and_show(struct cpufreq_policy *policy,
}
EXPORT_SYMBOL_GPL(cpufreq_table_validate_and_show);
-struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu);
-
-struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
-{
- struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
- return policy ? policy->freq_table : NULL;
-}
-EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
-
MODULE_AUTHOR("Dominik Brodowski <linux@brodo.de>");
MODULE_DESCRIPTION("CPUfreq frequency table helpers");
MODULE_LICENSE("GPL");
--
2.4.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] cpufreq: Allow accessing freq_table for offline CPUs
2015-07-08 7:38 [PATCH] cpufreq: Allow accessing freq_table for offline CPUs Viresh Kumar
2015-07-08 8:05 ` Pi-Cheng Chen
2015-07-08 9:01 ` Javi Merino
@ 2015-07-09 0:40 ` Rafael J. Wysocki
2015-07-09 5:13 ` Viresh Kumar
2 siblings, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2015-07-09 0:40 UTC (permalink / raw)
To: Viresh Kumar
Cc: linaro-kernel, linux-pm, rui.zhang, edubezval, javi.merino,
Pi-Cheng Chen, open list
On Wednesday, July 08, 2015 01:08:15 PM Viresh Kumar wrote:
> Users of freq table may want to access it for any CPU from
> policy->related_cpus mask. One such user is cpu-cooling layer. It gets a
> list of 'clip_cpus' (equivalent to policy->related_cpus) during
> registration and tries to get freq_table for the first CPU of this mask.
>
> If the CPU, for which it tries to fetch freq_table, is offline,
> cpufreq_frequency_get_table() fails. This happens because it relies on
> cpufreq_cpu_get_raw() for its functioning which returns policy only for
> online CPUs.
>
> Fix this by considering offline CPUs, but with a restriction that the
> policy is active at that time.
The above sentence is completely unclear to anyone unfamiliar with the
code in question.
The fix is to access the policy data structure for the given CPU directly
(which also returns a valid policy for offline CPUs), but the policy
itself has to be active (meaning that at least one CPU using it is online)
for the frequency table to be returned.
Right?
> Because we will be using 'cpufreq_cpu_data' now, which is internal to
> cpufreq-core, lets also move cpufreq_frequency_get_table() to cpufreq.c
> file.
>
> Reported-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> This was recently tried by Pi cheng but in a completely different way:
> http://marc.info/?l=linux-pm&m=143572367402758&w=2
>
> Its not broken by any stuff in 4.2, but would be good if we can push
> this for 4.2 as well.
>
> drivers/cpufreq/cpufreq.c | 9 +++++++++
> drivers/cpufreq/freq_table.c | 7 -------
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2c22e3902e72..26063afb3eba 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -169,6 +169,15 @@ struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy)
> }
> EXPORT_SYMBOL_GPL(get_governor_parent_kobj);
>
> +struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
> +{
> + struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
> +
> + return policy && !policy_is_inactive(policy) ?
> + policy->freq_table : NULL;
> +}
> +EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
> +
> static inline u64 get_cpu_idle_time_jiffy(unsigned int cpu, u64 *wall)
> {
> u64 idle_time;
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index df14766a8e06..7551f01e8536 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -299,13 +299,6 @@ EXPORT_SYMBOL_GPL(cpufreq_table_validate_and_show);
>
> struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu);
>
> -struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
> -{
> - struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
> - return policy ? policy->freq_table : NULL;
> -}
> -EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
> -
> MODULE_AUTHOR("Dominik Brodowski <linux@brodo.de>");
> MODULE_DESCRIPTION("CPUfreq frequency table helpers");
> MODULE_LICENSE("GPL");
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] cpufreq: Allow accessing freq_table for offline CPUs
2015-07-09 0:40 ` [PATCH] " Rafael J. Wysocki
@ 2015-07-09 5:13 ` Viresh Kumar
2015-07-10 0:10 ` Rafael J. Wysocki
0 siblings, 1 reply; 7+ messages in thread
From: Viresh Kumar @ 2015-07-09 5:13 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linaro-kernel, linux-pm, rui.zhang, edubezval, javi.merino,
Pi-Cheng Chen, open list
On 09-07-15, 02:40, Rafael J. Wysocki wrote:
> The above sentence is completely unclear to anyone unfamiliar with the
> code in question.
>
> The fix is to access the policy data structure for the given CPU directly
> (which also returns a valid policy for offline CPUs), but the policy
> itself has to be active (meaning that at least one CPU using it is online)
> for the frequency table to be returned.
>
> Right?
Yeah..
-------------------8<--------------------
Message-Id: <9c37b9a711b6dc6e419e256fd62fcc93a29db4e4.1436418686.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 8 Jul 2015 12:53:03 +0530
Subject: [PATCH V3] cpufreq: Allow accessing freq_table for offline CPUs
Users of freq table may want to access it for any CPU from
policy->related_cpus mask. One such user is cpu-cooling layer. It gets a
list of 'clip_cpus' (equivalent to policy->related_cpus) during
registration and tries to get freq_table for the first CPU of this mask.
If the CPU, for which it tries to fetch freq_table, is offline,
cpufreq_frequency_get_table() fails. This happens because it relies on
cpufreq_cpu_get_raw() for its functioning which returns policy only for
online CPUs.
The fix is to access the policy data structure for the given CPU
directly (which also returns a valid policy for offline CPUs), but the
policy itself has to be active (meaning that at least one CPU using it
is online) for the frequency table to be returned.
Because we will be using 'cpufreq_cpu_data' now, which is internal to
cpufreq-core, lets also move cpufreq_frequency_get_table() to cpufreq.c
file.
Reported-and-tested-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V3-> Changelog improvement suggested by Rafael.
drivers/cpufreq/cpufreq.c | 9 +++++++++
drivers/cpufreq/freq_table.c | 9 ---------
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 36e97a5a7e20..a7b6ac6e048e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -169,6 +169,15 @@ struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy)
}
EXPORT_SYMBOL_GPL(get_governor_parent_kobj);
+struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
+{
+ struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
+
+ return policy && !policy_is_inactive(policy) ?
+ policy->freq_table : NULL;
+}
+EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
+
static inline u64 get_cpu_idle_time_jiffy(unsigned int cpu, u64 *wall)
{
u64 idle_time;
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index df14766a8e06..dfbbf981ed56 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -297,15 +297,6 @@ int cpufreq_table_validate_and_show(struct cpufreq_policy *policy,
}
EXPORT_SYMBOL_GPL(cpufreq_table_validate_and_show);
-struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu);
-
-struct cpufreq_frequency_table *cpufreq_frequency_get_table(unsigned int cpu)
-{
- struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
- return policy ? policy->freq_table : NULL;
-}
-EXPORT_SYMBOL_GPL(cpufreq_frequency_get_table);
-
MODULE_AUTHOR("Dominik Brodowski <linux@brodo.de>");
MODULE_DESCRIPTION("CPUfreq frequency table helpers");
MODULE_LICENSE("GPL");
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] cpufreq: Allow accessing freq_table for offline CPUs
2015-07-09 5:13 ` Viresh Kumar
@ 2015-07-10 0:10 ` Rafael J. Wysocki
0 siblings, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2015-07-10 0:10 UTC (permalink / raw)
To: Viresh Kumar
Cc: linaro-kernel, linux-pm, rui.zhang, edubezval, javi.merino,
Pi-Cheng Chen, open list
On Thursday, July 09, 2015 10:43:56 AM Viresh Kumar wrote:
> On 09-07-15, 02:40, Rafael J. Wysocki wrote:
> > The above sentence is completely unclear to anyone unfamiliar with the
> > code in question.
> >
> > The fix is to access the policy data structure for the given CPU directly
> > (which also returns a valid policy for offline CPUs), but the policy
> > itself has to be active (meaning that at least one CPU using it is online)
> > for the frequency table to be returned.
> >
> > Right?
>
> Yeah..
Applied with minor tweaks to the subject & changelog, thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 7+ messages in thread