2024-03-12 10:38:11

by Sibi Sankar

Subject: [PATCH V3] cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw

In the existing code, per-policy flags doesn't have any impact i.e.
if cpufreq_driver boost is enabled and one or more of the per-policy
boost is disabled, the cpufreq driver will behave as if boost is
enabled. Fix this by incorporating per-policy boost flag in the policy->max
calculus used in cpufreq_frequency_table_cpuinfo and setting the default
per-policy boost to mirror the cpufreq_driver boost flag.

Fixes: 218a06a79d9a ("cpufreq: Support per-policy performance boost")
Reported-by: Dietmar Eggemann <[email protected]>
Reviewed-by: Viresh Kumar <[email protected]>
Reviewed-by: Dhruva Gole <[email protected]>
Signed-off-by: Sibi Sankar <[email protected]>
---

v3:
* Pickup Rbs.
* Simplify per-policy boost setting. [Viresh]

v2:
* Enable per-policy boost flag in the core instead. [Viresh]
* Add more details regarding the bug. [Viresh]
* Drop cover-letter and patch 2.

Logs reported-by Dietmar Eggemann:
https://lore.kernel.org/lkml/[email protected]/

drivers/cpufreq/cpufreq.c | 18 ++++++++++++------
drivers/cpufreq/freq_table.c | 2 +-
2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f6f8d7f450e7..66e10a19d76a 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -653,14 +653,16 @@ static ssize_t store_local_boost(struct cpufreq_policy *policy,
if (policy->boost_enabled == enable)
return count;

+ policy->boost_enabled = enable;
+
cpus_read_lock();
ret = cpufreq_driver->set_boost(policy, enable);
cpus_read_unlock();

- if (ret)
+ if (ret) {
+ policy->boost_enabled = !policy->boost_enabled;
return ret;
-
- policy->boost_enabled = enable;
+ }

return count;
}
@@ -1428,6 +1430,9 @@ static int cpufreq_online(unsigned int cpu)
goto out_free_policy;
}

+ /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
+ policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
+
/*
* The initialization has succeeded and the policy is online.
* If there is a problem with its frequency table, take it
@@ -2769,11 +2774,12 @@ int cpufreq_boost_trigger_state(int state)

cpus_read_lock();
for_each_active_policy(policy) {
+ policy->boost_enabled = state;
ret = cpufreq_driver->set_boost(policy, state);
- if (ret)
+ if (ret) {
+ policy->boost_enabled = !policy->boost_enabled;
goto err_reset_state;
-
- policy->boost_enabled = state;
+ }
}
cpus_read_unlock();

diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index c4d4643b6ca6..c17dc51a5a02 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -40,7 +40,7 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
cpufreq_for_each_valid_entry(pos, table) {
freq = pos->frequency;

- if (!cpufreq_boost_enabled()
+ if ((!cpufreq_boost_enabled() || !policy->boost_enabled)
&& (pos->flags & CPUFREQ_BOOST_FREQ))
continue;

--
2.34.1

2024-03-13 03:58:27

by Yipeng Zou

Subject: Re: [PATCH V3] cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw

Hi,

Oh, I just catch this issue too recently.

Also test this patch on my board and it's works fine to me.

Tested-by:Yipeng Zou <[email protected]> <mailto:[email protected]>

Reviewed-by: Yipeng Zou <[email protected]> <mailto:[email protected]>

在 2024/3/12 18:37, Sibi Sankar 写道:
> In the existing code, per-policy flags doesn't have any impact i.e.
> if cpufreq_driver boost is enabled and one or more of the per-policy
> boost is disabled, the cpufreq driver will behave as if boost is
> enabled. Fix this by incorporating per-policy boost flag in the policy->max
> calculus used in cpufreq_frequency_table_cpuinfo and setting the default
> per-policy boost to mirror the cpufreq_driver boost flag.
>
> Fixes: 218a06a79d9a ("cpufreq: Support per-policy performance boost")
> Reported-by: Dietmar Eggemann<[email protected]>
> Reviewed-by: Viresh Kumar<[email protected]>
> Reviewed-by: Dhruva Gole<[email protected]>
> Signed-off-by: Sibi Sankar<[email protected]>
> ---
>
> v3:
> * Pickup Rbs.
> * Simplify per-policy boost setting. [Viresh]
>
> v2:
> * Enable per-policy boost flag in the core instead. [Viresh]
> * Add more details regarding the bug. [Viresh]
> * Drop cover-letter and patch 2.
>
> Logs reported-by Dietmar Eggemann:
> https://lore.kernel.org/lkml/[email protected]/
>
> drivers/cpufreq/cpufreq.c | 18 ++++++++++++------
> drivers/cpufreq/freq_table.c | 2 +-
> 2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index f6f8d7f450e7..66e10a19d76a 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -653,14 +653,16 @@ static ssize_t store_local_boost(struct cpufreq_policy *policy,
> if (policy->boost_enabled == enable)
> return count;
>
> + policy->boost_enabled = enable;
> +
> cpus_read_lock();
> ret = cpufreq_driver->set_boost(policy, enable);
> cpus_read_unlock();
>
> - if (ret)
> + if (ret) {
> + policy->boost_enabled = !policy->boost_enabled;
> return ret;
> -
> - policy->boost_enabled = enable;
> + }
>
> return count;
> }
> @@ -1428,6 +1430,9 @@ static int cpufreq_online(unsigned int cpu)
> goto out_free_policy;
> }
>
> + /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
> + policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
> +
> /*
> * The initialization has succeeded and the policy is online.
> * If there is a problem with its frequency table, take it
> @@ -2769,11 +2774,12 @@ int cpufreq_boost_trigger_state(int state)
>
> cpus_read_lock();
> for_each_active_policy(policy) {
> + policy->boost_enabled = state;
> ret = cpufreq_driver->set_boost(policy, state);
> - if (ret)
> + if (ret) {
> + policy->boost_enabled = !policy->boost_enabled;
> goto err_reset_state;
> -
> - policy->boost_enabled = state;
> + }
> }
> cpus_read_unlock();
>
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index c4d4643b6ca6..c17dc51a5a02 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -40,7 +40,7 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> cpufreq_for_each_valid_entry(pos, table) {
> freq = pos->frequency;
>
> - if (!cpufreq_boost_enabled()
> + if ((!cpufreq_boost_enabled() || !policy->boost_enabled)
> && (pos->flags & CPUFREQ_BOOST_FREQ))
> continue;
>

--
Regards,
Yipeng Zou

2024-03-13 19:58:28

by Rafael J. Wysocki

Subject: Re: [PATCH V3] cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw

On Tue, Mar 12, 2024 at 11:37 AM Sibi Sankar <[email protected]> wrote:
>
> In the existing code, per-policy flags doesn't have any impact i.e.
> if cpufreq_driver boost is enabled and one or more of the per-policy
> boost is disabled, the cpufreq driver will behave as if boost is
> enabled. Fix this by incorporating per-policy boost flag in the policy->max
> calculus used in cpufreq_frequency_table_cpuinfo and setting the default
> per-policy boost to mirror the cpufreq_driver boost flag.
>
> Fixes: 218a06a79d9a ("cpufreq: Support per-policy performance boost")
> Reported-by: Dietmar Eggemann <[email protected]>
> Reviewed-by: Viresh Kumar <[email protected]>
> Reviewed-by: Dhruva Gole <[email protected]>
> Signed-off-by: Sibi Sankar <[email protected]>
> ---
>
> v3:
> * Pickup Rbs.
> * Simplify per-policy boost setting. [Viresh]
>
> v2:
> * Enable per-policy boost flag in the core instead. [Viresh]
> * Add more details regarding the bug. [Viresh]
> * Drop cover-letter and patch 2.
>
> Logs reported-by Dietmar Eggemann:
> https://lore.kernel.org/lkml/[email protected]/
>
> drivers/cpufreq/cpufreq.c | 18 ++++++++++++------
> drivers/cpufreq/freq_table.c | 2 +-
> 2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index f6f8d7f450e7..66e10a19d76a 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -653,14 +653,16 @@ static ssize_t store_local_boost(struct cpufreq_policy *policy,
> if (policy->boost_enabled == enable)
> return count;
>
> + policy->boost_enabled = enable;
> +
> cpus_read_lock();
> ret = cpufreq_driver->set_boost(policy, enable);
> cpus_read_unlock();
>
> - if (ret)
> + if (ret) {
> + policy->boost_enabled = !policy->boost_enabled;
> return ret;
> -
> - policy->boost_enabled = enable;
> + }
>
> return count;
> }
> @@ -1428,6 +1430,9 @@ static int cpufreq_online(unsigned int cpu)
> goto out_free_policy;
> }
>
> + /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
> + policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
> +
> /*
> * The initialization has succeeded and the policy is online.
> * If there is a problem with its frequency table, take it
> @@ -2769,11 +2774,12 @@ int cpufreq_boost_trigger_state(int state)
>
> cpus_read_lock();
> for_each_active_policy(policy) {
> + policy->boost_enabled = state;
> ret = cpufreq_driver->set_boost(policy, state);
> - if (ret)
> + if (ret) {
> + policy->boost_enabled = !policy->boost_enabled;
> goto err_reset_state;
> -
> - policy->boost_enabled = state;
> + }
> }
> cpus_read_unlock();
>
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index c4d4643b6ca6..c17dc51a5a02 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -40,7 +40,7 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> cpufreq_for_each_valid_entry(pos, table) {
> freq = pos->frequency;
>
> - if (!cpufreq_boost_enabled()
> + if ((!cpufreq_boost_enabled() || !policy->boost_enabled)
> && (pos->flags & CPUFREQ_BOOST_FREQ))
> continue;
>
> --

Applied as 6.9-rc material, thanks!