| Message ID | 20240312103723.3469762-1-quic_sibis@quicinc.com (mailing list archive) |
|---|---|
| State | In Next |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | [V3] cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw | expand |
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 <zouyipeng@huawei.com> <mailto:zouyipeng@huawei.com> Reviewed-by: Yipeng Zou <zouyipeng@huawei.com> <mailto:zouyipeng@huawei.com> 在 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<dietmar.eggemann@arm.com> > Reviewed-by: Viresh Kumar<viresh.kumar@linaro.org> > Reviewed-by: Dhruva Gole<d-gole@ti.com> > Signed-off-by: Sibi Sankar<quic_sibis@quicinc.com> > --- > > 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/265e5f2c-9b45-420f-89b1-44369aeb8418@arm.com/ > > 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; >
On Tue, Mar 12, 2024 at 11:37 AM Sibi Sankar <quic_sibis@quicinc.com> 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 <dietmar.eggemann@arm.com> > Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org> > Reviewed-by: Dhruva Gole <d-gole@ti.com> > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > > 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/265e5f2c-9b45-420f-89b1-44369aeb8418@arm.com/ > > 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!
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;
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 <dietmar.eggemann@arm.com> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org> Reviewed-by: Dhruva Gole <d-gole@ti.com> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> --- 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/265e5f2c-9b45-420f-89b1-44369aeb8418@arm.com/ drivers/cpufreq/cpufreq.c | 18 ++++++++++++------ drivers/cpufreq/freq_table.c | 2 +- 2 files changed, 13 insertions(+), 7 deletions(-)