Re: [PATCH 08/12] cpufreq: governor: synchronize work-handler with governor callbacks

[Preeti U Murthy <preeti@linux.vnet.ibm.com>]

On 06/11/2015 04:21 PM, Viresh Kumar wrote:
> Currently the work-handler's for all the CPUs are synchronized with one
> another but not with cpufreq_governor_dbs(). Both work-handlers and
> cpufreq_governor_dbs() enqueue work. And these not being in sync is an
> open invitation to all kind of races to come in.

This patch is not convincing. What are the precise races between
cpufreq_governor_dbs() and the work handlers ?

It is true that the work handlers can get queued after a governor exit
due to a race between different callers of cpufreq_governor_dbs() and
they can dereference freed data structures. But that is about invalid
interleaving of states which your next patch aims at fixing.

AFAICT, preventing invalid states from succeeding will avoid this race.
I don't see the need for another lock here.

> 
> So, this patch replaces 'timer_mutex' with 'cdata->mutex' so that both
> work-handler and callback are in sync.
> 
> Also in update_sampling_rate() we are dropping the mutex while
> canceling delayed-work. There is no need to do that, keep lock active

Why? What is the race that we are fixing by taking the lock here ?

> for the entire length of routine as that also enqueues works.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

Regards
Preeti U Murthy