[PATCH 0/2] Fix per-policy boost behavior

[PATCH 0/2] Fix per-policy boost behavior

[Sibi Sankar]

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/265e5f2c-9b45-420f-89b1-44369aeb8418@arm.com/

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

[PATCH 1/2] cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw

[Sibi Sankar]

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 <dietmar.eggemann@arm.com>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 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

[PATCH 2/2] cpufreq: apple-soc: Align per-policy and global boost flags

[Sibi Sankar]

With the cpufreq_driver.boost_enabled flag set by default, the per-policy
boost flags are left in a disabled state. Fix this by enabling the
per-policy boost flag by default on policies supporting boost.

Logs:
root:/sys/devices/system/cpu/cpufreq# cat boost policy*/boost
1
0
0

Fixes: 218a06a79d9a ("cpufreq: Support per-policy performance boost")
Reported-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---
 drivers/cpufreq/apple-soc-cpufreq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/cpufreq/apple-soc-cpufreq.c b/drivers/cpufreq/apple-soc-cpufreq.c
index 021f423705e1..b85c250e30d0 100644
--- a/drivers/cpufreq/apple-soc-cpufreq.c
+++ b/drivers/cpufreq/apple-soc-cpufreq.c
@@ -289,6 +289,7 @@ static int apple_soc_cpufreq_init(struct cpufreq_policy *policy)
 		} else {
 			apple_soc_cpufreq_hw_attr[1] = &cpufreq_freq_attr_scaling_boost_freqs;
 			apple_soc_cpufreq_driver.boost_enabled = true;
+			policy->boost_enabled = true;
 		}
 	}
 
-- 
2.34.1

Re: [PATCH 0/2] Fix per-policy boost behavior

[Jie Zhan]

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/265e5f2c-9b45-420f-89b1-44369aeb8418@arm.com/
>
> 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(-)
>

Re: [PATCH 0/2] Fix per-policy boost behavior

[Sibi Sankar]

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/265e5f2c-9b45-420f-89b1-44369aeb8418@arm.com/

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

Re: [PATCH 0/2] Fix per-policy boost behavior

[Viresh Kumar]

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

Re: [PATCH 0/2] Fix per-policy boost behavior

[Sibi Sankar]

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

> 

Re: [PATCH 0/2] Fix per-policy boost behavior

[Viresh Kumar]

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

Re: [PATCH 0/2] Fix per-policy boost behavior

[Sibi Sankar]

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

>