Re: [PATCH v2] cpufreq: Catch double invocations of cpufreq_freq_transition_begin/end

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

On 04/29/2014 06:39 PM, Meelis Roos wrote:
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>>
>> v2: Removed the coverage of ASYNC_NOTIFICATION drivers, in order to avoid
>> false-positives.
> 
> I am confused - on top of what patches should I test it?
> 
>

Well, actually this is not a fix. Its only a debug infrastructure to
catch the problems you reported, more easily. And this patch is
independent, it doesn't depend on any patch. So if you try this patch
as it is on your system (which doesn't include any of the other patches
I sent) then you'll see a warning during boot (before you hit the hang).
That's the intended behavior of this debug patch - to throw warnings,
if a scenario is detected which can lead to a hang.

You have already tested the fix that I sent for longhaul (and other
2 drivers) and they have made it to Rafael's bleeding-edge branch.

http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/log/?h=bleeding-edge

So if you instead test this debug patch on *top* of Rafael's tree,
you'll see that the hang is not reproducible (because of the longhaul
fix that went in) and also this debug patch will not print any warning.
That's again the intended behavior.

Regards,
Srivatsa S. Bhat

> 
>>
>>  drivers/cpufreq/cpufreq.c |    7 +++++++
>>  include/linux/cpufreq.h   |    1 +
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index abda660..afcac67 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -354,6 +354,11 @@ static void cpufreq_notify_post_transition(struct cpufreq_policy *policy,
>>  void cpufreq_freq_transition_begin(struct cpufreq_policy *policy,
>>  		struct cpufreq_freqs *freqs)
>>  {
>> +
>> +	/* Catch double invocations of _begin() which lead to self-deadlock */
>> +	WARN_ON(!(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION)
>> +				&& current == policy->transition_task);
>> +
>>  wait:
>>  	wait_event(policy->transition_wait, !policy->transition_ongoing);
>>  
>> @@ -365,6 +370,7 @@ wait:
>>  	}
>>  
>>  	policy->transition_ongoing = true;
>> +	policy->transition_task = current;
>>  
>>  	spin_unlock(&policy->transition_lock);
>>  
>> @@ -381,6 +387,7 @@ void cpufreq_freq_transition_end(struct cpufreq_policy *policy,
>>  	cpufreq_notify_post_transition(policy, freqs, transition_failed);
>>  
>>  	policy->transition_ongoing = false;
>> +	policy->transition_task = NULL;
>>  
>>  	wake_up(&policy->transition_wait);
>>  }
>> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
>> index 5ae5100..8f44d79 100644
>> --- a/include/linux/cpufreq.h
>> +++ b/include/linux/cpufreq.h
>> @@ -110,6 +110,7 @@ struct cpufreq_policy {
>>  	bool			transition_ongoing; /* Tracks transition status */
>>  	spinlock_t		transition_lock;
>>  	wait_queue_head_t	transition_wait;
>> +	struct task_struct	*transition_task; /* Task which is doing the transition */
>>  };
>>  
>>  /* Only for ACPI */
>>
>