* [PATCH v2] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
@ 2019-04-26 6:31 Yue Hu
2019-04-29 5:15 ` Viresh Kumar
0 siblings, 1 reply; 4+ messages in thread
From: Yue Hu @ 2019-04-26 6:31 UTC (permalink / raw)
To: rjw, viresh.kumar, 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 fix a little comment.
Signed-off-by: Yue Hu <huyue2@yulong.com>
---
v2: fix ->setplicy typo.
drivers/cpufreq/cpufreq.c | 66 ++++++++++++++++++++++++++++-------------------
1 file changed, 40 insertions(+), 26 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 0322cce..b822a3e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -578,6 +578,20 @@ static struct cpufreq_governor *find_governor(const char *str_governor)
return NULL;
}
+static int cpufreq_parse_static_governor(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
*/
@@ -585,15 +599,7 @@ 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;
- }
+ return cpufreq_parse_static_governor(str_governor, policy);
} else {
struct cpufreq_governor *t;
@@ -1020,32 +1026,40 @@ __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) {
+ /* 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_static_governor(def_gov->name,
+ &new_policy);
+ }
}
- /* set default policy */
+ /* Set new policy */
return cpufreq_set_policy(policy, &new_policy);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
2019-04-26 6:31 [PATCH v2] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy() Yue Hu
@ 2019-04-29 5:15 ` Viresh Kumar
2019-04-29 6:35 ` Yue Hu
0 siblings, 1 reply; 4+ messages in thread
From: Viresh Kumar @ 2019-04-29 5:15 UTC (permalink / raw)
To: Yue Hu; +Cc: rjw, rafael.j.wysocki, linux-pm, huyue2
On 26-04-19, 14:31, 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 fix a little comment.
>
> Signed-off-by: Yue Hu <huyue2@yulong.com>
> ---
> v2: fix ->setplicy typo.
>
> drivers/cpufreq/cpufreq.c | 66 ++++++++++++++++++++++++++++-------------------
> 1 file changed, 40 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 0322cce..b822a3e 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -578,6 +578,20 @@ static struct cpufreq_governor *find_governor(const char *str_governor)
> return NULL;
> }
>
> +static int cpufreq_parse_static_governor(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
> */
> @@ -585,15 +599,7 @@ static int cpufreq_parse_governor(char *str_governor,
> struct cpufreq_policy *policy)
> {
There were only two callers of cpufreq_parse_governor() and one of them you have
removed already.
> 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;
> - }
> + return cpufreq_parse_static_governor(str_governor, policy);
So I will rather remove above completely from here and let
cpufreq_parse_governor() governor only handle !set_policy.
> } else {
> struct cpufreq_governor *t;
>
> @@ -1020,32 +1026,40 @@ __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)
You must use {} here in the if block as well for two reasons:
- Below pr_debug is across multiple lines
- And else part already uses {}
> + 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) {
> + /* 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_static_governor(def_gov->name,
> + &new_policy);
> + }
> }
> - /* set default policy */
> + /* Set new policy */
Just drop the comment and make it a blank line.
> return cpufreq_set_policy(policy, &new_policy);
> }
>
> --
> 1.9.1
--
viresh
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
2019-04-29 5:15 ` Viresh Kumar
@ 2019-04-29 6:35 ` Yue Hu
2019-04-29 6:40 ` Viresh Kumar
0 siblings, 1 reply; 4+ messages in thread
From: Yue Hu @ 2019-04-29 6:35 UTC (permalink / raw)
To: Viresh Kumar; +Cc: rjw, rafael.j.wysocki, linux-pm, huyue2
On Mon, 29 Apr 2019 10:45:07 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 26-04-19, 14:31, 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 fix a little comment.
> >
> > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > ---
> > v2: fix ->setplicy typo.
> >
> > drivers/cpufreq/cpufreq.c | 66 ++++++++++++++++++++++++++++-------------------
> > 1 file changed, 40 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 0322cce..b822a3e 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -578,6 +578,20 @@ static struct cpufreq_governor *find_governor(const char *str_governor)
> > return NULL;
> > }
> >
> > +static int cpufreq_parse_static_governor(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
> > */
> > @@ -585,15 +599,7 @@ static int cpufreq_parse_governor(char *str_governor,
> > struct cpufreq_policy *policy)
> > {
>
> There were only two callers of cpufreq_parse_governor() and one of them you have
> removed already.
>
> > 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;
> > - }
> > + return cpufreq_parse_static_governor(str_governor, policy);
>
> So I will rather remove above completely from here and let
> cpufreq_parse_governor() governor only handle !set_policy.
So, also need update store_scaling_governor() as below if doing that above, right?
if (cpufreq_driver->setpolicy) {
if (cpufreq_parse_static_governor(str_governor, &new_policy))
return -EINVAL;
} else {
if (cpufreq_parse_governor(str_governor, &new_policy))
return -EINVAL;
}
Moreover, change cpufreq_parse_static_governor() to cpufreq_parse_generic_policy()
should be better?
>
> > } else {
> > struct cpufreq_governor *t;
> >
> > @@ -1020,32 +1026,40 @@ __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)
>
> You must use {} here in the if block as well for two reasons:
> - Below pr_debug is across multiple lines
> - And else part already uses {}
>
Ok, fix it in v3.
> > + 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) {
> > + /* 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_static_governor(def_gov->name,
> > + &new_policy);
> > + }
> > }
> > - /* set default policy */
> > + /* Set new policy */
>
> Just drop the comment and make it a blank line.
Ok, fix it in v3.
>
> > return cpufreq_set_policy(policy, &new_policy);
> > }
> >
> > --
> > 1.9.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()
2019-04-29 6:35 ` Yue Hu
@ 2019-04-29 6:40 ` Viresh Kumar
0 siblings, 0 replies; 4+ messages in thread
From: Viresh Kumar @ 2019-04-29 6:40 UTC (permalink / raw)
To: Yue Hu; +Cc: rjw, rafael.j.wysocki, linux-pm, huyue2
On 29-04-19, 14:35, Yue Hu wrote:
> So, also need update store_scaling_governor() as below if doing that above, right?
>
> if (cpufreq_driver->setpolicy) {
> if (cpufreq_parse_static_governor(str_governor, &new_policy))
> return -EINVAL;
> } else {
> if (cpufreq_parse_governor(str_governor, &new_policy))
> return -EINVAL;
> }
right.
> Moreover, change cpufreq_parse_static_governor() to cpufreq_parse_generic_policy()
> should be better?
Maybe cpufreq_parse_policy().
--
viresh
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-04-29 6:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-26 6:31 [PATCH v2] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy() Yue Hu
2019-04-29 5:15 ` Viresh Kumar
2019-04-29 6:35 ` Yue Hu
2019-04-29 6:40 ` Viresh Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).