| Message ID | 20240227165309.620422-1-quic_sibis@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix per-policy boost behavior | expand |
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(-) >
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.
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(-) >> >
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.
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 >
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(-)