Fix per-policy boost behavior by incorporating per-policy boost flag
in the policy->max calculation and setting the correct per-policy
boost_enabled value on devices that use cpufreq_enable_boost_support().
Logs reported-by Dietmar Eggemann [1]:
[1] https://lore.kernel.org/lkml/[email protected]/
Sibi Sankar (2):
cpufreq: Fix per-policy boost behavior on SoCs using
cpufreq_boost_set_sw
cpufreq: apple-soc: Align per-policy and global boost flags
drivers/cpufreq/apple-soc-cpufreq.c | 1 +
drivers/cpufreq/cpufreq.c | 15 +++++++++------
drivers/cpufreq/freq_table.c | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
--
2.34.1
Incorporate per-policy boost flag in the policy->max calculus used in
cpufreq_frequency_table_cpuinfo. This fixes the per-policy boost
behavior on SoCs using cpufreq_boost_set_sw callback.
Fixes: 218a06a79d9a ("cpufreq: Support per-policy performance boost")
Reported-by: Dietmar Eggemann <[email protected]>
Signed-off-by: Sibi Sankar <[email protected]>
---
drivers/cpufreq/cpufreq.c | 15 +++++++++------
drivers/cpufreq/freq_table.c | 2 +-
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ff69e9335645..76002aa3d12d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -644,14 +644,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;
}
@@ -2791,11 +2793,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
Hi Sibi,
Thanks for pointing this issue out.
However, I can't clearly see how the existing code fails.
cpufreq_frequency_table_cpuinfo() checks cpufreq_boost_enabled(),
and that should be already set in cpufreq_boost_trigger_state() before
calling cpufreq_boost_set_sw(), so presumably cpufreq_boost_set_sw()
is supposed to work as expected.
Can you explain this a bit further?
Cheers,
Jie
On 28/02/2024 00:53, Sibi Sankar wrote:
> Fix per-policy boost behavior by incorporating per-policy boost flag
> in the policy->max calculation and setting the correct per-policy
> boost_enabled value on devices that use cpufreq_enable_boost_support().
>
> Logs reported-by Dietmar Eggemann [1]:
>
> [1] https://lore.kernel.org/lkml/[email protected]/
>
> Sibi Sankar (2):
> cpufreq: Fix per-policy boost behavior on SoCs using
> cpufreq_boost_set_sw
> cpufreq: apple-soc: Align per-policy and global boost flags
>
> drivers/cpufreq/apple-soc-cpufreq.c | 1 +
> drivers/cpufreq/cpufreq.c | 15 +++++++++------
> drivers/cpufreq/freq_table.c | 2 +-
> 3 files changed, 11 insertions(+), 7 deletions(-)
>
On 27-02-24, 22:23, Sibi Sankar wrote:
> Fix per-policy boost behavior by incorporating per-policy boost flag
> in the policy->max calculation and setting the correct per-policy
> boost_enabled value on devices that use cpufreq_enable_boost_support().
I don't see the problem explained anywhere and the patches look
incorrect too. The drivers aren't supposed to update the
policy->boose_enabled value.
--
viresh
On 2/28/24 07:36, Jie Zhan wrote:
> Hi Sibi,
>
> Thanks for pointing this issue out.
>
> However, I can't clearly see how the existing code fails.
>
> cpufreq_frequency_table_cpuinfo() checks cpufreq_boost_enabled(),
> and that should be already set in cpufreq_boost_trigger_state() before
> calling cpufreq_boost_set_sw(), so presumably cpufreq_boost_set_sw()
> is supposed to work as expected.
>
> Can you explain this a bit further?
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. The second issue was just book keeping, meaning some drivers
enable boost by default, however the per-policy boost flags are set
as disabled during boot.
-Sibi
>
> Cheers,
> Jie
>
> On 28/02/2024 00:53, Sibi Sankar wrote:
>> Fix per-policy boost behavior by incorporating per-policy boost flag
>> in the policy->max calculation and setting the correct per-policy
>> boost_enabled value on devices that use cpufreq_enable_boost_support().
>>
>> Logs reported-by Dietmar Eggemann [1]:
>>
>> [1]
>> https://lore.kernel.org/lkml/[email protected]/
you can also have a look at ^^ thread for more info.
>>
>> Sibi Sankar (2):
>> cpufreq: Fix per-policy boost behavior on SoCs using
>> cpufreq_boost_set_sw
>> cpufreq: apple-soc: Align per-policy and global boost flags
>>
>> drivers/cpufreq/apple-soc-cpufreq.c | 1 +
>> drivers/cpufreq/cpufreq.c | 15 +++++++++------
>> drivers/cpufreq/freq_table.c | 2 +-
>> 3 files changed, 11 insertions(+), 7 deletions(-)
>>
>
On 2/28/24 10:37, Viresh Kumar wrote:
> On 27-02-24, 22:23, Sibi Sankar wrote:
>> Fix per-policy boost behavior by incorporating per-policy boost flag
>> in the policy->max calculation and setting the correct per-policy
>> boost_enabled value on devices that use cpufreq_enable_boost_support().
>
> I don't see the problem explained anywhere and the patches look
> incorrect too. The drivers aren't supposed to update the
> policy->boose_enabled value.
Hey Viresh,
Thanks for taking time to review the series.
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. I had to update the policy->boost_enabled value because we seem
to allow enabling cpufreq_driver.boost_enabled from the driver, but I
can drop that because it was just for book keeping. I didn't want
to include redundant info from another mail thread that I referenced in
the cover letter, but will add more info in the re-spin.
-Sibi
>
On 28-02-24, 10:44, Sibi Sankar 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.
I see. Good catch. The first patch is fine, just explain the problem
properly and mention that no one is checking the policy->boost_enabled
field. It is never read.
> I had to update the policy->boost_enabled value because we seem
> to allow enabling cpufreq_driver.boost_enabled from the driver, but I
> can drop that because it was just for book keeping.
So with cpufreq_driver->boost_enabled at init time, policy's
boost_enabled must be set too. Do that in the core during
initialization of the policy instead.
> I didn't want
> to include redundant info from another mail thread that I referenced in
> the cover letter, but will add more info in the re-spin.
You don't have to, but you need to explain the exact problem in a bit
more detail since it wasn't obvious here.
--
viresh
On 2/28/24 12:05, Viresh Kumar wrote:
> On 28-02-24, 10:44, Sibi Sankar 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.
>
> I see. Good catch. The first patch is fine, just explain the problem
> properly and mention that no one is checking the policy->boost_enabled
> field. It is never read.
>
>> I had to update the policy->boost_enabled value because we seem
>> to allow enabling cpufreq_driver.boost_enabled from the driver, but I
>> can drop that because it was just for book keeping.
>
> So with cpufreq_driver->boost_enabled at init time, policy's
> boost_enabled must be set too. Do that in the core during
> initialization of the policy instead.
>
>> I didn't want
>> to include redundant info from another mail thread that I referenced in
>> the cover letter, but will add more info in the re-spin.
>
> You don't have to, but you need to explain the exact problem in a bit
> more detail since it wasn't obvious here.
ack, will make these changes in the next re-spin.
-Sibi
>