[PATCH 0/7] cpufreq: Some optimizations for cpufreq.c.

[PATCH 0/7] cpufreq: Some optimizations for cpufreq.c.

[Lifeng Zheng]

This patch series makes some minor optimizations for cpufreq.c to make
codes cleaner.

Lifeng Zheng (7):
  cpufreq: Disable cpufreq-based invariance when fail to register driver
  cpufreq: Init policy->rwsem before it may be possibly used
  cpufreq: Contain scaling_cur_freq.attr in cpufreq_attrs
  cpufreq: Remove duplicate check in __cpufreq_offline()
  cpufreq: Move the check of cpufreq_driver->get into
    cpufreq_verify_current_freq()
  cpufreq: Refactor code about starting governor in cpufreq_set_policy()
  cpufreq: Exit governor when failed to start old governor

 drivers/cpufreq/cpufreq.c | 53 +++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

-- 
2.33.0

[PATCH 1/7] cpufreq: Disable cpufreq-based invariance when fail to register driver

[Lifeng Zheng]

The cpufreq-based invariance is enabled in cpufreq_register_driver(), but
never disabled after that when fail. Add a
static_branch_disable_cpuslocked() to do that as
cpufreq_unregister_driver() does.

Fixes: 874f63531064 ("cpufreq: report whether cpufreq supports Frequency Invariance (FI)")
Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/cpufreq/cpufreq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index d7426e1d8bdd..1bc665b5bba8 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2991,6 +2991,7 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 err_boost_unreg:
 	remove_boost_sysfs_file();
 err_null_driver:
+	static_branch_disable_cpuslocked(&cpufreq_freq_invariance);
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
 	cpufreq_driver = NULL;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-- 
2.33.0

[PATCH 2/7] cpufreq: Init policy->rwsem before it may be possibly used

[Lifeng Zheng]

In cpufreq_policy_put_kobj(), policy->rwsem is used. But in
cpufreq_policy_alloc(), if freq_qos_add_notifier() returns an error, error
path via err_kobj_remove or err_min_qos_notifier will be reached and
cpufreq_policy_put_kobj() will be called before policy->rwsem is
initialized. Thus, the calling of init_rwsem() should be moved to where
before these two error paths can be reached.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/cpufreq/cpufreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1bc665b5bba8..efc1f4ac85cb 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1284,6 +1284,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 		goto err_free_real_cpus;
 	}
 
+	init_rwsem(&policy->rwsem);
+
 	freq_constraints_init(&policy->constraints);
 
 	policy->nb_min.notifier_call = cpufreq_notifier_min;
@@ -1306,7 +1308,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	}
 
 	INIT_LIST_HEAD(&policy->policy_list);
-	init_rwsem(&policy->rwsem);
 	spin_lock_init(&policy->transition_lock);
 	init_waitqueue_head(&policy->transition_wait);
 	INIT_WORK(&policy->update, handle_update);
-- 
2.33.0

[PATCH 3/7] cpufreq: Contain scaling_cur_freq.attr in cpufreq_attrs

[Lifeng Zheng]

After commit c034b02e213d ("cpufreq: expose scaling_cur_freq sysfs file for
set_policy() drivers"), the file scaling_cur_freq is exposed to all
drivers. No need to create this file separately. It's better to be
contained in cpufreq_attrs.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/cpufreq/cpufreq.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index efc1f4ac85cb..2303890de0ba 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -967,6 +967,7 @@ static struct attribute *cpufreq_attrs[] = {
 	&cpuinfo_min_freq.attr,
 	&cpuinfo_max_freq.attr,
 	&cpuinfo_transition_latency.attr,
+	&scaling_cur_freq.attr,
 	&scaling_min_freq.attr,
 	&scaling_max_freq.attr,
 	&affected_cpus.attr,
@@ -1095,10 +1096,6 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
 			return ret;
 	}
 
-	ret = sysfs_create_file(&policy->kobj, &scaling_cur_freq.attr);
-	if (ret)
-		return ret;
-
 	if (cpufreq_driver->bios_limit) {
 		ret = sysfs_create_file(&policy->kobj, &bios_limit.attr);
 		if (ret)
-- 
2.33.0

[PATCH 4/7] cpufreq: Remove duplicate check in __cpufreq_offline()

[Lifeng Zheng]

The has_target() checks in __cpufreq_offline() are duplicate. Remove one of
them and put the operations of exiting governor together with storing last
governor's name.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/cpufreq/cpufreq.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2303890de0ba..c4891bf5dc84 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1692,14 +1692,13 @@ static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy *policy)
 		return;
 	}
 
-	if (has_target())
+	if (has_target()) {
 		strscpy(policy->last_governor, policy->governor->name,
 			CPUFREQ_NAME_LEN);
-	else
-		policy->last_policy = policy->policy;
-
-	if (has_target())
 		cpufreq_exit_governor(policy);
+	} else {
+		policy->last_policy = policy->policy;
+	}
 
 	/*
 	 * Perform the ->offline() during light-weight tear-down, as
-- 
2.33.0

[PATCH 5/7] cpufreq: Move the check of cpufreq_driver->get into cpufreq_verify_current_freq()

[Lifeng Zheng]

Move the check of cpufreq_driver->get into cpufreq_verify_current_freq() in
case of calling it without check.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/cpufreq/cpufreq.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index c4891bf5dc84..9b2578b905a5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1800,6 +1800,9 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
 {
 	unsigned int new_freq;
 
+	if (!cpufreq_driver->get)
+		return 0;
+
 	new_freq = cpufreq_driver->get(policy->cpu);
 	if (!new_freq)
 		return 0;
@@ -1922,10 +1925,7 @@ unsigned int cpufreq_get(unsigned int cpu)
 
 	guard(cpufreq_policy_read)(policy);
 
-	if (cpufreq_driver->get)
-		return __cpufreq_get(policy);
-
-	return 0;
+	return __cpufreq_get(policy);
 }
 EXPORT_SYMBOL(cpufreq_get);
 
@@ -2479,8 +2479,7 @@ int cpufreq_start_governor(struct cpufreq_policy *policy)
 
 	pr_debug("%s: for CPU %u\n", __func__, policy->cpu);
 
-	if (cpufreq_driver->get)
-		cpufreq_verify_current_freq(policy, false);
+	cpufreq_verify_current_freq(policy, false);
 
 	if (policy->governor->start) {
 		ret = policy->governor->start(policy);
-- 
2.33.0

[PATCH 6/7] cpufreq: Refactor code about starting governor in cpufreq_set_policy()

[Lifeng Zheng]

Refactor code about starting governor without functional change in
cpufreq_set_policy() to make it more readable.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/cpufreq/cpufreq.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9b2578b905a5..7b82ffb50283 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2698,15 +2698,19 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 	/* start new governor */
 	policy->governor = new_gov;
 	ret = cpufreq_init_governor(policy);
-	if (!ret) {
-		ret = cpufreq_start_governor(policy);
-		if (!ret) {
-			pr_debug("governor change\n");
-			return 0;
-		}
+	if (ret)
+		goto start_old_gov;
+
+	ret = cpufreq_start_governor(policy);
+	if (ret) {
 		cpufreq_exit_governor(policy);
+		goto start_old_gov;
 	}
 
+	pr_debug("governor change\n");
+	return 0;
+
+start_old_gov:
 	/* new governor failed, so re-start old one */
 	pr_debug("starting governor %s failed\n", policy->governor->name);
 	if (old_gov) {
-- 
2.33.0

[PATCH 7/7] cpufreq: Exit governor when failed to start old governor

[Lifeng Zheng]

Detect the result of starting old governor in cpufreq_set_policy(). If it
fails, exit the governor and clear policy->governor.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/cpufreq/cpufreq.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 7b82ffb50283..2b431845a8a3 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2715,10 +2715,12 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 	pr_debug("starting governor %s failed\n", policy->governor->name);
 	if (old_gov) {
 		policy->governor = old_gov;
-		if (cpufreq_init_governor(policy))
+		if (cpufreq_init_governor(policy)) {
 			policy->governor = NULL;
-		else
-			cpufreq_start_governor(policy);
+		} else if (cpufreq_start_governor(policy)) {
+			cpufreq_exit_governor(policy);
+			policy->governor = NULL;
+		}
 	}
 
 	return ret;
-- 
2.33.0

Re: [PATCH 7/7] cpufreq: Exit governor when failed to start old governor

[Rafael J. Wysocki]

On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Detect the result of starting old governor in cpufreq_set_policy(). If it
> fails, exit the governor and clear policy->governor.
>
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/cpufreq/cpufreq.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 7b82ffb50283..2b431845a8a3 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2715,10 +2715,12 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
>         pr_debug("starting governor %s failed\n", policy->governor->name);
>         if (old_gov) {
>                 policy->governor = old_gov;
> -               if (cpufreq_init_governor(policy))
> +               if (cpufreq_init_governor(policy)) {
>                         policy->governor = NULL;
> -               else
> -                       cpufreq_start_governor(policy);
> +               } else if (cpufreq_start_governor(policy)) {
> +                       cpufreq_exit_governor(policy);

This may introduce a governor module reference imbalance AFAICS.

> +                       policy->governor = NULL;
> +               }
>         }
>
>         return ret;
> --

Re: [PATCH 6/7] cpufreq: Refactor code about starting governor in cpufreq_set_policy()

[Rafael J. Wysocki]

On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Refactor code about starting governor without functional change in
> cpufreq_set_policy() to make it more readable.

Sorry, but I don't see the point.

> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/cpufreq/cpufreq.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 9b2578b905a5..7b82ffb50283 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2698,15 +2698,19 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
>         /* start new governor */
>         policy->governor = new_gov;
>         ret = cpufreq_init_governor(policy);
> -       if (!ret) {
> -               ret = cpufreq_start_governor(policy);
> -               if (!ret) {
> -                       pr_debug("governor change\n");
> -                       return 0;
> -               }
> +       if (ret)
> +               goto start_old_gov;
> +
> +       ret = cpufreq_start_governor(policy);
> +       if (ret) {
>                 cpufreq_exit_governor(policy);
> +               goto start_old_gov;
>         }
>
> +       pr_debug("governor change\n");
> +       return 0;
> +
> +start_old_gov:
>         /* new governor failed, so re-start old one */
>         pr_debug("starting governor %s failed\n", policy->governor->name);
>         if (old_gov) {
> --

Re: [PATCH 1/7] cpufreq: Disable cpufreq-based invariance when fail to register driver

[Rafael J. Wysocki]

On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> The cpufreq-based invariance is enabled in cpufreq_register_driver(), but
> never disabled after that when fail. Add a
> static_branch_disable_cpuslocked() to do that as
> cpufreq_unregister_driver() does.

What about moving the invariance initialization to the point when 0 is
going to be returned?

> Fixes: 874f63531064 ("cpufreq: report whether cpufreq supports Frequency Invariance (FI)")
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/cpufreq/cpufreq.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index d7426e1d8bdd..1bc665b5bba8 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2991,6 +2991,7 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>  err_boost_unreg:
>         remove_boost_sysfs_file();
>  err_null_driver:
> +       static_branch_disable_cpuslocked(&cpufreq_freq_invariance);
>         write_lock_irqsave(&cpufreq_driver_lock, flags);
>         cpufreq_driver = NULL;
>         write_unlock_irqrestore(&cpufreq_driver_lock, flags);
> --

Re: [PATCH 2/7] cpufreq: Init policy->rwsem before it may be possibly used

[Rafael J. Wysocki]

On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> In cpufreq_policy_put_kobj(), policy->rwsem is used. But in
> cpufreq_policy_alloc(), if freq_qos_add_notifier() returns an error, error
> path via err_kobj_remove or err_min_qos_notifier will be reached and
> cpufreq_policy_put_kobj() will be called before policy->rwsem is
> initialized. Thus, the calling of init_rwsem() should be moved to where
> before these two error paths can be reached.

Since this is a fix, any chance to add a Fixes: tag here?

> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/cpufreq/cpufreq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1bc665b5bba8..efc1f4ac85cb 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1284,6 +1284,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>                 goto err_free_real_cpus;
>         }
>
> +       init_rwsem(&policy->rwsem);
> +
>         freq_constraints_init(&policy->constraints);
>
>         policy->nb_min.notifier_call = cpufreq_notifier_min;
> @@ -1306,7 +1308,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>         }
>
>         INIT_LIST_HEAD(&policy->policy_list);
> -       init_rwsem(&policy->rwsem);
>         spin_lock_init(&policy->transition_lock);
>         init_waitqueue_head(&policy->transition_wait);
>         INIT_WORK(&policy->update, handle_update);
> --
> 2.33.0
>

Re: [PATCH 3/7] cpufreq: Contain scaling_cur_freq.attr in cpufreq_attrs

[Rafael J. Wysocki]

On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> After commit c034b02e213d ("cpufreq: expose scaling_cur_freq sysfs file for
> set_policy() drivers"), the file scaling_cur_freq is exposed to all
> drivers. No need to create this file separately. It's better to be
> contained in cpufreq_attrs.

Fair enough.

> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/cpufreq/cpufreq.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index efc1f4ac85cb..2303890de0ba 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -967,6 +967,7 @@ static struct attribute *cpufreq_attrs[] = {
>         &cpuinfo_min_freq.attr,
>         &cpuinfo_max_freq.attr,
>         &cpuinfo_transition_latency.attr,
> +       &scaling_cur_freq.attr,
>         &scaling_min_freq.attr,
>         &scaling_max_freq.attr,
>         &affected_cpus.attr,
> @@ -1095,10 +1096,6 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
>                         return ret;
>         }
>
> -       ret = sysfs_create_file(&policy->kobj, &scaling_cur_freq.attr);
> -       if (ret)
> -               return ret;
> -
>         if (cpufreq_driver->bios_limit) {
>                 ret = sysfs_create_file(&policy->kobj, &bios_limit.attr);
>                 if (ret)
> --
> 2.33.0
>

Re: [PATCH 4/7] cpufreq: Remove duplicate check in __cpufreq_offline()

[Rafael J. Wysocki]

On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> The has_target() checks in __cpufreq_offline() are duplicate. Remove one of
> them and put the operations of exiting governor together with storing last
> governor's name.
>
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/cpufreq/cpufreq.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2303890de0ba..c4891bf5dc84 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1692,14 +1692,13 @@ static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy *policy)
>                 return;
>         }
>
> -       if (has_target())
> +       if (has_target()) {
>                 strscpy(policy->last_governor, policy->governor->name,
>                         CPUFREQ_NAME_LEN);
> -       else
> -               policy->last_policy = policy->policy;
> -
> -       if (has_target())
>                 cpufreq_exit_governor(policy);
> +       } else {
> +               policy->last_policy = policy->policy;
> +       }
>
>         /*
>          * Perform the ->offline() during light-weight tear-down, as
> --

This is fine by me, thanks!

Re: [PATCH 5/7] cpufreq: Move the check of cpufreq_driver->get into cpufreq_verify_current_freq()

[Rafael J. Wysocki]

On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Move the check of cpufreq_driver->get into cpufreq_verify_current_freq() in
> case of calling it without check.
>
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/cpufreq/cpufreq.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index c4891bf5dc84..9b2578b905a5 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1800,6 +1800,9 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
>  {
>         unsigned int new_freq;
>
> +       if (!cpufreq_driver->get)
> +               return 0;
> +

This will duplicate the check in cpufreq_policy_refresh(), won't it?

>         new_freq = cpufreq_driver->get(policy->cpu);
>         if (!new_freq)
>                 return 0;
> @@ -1922,10 +1925,7 @@ unsigned int cpufreq_get(unsigned int cpu)
>
>         guard(cpufreq_policy_read)(policy);
>
> -       if (cpufreq_driver->get)
> -               return __cpufreq_get(policy);
> -
> -       return 0;
> +       return __cpufreq_get(policy);
>  }
>  EXPORT_SYMBOL(cpufreq_get);
>
> @@ -2479,8 +2479,7 @@ int cpufreq_start_governor(struct cpufreq_policy *policy)
>
>         pr_debug("%s: for CPU %u\n", __func__, policy->cpu);
>
> -       if (cpufreq_driver->get)
> -               cpufreq_verify_current_freq(policy, false);
> +       cpufreq_verify_current_freq(policy, false);
>
>         if (policy->governor->start) {
>                 ret = policy->governor->start(policy);
> --
> 2.33.0
>

Re: [PATCH 3/7] cpufreq: Contain scaling_cur_freq.attr in cpufreq_attrs

[Rafael J. Wysocki]

On Mon, Jun 23, 2025 at 5:30 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
> >
> > After commit c034b02e213d ("cpufreq: expose scaling_cur_freq sysfs file for
> > set_policy() drivers"), the file scaling_cur_freq is exposed to all
> > drivers. No need to create this file separately. It's better to be
> > contained in cpufreq_attrs.
>
> Fair enough.

And so applied as 6.17 material along with the [4/7].

The other patches in the series need more work IMV.

Thanks!

Re: [PATCH 1/7] cpufreq: Disable cpufreq-based invariance when fail to register driver

[zhenglifeng (A)]

On 2025/6/23 23:27, Rafael J. Wysocki wrote:

> On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>
>> The cpufreq-based invariance is enabled in cpufreq_register_driver(), but
>> never disabled after that when fail. Add a
>> static_branch_disable_cpuslocked() to do that as
>> cpufreq_unregister_driver() does.
> 
> What about moving the invariance initialization to the point when 0 is
> going to be returned?

Yes, that'll do it, too, thank you.

> 
>> Fixes: 874f63531064 ("cpufreq: report whether cpufreq supports Frequency Invariance (FI)")
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/cpufreq/cpufreq.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index d7426e1d8bdd..1bc665b5bba8 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -2991,6 +2991,7 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>>  err_boost_unreg:
>>         remove_boost_sysfs_file();
>>  err_null_driver:
>> +       static_branch_disable_cpuslocked(&cpufreq_freq_invariance);
>>         write_lock_irqsave(&cpufreq_driver_lock, flags);
>>         cpufreq_driver = NULL;
>>         write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>> --

Re: [PATCH 2/7] cpufreq: Init policy->rwsem before it may be possibly used

[zhenglifeng (A)]

On 2025/6/23 23:29, Rafael J. Wysocki wrote:

> On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>
>> In cpufreq_policy_put_kobj(), policy->rwsem is used. But in
>> cpufreq_policy_alloc(), if freq_qos_add_notifier() returns an error, error
>> path via err_kobj_remove or err_min_qos_notifier will be reached and
>> cpufreq_policy_put_kobj() will be called before policy->rwsem is
>> initialized. Thus, the calling of init_rwsem() should be moved to where
>> before these two error paths can be reached.
> 
> Since this is a fix, any chance to add a Fixes: tag here?

You are right. Will add it. Thanks.

> 
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/cpufreq/cpufreq.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 1bc665b5bba8..efc1f4ac85cb 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1284,6 +1284,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>>                 goto err_free_real_cpus;
>>         }
>>
>> +       init_rwsem(&policy->rwsem);
>> +
>>         freq_constraints_init(&policy->constraints);
>>
>>         policy->nb_min.notifier_call = cpufreq_notifier_min;
>> @@ -1306,7 +1308,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>>         }
>>
>>         INIT_LIST_HEAD(&policy->policy_list);
>> -       init_rwsem(&policy->rwsem);
>>         spin_lock_init(&policy->transition_lock);
>>         init_waitqueue_head(&policy->transition_wait);
>>         INIT_WORK(&policy->update, handle_update);
>> --
>> 2.33.0
>>
> 

Re: [PATCH 3/7] cpufreq: Contain scaling_cur_freq.attr in cpufreq_attrs

[zhenglifeng (A)]

On 2025/7/3 22:59, Rafael J. Wysocki wrote:

> On Mon, Jun 23, 2025 at 5:30 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>>
>>> After commit c034b02e213d ("cpufreq: expose scaling_cur_freq sysfs file for
>>> set_policy() drivers"), the file scaling_cur_freq is exposed to all
>>> drivers. No need to create this file separately. It's better to be
>>> contained in cpufreq_attrs.
>>
>> Fair enough.
> 
> And so applied as 6.17 material along with the [4/7].
> 
> The other patches in the series need more work IMV.
> 
> Thanks!

Thanks! Then in the next version these two patches won't be sent again.

Re: [PATCH 5/7] cpufreq: Move the check of cpufreq_driver->get into cpufreq_verify_current_freq()

[zhenglifeng (A)]

On 2025/6/23 23:35, Rafael J. Wysocki wrote:

> On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>
>> Move the check of cpufreq_driver->get into cpufreq_verify_current_freq() in
>> case of calling it without check.
>>
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/cpufreq/cpufreq.c | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index c4891bf5dc84..9b2578b905a5 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1800,6 +1800,9 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
>>  {
>>         unsigned int new_freq;
>>
>> +       if (!cpufreq_driver->get)
>> +               return 0;
>> +
> 
> This will duplicate the check in cpufreq_policy_refresh(), won't it?

Yes, the check will duplicate but I think it's OK. If remove the check in
cpufreq_policy_refresh(), the logic will be changed.

> 
>>         new_freq = cpufreq_driver->get(policy->cpu);
>>         if (!new_freq)
>>                 return 0;
>> @@ -1922,10 +1925,7 @@ unsigned int cpufreq_get(unsigned int cpu)
>>
>>         guard(cpufreq_policy_read)(policy);
>>
>> -       if (cpufreq_driver->get)
>> -               return __cpufreq_get(policy);
>> -
>> -       return 0;
>> +       return __cpufreq_get(policy);
>>  }
>>  EXPORT_SYMBOL(cpufreq_get);
>>
>> @@ -2479,8 +2479,7 @@ int cpufreq_start_governor(struct cpufreq_policy *policy)
>>
>>         pr_debug("%s: for CPU %u\n", __func__, policy->cpu);
>>
>> -       if (cpufreq_driver->get)
>> -               cpufreq_verify_current_freq(policy, false);
>> +       cpufreq_verify_current_freq(policy, false);
>>
>>         if (policy->governor->start) {
>>                 ret = policy->governor->start(policy);
>> --
>> 2.33.0
>>

Re: [PATCH 6/7] cpufreq: Refactor code about starting governor in cpufreq_set_policy()

[zhenglifeng (A)]

On 2025/6/23 23:13, Rafael J. Wysocki wrote:

> On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>
>> Refactor code about starting governor without functional change in
>> cpufreq_set_policy() to make it more readable.
> 
> Sorry, but I don't see the point.

Just some refactoring to reduce nest level and make the style more similar
to other code in this file. If you don't like it, I'll drop it.

> 
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/cpufreq/cpufreq.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 9b2578b905a5..7b82ffb50283 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -2698,15 +2698,19 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
>>         /* start new governor */
>>         policy->governor = new_gov;
>>         ret = cpufreq_init_governor(policy);
>> -       if (!ret) {
>> -               ret = cpufreq_start_governor(policy);
>> -               if (!ret) {
>> -                       pr_debug("governor change\n");
>> -                       return 0;
>> -               }
>> +       if (ret)
>> +               goto start_old_gov;
>> +
>> +       ret = cpufreq_start_governor(policy);
>> +       if (ret) {
>>                 cpufreq_exit_governor(policy);
>> +               goto start_old_gov;
>>         }
>>
>> +       pr_debug("governor change\n");
>> +       return 0;
>> +
>> +start_old_gov:
>>         /* new governor failed, so re-start old one */
>>         pr_debug("starting governor %s failed\n", policy->governor->name);
>>         if (old_gov) {
>> --
> 

Re: [PATCH 7/7] cpufreq: Exit governor when failed to start old governor

[zhenglifeng (A)]

On 2025/6/23 23:12, Rafael J. Wysocki wrote:

> On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>>
>> Detect the result of starting old governor in cpufreq_set_policy(). If it
>> fails, exit the governor and clear policy->governor.
>>
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/cpufreq/cpufreq.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 7b82ffb50283..2b431845a8a3 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -2715,10 +2715,12 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
>>         pr_debug("starting governor %s failed\n", policy->governor->name);
>>         if (old_gov) {
>>                 policy->governor = old_gov;
>> -               if (cpufreq_init_governor(policy))
>> +               if (cpufreq_init_governor(policy)) {
>>                         policy->governor = NULL;
>> -               else
>> -                       cpufreq_start_governor(policy);
>> +               } else if (cpufreq_start_governor(policy)) {
>> +                       cpufreq_exit_governor(policy);
> 
> This may introduce a governor module reference imbalance AFAICS.

Sorry, I don't really understand this. Could you explain more? Thanks.

> 
>> +                       policy->governor = NULL;
>> +               }
>>         }
>>
>>         return ret;
>> --
> 

Re: [PATCH 7/7] cpufreq: Exit governor when failed to start old governor

[Rafael J. Wysocki]

On Fri, Jul 4, 2025 at 11:38 AM zhenglifeng (A) <zhenglifeng1@huawei.com> wrote:
>
> On 2025/6/23 23:12, Rafael J. Wysocki wrote:
>
> > On Mon, Jun 23, 2025 at 3:34 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
> >>
> >> Detect the result of starting old governor in cpufreq_set_policy(). If it
> >> fails, exit the governor and clear policy->governor.
> >>
> >> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> >> ---
> >>  drivers/cpufreq/cpufreq.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >> index 7b82ffb50283..2b431845a8a3 100644
> >> --- a/drivers/cpufreq/cpufreq.c
> >> +++ b/drivers/cpufreq/cpufreq.c
> >> @@ -2715,10 +2715,12 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
> >>         pr_debug("starting governor %s failed\n", policy->governor->name);
> >>         if (old_gov) {
> >>                 policy->governor = old_gov;
> >> -               if (cpufreq_init_governor(policy))
> >> +               if (cpufreq_init_governor(policy)) {
> >>                         policy->governor = NULL;
> >> -               else
> >> -                       cpufreq_start_governor(policy);
> >> +               } else if (cpufreq_start_governor(policy)) {
> >> +                       cpufreq_exit_governor(policy);
> >
> > This may introduce a governor module reference imbalance AFAICS.
>
> Sorry, I don't really understand this. Could you explain more? Thanks.

It looks like I've confused cpufreq_start_governor() with
cpufreq_init_governor(), sorry for the noise.