Re: CPUfreq lockdep issue

[Viresh Kumar <viresh.kumar@linaro.org>]

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?

> 		...
> 		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.

> 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 ?

-- 
viresh