| Message ID | 2ffbaf079a21c2810c402cb5bba4e9c14c4a0ff4.1623825725.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: cppc: Add support for frequency invariance | expand |
On Wed, Jun 16, 2021 at 8:48 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On CPU hot-unplug, the cpufreq core doesn't call any driver specific > callback unless all the CPUs of a policy went away, in which case we > call stop_cpu() callback. > > For the CPPC cpufreq driver, we need to perform per-cpu init/exit work > which that can't be performed from policy specific init()/exit() > callbacks. > > This patch adds a new callback, start_cpu() and modifies the stop_cpu() > callback, to perform such CPU specific work. > > These routines are called whenever a CPU is added or removed from a > policy. > > Note that this also moves the setting of policy->cpus to online CPUs > only, outside of rwsem as we needed to call start_cpu() for online CPUs > only. This shouldn't have any side effects. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > Documentation/cpu-freq/cpu-drivers.rst | 7 +++++-- > drivers/cpufreq/cpufreq.c | 19 +++++++++++++++---- > include/linux/cpufreq.h | 5 ++++- > 3 files changed, 24 insertions(+), 7 deletions(-) > > diff --git a/Documentation/cpu-freq/cpu-drivers.rst b/Documentation/cpu-freq/cpu-drivers.rst > index a697278ce190..15cfe42b4075 100644 > --- a/Documentation/cpu-freq/cpu-drivers.rst > +++ b/Documentation/cpu-freq/cpu-drivers.rst > @@ -71,8 +71,11 @@ And optionally > .exit - A pointer to a per-policy cleanup function called during > CPU_POST_DEAD phase of cpu hotplug process. > > - .stop_cpu - A pointer to a per-policy stop function called during > - CPU_DOWN_PREPARE phase of cpu hotplug process. > + .start_cpu - A pointer to a per-policy per-cpu start function called > + during CPU online phase. > + > + .stop_cpu - A pointer to a per-policy per-cpu stop function called > + during CPU offline phase. > > .suspend - A pointer to a per-policy suspend function which is called > with interrupts disabled and _after_ the governor is stopped for the > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 802abc925b2a..128dfb1b0cdf 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1119,6 +1119,10 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cp > > cpumask_set_cpu(cpu, policy->cpus); > > + /* Do CPU specific initialization if required */ > + if (cpufreq_driver->start_cpu) > + cpufreq_driver->start_cpu(policy, cpu); > + > if (has_target()) { > ret = cpufreq_start_governor(policy); > if (ret) > @@ -1375,13 +1379,19 @@ static int cpufreq_online(unsigned int cpu) > cpumask_copy(policy->related_cpus, policy->cpus); > } > > - down_write(&policy->rwsem); > /* > * affected cpus must always be the one, which are online. We aren't > * managing offline cpus here. > */ > cpumask_and(policy->cpus, policy->cpus, cpu_online_mask); > > + /* Do CPU specific initialization if required */ > + if (cpufreq_driver->start_cpu) { > + for_each_cpu(j, policy->cpus) > + cpufreq_driver->start_cpu(policy, j); > + } > + > + down_write(&policy->rwsem); > if (new_policy) { > for_each_cpu(j, policy->related_cpus) { > per_cpu(cpufreq_cpu_data, j) = policy; > @@ -1581,6 +1591,10 @@ static int cpufreq_offline(unsigned int cpu) > policy->cpu = cpumask_any(policy->cpus); > } > > + /* Do CPU specific de-initialization if required */ > + if (cpufreq_driver->stop_cpu) > + cpufreq_driver->stop_cpu(policy, cpu); > + > /* Start governor again for active policy */ > if (!policy_is_inactive(policy)) { > if (has_target()) { > @@ -1597,9 +1611,6 @@ static int cpufreq_offline(unsigned int cpu) > policy->cdev = NULL; > } > > - if (cpufreq_driver->stop_cpu) > - cpufreq_driver->stop_cpu(policy); > - This should be a separate patch IMO, after you've migrated everyone to ->offline/->exit. BTW, IMO it might be better to migrate ->stop_cpu to ->offline rather than to ->exit. > if (has_target()) > cpufreq_exit_governor(policy); > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 353969c7acd3..c281b3df4e2f 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -371,7 +371,10 @@ struct cpufreq_driver { > int (*online)(struct cpufreq_policy *policy); > int (*offline)(struct cpufreq_policy *policy); > int (*exit)(struct cpufreq_policy *policy); > - void (*stop_cpu)(struct cpufreq_policy *policy); > + > + /* CPU specific start/stop */ > + void (*start_cpu)(struct cpufreq_policy *policy, unsigned int cpu); > + void (*stop_cpu)(struct cpufreq_policy *policy, unsigned int cpu); > int (*suspend)(struct cpufreq_policy *policy); > int (*resume)(struct cpufreq_policy *policy); > > -- > 2.31.1.272.g89b43f80a514 >
On 17-06-21, 15:33, Rafael J. Wysocki wrote: > On Wed, Jun 16, 2021 at 8:48 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > + /* Do CPU specific de-initialization if required */ > > + if (cpufreq_driver->stop_cpu) > > + cpufreq_driver->stop_cpu(policy, cpu); > > + > > /* Start governor again for active policy */ > > if (!policy_is_inactive(policy)) { > > if (has_target()) { > > @@ -1597,9 +1611,6 @@ static int cpufreq_offline(unsigned int cpu) > > policy->cdev = NULL; > > } > > > > - if (cpufreq_driver->stop_cpu) > > - cpufreq_driver->stop_cpu(policy); > > - > > This should be a separate patch IMO, after you've migrated everyone to > ->offline/->exit. Right, anyway this patch may not be required anymore. I will send a patch to remove this. > BTW, IMO it might be better to migrate ->stop_cpu to ->offline rather > than to ->exit. This is a bit tricky IMO. First, offline() isn't implemented by everyone, out of the three implementations which were using stop_cpu(), only intel-pstate had offline() as well. Second, the primary purpose of online/offline callbacks was suspend/resume oriented and for the same reason, we don't call online() when the policy first comes up and so in case of errors during bring up, we end up calling exit() directly and not offline(). IMO this is a very specific thing to drivers and they need to see what fits best for them, exit() or offline() or both. -- viresh
diff --git a/Documentation/cpu-freq/cpu-drivers.rst b/Documentation/cpu-freq/cpu-drivers.rst index a697278ce190..15cfe42b4075 100644 --- a/Documentation/cpu-freq/cpu-drivers.rst +++ b/Documentation/cpu-freq/cpu-drivers.rst @@ -71,8 +71,11 @@ And optionally .exit - A pointer to a per-policy cleanup function called during CPU_POST_DEAD phase of cpu hotplug process. - .stop_cpu - A pointer to a per-policy stop function called during - CPU_DOWN_PREPARE phase of cpu hotplug process. + .start_cpu - A pointer to a per-policy per-cpu start function called + during CPU online phase. + + .stop_cpu - A pointer to a per-policy per-cpu stop function called + during CPU offline phase. .suspend - A pointer to a per-policy suspend function which is called with interrupts disabled and _after_ the governor is stopped for the diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 802abc925b2a..128dfb1b0cdf 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1119,6 +1119,10 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cp cpumask_set_cpu(cpu, policy->cpus); + /* Do CPU specific initialization if required */ + if (cpufreq_driver->start_cpu) + cpufreq_driver->start_cpu(policy, cpu); + if (has_target()) { ret = cpufreq_start_governor(policy); if (ret) @@ -1375,13 +1379,19 @@ static int cpufreq_online(unsigned int cpu) cpumask_copy(policy->related_cpus, policy->cpus); } - down_write(&policy->rwsem); /* * affected cpus must always be the one, which are online. We aren't * managing offline cpus here. */ cpumask_and(policy->cpus, policy->cpus, cpu_online_mask); + /* Do CPU specific initialization if required */ + if (cpufreq_driver->start_cpu) { + for_each_cpu(j, policy->cpus) + cpufreq_driver->start_cpu(policy, j); + } + + down_write(&policy->rwsem); if (new_policy) { for_each_cpu(j, policy->related_cpus) { per_cpu(cpufreq_cpu_data, j) = policy; @@ -1581,6 +1591,10 @@ static int cpufreq_offline(unsigned int cpu) policy->cpu = cpumask_any(policy->cpus); } + /* Do CPU specific de-initialization if required */ + if (cpufreq_driver->stop_cpu) + cpufreq_driver->stop_cpu(policy, cpu); + /* Start governor again for active policy */ if (!policy_is_inactive(policy)) { if (has_target()) { @@ -1597,9 +1611,6 @@ static int cpufreq_offline(unsigned int cpu) policy->cdev = NULL; } - if (cpufreq_driver->stop_cpu) - cpufreq_driver->stop_cpu(policy); - if (has_target()) cpufreq_exit_governor(policy); diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 353969c7acd3..c281b3df4e2f 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -371,7 +371,10 @@ struct cpufreq_driver { int (*online)(struct cpufreq_policy *policy); int (*offline)(struct cpufreq_policy *policy); int (*exit)(struct cpufreq_policy *policy); - void (*stop_cpu)(struct cpufreq_policy *policy); + + /* CPU specific start/stop */ + void (*start_cpu)(struct cpufreq_policy *policy, unsigned int cpu); + void (*stop_cpu)(struct cpufreq_policy *policy, unsigned int cpu); int (*suspend)(struct cpufreq_policy *policy); int (*resume)(struct cpufreq_policy *policy);
On CPU hot-unplug, the cpufreq core doesn't call any driver specific callback unless all the CPUs of a policy went away, in which case we call stop_cpu() callback. For the CPPC cpufreq driver, we need to perform per-cpu init/exit work which that can't be performed from policy specific init()/exit() callbacks. This patch adds a new callback, start_cpu() and modifies the stop_cpu() callback, to perform such CPU specific work. These routines are called whenever a CPU is added or removed from a policy. Note that this also moves the setting of policy->cpus to online CPUs only, outside of rwsem as we needed to call start_cpu() for online CPUs only. This shouldn't have any side effects. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- Documentation/cpu-freq/cpu-drivers.rst | 7 +++++-- drivers/cpufreq/cpufreq.c | 19 +++++++++++++++---- include/linux/cpufreq.h | 5 ++++- 3 files changed, 24 insertions(+), 7 deletions(-) -- 2.31.1.272.g89b43f80a514