Re: CPUfreq lockdep issue

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On to, 2016-02-18 at 17:04 +0530, Viresh Kumar wrote:
> On 18-02-16, 13:06, Joonas Lahtinen wrote:
> > Hi,
> > 
> > The Intel P-state driver has a lockdep issue as described below. It
> > could in theory cause a deadlock if initialization and suspend were to
> > be performed simultaneously. Conflicting calling paths are as follows:
> > 
> > intel_pstate_init(...)
> > 	...cpufreq_online(...)
> > 		down_write(&policy->rwsem); // Locks policy->rwsem
> > 		...
> > 		cpufreq_init_policy(policy);
> > 			...intel_pstate_hwp_set();
> > 				get_online_cpus(); // Temporarily locks cpu_hotplug.lock
> 
> Why is this one required?

Otherwise CPUs could be added or removed during the execution of
intel_pstate_hwp_set(), which has the following code:

       	get_online_cpus();
        for_each_online_cpu(cpu) {
		...
		wrmsrl_on_cpu(cpu, MSR_HWP_REQUEST, value);
	}
> 
> > 		...
> > 		up_write(&policy->rwsem);
> > 
> > pm_suspend(...)
> > 	...disable_nonboot_cpus()
> > 		_cpu_down()
> > 			cpu_hotplug_begin(); // Locks cpu_hotplug.lock
> > 			__cpu_notify(CPU_DOWN_PREPARE, ...);
> > 				...cpufreq_offline_prepare();
> > 					down_write(&policy->rwsem); // Locks policy->rwsem
> > 
> > Quickly looking at the code, some refactoring has to be done to fix the
> > issue. I think it would a good idea to document some of the driver
> > callbacks related to what locks are held etc. in order to avoid future
> > situations like this.
> > 
> > Because get_online_cpus() is of recursive nature and the way it
> > currently works, adding wider get_online_cpus() scope up around
> > cpufreq_online() does not fix the issue because it only momentarily
> > locks cpu_hotplug.lock and proceeds to do so again at next call.
> > 
> > Moving get_online_cpus() completely away from pstate_hwp_set() and
> > assuring it is called higher in the call chain might be a viable
> > solution. Then it could be made sure get_online_cpus() is not called
> > while policy->rwsem is being held already.
> 
> I don't think that will be a good solution. So what you are
> essentially saying is, take policy->rwsem after get_online_cpus()
> only.

Yes, grab the policy lock only after we've made sure we can apply the
policy to the online CPUs.

> 
> > Do you think that would be an appropriate way of fixing it?
> 
> At least I don't. Why do we need to call get_online_cpus()
> intel-pstate governor ?

See above for the code.

Regards, Joonas

> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation