[PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()

[PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()

[Yue Hu]

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

Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()

[Viresh Kumar]

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

Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()

[Rafael J. Wysocki]

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
>

Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()

[Yue Hu]

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
> >  

Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()

[Rafael J. Wysocki]

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

Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()

[Yue Hu]

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
> 
> 
> 

Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()

[Yue Hu]

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
> 
> 
> 

Re: [PATCH v3] cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy()

[Rafael J. Wysocki]

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!