| Message ID | 1589888489-13828-2-git-send-email-wangxiongfeng2@huawei.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | add SW BOOST support for CPPC | expand |
On 2020/5/19 19:41, Xiongfeng Wang wrote: > Macro 'for_each_active_policy()' is defined internally. To avoid some > cpufreq driver needing this macro to iterate over all the policies in > '.set_boost' callback, we redefine '.set_boost' to act on only one > policy and pass the policy as an argument. > 'cpufreq_boost_trigger_state()' iterate over all the policies to set > boost for the system. This is preparation for adding SW BOOST support > for CPPC. > > Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> > --- > drivers/cpufreq/acpi-cpufreq.c | 4 ++-- > drivers/cpufreq/cpufreq.c | 53 +++++++++++++++++++++--------------------- > include/linux/cpufreq.h | 2 +- > 3 files changed, 30 insertions(+), 29 deletions(-) > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > index 289e8ce..b0a9eb5 100644 > --- a/drivers/cpufreq/acpi-cpufreq.c > +++ b/drivers/cpufreq/acpi-cpufreq.c > @@ -126,7 +126,7 @@ static void boost_set_msr_each(void *p_en) > boost_set_msr(enable); > } > > -static int set_boost(int val) > +static int set_boost(struct cpufreq_policy *policy, int val) > { > get_online_cpus(); > on_each_cpu(boost_set_msr_each, (void *)(long)val, 1); > @@ -162,7 +162,7 @@ static ssize_t store_cpb(struct cpufreq_policy *policy, const char *buf, > if (ret || val > 1) > return -EINVAL; > > - set_boost(val); > + set_boost(policy, val); > > return count; > } My original thought is as below. Since '/sys/devices/system/cpu/cpufreq/boost' can be used to set boost for the system. The file 'cpb' below each policy can be used to set boost for the policy. But this will change the existing logic. --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -126,12 +126,14 @@ static void boost_set_msr_each(void *p_en) boost_set_msr(enable); } -static int set_boost(int val) +static int set_boost(struct cpufreq_policy *policy, int val) { get_online_cpus(); - on_each_cpu(boost_set_msr_each, (void *)(long)val, 1); + on_each_cpu_mask(policy->cpus, boost_set_msr_each, + (void *)(long)val, 1); put_online_cpus(); - pr_debug("Core Boosting %sabled.\n", val ? "en" : "dis"); + pr_debug("CPU %*pbl: Core Boosting %sabled.\n", + cpumask_pr_args(policy->cpus), val ? "en" : "dis"); return 0; } @@ -162,7 +164,7 @@ static ssize_t store_cpb(struct cpufreq_policy *policy, const char *buf, if (ret || val > 1) return -EINVAL; - set_boost(val); + set_boost(policy, val); return count; } > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index d03f250..d0d86b1 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > > int cpufreq_boost_trigger_state(int state) > { > + struct cpufreq_policy *policy; > unsigned long flags; > int ret = 0; > > @@ -2570,16 +2565,22 @@ int cpufreq_boost_trigger_state(int state) > cpufreq_driver->boost_enabled = state; > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > - ret = cpufreq_driver->set_boost(state); > - if (ret) { > - write_lock_irqsave(&cpufreq_driver_lock, flags); > - cpufreq_driver->boost_enabled = !state; > - write_unlock_irqrestore(&cpufreq_driver_lock, flags); > - > - pr_err("%s: Cannot %s BOOST\n", > - __func__, state ? "enable" : "disable"); > + for_each_active_policy(policy) { > + ret = cpufreq_driver->set_boost(policy, state); > + if (ret) > + goto err_reset_state; > } > > + return 0; > + > +err_reset_state: > + write_lock_irqsave(&cpufreq_driver_lock, flags); > + cpufreq_driver->boost_enabled = !state; > + write_unlock_irqrestore(&cpufreq_driver_lock, flags); > + > + pr_err("%s: Cannot %s BOOST\n", > + __func__, state ? "enable" : "disable"); > + > return ret; > } Is it better to set 'boost_enabled' after set boost successfully rather than setting it in the begining and resetting it if setting boost failed. Thanks, Xiongfeng > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 67d5950..3494f67 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -367,7 +367,7 @@ struct cpufreq_driver { > > /* platform specific boost support code */ > bool boost_enabled; > - int (*set_boost)(int state); > + int (*set_boost)(struct cpufreq_policy *policy, int state); > }; > > /* flags */ >
On 19-05-20, 19:41, Xiongfeng Wang wrote: > Macro 'for_each_active_policy()' is defined internally. To avoid some > cpufreq driver needing this macro to iterate over all the policies in > '.set_boost' callback, we redefine '.set_boost' to act on only one > policy and pass the policy as an argument. > 'cpufreq_boost_trigger_state()' iterate over all the policies to set > boost for the system. This is preparation for adding SW BOOST support > for CPPC. > > Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> > --- > drivers/cpufreq/acpi-cpufreq.c | 4 ++-- > drivers/cpufreq/cpufreq.c | 53 +++++++++++++++++++++--------------------- > include/linux/cpufreq.h | 2 +- > 3 files changed, 30 insertions(+), 29 deletions(-) > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > index 289e8ce..b0a9eb5 100644 > --- a/drivers/cpufreq/acpi-cpufreq.c > +++ b/drivers/cpufreq/acpi-cpufreq.c > @@ -126,7 +126,7 @@ static void boost_set_msr_each(void *p_en) > boost_set_msr(enable); > } > > -static int set_boost(int val) > +static int set_boost(struct cpufreq_policy *policy, int val) > { > get_online_cpus(); > on_each_cpu(boost_set_msr_each, (void *)(long)val, 1); I think (Rafael can confirm), that you need to update this as well. You don't need to run for each cpu now, but for each CPU in the policy. > @@ -162,7 +162,7 @@ static ssize_t store_cpb(struct cpufreq_policy *policy, const char *buf, > if (ret || val > 1) > return -EINVAL; > > - set_boost(val); > + set_boost(policy, val); > > return count; > } > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index d03f250..d0d86b1 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -2532,34 +2532,29 @@ void cpufreq_update_limits(unsigned int cpu) > /********************************************************************* > * BOOST * > *********************************************************************/ > -static int cpufreq_boost_set_sw(int state) > +static int cpufreq_boost_set_sw(struct cpufreq_policy *policy, int state) > { > - struct cpufreq_policy *policy; > - > - for_each_active_policy(policy) { > - int ret; > - > - if (!policy->freq_table) > - return -ENXIO; > + int ret; > > - ret = cpufreq_frequency_table_cpuinfo(policy, > - policy->freq_table); > - if (ret) { > - pr_err("%s: Policy frequency update failed\n", > - __func__); > - return ret; > - } > + if (!policy->freq_table) > + return -ENXIO; > > - ret = freq_qos_update_request(policy->max_freq_req, policy->max); > - if (ret < 0) > - return ret; > + ret = cpufreq_frequency_table_cpuinfo(policy, policy->freq_table); > + if (ret) { > + pr_err("%s: Policy frequency update failed\n", __func__); > + return ret; > } > > + ret = freq_qos_update_request(policy->max_freq_req, policy->max); > + if (ret < 0) > + return ret; > + > return 0; > } > > int cpufreq_boost_trigger_state(int state) > { > + struct cpufreq_policy *policy; > unsigned long flags; > int ret = 0; > > @@ -2570,16 +2565,22 @@ int cpufreq_boost_trigger_state(int state) > cpufreq_driver->boost_enabled = state; > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > - ret = cpufreq_driver->set_boost(state); > - if (ret) { > - write_lock_irqsave(&cpufreq_driver_lock, flags); > - cpufreq_driver->boost_enabled = !state; > - write_unlock_irqrestore(&cpufreq_driver_lock, flags); > - > - pr_err("%s: Cannot %s BOOST\n", > - __func__, state ? "enable" : "disable"); > + for_each_active_policy(policy) { > + ret = cpufreq_driver->set_boost(policy, state); > + if (ret) > + goto err_reset_state; > } > > + return 0; > + > +err_reset_state: > + write_lock_irqsave(&cpufreq_driver_lock, flags); > + cpufreq_driver->boost_enabled = !state; > + write_unlock_irqrestore(&cpufreq_driver_lock, flags); > + > + pr_err("%s: Cannot %s BOOST\n", > + __func__, state ? "enable" : "disable"); > + > return ret; > } > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 67d5950..3494f67 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -367,7 +367,7 @@ struct cpufreq_driver { > > /* platform specific boost support code */ > bool boost_enabled; > - int (*set_boost)(int state); > + int (*set_boost)(struct cpufreq_policy *policy, int state); > }; > > /* flags */ > -- > 1.7.12.4
On Wed, May 20, 2020 at 6:59 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 19-05-20, 19:41, Xiongfeng Wang wrote: > > Macro 'for_each_active_policy()' is defined internally. To avoid some > > cpufreq driver needing this macro to iterate over all the policies in > > '.set_boost' callback, we redefine '.set_boost' to act on only one > > policy and pass the policy as an argument. > > 'cpufreq_boost_trigger_state()' iterate over all the policies to set > > boost for the system. This is preparation for adding SW BOOST support > > for CPPC. > > > > Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> > > --- > > drivers/cpufreq/acpi-cpufreq.c | 4 ++-- > > drivers/cpufreq/cpufreq.c | 53 +++++++++++++++++++++--------------------- > > include/linux/cpufreq.h | 2 +- > > 3 files changed, 30 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > > index 289e8ce..b0a9eb5 100644 > > --- a/drivers/cpufreq/acpi-cpufreq.c > > +++ b/drivers/cpufreq/acpi-cpufreq.c > > @@ -126,7 +126,7 @@ static void boost_set_msr_each(void *p_en) > > boost_set_msr(enable); > > } > > > > -static int set_boost(int val) > > +static int set_boost(struct cpufreq_policy *policy, int val) > > { > > get_online_cpus(); > > on_each_cpu(boost_set_msr_each, (void *)(long)val, 1); > > I think (Rafael can confirm), that you need to update this as well. You don't > need to run for each cpu now, but for each CPU in the policy. Right, the caller will iterate over policies. Accordingly, the CPU hotplug locking needs to go to the caller too.
On 20-05-20, 12:36, Rafael J. Wysocki wrote: > On Wed, May 20, 2020 at 6:59 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 19-05-20, 19:41, Xiongfeng Wang wrote: > > > Macro 'for_each_active_policy()' is defined internally. To avoid some > > > cpufreq driver needing this macro to iterate over all the policies in > > > '.set_boost' callback, we redefine '.set_boost' to act on only one > > > policy and pass the policy as an argument. > > > 'cpufreq_boost_trigger_state()' iterate over all the policies to set > > > boost for the system. This is preparation for adding SW BOOST support > > > for CPPC. > > > > > > Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> > > > --- > > > drivers/cpufreq/acpi-cpufreq.c | 4 ++-- > > > drivers/cpufreq/cpufreq.c | 53 +++++++++++++++++++++--------------------- > > > include/linux/cpufreq.h | 2 +- > > > 3 files changed, 30 insertions(+), 29 deletions(-) > > > > > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > > > index 289e8ce..b0a9eb5 100644 > > > --- a/drivers/cpufreq/acpi-cpufreq.c > > > +++ b/drivers/cpufreq/acpi-cpufreq.c > > > @@ -126,7 +126,7 @@ static void boost_set_msr_each(void *p_en) > > > boost_set_msr(enable); > > > } > > > > > > -static int set_boost(int val) > > > +static int set_boost(struct cpufreq_policy *policy, int val) > > > { > > > get_online_cpus(); > > > on_each_cpu(boost_set_msr_each, (void *)(long)val, 1); > > > > I think (Rafael can confirm), that you need to update this as well. You don't > > need to run for each cpu now, but for each CPU in the policy. > > Right, the caller will iterate over policies. > > Accordingly, the CPU hotplug locking needs to go to the caller too. Hmm, why is that required ? Can't we call boost_set_msr_each() for all CPUs of a policy under the locks ? And then let the next call take the lock again ? I thought we don't want a CPU to disappear while we are trying to run boost_set_msr_each() for it (or miss one that just got added) and that should work with the locks being there in this routine.
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index 289e8ce..b0a9eb5 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -126,7 +126,7 @@ static void boost_set_msr_each(void *p_en) boost_set_msr(enable); } -static int set_boost(int val) +static int set_boost(struct cpufreq_policy *policy, int val) { get_online_cpus(); on_each_cpu(boost_set_msr_each, (void *)(long)val, 1); @@ -162,7 +162,7 @@ static ssize_t store_cpb(struct cpufreq_policy *policy, const char *buf, if (ret || val > 1) return -EINVAL; - set_boost(val); + set_boost(policy, val); return count; } diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index d03f250..d0d86b1 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2532,34 +2532,29 @@ void cpufreq_update_limits(unsigned int cpu) /********************************************************************* * BOOST * *********************************************************************/ -static int cpufreq_boost_set_sw(int state) +static int cpufreq_boost_set_sw(struct cpufreq_policy *policy, int state) { - struct cpufreq_policy *policy; - - for_each_active_policy(policy) { - int ret; - - if (!policy->freq_table) - return -ENXIO; + int ret; - ret = cpufreq_frequency_table_cpuinfo(policy, - policy->freq_table); - if (ret) { - pr_err("%s: Policy frequency update failed\n", - __func__); - return ret; - } + if (!policy->freq_table) + return -ENXIO; - ret = freq_qos_update_request(policy->max_freq_req, policy->max); - if (ret < 0) - return ret; + ret = cpufreq_frequency_table_cpuinfo(policy, policy->freq_table); + if (ret) { + pr_err("%s: Policy frequency update failed\n", __func__); + return ret; } + ret = freq_qos_update_request(policy->max_freq_req, policy->max); + if (ret < 0) + return ret; + return 0; } int cpufreq_boost_trigger_state(int state) { + struct cpufreq_policy *policy; unsigned long flags; int ret = 0; @@ -2570,16 +2565,22 @@ int cpufreq_boost_trigger_state(int state) cpufreq_driver->boost_enabled = state; write_unlock_irqrestore(&cpufreq_driver_lock, flags); - ret = cpufreq_driver->set_boost(state); - if (ret) { - write_lock_irqsave(&cpufreq_driver_lock, flags); - cpufreq_driver->boost_enabled = !state; - write_unlock_irqrestore(&cpufreq_driver_lock, flags); - - pr_err("%s: Cannot %s BOOST\n", - __func__, state ? "enable" : "disable"); + for_each_active_policy(policy) { + ret = cpufreq_driver->set_boost(policy, state); + if (ret) + goto err_reset_state; } + return 0; + +err_reset_state: + write_lock_irqsave(&cpufreq_driver_lock, flags); + cpufreq_driver->boost_enabled = !state; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + + pr_err("%s: Cannot %s BOOST\n", + __func__, state ? "enable" : "disable"); + return ret; } diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 67d5950..3494f67 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -367,7 +367,7 @@ struct cpufreq_driver { /* platform specific boost support code */ bool boost_enabled; - int (*set_boost)(int state); + int (*set_boost)(struct cpufreq_policy *policy, int state); }; /* flags */
Macro 'for_each_active_policy()' is defined internally. To avoid some cpufreq driver needing this macro to iterate over all the policies in '.set_boost' callback, we redefine '.set_boost' to act on only one policy and pass the policy as an argument. 'cpufreq_boost_trigger_state()' iterate over all the policies to set boost for the system. This is preparation for adding SW BOOST support for CPPC. Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> --- drivers/cpufreq/acpi-cpufreq.c | 4 ++-- drivers/cpufreq/cpufreq.c | 53 +++++++++++++++++++++--------------------- include/linux/cpufreq.h | 2 +- 3 files changed, 30 insertions(+), 29 deletions(-)