Re: [PATCH 3/3] cpufreq: Always update current frequency before startig governor

[Rafael J. Wysocki]

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@xxxxxxxxx>
> > 
> > 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@xxxxxxxxx>
> > ---
> >  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