Re: [RFC V2] cpufreq: make sure frequency transitions are serialized

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 03/19/2014 11:38 AM, Viresh Kumar wrote:
> On 18 March 2014 18:20, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> On 03/14/2014 01:13 PM, Viresh Kumar wrote:
>>> +     if ((state != CPUFREQ_PRECHANGE) && (state != CPUFREQ_POSTCHANGE))
>>
>> Wait a min, when is this condition ever true? I mean, what else can
>> 'state' ever be, apart from CPUFREQ_PRECHANGE and POSTCHANGE?
> 
> There were two more 'unused' states available:
> CPUFREQ_RESUMECHANGE and CPUFREQ_SUSPENDCHANGE
>
> I have sent a patch to remove them now and this code would go away..
>

Ok..
 
>>> +             return notify_transition_for_each_cpu(policy, freqs, state);
>>> +
>>> +     /* Serialize pre-post notifications */
>>> +     mutex_lock(&policy->transition_lock);
>>
>> Nope, this is definitely not the way to go, IMHO. We should enforce that
>> the *callers* serialize the transitions, something like this:
>>
>>         cpufreq_transition_lock();
>>
>>         cpufreq_notify_transition(CPUFREQ_PRECHANGE);
>>
>>         //Perform the frequency change
>>
>>         cpufreq_notify_transition(CPUFREQ_POSTCHANGE);
>>
>>         cpufreq_transition_unlock();
>>
>> That's it!
>>
>> [ We can either introduce a new "transition" lock or perhaps even reuse
>> the cpufreq_driver_lock if it fits... but the point is, the _caller_ has
>> to perform the locking; trying to be smart inside cpufreq_notify_transition()
>> is a recipe for headache :-( ]
>>
>> Is there any problem with this approach due to which you didn't take
>> this route?
> 

Wait, I think I remember. The problem was about dealing with drivers that
do asynchronous notification (those that have the ASYNC_NOTIFICATION flag
set). In particular, exynos-5440 driver sends out the POSTCHANGE notification
from a workqueue worker, much later than sending the PRECHANGE notification.

>From what I saw, this is how the exynos-5440 driver works:

1. ->target() is invoked, and the driver writes to a register and returns
   to its caller.

2. An interrupt occurs that indicates that the frequency was changed.

3. The interrupt handler kicks off a worker thread which then sends out
   the POSTCHANGE notification.

So the important question here is, how does the exynos-5440 driver
protect itself from say 2 ->target() calls which occur in close sequence
(before allowing the entire chain for the first call to complete)?

As far as I can see there is no such synchronization in the driver at
the moment. Adding Amit to CC for his comments.

Regards,
Srivatsa S. Bhat

> I didn't wanted drivers to handle this as core must make sure things are in
> order. Over that it would have helped by not pasting redundant code
> everywhere..
> 
> Drivers are anyway going to call cpufreq_notify_transition(), why increase
> burden on them?
> 
>>> +     if (unlikely(WARN_ON(!policy->transition_ongoing &&
>>> +                             (state == CPUFREQ_POSTCHANGE)))) {
>>> +             mutex_unlock(&policy->transition_lock);
>>> +             return;
>>> +     }
>>> +
>>> +     if (state == CPUFREQ_PRECHANGE) {
>>> +             while (policy->transition_ongoing) {
>>> +                     mutex_unlock(&policy->transition_lock);
>>> +                     /* TODO: Can we do something better here? */
>>> +                     cpu_relax();
>>> +                     mutex_lock(&policy->transition_lock);
>>
>> If the caller takes care of the synchronization, we can avoid
>> these sorts of acrobatics ;-)
> 
> If we are fine with taking a mutex for the entire transition, then
> we can avoid above kind of acrobatics by just taking the mutex
> from PRECHANGE and leaving it at POSTCHANGE..
> 
> It will look like this then, hope this looks fine :)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 2677ff1..3b9eac4 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -335,8 +335,15 @@ static void __cpufreq_notify_transition(struct
> cpufreq_policy *policy,
>  void cpufreq_notify_transition(struct cpufreq_policy *policy,
>                 struct cpufreq_freqs *freqs, unsigned int state)
>  {
> +       if (state == CPUFREQ_PRECHANGE)
> +               mutex_lock(&policy->transition_lock);
> +
> +       /* Send notifications */
>         for_each_cpu(freqs->cpu, policy->cpus)
>                 __cpufreq_notify_transition(policy, freqs, state);
> +
> +       if (state == CPUFREQ_POSTCHANGE)
> +               mutex_unlock(&policy->transition_lock);
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_notify_transition);
> 
> @@ -983,6 +990,7 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)
> 
>         INIT_LIST_HEAD(&policy->policy_list);
>         init_rwsem(&policy->rwsem);
> +       mutex_init(&policy->transition_lock);
> 
>         return policy;
> 
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 31c431e..5f9209a 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -104,6 +104,7 @@ struct cpufreq_policy {
>          *     __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
>          */
>         struct rw_semaphore     rwsem;
> +       struct mutex            transition_lock;
>  };
>