Messages in this thread | | | From | "Rafael J. Wysocki" <> | Subject | Re: [PATCH 3/3] cpufreq: Always update current frequency before startig governor | Date | Tue, 22 Mar 2016 15:33:19 +0100 |
| |
On Tuesday, March 22, 2016 09:00:32 AM Viresh Kumar wrote: > On 21-03-16, 15:47, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Make policy->cur match the current frequency returned by the driver's > > ->get() callback before starting the governor in case they went out of > > sync in the meantime and drop the piece of code attempting to > > resync policy->cur with the real frequency of the boot CPU from > > cpufreq_resume() as it serves no purpose any more (and it's racy and > > super-ugly anyway). > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/cpufreq/cpufreq.c | 14 +++----------- > > 1 file changed, 3 insertions(+), 11 deletions(-) > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c > > =================================================================== > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > > +++ linux-pm/drivers/cpufreq/cpufreq.c > > @@ -1680,17 +1680,6 @@ void cpufreq_resume(void) > > __func__, policy); > > } > > } > > - > > - /* > > - * schedule call cpufreq_update_policy() for first-online CPU, as that > > - * wouldn't be hotplugged-out on suspend. It will verify that the > > - * current freq is in sync with what we believe it to be. > > - */ > > - policy = cpufreq_cpu_get_raw(cpumask_first(cpu_online_mask)); > > - if (WARN_ON(!policy)) > > - return; > > - > > - schedule_work(&policy->update); > > } > > > > /** > > @@ -2062,6 +2051,9 @@ static int cpufreq_start_governor(struct > > { > > int ret; > > > > + if (cpufreq_driver->get && !cpufreq_driver->setpolicy) > > + cpufreq_update_current_freq(policy); > > + > > ret = cpufreq_governor(policy, CPUFREQ_GOV_START); > > return ret ? ret : cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); > > } > > This looks fine, but I am searching for answers to few doubts, maybe > you can help.. > > Why we did the same in process context earlier? And why it wouldn't be > a problem now, when we do it in interrupt context? Will IRQs be > disabled here? If so, then will you hit following ?
I'm not sure I'm following.
This is process context too.
Look at the call sites of cpufreq_start_governor() (patch [1/3]): - cpufreq_offline() - process context - cpufreq_resume() - process context - cpufreq_set_policy() - process context - cpufreq_add_policy_cpu() - process context
Besides, calling cpufreq_governor() from interrupt context wouldn't reall work, because that acquires mutexes etc, like in cpufreq_governor_init().
> static void __cpufreq_notify_transition(struct cpufreq_policy *policy, > struct cpufreq_freqs *freqs, unsigned int state) > { > BUG_ON(irqs_disabled()); > > ... > } > > And will calling notifiers from interrupt-context just fine ?
If your question is why the original code doesn't call cpufreq_update_policy() directly, I think the reason is because cpufreq_resume() used to be one of the syscore ops and *that* would have been run in interrupt context.
Thanks, Rafael
| |