mbox series

[0/2] Fix per-policy boost behavior

Message ID 20240227165309.620422-1-quic_sibis@quicinc.com
Headers
Series Fix per-policy boost behavior |

Message

Sibi Sankar Feb. 27, 2024, 4:53 p.m. UTC 
  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(-)

Comments

Jie Zhan Feb. 28, 2024, 2:06 a.m. UTC | #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/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(-)
>
Viresh Kumar Feb. 28, 2024, 5:07 a.m. UTC | #2 
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.
Sibi Sankar Feb. 28, 2024, 5:07 a.m. UTC | #3 
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(-)
>>
>
Sibi Sankar Feb. 28, 2024, 5:14 a.m. UTC | #4 
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

>
Viresh Kumar Feb. 28, 2024, 6:35 a.m. UTC | #5 
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.
Sibi Sankar Feb. 28, 2024, 10:09 a.m. UTC | #6 
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

>