* [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
@ 2019-04-29 7:24 Yue Hu
2019-04-29 7:26 ` Viresh Kumar
2019-04-29 7:37 ` Rafael J. Wysocki
0 siblings, 2 replies; 8+ messages in thread
From: Yue Hu @ 2019-04-29 7:24 UTC (permalink / raw)
To: viresh.kumar, rjw, rafael.j.wysocki; +Cc: linux-pm, huyue2
From: Yue Hu <huyue2@yulong.com>
In cpufreq_init_policy() we will check if there's last_governor for target
and setpolicy type. However last_governor is set only if has_target() is
true in cpufreq_offline(). That means find last_governor for setpolicy
type is pointless. Also new_policy.governor will not be used if ->setpolicy
callback is set in cpufreq_set_policy().
Moreover, there's duplicate ->setpolicy check in using default policy path.
Let's add a new helper function to avoid it. Also update a little comment.
Signed-off-by: Yue Hu <huyue2@yulong.com>
---
v2: fix ->setplicy typo.
v3:
- let cpufreq_parse_governor() only handle !set_policy.
- fix using {} in the if block.
- change helper function name.
- update comment, commit message.
drivers/cpufreq/cpufreq.c | 116 ++++++++++++++++++++++++++--------------------
1 file changed, 65 insertions(+), 51 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 0322cce..ce8a01d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -578,50 +578,52 @@ static struct cpufreq_governor *find_governor(const char *str_governor)
return NULL;
}
+static int cpufreq_parse_policy(char *str_governor,
+ struct cpufreq_policy *policy)
+{
+ if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN)) {
+ policy->policy = CPUFREQ_POLICY_PERFORMANCE;
+ return 0;
+ }
+ if (!strncasecmp(str_governor, "powersave", CPUFREQ_NAME_LEN)) {
+ policy->policy = CPUFREQ_POLICY_POWERSAVE;
+ return 0;
+ }
+ return -EINVAL;
+}
+
/**
- * cpufreq_parse_governor - parse a governor string
+ * cpufreq_parse_governor - parse a governor string only for !setpolicy
*/
static int cpufreq_parse_governor(char *str_governor,
struct cpufreq_policy *policy)
{
- if (cpufreq_driver->setpolicy) {
- if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN)) {
- policy->policy = CPUFREQ_POLICY_PERFORMANCE;
- return 0;
- }
-
- if (!strncasecmp(str_governor, "powersave", CPUFREQ_NAME_LEN)) {
- policy->policy = CPUFREQ_POLICY_POWERSAVE;
- return 0;
- }
- } else {
- struct cpufreq_governor *t;
+ struct cpufreq_governor *t;
- mutex_lock(&cpufreq_governor_mutex);
+ mutex_lock(&cpufreq_governor_mutex);
- t = find_governor(str_governor);
- if (!t) {
- int ret;
+ t = find_governor(str_governor);
+ if (!t) {
+ int ret;
- mutex_unlock(&cpufreq_governor_mutex);
+ mutex_unlock(&cpufreq_governor_mutex);
- ret = request_module("cpufreq_%s", str_governor);
- if (ret)
- return -EINVAL;
+ ret = request_module("cpufreq_%s", str_governor);
+ if (ret)
+ return -EINVAL;
- mutex_lock(&cpufreq_governor_mutex);
+ mutex_lock(&cpufreq_governor_mutex);
- t = find_governor(str_governor);
- }
- if (t && !try_module_get(t->owner))
- t = NULL;
+ t = find_governor(str_governor);
+ }
+ if (t && !try_module_get(t->owner))
+ t = NULL;
- mutex_unlock(&cpufreq_governor_mutex);
+ mutex_unlock(&cpufreq_governor_mutex);
- if (t) {
- policy->governor = t;
- return 0;
- }
+ if (t) {
+ policy->governor = t;
+ return 0;
}
return -EINVAL;
@@ -746,8 +748,13 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
if (ret != 1)
return -EINVAL;
- if (cpufreq_parse_governor(str_governor, &new_policy))
- return -EINVAL;
+ if (cpufreq_driver->setpolicy) {
+ if (cpufreq_parse_policy(str_governor, &new_policy))
+ return -EINVAL;
+ } else {
+ if (cpufreq_parse_governor(str_governor, &new_policy))
+ return -EINVAL;
+ }
ret = cpufreq_set_policy(policy, &new_policy);
@@ -1020,32 +1027,39 @@ __weak struct cpufreq_governor *cpufreq_default_governor(void)
static int cpufreq_init_policy(struct cpufreq_policy *policy)
{
- struct cpufreq_governor *gov = NULL;
+ struct cpufreq_governor *gov = NULL, *def_gov = NULL;
struct cpufreq_policy new_policy;
memcpy(&new_policy, policy, sizeof(*policy));
- /* Update governor of new_policy to the governor used before hotplug */
- gov = find_governor(policy->last_governor);
- if (gov) {
- pr_debug("Restoring governor %s for cpu %d\n",
+ def_gov = cpufreq_default_governor();
+
+ if (has_target()) {
+ /*
+ * Update governor of new_policy to the governor used before
+ * hotplug
+ */
+ gov = find_governor(policy->last_governor);
+ if (gov) {
+ pr_debug("Restoring governor %s for cpu %d\n",
policy->governor->name, policy->cpu);
+ } else {
+ if (!def_gov)
+ return -ENODATA;
+ gov = def_gov;
+ }
+ new_policy.governor = gov;
} else {
- gov = cpufreq_default_governor();
- if (!gov)
- return -ENODATA;
- }
-
- new_policy.governor = gov;
-
- /* Use the default policy if there is no last_policy. */
- if (cpufreq_driver->setpolicy) {
- if (policy->last_policy)
+ /* Use the default policy if there is no last_policy. */
+ if (policy->last_policy) {
new_policy.policy = policy->last_policy;
- else
- cpufreq_parse_governor(gov->name, &new_policy);
+ } else {
+ if (!def_gov)
+ return -ENODATA;
+ cpufreq_parse_policy(def_gov->name, &new_policy);
+ }
}
- /* set default policy */
+
return cpufreq_set_policy(policy, &new_policy);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
2019-04-29 7:24 [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy() Yue Hu
@ 2019-04-29 7:26 ` Viresh Kumar
2019-04-29 7:37 ` Rafael J. Wysocki
1 sibling, 0 replies; 8+ messages in thread
From: Viresh Kumar @ 2019-04-29 7:26 UTC (permalink / raw)
To: Yue Hu; +Cc: rjw, rafael.j.wysocki, linux-pm, huyue2
On 29-04-19, 15:24, Yue Hu wrote:
> From: Yue Hu <huyue2@yulong.com>
>
> In cpufreq_init_policy() we will check if there's last_governor for target
> and setpolicy type. However last_governor is set only if has_target() is
> true in cpufreq_offline(). That means find last_governor for setpolicy
> type is pointless. Also new_policy.governor will not be used if ->setpolicy
> callback is set in cpufreq_set_policy().
>
> Moreover, there's duplicate ->setpolicy check in using default policy path.
> Let's add a new helper function to avoid it. Also update a little comment.
>
> Signed-off-by: Yue Hu <huyue2@yulong.com>
> ---
> v2: fix ->setplicy typo.
> v3:
> - let cpufreq_parse_governor() only handle !set_policy.
> - fix using {} in the if block.
> - change helper function name.
> - update comment, commit message.
>
> drivers/cpufreq/cpufreq.c | 116 ++++++++++++++++++++++++++--------------------
> 1 file changed, 65 insertions(+), 51 deletions(-)
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
2019-04-29 7:24 [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy() Yue Hu
2019-04-29 7:26 ` Viresh Kumar
@ 2019-04-29 7:37 ` Rafael J. Wysocki
2019-04-29 7:56 ` Yue Hu
1 sibling, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2019-04-29 7:37 UTC (permalink / raw)
To: Yue Hu; +Cc: Viresh Kumar, Rafael J. Wysocki, Rafael Wysocki, Linux PM, huyue2
On Mon, Apr 29, 2019 at 9:24 AM Yue Hu <zbestahu@gmail.com> wrote:
>
> From: Yue Hu <huyue2@yulong.com>
>
> In cpufreq_init_policy() we will check if there's last_governor for target
> and setpolicy type. However last_governor is set only if has_target() is
> true in cpufreq_offline(). That means find last_governor for setpolicy
> type is pointless. Also new_policy.governor will not be used if ->setpolicy
> callback is set in cpufreq_set_policy().
>
> Moreover, there's duplicate ->setpolicy check in using default policy path.
> Let's add a new helper function to avoid it. Also update a little comment.
>
> Signed-off-by: Yue Hu <huyue2@yulong.com>
Have you tested this with the intel_pstate driver (in the active mode)?
> ---
> v2: fix ->setplicy typo.
> v3:
> - let cpufreq_parse_governor() only handle !set_policy.
> - fix using {} in the if block.
> - change helper function name.
> - update comment, commit message.
>
> drivers/cpufreq/cpufreq.c | 116 ++++++++++++++++++++++++++--------------------
> 1 file changed, 65 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 0322cce..ce8a01d 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -578,50 +578,52 @@ static struct cpufreq_governor *find_governor(const char *str_governor)
> return NULL;
> }
>
> +static int cpufreq_parse_policy(char *str_governor,
> + struct cpufreq_policy *policy)
> +{
> + if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN)) {
> + policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> + return 0;
> + }
> + if (!strncasecmp(str_governor, "powersave", CPUFREQ_NAME_LEN)) {
> + policy->policy = CPUFREQ_POLICY_POWERSAVE;
> + return 0;
> + }
> + return -EINVAL;
> +}
> +
> /**
> - * cpufreq_parse_governor - parse a governor string
> + * cpufreq_parse_governor - parse a governor string only for !setpolicy
> */
> static int cpufreq_parse_governor(char *str_governor,
> struct cpufreq_policy *policy)
> {
> - if (cpufreq_driver->setpolicy) {
> - if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN)) {
> - policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> - return 0;
> - }
> -
> - if (!strncasecmp(str_governor, "powersave", CPUFREQ_NAME_LEN)) {
> - policy->policy = CPUFREQ_POLICY_POWERSAVE;
> - return 0;
> - }
> - } else {
> - struct cpufreq_governor *t;
> + struct cpufreq_governor *t;
>
> - mutex_lock(&cpufreq_governor_mutex);
> + mutex_lock(&cpufreq_governor_mutex);
>
> - t = find_governor(str_governor);
> - if (!t) {
> - int ret;
> + t = find_governor(str_governor);
> + if (!t) {
> + int ret;
>
> - mutex_unlock(&cpufreq_governor_mutex);
> + mutex_unlock(&cpufreq_governor_mutex);
>
> - ret = request_module("cpufreq_%s", str_governor);
> - if (ret)
> - return -EINVAL;
> + ret = request_module("cpufreq_%s", str_governor);
> + if (ret)
> + return -EINVAL;
>
> - mutex_lock(&cpufreq_governor_mutex);
> + mutex_lock(&cpufreq_governor_mutex);
>
> - t = find_governor(str_governor);
> - }
> - if (t && !try_module_get(t->owner))
> - t = NULL;
> + t = find_governor(str_governor);
> + }
> + if (t && !try_module_get(t->owner))
> + t = NULL;
>
> - mutex_unlock(&cpufreq_governor_mutex);
> + mutex_unlock(&cpufreq_governor_mutex);
>
> - if (t) {
> - policy->governor = t;
> - return 0;
> - }
> + if (t) {
> + policy->governor = t;
> + return 0;
> }
>
> return -EINVAL;
> @@ -746,8 +748,13 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
> if (ret != 1)
> return -EINVAL;
>
> - if (cpufreq_parse_governor(str_governor, &new_policy))
> - return -EINVAL;
> + if (cpufreq_driver->setpolicy) {
> + if (cpufreq_parse_policy(str_governor, &new_policy))
> + return -EINVAL;
> + } else {
> + if (cpufreq_parse_governor(str_governor, &new_policy))
> + return -EINVAL;
> + }
>
> ret = cpufreq_set_policy(policy, &new_policy);
>
> @@ -1020,32 +1027,39 @@ __weak struct cpufreq_governor *cpufreq_default_governor(void)
>
> static int cpufreq_init_policy(struct cpufreq_policy *policy)
> {
> - struct cpufreq_governor *gov = NULL;
> + struct cpufreq_governor *gov = NULL, *def_gov = NULL;
> struct cpufreq_policy new_policy;
>
> memcpy(&new_policy, policy, sizeof(*policy));
>
> - /* Update governor of new_policy to the governor used before hotplug */
> - gov = find_governor(policy->last_governor);
> - if (gov) {
> - pr_debug("Restoring governor %s for cpu %d\n",
> + def_gov = cpufreq_default_governor();
> +
> + if (has_target()) {
> + /*
> + * Update governor of new_policy to the governor used before
> + * hotplug
> + */
> + gov = find_governor(policy->last_governor);
> + if (gov) {
> + pr_debug("Restoring governor %s for cpu %d\n",
> policy->governor->name, policy->cpu);
> + } else {
> + if (!def_gov)
> + return -ENODATA;
> + gov = def_gov;
> + }
> + new_policy.governor = gov;
> } else {
> - gov = cpufreq_default_governor();
> - if (!gov)
> - return -ENODATA;
> - }
> -
> - new_policy.governor = gov;
> -
> - /* Use the default policy if there is no last_policy. */
> - if (cpufreq_driver->setpolicy) {
> - if (policy->last_policy)
> + /* Use the default policy if there is no last_policy. */
> + if (policy->last_policy) {
> new_policy.policy = policy->last_policy;
> - else
> - cpufreq_parse_governor(gov->name, &new_policy);
> + } else {
> + if (!def_gov)
> + return -ENODATA;
> + cpufreq_parse_policy(def_gov->name, &new_policy);
> + }
> }
> - /* set default policy */
> +
> return cpufreq_set_policy(policy, &new_policy);
> }
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
2019-04-29 7:37 ` Rafael J. Wysocki
@ 2019-04-29 7:56 ` Yue Hu
2019-04-29 8:37 ` Rafael J. Wysocki
0 siblings, 1 reply; 8+ messages in thread
From: Yue Hu @ 2019-04-29 7:56 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Viresh Kumar, Rafael J. Wysocki, Rafael Wysocki, Linux PM, huyue2
On Mon, 29 Apr 2019 09:37:27 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:
> On Mon, Apr 29, 2019 at 9:24 AM Yue Hu <zbestahu@gmail.com> wrote:
> >
> > From: Yue Hu <huyue2@yulong.com>
> >
> > In cpufreq_init_policy() we will check if there's last_governor for target
> > and setpolicy type. However last_governor is set only if has_target() is
> > true in cpufreq_offline(). That means find last_governor for setpolicy
> > type is pointless. Also new_policy.governor will not be used if ->setpolicy
> > callback is set in cpufreq_set_policy().
> >
> > Moreover, there's duplicate ->setpolicy check in using default policy path.
> > Let's add a new helper function to avoid it. Also update a little comment.
> >
> > Signed-off-by: Yue Hu <huyue2@yulong.com>
>
> Have you tested this with the intel_pstate driver (in the active mode)?
No, just tested for ARM. It should be common logic from code perspective. Has
any issue in this change?
>
> > ---
> > v2: fix ->setplicy typo.
> > v3:
> > - let cpufreq_parse_governor() only handle !set_policy.
> > - fix using {} in the if block.
> > - change helper function name.
> > - update comment, commit message.
> >
> > drivers/cpufreq/cpufreq.c | 116 ++++++++++++++++++++++++++--------------------
> > 1 file changed, 65 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 0322cce..ce8a01d 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -578,50 +578,52 @@ static struct cpufreq_governor *find_governor(const char *str_governor)
> > return NULL;
> > }
> >
> > +static int cpufreq_parse_policy(char *str_governor,
> > + struct cpufreq_policy *policy)
> > +{
> > + if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN)) {
> > + policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> > + return 0;
> > + }
> > + if (!strncasecmp(str_governor, "powersave", CPUFREQ_NAME_LEN)) {
> > + policy->policy = CPUFREQ_POLICY_POWERSAVE;
> > + return 0;
> > + }
> > + return -EINVAL;
> > +}
> > +
> > /**
> > - * cpufreq_parse_governor - parse a governor string
> > + * cpufreq_parse_governor - parse a governor string only for !setpolicy
> > */
> > static int cpufreq_parse_governor(char *str_governor,
> > struct cpufreq_policy *policy)
> > {
> > - if (cpufreq_driver->setpolicy) {
> > - if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN)) {
> > - policy->policy = CPUFREQ_POLICY_PERFORMANCE;
> > - return 0;
> > - }
> > -
> > - if (!strncasecmp(str_governor, "powersave", CPUFREQ_NAME_LEN)) {
> > - policy->policy = CPUFREQ_POLICY_POWERSAVE;
> > - return 0;
> > - }
> > - } else {
> > - struct cpufreq_governor *t;
> > + struct cpufreq_governor *t;
> >
> > - mutex_lock(&cpufreq_governor_mutex);
> > + mutex_lock(&cpufreq_governor_mutex);
> >
> > - t = find_governor(str_governor);
> > - if (!t) {
> > - int ret;
> > + t = find_governor(str_governor);
> > + if (!t) {
> > + int ret;
> >
> > - mutex_unlock(&cpufreq_governor_mutex);
> > + mutex_unlock(&cpufreq_governor_mutex);
> >
> > - ret = request_module("cpufreq_%s", str_governor);
> > - if (ret)
> > - return -EINVAL;
> > + ret = request_module("cpufreq_%s", str_governor);
> > + if (ret)
> > + return -EINVAL;
> >
> > - mutex_lock(&cpufreq_governor_mutex);
> > + mutex_lock(&cpufreq_governor_mutex);
> >
> > - t = find_governor(str_governor);
> > - }
> > - if (t && !try_module_get(t->owner))
> > - t = NULL;
> > + t = find_governor(str_governor);
> > + }
> > + if (t && !try_module_get(t->owner))
> > + t = NULL;
> >
> > - mutex_unlock(&cpufreq_governor_mutex);
> > + mutex_unlock(&cpufreq_governor_mutex);
> >
> > - if (t) {
> > - policy->governor = t;
> > - return 0;
> > - }
> > + if (t) {
> > + policy->governor = t;
> > + return 0;
> > }
> >
> > return -EINVAL;
> > @@ -746,8 +748,13 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy,
> > if (ret != 1)
> > return -EINVAL;
> >
> > - if (cpufreq_parse_governor(str_governor, &new_policy))
> > - return -EINVAL;
> > + if (cpufreq_driver->setpolicy) {
> > + if (cpufreq_parse_policy(str_governor, &new_policy))
> > + return -EINVAL;
> > + } else {
> > + if (cpufreq_parse_governor(str_governor, &new_policy))
> > + return -EINVAL;
> > + }
> >
> > ret = cpufreq_set_policy(policy, &new_policy);
> >
> > @@ -1020,32 +1027,39 @@ __weak struct cpufreq_governor *cpufreq_default_governor(void)
> >
> > static int cpufreq_init_policy(struct cpufreq_policy *policy)
> > {
> > - struct cpufreq_governor *gov = NULL;
> > + struct cpufreq_governor *gov = NULL, *def_gov = NULL;
> > struct cpufreq_policy new_policy;
> >
> > memcpy(&new_policy, policy, sizeof(*policy));
> >
> > - /* Update governor of new_policy to the governor used before hotplug */
> > - gov = find_governor(policy->last_governor);
> > - if (gov) {
> > - pr_debug("Restoring governor %s for cpu %d\n",
> > + def_gov = cpufreq_default_governor();
> > +
> > + if (has_target()) {
> > + /*
> > + * Update governor of new_policy to the governor used before
> > + * hotplug
> > + */
> > + gov = find_governor(policy->last_governor);
> > + if (gov) {
> > + pr_debug("Restoring governor %s for cpu %d\n",
> > policy->governor->name, policy->cpu);
> > + } else {
> > + if (!def_gov)
> > + return -ENODATA;
> > + gov = def_gov;
> > + }
> > + new_policy.governor = gov;
> > } else {
> > - gov = cpufreq_default_governor();
> > - if (!gov)
> > - return -ENODATA;
> > - }
> > -
> > - new_policy.governor = gov;
> > -
> > - /* Use the default policy if there is no last_policy. */
> > - if (cpufreq_driver->setpolicy) {
> > - if (policy->last_policy)
> > + /* Use the default policy if there is no last_policy. */
> > + if (policy->last_policy) {
> > new_policy.policy = policy->last_policy;
> > - else
> > - cpufreq_parse_governor(gov->name, &new_policy);
> > + } else {
> > + if (!def_gov)
> > + return -ENODATA;
> > + cpufreq_parse_policy(def_gov->name, &new_policy);
> > + }
> > }
> > - /* set default policy */
> > +
> > return cpufreq_set_policy(policy, &new_policy);
> > }
> >
> > --
> > 1.9.1
> >
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
2019-04-29 7:56 ` Yue Hu
@ 2019-04-29 8:37 ` Rafael J. Wysocki
2019-04-30 1:44 ` Yue Hu
2019-05-13 2:11 ` Yue Hu
0 siblings, 2 replies; 8+ messages in thread
From: Rafael J. Wysocki @ 2019-04-29 8:37 UTC (permalink / raw)
To: Yue Hu; +Cc: Rafael J. Wysocki, Viresh Kumar, Rafael Wysocki, Linux PM, huyue2
On Monday, April 29, 2019 9:56:40 AM CEST Yue Hu wrote:
> On Mon, 29 Apr 2019 09:37:27 +0200
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
> > On Mon, Apr 29, 2019 at 9:24 AM Yue Hu <zbestahu@gmail.com> wrote:
> > >
> > > From: Yue Hu <huyue2@yulong.com>
> > >
> > > In cpufreq_init_policy() we will check if there's last_governor for target
> > > and setpolicy type. However last_governor is set only if has_target() is
> > > true in cpufreq_offline(). That means find last_governor for setpolicy
> > > type is pointless. Also new_policy.governor will not be used if ->setpolicy
> > > callback is set in cpufreq_set_policy().
> > >
> > > Moreover, there's duplicate ->setpolicy check in using default policy path.
> > > Let's add a new helper function to avoid it. Also update a little comment.
> > >
> > > Signed-off-by: Yue Hu <huyue2@yulong.com>
> >
> > Have you tested this with the intel_pstate driver (in the active mode)?
>
> No, just tested for ARM. It should be common logic from code perspective.
But it is prudent to test changes on various configurations that may be affected.
Testing intel_pstate shouldn't be too difficult.
> Has any issue in this change?
Not in principle, but I need to check the details.
In general I'm a bit hesitant to take changes that haven't been tested properly.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
2019-04-29 8:37 ` Rafael J. Wysocki
@ 2019-04-30 1:44 ` Yue Hu
2019-05-13 2:11 ` Yue Hu
1 sibling, 0 replies; 8+ messages in thread
From: Yue Hu @ 2019-04-30 1:44 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Viresh Kumar, Rafael Wysocki, Linux PM, huyue2
On Mon, 29 Apr 2019 10:37:45 +0200
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> On Monday, April 29, 2019 9:56:40 AM CEST Yue Hu wrote:
> > On Mon, 29 Apr 2019 09:37:27 +0200
> > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >
> > > On Mon, Apr 29, 2019 at 9:24 AM Yue Hu <zbestahu@gmail.com> wrote:
> > > >
> > > > From: Yue Hu <huyue2@yulong.com>
> > > >
> > > > In cpufreq_init_policy() we will check if there's last_governor for target
> > > > and setpolicy type. However last_governor is set only if has_target() is
> > > > true in cpufreq_offline(). That means find last_governor for setpolicy
> > > > type is pointless. Also new_policy.governor will not be used if ->setpolicy
> > > > callback is set in cpufreq_set_policy().
> > > >
> > > > Moreover, there's duplicate ->setpolicy check in using default policy path.
> > > > Let's add a new helper function to avoid it. Also update a little comment.
> > > >
> > > > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > >
> > > Have you tested this with the intel_pstate driver (in the active mode)?
> >
> > No, just tested for ARM. It should be common logic from code perspective.
>
> But it is prudent to test changes on various configurations that may be affected.
>
> Testing intel_pstate shouldn't be too difficult.
>
> > Has any issue in this change?
>
> Not in principle, but I need to check the details.
>
> In general I'm a bit hesitant to take changes that haven't been tested properly.
Ok, i will try to test it with intel_pstate driver later.
>
> Thanks,
> Rafael
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
2019-04-29 8:37 ` Rafael J. Wysocki
2019-04-30 1:44 ` Yue Hu
@ 2019-05-13 2:11 ` Yue Hu
2019-05-14 21:34 ` Rafael J. Wysocki
1 sibling, 1 reply; 8+ messages in thread
From: Yue Hu @ 2019-05-13 2:11 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Viresh Kumar, Rafael Wysocki, Linux PM, huyue2
On Mon, 29 Apr 2019 10:37:45 +0200
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> On Monday, April 29, 2019 9:56:40 AM CEST Yue Hu wrote:
> > On Mon, 29 Apr 2019 09:37:27 +0200
> > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> >
> > > On Mon, Apr 29, 2019 at 9:24 AM Yue Hu <zbestahu@gmail.com> wrote:
> > > >
> > > > From: Yue Hu <huyue2@yulong.com>
> > > >
> > > > In cpufreq_init_policy() we will check if there's last_governor for target
> > > > and setpolicy type. However last_governor is set only if has_target() is
> > > > true in cpufreq_offline(). That means find last_governor for setpolicy
> > > > type is pointless. Also new_policy.governor will not be used if ->setpolicy
> > > > callback is set in cpufreq_set_policy().
> > > >
> > > > Moreover, there's duplicate ->setpolicy check in using default policy path.
> > > > Let's add a new helper function to avoid it. Also update a little comment.
> > > >
> > > > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > >
> > > Have you tested this with the intel_pstate driver (in the active mode)?
> >
> > No, just tested for ARM. It should be common logic from code perspective.
>
> But it is prudent to test changes on various configurations that may be affected.
>
> Testing intel_pstate shouldn't be too difficult.
Hi Rafael,
Sorry for late test since i take time to emulate/find the board with Sandy brigdge
processor.
I'm sure intel_pstate setpolicy driver is working fine by tests containing policy init,
hotplug and changing scaling_governor.
Thx.
>
> > Has any issue in this change?
>
> Not in principle, but I need to check the details.
>
> In general I'm a bit hesitant to take changes that haven't been tested properly.
>
> Thanks,
> Rafael
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
2019-05-13 2:11 ` Yue Hu
@ 2019-05-14 21:34 ` Rafael J. Wysocki
0 siblings, 0 replies; 8+ messages in thread
From: Rafael J. Wysocki @ 2019-05-14 21:34 UTC (permalink / raw)
To: Yue Hu; +Cc: Rafael J. Wysocki, Viresh Kumar, Rafael Wysocki, Linux PM, huyue2
On Monday, May 13, 2019 4:11:40 AM CEST Yue Hu wrote:
> On Mon, 29 Apr 2019 10:37:45 +0200
> "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
>
> > On Monday, April 29, 2019 9:56:40 AM CEST Yue Hu wrote:
> > > On Mon, 29 Apr 2019 09:37:27 +0200
> > > "Rafael J. Wysocki" <rafael@kernel.org> wrote:
> > >
> > > > On Mon, Apr 29, 2019 at 9:24 AM Yue Hu <zbestahu@gmail.com> wrote:
> > > > >
> > > > > From: Yue Hu <huyue2@yulong.com>
> > > > >
> > > > > In cpufreq_init_policy() we will check if there's last_governor for target
> > > > > and setpolicy type. However last_governor is set only if has_target() is
> > > > > true in cpufreq_offline(). That means find last_governor for setpolicy
> > > > > type is pointless. Also new_policy.governor will not be used if ->setpolicy
> > > > > callback is set in cpufreq_set_policy().
> > > > >
> > > > > Moreover, there's duplicate ->setpolicy check in using default policy path.
> > > > > Let's add a new helper function to avoid it. Also update a little comment.
> > > > >
> > > > > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > > >
> > > > Have you tested this with the intel_pstate driver (in the active mode)?
> > >
> > > No, just tested for ARM. It should be common logic from code perspective.
> >
> > But it is prudent to test changes on various configurations that may be affected.
> >
> > Testing intel_pstate shouldn't be too difficult.
>
> Hi Rafael,
>
> Sorry for late test since i take time to emulate/find the board with Sandy brigdge
> processor.
>
> I'm sure intel_pstate setpolicy driver is working fine by tests containing policy init,
> hotplug and changing scaling_governor.
Now applied, thanks!
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-05-14 21:34 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-29 7:24 [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy() Yue Hu
2019-04-29 7:26 ` Viresh Kumar
2019-04-29 7:37 ` Rafael J. Wysocki
2019-04-29 7:56 ` Yue Hu
2019-04-29 8:37 ` Rafael J. Wysocki
2019-04-30 1:44 ` Yue Hu
2019-05-13 2:11 ` Yue Hu
2019-05-14 21:34 ` Rafael J. Wysocki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.